Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1633270 > unrolled thread

[PATCH v2 0/4] iio: accel: adxl345: Add support for buffered readings

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/4] iio: accel: adxl345: Add support for buffered readings Eva Rachel Retuya <eraretuya@gmail.com> - 2017-04-29 09:50 +0200
    [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and data_ready functions Eva Rachel Retuya <eraretuya@gmail.com> - 2017-04-29 10:00 +0200
      Re: [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and  data_ready functions Jonathan Cameron <jic23@kernel.org> - 2017-05-01 02:30 +0200
        Re: [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and  data_ready functions Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-05-01 21:50 +0200
          Re: [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and data_ready functions Jonathan Cameron <jic23@jic23.retrosnub.co.uk> - 2017-05-01 21:50 +0200
            Re: [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and  data_ready functions Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-05-01 22:10 +0200
              Re: [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and data_ready functions Jonathan Cameron <jic23@jic23.retrosnub.co.uk> - 2017-05-01 22:20 +0200
        Re: [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and  data_ready functions Eva Rachel Retuya <eraretuya@gmail.com> - 2017-05-02 13:40 +0200
          Re: [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and  data_ready functions Jonathan Cameron <jic23@kernel.org> - 2017-05-02 18:40 +0200
            Re: [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and  data_ready functions Eva Rachel Retuya <eraretuya@gmail.com> - 2017-05-10 15:10 +0200
      Re: [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and  data_ready functions Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-05-01 13:30 +0200
        Re: [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and  data_ready functions Eva Rachel Retuya <eraretuya@gmail.com> - 2017-05-02 13:50 +0200
    [PATCH v2 4/4] iio: accel: adxl345: Add support for triggered buffer Eva Rachel Retuya <eraretuya@gmail.com> - 2017-04-29 10:00 +0200
      Re: [PATCH v2 4/4] iio: accel: adxl345: Add support for triggered  buffer Jonathan Cameron <jic23@kernel.org> - 2017-05-01 02:50 +0200
        Re: [PATCH v2 4/4] iio: accel: adxl345: Add support for triggered  buffer Eva Rachel Retuya <eraretuya@gmail.com> - 2017-05-02 14:30 +0200
          Re: [PATCH v2 4/4] iio: accel: adxl345: Add support for triggered  buffer Jonathan Cameron <jic23@kernel.org> - 2017-05-02 18:10 +0200
      Re: [PATCH v2 4/4] iio: accel: adxl345: Add support for triggered buffer Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-05-01 13:30 +0200
        Re: [PATCH v2 4/4] iio: accel: adxl345: Add support for triggered  buffer Eva Rachel Retuya <eraretuya@gmail.com> - 2017-05-02 14:30 +0200
    [PATCH v2 1/4] dt-bindings: iio: accel: adxl345: Add optional interrupt-names support Eva Rachel Retuya <eraretuya@gmail.com> - 2017-04-29 10:00 +0200
    [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger Eva Rachel Retuya <eraretuya@gmail.com> - 2017-04-29 10:00 +0200
      Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger Jonathan Cameron <jic23@kernel.org> - 2017-05-01 02:40 +0200
        Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger Rob Herring <robh+dt@kernel.org> - 2017-05-02 05:10 +0200
          Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger Jonathan Cameron <jic23@kernel.org> - 2017-05-02 18:00 +0200
            Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger Eva Rachel Retuya <eraretuya@gmail.com> - 2017-05-10 16:40 +0200
        Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger Eva Rachel Retuya <eraretuya@gmail.com> - 2017-05-02 14:00 +0200
      Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-05-01 14:00 +0200
        Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger Eva Rachel Retuya <eraretuya@gmail.com> - 2017-05-02 14:20 +0200
          Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-05-02 23:10 +0200
            Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger Eva Rachel Retuya <eraretuya@gmail.com> - 2017-05-10 15:30 +0200
          Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger Jonathan Cameron <jic23@kernel.org> - 2017-05-05 20:30 +0200
            Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger Eva Rachel Retuya <eraretuya@gmail.com> - 2017-05-10 15:40 +0200

Page 1 of 2  [1] 2  Next page →

#1633270 — [PATCH v2 0/4] iio: accel: adxl345: Add support for buffered readings

Introduce the DATA_READY trigger and enable triggered buffering. Additional
changes include introduction of functions set_mode and data_ready, allow
either INT1/INT2 pin be used by specifying interrupt-names.

Triggered buffer was tested on both DATA_READY trigger and the hrtimer software
trigger.

~ # ls /sys/bus/iio/devices/
iio:device0  trigger0     trigger1
~ # ls /config/iio/triggers/hrtimer/
t1
~ # cat /sys/bus/iio/devices/trigger0/name
t1
~ # cat /sys/bus/iio/devices/trigger1/name
adxl345-dev0
~ # iio_generic_buffer -n adxl345 -t t1 -c 5 -l 20 -a
iio device number being used is 0
iio trigger number being used is 0
Enabling all channels
Enabling: in_accel_y_en
Enabling: in_accel_x_en
Enabling: in_timestamp_en
Enabling: in_accel_z_en
/sys/bus/iio/devices/iio:device0 t1
0.306400 0.880900 9.575000 1493432411145155964 
0.306400 0.880900 9.575000 1493432411185154872 
0.306400 0.919200 9.536700 1493432411225167667 
0.306400 0.880900 9.536700 1493432411265157809 
0.344700 0.919200 9.536700 1493432411295108767 
Disabling: in_accel_y_en
Disabling: in_accel_x_en
Disabling: in_timestamp_en
Disabling: in_accel_z_en
~ # iio_generic_buffer -n adxl345 -t adxl345-dev0 -c 5 -l 20 -a
iio device number being used is 0
iio trigger number being used is 1
Enabling all channels
Enabling: in_accel_y_en
Enabling: in_accel_x_en
Enabling: in_timestamp_en
Enabling: in_accel_z_en
/sys/bus/iio/devices/iio:device0 adxl345-dev0
0.344700 0.919200 9.689900 1493432411336475189 
0.344700 0.880900 9.575000 1493432411336475189 
0.306400 0.957500 9.651600 1493432411336475189 
0.306400 0.880900 9.575000 1493432411336475189 
0.306400 0.919200 9.536700 1493432411336475189 
Disabling: in_accel_y_en
Disabling: in_accel_x_en
Disabling: in_timestamp_en
Disabling: in_accel_z_en
~ # 

Changes in v2:
* Provide a detailed commit message to those missing it
* Add Rob's Acked-by tag
* Make function naming more clear: drdy -> data_ready
  * Switch from while to do..while
  * Rename regval to val
  * Switch to usleep_range() for shorter delay
  * Add comment to justify delay
  * Change error code -EIO to -EAGAIN
* Endianness issue: scrap the get_triple function entirely, make no
  changes in terms of reading registers in read_raw
* Introduce mutex earlier rather than in succeeding patch
* Probe:
  * Fix random whitespace omission
  * Remove configuring to standby mode, the device powers on in standby
    mode so this is not needed
  * use variable 'regval' to hold value to be written to the register and call
    regmap_write() unconditionally
  * fix line splitting in devm_request_threaded_irq() and devm_iio_trigger_alloc()
  * Switch to devm_iio_trigger_register()
  * Switch to devm_iio_triggered_buffer_setup()
* Move the of_irq_get_byname() check in core file in order to avoid
  introducing another parameter in probe()
* adxl345_irq():
  * return values directly
  * switch from iio_trigger_poll() to iio_trigger_poll_chained(), the former
    should only be called at the top-half not at the bottom-half.
* adxl345_drdy_trigger_set_state():
  * move regmap_get_device() to definition block
  * regmap_update_bits(): line splitting - one parameter per line, remove extra
    parenthesis
* adxl345_trigger_handler()
  * if using external trigger, place a adxl345_data_ready() call before
    performing a bulk read
  * Since get_triple() is scrapped, place a direct bulk read here
  * Move mutex unlocking below goto label
* Remove i2c_check_functionality() that could introduce regression

Eva Rachel Retuya (4):
  dt-bindings: iio: accel: adxl345: Add optional interrupt-names support
  iio: accel: adxl345_core: Introduce set_mode and data_ready functions
  iio: accel: adxl345: Setup DATA_READY trigger
  iio: accel: adxl345: Add support for triggered buffer

 .../devicetree/bindings/iio/accel/adxl345.txt      |   4 +
 drivers/iio/accel/Kconfig                          |   2 +
 drivers/iio/accel/adxl345.h                        |   2 +-
 drivers/iio/accel/adxl345_core.c                   | 281 ++++++++++++++++++++-
 drivers/iio/accel/adxl345_i2c.c                    |   3 +-
 drivers/iio/accel/adxl345_spi.c                    |   2 +-
 6 files changed, 278 insertions(+), 16 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1633271 — [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and data_ready functions

Move code that enables measurement/standby mode into its own function
and call that function when appropriate. Previously, we set the sensor
to measurement in probe and back to standby during remove. Change it
here to only enter measurement mode when request for data is initiated.

The DATA_READY bit is always set if the corresponding event occurs.
Introduce the data_ready function that monitors the INT_SOURCE register
of this bit. This is done to ensure consistent readings.

Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
---
Changes in v2:
* Make function naming more clear: drdy -> data_ready
  * Switch from while to do..while
  * Rename regval to val
  * Switch to usleep_range() for shorter delay
  * Add comment to justify delay
  * Change error code -EIO to -EAGAIN
* Endianness issue: scrap the get_triple function entirely, make no
  changes in terms of reading registers in read_raw
* Introduce mutex here rather than in succeeding patch
* Probe:
  * Fix random whitespace omission
  * Remove configuring to standby mode, the device powers on in standby
    mode so this is not needed

 drivers/iio/accel/adxl345_core.c | 84 +++++++++++++++++++++++++++++++++-------
 1 file changed, 69 insertions(+), 15 deletions(-)

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 9ccb582..b8a212c 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -8,6 +8,7 @@
  * directory of this archive for more details.
  */
 
+#include <linux/delay.h>
 #include <linux/module.h>
 #include <linux/regmap.h>
 
@@ -17,6 +18,7 @@
 
 #define ADXL345_REG_DEVID		0x00
 #define ADXL345_REG_POWER_CTL		0x2D
+#define ADXL345_REG_INT_SOURCE		0x30
 #define ADXL345_REG_DATA_FORMAT		0x31
 #define ADXL345_REG_DATAX0		0x32
 #define ADXL345_REG_DATAY0		0x34
@@ -25,6 +27,10 @@
 #define ADXL345_POWER_CTL_MEASURE	BIT(3)
 #define ADXL345_POWER_CTL_STANDBY	0x00
 
+/* INT_ENABLE/INT_MAP/INT_SOURCE bits */
+#define ADXL345_INT_DATA_READY		BIT(7)
+#define ADXL345_INT_OVERRUN		0
+
 #define ADXL345_DATA_FORMAT_FULL_RES	BIT(3) /* Up to 13-bits resolution */
 #define ADXL345_DATA_FORMAT_2G		0
 #define ADXL345_DATA_FORMAT_4G		1
@@ -44,9 +50,49 @@ static const int adxl345_uscale = 38300;
 
 struct adxl345_data {
 	struct regmap *regmap;
+	struct mutex lock; /* protect this data structure */
 	u8 data_range;
 };
 
+static int adxl345_set_mode(struct adxl345_data *data, u8 mode)
+{
+	struct device *dev = regmap_get_device(data->regmap);
+	int ret;
+
+	ret = regmap_write(data->regmap, ADXL345_REG_POWER_CTL, mode);
+	if (ret < 0) {
+		dev_err(dev, "Failed to set power mode, %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int adxl345_data_ready(struct adxl345_data *data)
+{
+	struct device *dev = regmap_get_device(data->regmap);
+	int tries = 5;
+	u32 val;
+	int ret;
+
+	do {
+		/*
+		 * 1/ODR + 1.1ms; 11.1ms at ODR of 0.10 Hz
+		 * Sensor currently operates at default ODR of 100 Hz
+		 */
+		usleep_range(1100, 11100);
+
+		ret = regmap_read(data->regmap, ADXL345_REG_INT_SOURCE, &val);
+		if (ret < 0)
+			return ret;
+		if ((val & ADXL345_INT_DATA_READY) == ADXL345_INT_DATA_READY)
+			return 0;
+	} while (--tries);
+	dev_err(dev, "Data is not yet ready, try again.\n");
+
+	return -EAGAIN;
+}
+
 #define ADXL345_CHANNEL(reg, axis) {					\
 	.type = IIO_ACCEL,						\
 	.modified = 1,							\
@@ -72,6 +118,19 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&data->lock);
+		ret = adxl345_set_mode(data, ADXL345_POWER_CTL_MEASURE);
+		if (ret < 0) {
+			mutex_unlock(&data->lock);
+			return ret;
+		}
+
+		ret = adxl345_data_ready(data);
+		if (ret < 0) {
+			adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
+			mutex_unlock(&data->lock);
+			return ret;
+		}
 		/*
 		 * Data is stored in adjacent registers:
 		 * ADXL345_REG_DATA(X0/Y0/Z0) contain the least significant byte
@@ -79,10 +138,15 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
 		 */
 		ret = regmap_bulk_read(data->regmap, chan->address, &regval,
 				       sizeof(regval));
-		if (ret < 0)
+		mutex_unlock(&data->lock);
+		if (ret < 0) {
+			adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
 			return ret;
+		}
 
 		*val = sign_extend32(le16_to_cpu(regval), 12);
+		adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
+
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
 		*val = 0;
@@ -136,6 +200,8 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 		return ret;
 	}
 
+	mutex_init(&data->lock);
+
 	indio_dev->dev.parent = dev;
 	indio_dev->name = name;
 	indio_dev->info = &adxl345_info;
@@ -143,20 +209,9 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	indio_dev->channels = adxl345_channels;
 	indio_dev->num_channels = ARRAY_SIZE(adxl345_channels);
 
-	/* Enable measurement mode */
-	ret = regmap_write(data->regmap, ADXL345_REG_POWER_CTL,
-			   ADXL345_POWER_CTL_MEASURE);
-	if (ret < 0) {
-		dev_err(dev, "Failed to enable measurement mode: %d\n", ret);
-		return ret;
-	}
-
 	ret = iio_device_register(indio_dev);
-	if (ret < 0) {
+	if (ret < 0)
 		dev_err(dev, "iio_device_register failed: %d\n", ret);
-		regmap_write(data->regmap, ADXL345_REG_POWER_CTL,
-			     ADXL345_POWER_CTL_STANDBY);
-	}
 
 	return ret;
 }
@@ -169,8 +224,7 @@ int adxl345_core_remove(struct device *dev)
 
 	iio_device_unregister(indio_dev);
 
-	return regmap_write(data->regmap, ADXL345_REG_POWER_CTL,
-			    ADXL345_POWER_CTL_STANDBY);
+	return adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
 }
 EXPORT_SYMBOL_GPL(adxl345_core_remove);
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1633514 — Re: [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and data_ready functions

On 29/04/17 08:48, Eva Rachel Retuya wrote:
> Move code that enables measurement/standby mode into its own function
> and call that function when appropriate. Previously, we set the sensor
> to measurement in probe and back to standby during remove. Change it
> here to only enter measurement mode when request for data is initiated.
> 
> The DATA_READY bit is always set if the corresponding event occurs.
> Introduce the data_ready function that monitors the INT_SOURCE register
> of this bit. This is done to ensure consistent readings.
> 
> Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
> ---
> Changes in v2:
> * Make function naming more clear: drdy -> data_ready
>   * Switch from while to do..while
>   * Rename regval to val
>   * Switch to usleep_range() for shorter delay
>   * Add comment to justify delay
>   * Change error code -EIO to -EAGAIN
> * Endianness issue: scrap the get_triple function entirely, make no
>   changes in terms of reading registers in read_raw
> * Introduce mutex here rather than in succeeding patch
> * Probe:
>   * Fix random whitespace omission
>   * Remove configuring to standby mode, the device powers on in standby
>     mode so this is not needed
> 
>  drivers/iio/accel/adxl345_core.c | 84 +++++++++++++++++++++++++++++++++-------
>  1 file changed, 69 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 9ccb582..b8a212c 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -8,6 +8,7 @@
>   * directory of this archive for more details.
>   */
>  
> +#include <linux/delay.h>
>  #include <linux/module.h>
>  #include <linux/regmap.h>
>  
> @@ -17,6 +18,7 @@
>  
>  #define ADXL345_REG_DEVID		0x00
>  #define ADXL345_REG_POWER_CTL		0x2D
> +#define ADXL345_REG_INT_SOURCE		0x30
>  #define ADXL345_REG_DATA_FORMAT		0x31
>  #define ADXL345_REG_DATAX0		0x32
>  #define ADXL345_REG_DATAY0		0x34
> @@ -25,6 +27,10 @@
>  #define ADXL345_POWER_CTL_MEASURE	BIT(3)
>  #define ADXL345_POWER_CTL_STANDBY	0x00
>  
> +/* INT_ENABLE/INT_MAP/INT_SOURCE bits */
> +#define ADXL345_INT_DATA_READY		BIT(7)
> +#define ADXL345_INT_OVERRUN		0
> +
>  #define ADXL345_DATA_FORMAT_FULL_RES	BIT(3) /* Up to 13-bits resolution */
>  #define ADXL345_DATA_FORMAT_2G		0
>  #define ADXL345_DATA_FORMAT_4G		1
> @@ -44,9 +50,49 @@ static const int adxl345_uscale = 38300;
>  
>  struct adxl345_data {
>  	struct regmap *regmap;
> +	struct mutex lock; /* protect this data structure */
>  	u8 data_range;
>  };
>  
> +static int adxl345_set_mode(struct adxl345_data *data, u8 mode)
> +{
> +	struct device *dev = regmap_get_device(data->regmap);
> +	int ret;
> +
> +	ret = regmap_write(data->regmap, ADXL345_REG_POWER_CTL, mode);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to set power mode, %d\n", ret);
> +		return ret;
drop the return ret here and just return ret at the end of the function.
One of the static checkers will probably moan about this otherwise.
> +	}
> +
> +	return 0;
> +}
> +
> +static int adxl345_data_ready(struct adxl345_data *data)
> +{
So this is a polling the dataready bit.  Will ensure we always
get fresh data when a read occurs.  Please add a comment to
that effect as that's not always how devices work.
> +	struct device *dev = regmap_get_device(data->regmap);
> +	int tries = 5;
> +	u32 val;
> +	int ret;
> +
> +	do {
> +		/*
> +		 * 1/ODR + 1.1ms; 11.1ms at ODR of 0.10 Hz
> +		 * Sensor currently operates at default ODR of 100 Hz
> +		 */
> +		usleep_range(1100, 11100);
That's a huge range to allow... I'm not following the argument for why.
Or do we have a stray 0?

> +
> +		ret = regmap_read(data->regmap, ADXL345_REG_INT_SOURCE, &val);
> +		if (ret < 0)
> +			return ret;
> +		if ((val & ADXL345_INT_DATA_READY) == ADXL345_INT_DATA_READY)
> +			return 0;
> +	} while (--tries);
> +	dev_err(dev, "Data is not yet ready, try again.\n");
> +
This is almost certainly a hardware fault. I'd be more brutal with
the error and return -EIO.  If you get here your hardware is very unlikely
to be working correctly if you try again.
> +	return -EAGAIN;
> +}
> +
>  #define ADXL345_CHANNEL(reg, axis) {					\
>  	.type = IIO_ACCEL,						\
>  	.modified = 1,							\
> @@ -72,6 +118,19 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&data->lock);
> +		ret = adxl345_set_mode(data, ADXL345_POWER_CTL_MEASURE);
> +		if (ret < 0) {
> +			mutex_unlock(&data->lock);
> +			return ret;
> +		}
> +
> +		ret = adxl345_data_ready(data);
> +		if (ret < 0) {
> +			adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
> +			mutex_unlock(&data->lock);
What is the logic that puts the mutex_unlock here in the error case
and before the set_mode in the normal path?  Even if it doesn't
matter make them the same as it is less likely to raise questions
in the future!
> +			return ret;
> +		}
>  		/*
>  		 * Data is stored in adjacent registers:
>  		 * ADXL345_REG_DATA(X0/Y0/Z0) contain the least significant byte
> @@ -79,10 +138,15 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
>  		 */
>  		ret = regmap_bulk_read(data->regmap, chan->address, &regval,
>  				       sizeof(regval));
> -		if (ret < 0)
> +		mutex_unlock(&data->lock);
> +		if (ret < 0) {
> +			adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
>  			return ret;
> +		}
>  
>  		*val = sign_extend32(le16_to_cpu(regval), 12);
> +		adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
> +
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_SCALE:
>  		*val = 0;
> @@ -136,6 +200,8 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  		return ret;
>  	}
>  
> +	mutex_init(&data->lock);
> +
>  	indio_dev->dev.parent = dev;
>  	indio_dev->name = name;
>  	indio_dev->info = &adxl345_info;
> @@ -143,20 +209,9 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  	indio_dev->channels = adxl345_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(adxl345_channels);
>  
> -	/* Enable measurement mode */
> -	ret = regmap_write(data->regmap, ADXL345_REG_POWER_CTL,
> -			   ADXL345_POWER_CTL_MEASURE);
> -	if (ret < 0) {
> -		dev_err(dev, "Failed to enable measurement mode: %d\n", ret);
> -		return ret;
> -	}
> -
>  	ret = iio_device_register(indio_dev);
> -	if (ret < 0) {
> +	if (ret < 0)
>  		dev_err(dev, "iio_device_register failed: %d\n", ret);
> -		regmap_write(data->regmap, ADXL345_REG_POWER_CTL,
> -			     ADXL345_POWER_CTL_STANDBY);
> -	}
>  
>  	return ret;
>  }
> @@ -169,8 +224,7 @@ int adxl345_core_remove(struct device *dev)
>  
>  	iio_device_unregister(indio_dev);
>  
> -	return regmap_write(data->regmap, ADXL345_REG_POWER_CTL,
> -			    ADXL345_POWER_CTL_STANDBY);
> +	return adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
Under what circumstances would we not already be in the correct state?
A brief comment here would be good.
>  }
>  EXPORT_SYMBOL_GPL(adxl345_core_remove);
>  
> 

[toc] | [prev] | [next] | [standalone]

#1633837 — Re: [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and data_ready functions

On Mon, May 1, 2017 at 3:22 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 29/04/17 08:48, Eva Rachel Retuya wrote:

>> +static int adxl345_set_mode(struct adxl345_data *data, u8 mode)
>> +{
>> +     struct device *dev = regmap_get_device(data->regmap);
>> +     int ret;
>> +
>> +     ret = regmap_write(data->regmap, ADXL345_REG_POWER_CTL, mode);
>> +     if (ret < 0) {
>> +             dev_err(dev, "Failed to set power mode, %d\n", ret);
>> +             return ret;
> drop the return ret here and just return ret at the end of the function.
> One of the static checkers will probably moan about this otherwise.

But this will be not equivalent!

>> +     }
>> +
>> +     return 0;
>> +}

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1633839 — Re: [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and data_ready functions

On 1 May 2017 20:42:08 BST, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>On Mon, May 1, 2017 at 3:22 AM, Jonathan Cameron <jic23@kernel.org>
>wrote:
>> On 29/04/17 08:48, Eva Rachel Retuya wrote:
>
>>> +static int adxl345_set_mode(struct adxl345_data *data, u8 mode)
>>> +{
>>> +     struct device *dev = regmap_get_device(data->regmap);
>>> +     int ret;
>>> +
>>> +     ret = regmap_write(data->regmap, ADXL345_REG_POWER_CTL, mode);
>>> +     if (ret < 0) {
>>> +             dev_err(dev, "Failed to set power mode, %d\n", ret);
>>> +             return ret;
>> drop the return ret here and just return ret at the end of the
>function.
>> One of the static checkers will probably moan about this otherwise.
>
>But this will be not equivalent!
Why? Regmap_write returns 0 for success.
>
>>> +     }
>>> +
>>> +     return 0;
>>> +}

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[toc] | [prev] | [next] | [standalone]

#1633846 — Re: [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and data_ready functions

On Mon, May 1, 2017 at 10:48 PM, Jonathan Cameron
<jic23@jic23.retrosnub.co.uk> wrote:
> On 1 May 2017 20:42:08 BST, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>On Mon, May 1, 2017 at 3:22 AM, Jonathan Cameron <jic23@kernel.org>
>>wrote:
>>> On 29/04/17 08:48, Eva Rachel Retuya wrote:
>>
>>>> +static int adxl345_set_mode(struct adxl345_data *data, u8 mode)
>>>> +{
>>>> +     struct device *dev = regmap_get_device(data->regmap);
>>>> +     int ret;
>>>> +
>>>> +     ret = regmap_write(data->regmap, ADXL345_REG_POWER_CTL, mode);
>>>> +     if (ret < 0) {
>>>> +             dev_err(dev, "Failed to set power mode, %d\n", ret);
>>>> +             return ret;
>>> drop the return ret here and just return ret at the end of the
>>function.
>>> One of the static checkers will probably moan about this otherwise.
>>
>>But this will be not equivalent!
> Why? Regmap_write returns 0 for success.

If there is a qurantee that regmap_write() doesn't return any positive
values, I'm okay with replacement.

>>
>>>> +     }
>>>> +
>>>> +     return 0;
>>>> +}

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1633852 — Re: [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and data_ready functions

On 1 May 2017 21:07:44 BST, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>On Mon, May 1, 2017 at 10:48 PM, Jonathan Cameron
><jic23@jic23.retrosnub.co.uk> wrote:
>> On 1 May 2017 20:42:08 BST, Andy Shevchenko
><andy.shevchenko@gmail.com> wrote:
>>>On Mon, May 1, 2017 at 3:22 AM, Jonathan Cameron <jic23@kernel.org>
>>>wrote:
>>>> On 29/04/17 08:48, Eva Rachel Retuya wrote:
>>>
>>>>> +static int adxl345_set_mode(struct adxl345_data *data, u8 mode)
>>>>> +{
>>>>> +     struct device *dev = regmap_get_device(data->regmap);
>>>>> +     int ret;
>>>>> +
>>>>> +     ret = regmap_write(data->regmap, ADXL345_REG_POWER_CTL,
>mode);
>>>>> +     if (ret < 0) {
>>>>> +             dev_err(dev, "Failed to set power mode, %d\n", ret);
>>>>> +             return ret;
>>>> drop the return ret here and just return ret at the end of the
>>>function.
>>>> One of the static checkers will probably moan about this otherwise.
>>>
>>>But this will be not equivalent!
>> Why? Regmap_write returns 0 for success.
>
>If there is a qurantee that regmap_write() doesn't return any positive
>values, I'm okay with replacement.
Kernel doc says so.

>http://lxr.free-electrons.com/source/drivers/base/regmap/regmap.c#L1693
>>>
>>>>> +     }
>>>>> +
>>>>> +     return 0;
>>>>> +}

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[toc] | [prev] | [next] | [standalone]

#1634356 — Re: [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and data_ready functions

On Mon, May 01, 2017 at 01:22:52AM +0100, Jonathan Cameron wrote:
Hello Jonathan,
[...]
> > +static int adxl345_set_mode(struct adxl345_data *data, u8 mode)
> > +{
> > +	struct device *dev = regmap_get_device(data->regmap);
> > +	int ret;
> > +
> > +	ret = regmap_write(data->regmap, ADXL345_REG_POWER_CTL, mode);
> > +	if (ret < 0) {
> > +		dev_err(dev, "Failed to set power mode, %d\n", ret);
> > +		return ret;
> drop the return ret here and just return ret at the end of the function.
> One of the static checkers will probably moan about this otherwise.

OK.

> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int adxl345_data_ready(struct adxl345_data *data)
> > +{
> So this is a polling the dataready bit.  Will ensure we always
> get fresh data when a read occurs.  Please add a comment to
> that effect as that's not always how devices work.

OK.

> > +	struct device *dev = regmap_get_device(data->regmap);
> > +	int tries = 5;
> > +	u32 val;
> > +	int ret;
> > +
> > +	do {
> > +		/*
> > +		 * 1/ODR + 1.1ms; 11.1ms at ODR of 0.10 Hz
> > +		 * Sensor currently operates at default ODR of 100 Hz
> > +		 */
> > +		usleep_range(1100, 11100);
> That's a huge range to allow... I'm not following the argument for why.
> Or do we have a stray 0?
> 

Not a stray 0. Range is from 1.1ms to 11.1ms, this represents the
wake-up time when going to standby/other power saving modes ->
measurement mode. I'm going to clarify the comment on why it is needed
on the next revision.

> > +
> > +		ret = regmap_read(data->regmap, ADXL345_REG_INT_SOURCE, &val);
> > +		if (ret < 0)
> > +			return ret;
> > +		if ((val & ADXL345_INT_DATA_READY) == ADXL345_INT_DATA_READY)
> > +			return 0;
> > +	} while (--tries);
> > +	dev_err(dev, "Data is not yet ready, try again.\n");
> > +
> This is almost certainly a hardware fault. I'd be more brutal with
> the error and return -EIO.  If you get here your hardware is very unlikely
> to be working correctly if you try again.

OK, will change it to -EIO then.

> > +	return -EAGAIN;
> > +}
> > +
> >  #define ADXL345_CHANNEL(reg, axis) {					\
> >  	.type = IIO_ACCEL,						\
> >  	.modified = 1,							\
> > @@ -72,6 +118,19 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
> >  
> >  	switch (mask) {
> >  	case IIO_CHAN_INFO_RAW:
> > +		mutex_lock(&data->lock);
> > +		ret = adxl345_set_mode(data, ADXL345_POWER_CTL_MEASURE);
> > +		if (ret < 0) {
> > +			mutex_unlock(&data->lock);
> > +			return ret;
> > +		}
> > +
> > +		ret = adxl345_data_ready(data);
> > +		if (ret < 0) {
> > +			adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
> > +			mutex_unlock(&data->lock);
> What is the logic that puts the mutex_unlock here in the error case
> and before the set_mode in the normal path?  Even if it doesn't
> matter make them the same as it is less likely to raise questions
> in the future!

OK, will make it consistent.

> > +			return ret;
> > +		}
> >  		/*
> >  		 * Data is stored in adjacent registers:
> >  		 * ADXL345_REG_DATA(X0/Y0/Z0) contain the least significant byte
> > @@ -79,10 +138,15 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
> >  		 */
> >  		ret = regmap_bulk_read(data->regmap, chan->address, &regval,
> >  				       sizeof(regval));
> > -		if (ret < 0)
> > +		mutex_unlock(&data->lock);
> > +		if (ret < 0) {
> > +			adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
> >  			return ret;
> > +		}
> >  
> >  		*val = sign_extend32(le16_to_cpu(regval), 12);
> > +		adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
> > +
> >  		return IIO_VAL_INT;
> >  	case IIO_CHAN_INFO_SCALE:
> >  		*val = 0;
[...]
> > @@ -169,8 +224,7 @@ int adxl345_core_remove(struct device *dev)
> >  
> >  	iio_device_unregister(indio_dev);
> >  
> > -	return regmap_write(data->regmap, ADXL345_REG_POWER_CTL,
> > -			    ADXL345_POWER_CTL_STANDBY);
> > +	return adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
> Under what circumstances would we not already be in the correct state?
> A brief comment here would be good.

I'm leaving this unremoved to catch cases where in the sensor fails to
return to standby mode after a read. Will add the said comment.

Thanks,
Eva

> >  }
> >  EXPORT_SYMBOL_GPL(adxl345_core_remove);
> >  
> > 
> 

[toc] | [prev] | [next] | [standalone]

#1634520 — Re: [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and data_ready functions

On 02/05/17 12:39, Eva Rachel Retuya wrote:
> On Mon, May 01, 2017 at 01:22:52AM +0100, Jonathan Cameron wrote:
> Hello Jonathan,
> [...]
>>> +static int adxl345_set_mode(struct adxl345_data *data, u8 mode)
>>> +{
>>> +	struct device *dev = regmap_get_device(data->regmap);
>>> +	int ret;
>>> +
>>> +	ret = regmap_write(data->regmap, ADXL345_REG_POWER_CTL, mode);
>>> +	if (ret < 0) {
>>> +		dev_err(dev, "Failed to set power mode, %d\n", ret);
>>> +		return ret;
>> drop the return ret here and just return ret at the end of the function.
>> One of the static checkers will probably moan about this otherwise.
> 
> OK.
> 
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int adxl345_data_ready(struct adxl345_data *data)
>>> +{
>> So this is a polling the dataready bit.  Will ensure we always
>> get fresh data when a read occurs.  Please add a comment to
>> that effect as that's not always how devices work.
> 
> OK.
> 
>>> +	struct device *dev = regmap_get_device(data->regmap);
>>> +	int tries = 5;
>>> +	u32 val;
>>> +	int ret;
>>> +
>>> +	do {
>>> +		/*
>>> +		 * 1/ODR + 1.1ms; 11.1ms at ODR of 0.10 Hz
>>> +		 * Sensor currently operates at default ODR of 100 Hz
>>> +		 */
>>> +		usleep_range(1100, 11100);
>> That's a huge range to allow... I'm not following the argument for why.
>> Or do we have a stray 0?
>>
> 
> Not a stray 0. Range is from 1.1ms to 11.1ms, this represents the
> wake-up time when going to standby/other power saving modes ->
> measurement mode. I'm going to clarify the comment on why it is needed
> on the next revision.
The point about a range sleep is to allow the kernel flexibility in scheduling
so as to avoid waking the processor from lower power states when high precision is
not needed.

If the thing you are talking about needs the maximum time sometimes then you
should set the minimum value to that and add a bit to avoid unnecessary processor
wake ups.
> 
>>> +
>>> +		ret = regmap_read(data->regmap, ADXL345_REG_INT_SOURCE, &val);
>>> +		if (ret < 0)
>>> +			return ret;
>>> +		if ((val & ADXL345_INT_DATA_READY) == ADXL345_INT_DATA_READY)
>>> +			return 0;
>>> +	} while (--tries);
>>> +	dev_err(dev, "Data is not yet ready, try again.\n");
>>> +
>> This is almost certainly a hardware fault. I'd be more brutal with
>> the error and return -EIO.  If you get here your hardware is very unlikely
>> to be working correctly if you try again.
> 
> OK, will change it to -EIO then.
> 
>>> +	return -EAGAIN;
>>> +}
>>> +
>>>  #define ADXL345_CHANNEL(reg, axis) {					\
>>>  	.type = IIO_ACCEL,						\
>>>  	.modified = 1,							\
>>> @@ -72,6 +118,19 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
>>>  
>>>  	switch (mask) {
>>>  	case IIO_CHAN_INFO_RAW:
>>> +		mutex_lock(&data->lock);
>>> +		ret = adxl345_set_mode(data, ADXL345_POWER_CTL_MEASURE);
>>> +		if (ret < 0) {
>>> +			mutex_unlock(&data->lock);
>>> +			return ret;
>>> +		}
>>> +
>>> +		ret = adxl345_data_ready(data);
>>> +		if (ret < 0) {
>>> +			adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
>>> +			mutex_unlock(&data->lock);
>> What is the logic that puts the mutex_unlock here in the error case
>> and before the set_mode in the normal path?  Even if it doesn't
>> matter make them the same as it is less likely to raise questions
>> in the future!
> 
> OK, will make it consistent.
> 
>>> +			return ret;
>>> +		}
>>>  		/*
>>>  		 * Data is stored in adjacent registers:
>>>  		 * ADXL345_REG_DATA(X0/Y0/Z0) contain the least significant byte
>>> @@ -79,10 +138,15 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
>>>  		 */
>>>  		ret = regmap_bulk_read(data->regmap, chan->address, &regval,
>>>  				       sizeof(regval));
>>> -		if (ret < 0)
>>> +		mutex_unlock(&data->lock);
>>> +		if (ret < 0) {
>>> +			adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
>>>  			return ret;
>>> +		}
>>>  
>>>  		*val = sign_extend32(le16_to_cpu(regval), 12);
>>> +		adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
>>> +
>>>  		return IIO_VAL_INT;
>>>  	case IIO_CHAN_INFO_SCALE:
>>>  		*val = 0;
> [...]
>>> @@ -169,8 +224,7 @@ int adxl345_core_remove(struct device *dev)
>>>  
>>>  	iio_device_unregister(indio_dev);
>>>  
>>> -	return regmap_write(data->regmap, ADXL345_REG_POWER_CTL,
>>> -			    ADXL345_POWER_CTL_STANDBY);
>>> +	return adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
>> Under what circumstances would we not already be in the correct state?
>> A brief comment here would be good.
> 
> I'm leaving this unremoved to catch cases where in the sensor fails to
> return to standby mode after a read. Will add the said comment.
> 
> Thanks,
> Eva
> 
>>>  }
>>>  EXPORT_SYMBOL_GPL(adxl345_core_remove);
>>>  
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[toc] | [prev] | [next] | [standalone]

#1638800 — Re: [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and data_ready functions

On Tue, May 02, 2017 at 05:32:07PM +0100, Jonathan Cameron wrote:
> On 02/05/17 12:39, Eva Rachel Retuya wrote:
> > On Mon, May 01, 2017 at 01:22:52AM +0100, Jonathan Cameron wrote:
> > Hello Jonathan,
> > [...]
> >>> +static int adxl345_set_mode(struct adxl345_data *data, u8 mode)
> >>> +{
> >>> +	struct device *dev = regmap_get_device(data->regmap);
> >>> +	int ret;
> >>> +
> >>> +	ret = regmap_write(data->regmap, ADXL345_REG_POWER_CTL, mode);
> >>> +	if (ret < 0) {
> >>> +		dev_err(dev, "Failed to set power mode, %d\n", ret);
> >>> +		return ret;
> >> drop the return ret here and just return ret at the end of the function.
> >> One of the static checkers will probably moan about this otherwise.
> > 
> > OK.
> > 
> >>> +	}
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int adxl345_data_ready(struct adxl345_data *data)
> >>> +{
> >> So this is a polling the dataready bit.  Will ensure we always
> >> get fresh data when a read occurs.  Please add a comment to
> >> that effect as that's not always how devices work.
> > 
> > OK.
> > 
> >>> +	struct device *dev = regmap_get_device(data->regmap);
> >>> +	int tries = 5;
> >>> +	u32 val;
> >>> +	int ret;
> >>> +
> >>> +	do {
> >>> +		/*
> >>> +		 * 1/ODR + 1.1ms; 11.1ms at ODR of 0.10 Hz
> >>> +		 * Sensor currently operates at default ODR of 100 Hz
> >>> +		 */
> >>> +		usleep_range(1100, 11100);
> >> That's a huge range to allow... I'm not following the argument for why.
> >> Or do we have a stray 0?
> >>
> > 
> > Not a stray 0. Range is from 1.1ms to 11.1ms, this represents the
> > wake-up time when going to standby/other power saving modes ->
> > measurement mode. I'm going to clarify the comment on why it is needed
> > on the next revision.
> The point about a range sleep is to allow the kernel flexibility in scheduling
> so as to avoid waking the processor from lower power states when high precision is
> not needed.
> 
> If the thing you are talking about needs the maximum time sometimes then you
> should set the minimum value to that and add a bit to avoid unnecessary processor
> wake ups.

Thank you for explaining it. I will keep this in mind while working on
the 3rd revision.

Eva

[toc] | [prev] | [next] | [standalone]

#1633609 — Re: [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and data_ready functions

On Sat, Apr 29, 2017 at 10:48 AM, Eva Rachel Retuya <eraretuya@gmail.com> wrote:
> Move code that enables measurement/standby mode into its own function
> and call that function when appropriate. Previously, we set the sensor
> to measurement in probe and back to standby during remove. Change it
> here to only enter measurement mode when request for data is initiated.
>
> The DATA_READY bit is always set if the corresponding event occurs.
> Introduce the data_ready function that monitors the INT_SOURCE register
> of this bit. This is done to ensure consistent readings.

Couple of minors below.
Otherwise, FWIW:
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> +static int adxl345_data_ready(struct adxl345_data *data)
> +{
> +       struct device *dev = regmap_get_device(data->regmap);

> +       int tries = 5;

unsigned int tries = 5;

> +       u32 val;
> +       int ret;
> +
> +       do {

> +               /*
> +                * 1/ODR + 1.1ms; 11.1ms at ODR of 0.10 Hz
> +                * Sensor currently operates at default ODR of 100 Hz
> +                */
> +               usleep_range(1100, 11100);

From the above comment I can't get why we sleep first then attempt to
read and not otherwise. Please, elaborate.

> +
> +               ret = regmap_read(data->regmap, ADXL345_REG_INT_SOURCE, &val);
> +               if (ret < 0)
> +                       return ret;
> +               if ((val & ADXL345_INT_DATA_READY) == ADXL345_INT_DATA_READY)
> +                       return 0;
> +       } while (--tries);
> +       dev_err(dev, "Data is not yet ready, try again.\n");
> +
> +       return -EAGAIN;
> +}

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1634360 — Re: [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and data_ready functions

On Mon, May 01, 2017 at 02:21:02PM +0300, Andy Shevchenko wrote:
[...]
> Couple of minors below.
> Otherwise, FWIW:
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 

Hello Andy,

> > +static int adxl345_data_ready(struct adxl345_data *data)
> > +{
> > +       struct device *dev = regmap_get_device(data->regmap);
> 
> > +       int tries = 5;
> 
> unsigned int tries = 5;
> 

Ack.

> > +       u32 val;
> > +       int ret;
> > +
> > +       do {
> 
> > +               /*
> > +                * 1/ODR + 1.1ms; 11.1ms at ODR of 0.10 Hz
> > +                * Sensor currently operates at default ODR of 100 Hz
> > +                */
> > +               usleep_range(1100, 11100);
> 
> From the above comment I can't get why we sleep first then attempt to
> read and not otherwise. Please, elaborate.
> 

Before this function is called, the sensor is placed into measurement
mode. The delay is there to account for the "wake-up" time from standby
to measurement mode. So sleep first then attempt to read. In the first
version I was not yet aware of it until I look around in the datasheet
trying to justify how long to delay. I will clarify the comment to make
it more clear.

Thanks,
Eva

> > +
> > +               ret = regmap_read(data->regmap, ADXL345_REG_INT_SOURCE, &val);
> > +               if (ret < 0)
> > +                       return ret;
> > +               if ((val & ADXL345_INT_DATA_READY) == ADXL345_INT_DATA_READY)
> > +                       return 0;
> > +       } while (--tries);
> > +       dev_err(dev, "Data is not yet ready, try again.\n");
> > +
> > +       return -EAGAIN;
> > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1633275 — [PATCH v2 4/4] iio: accel: adxl345: Add support for triggered buffer

Previously, the only way to capture data is to read the exposed sysfs
files in_accel_[x/y/z]_raw and applying the scale from in_accel_scale.
Provide a way for continuous data capture that allows multiple data
channels to be read at once by setting up buffer support.

Initialize scan_type fields that describe the buffer and make sure to
claim and release direct mode in read_raw. The latter is done to ensure
no raw reads are attempted when utilizing the buffer and vice versa.

Lastly, add triggered buffer support that allows utilization of any
given trigger such as DATA_READY, hrtimer, etc. to initiate capturing of
data and placing said data in the buffer. The trigger handler performs
an all-axes read with timestamp.

Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
---
Changes in v2:
* Provide a more detailed commit message
* adxl345_trigger_handler()
  * if using external trigger, place a adxl345_data_ready() call before
    performing a bulk read
  * Since get_triple() is scrapped, place a direct bulk read here
  * Move mutex unlocking below goto label
* Switch to devm_iio_triggered_buffer_setup()
* Remove i2c_check_functionality() that could introduce regression

 drivers/iio/accel/Kconfig        |   2 +
 drivers/iio/accel/adxl345_core.c | 101 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 102 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
index 15de262..45092da 100644
--- a/drivers/iio/accel/Kconfig
+++ b/drivers/iio/accel/Kconfig
@@ -7,6 +7,8 @@ menu "Accelerometers"
 
 config ADXL345
 	tristate
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
 
 config ADXL345_I2C
 	tristate "Analog Devices ADXL345 3-Axis Digital Accelerometer I2C Driver"
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index b8be0d7..40dbdce 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -14,8 +14,11 @@
 #include <linux/of_irq.h>
 #include <linux/regmap.h>
 
+#include <linux/iio/buffer.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/trigger.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
 
 #include "adxl345.h"
 
@@ -55,12 +58,20 @@
  */
 static const int adxl345_uscale = 38300;
 
+enum adxl345_scan_index {
+	ADXL345_IDX_X,
+	ADXL345_IDX_Y,
+	ADXL345_IDX_Z,
+	ADXL345_IDX_TSTAMP,
+};
+
 struct adxl345_data {
 	struct iio_trigger *data_ready_trig;
 	bool data_ready_trig_on;
 	struct regmap *regmap;
 	struct mutex lock; /* protect this data structure */
 	u8 data_range;
+	s16 buffer[8]; /* 3 x 16-bit channels + padding + 64-bit timestamp */
 };
 
 static int adxl345_set_mode(struct adxl345_data *data, u8 mode)
@@ -109,12 +120,25 @@ static int adxl345_data_ready(struct adxl345_data *data)
 	.address = reg,							\
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
 	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
+	.scan_index = ADXL345_IDX_##axis,				\
+	.scan_type = {							\
+		.sign = 's',						\
+		.realbits = 13,						\
+		.storagebits = 16,					\
+		.endianness = IIO_LE,					\
+	},								\
 }
 
 static const struct iio_chan_spec adxl345_channels[] = {
 	ADXL345_CHANNEL(ADXL345_REG_DATAX0, X),
 	ADXL345_CHANNEL(ADXL345_REG_DATAY0, Y),
 	ADXL345_CHANNEL(ADXL345_REG_DATAZ0, Z),
+	IIO_CHAN_SOFT_TIMESTAMP(ADXL345_IDX_TSTAMP),
+};
+
+static const unsigned long adxl345_scan_masks[] = {
+	BIT(ADXL345_IDX_X) | BIT(ADXL345_IDX_Y) | BIT(ADXL345_IDX_Z),
+	0
 };
 
 static int adxl345_read_raw(struct iio_dev *indio_dev,
@@ -127,6 +151,10 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+
 		mutex_lock(&data->lock);
 		ret = adxl345_set_mode(data, ADXL345_POWER_CTL_MEASURE);
 		if (ret < 0) {
@@ -148,12 +176,14 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
 		ret = regmap_bulk_read(data->regmap, chan->address, &regval,
 				       sizeof(regval));
 		mutex_unlock(&data->lock);
+		iio_device_release_direct_mode(indio_dev);
 		if (ret < 0) {
 			adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
 			return ret;
 		}
 
-		*val = sign_extend32(le16_to_cpu(regval), 12);
+		*val = sign_extend32(le16_to_cpu(regval),
+				     chan->scan_type.realbits - 1);
 		adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
 
 		return IIO_VAL_INT;
@@ -186,6 +216,64 @@ static irqreturn_t adxl345_irq(int irq, void *p)
 	return IRQ_NONE;
 }
 
+static irqreturn_t adxl345_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct adxl345_data *data = iio_priv(indio_dev);
+	int ret;
+
+	mutex_lock(&data->lock);
+	/* Make sure data is ready when using external trigger */
+	if (!data->data_ready_trig_on) {
+		ret = adxl345_data_ready(data);
+		if (ret < 0)
+			goto error;
+	}
+
+	ret = regmap_bulk_read(data->regmap, ADXL345_REG_DATAX0, data->buffer,
+			       sizeof(__le16) * 3);
+	if (ret < 0)
+		goto error;
+
+	iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
+					   pf->timestamp);
+error:
+	mutex_unlock(&data->lock);
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static int adxl345_triggered_buffer_postenable(struct iio_dev *indio_dev)
+{
+	struct adxl345_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = iio_triggered_buffer_postenable(indio_dev);
+	if (ret)
+		return ret;
+
+	return adxl345_set_mode(data, ADXL345_POWER_CTL_MEASURE);
+}
+
+static int adxl345_triggered_buffer_predisable(struct iio_dev *indio_dev)
+{
+	struct adxl345_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
+	if (ret)
+		return ret;
+
+	return iio_triggered_buffer_predisable(indio_dev);
+}
+
+static const struct iio_buffer_setup_ops adxl345_buffer_setup_ops = {
+	.postenable = adxl345_triggered_buffer_postenable,
+	.predisable = adxl345_triggered_buffer_predisable,
+};
+
 static int adxl345_drdy_trigger_set_state(struct iio_trigger *trig, bool state)
 {
 	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
@@ -278,6 +366,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap, int irq,
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->channels = adxl345_channels;
 	indio_dev->num_channels = ARRAY_SIZE(adxl345_channels);
+	indio_dev->available_scan_masks = adxl345_scan_masks;
 
 	if (irq > 0) {
 		ret = devm_request_threaded_irq(dev,
@@ -311,6 +400,16 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap, int irq,
 		}
 	}
 
+	ret = devm_iio_triggered_buffer_setup(dev,
+					      indio_dev,
+					      iio_pollfunc_store_time,
+					      adxl345_trigger_handler,
+					      &adxl345_buffer_setup_ops);
+	if (ret < 0) {
+		dev_err(dev, "iio_triggered_buffer_setup failed: %d\n", ret);
+		return ret;
+	}
+
 	ret = iio_device_register(indio_dev);
 	if (ret < 0)
 		dev_err(dev, "iio_device_register failed: %d\n", ret);
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1633516 — Re: [PATCH v2 4/4] iio: accel: adxl345: Add support for triggered buffer

On 29/04/17 08:49, Eva Rachel Retuya wrote:
> Previously, the only way to capture data is to read the exposed sysfs
> files in_accel_[x/y/z]_raw and applying the scale from in_accel_scale.
> Provide a way for continuous data capture that allows multiple data
> channels to be read at once by setting up buffer support.
> 
> Initialize scan_type fields that describe the buffer and make sure to
> claim and release direct mode in read_raw. The latter is done to ensure
> no raw reads are attempted when utilizing the buffer and vice versa.
> 
> Lastly, add triggered buffer support that allows utilization of any
> given trigger such as DATA_READY, hrtimer, etc. to initiate capturing of
> data and placing said data in the buffer. The trigger handler performs
> an all-axes read with timestamp.
> 
> Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
Few minor bits inline...  I'm a little bit in two minds about the 
holding up waiting for new data when using another trigger...

Jonathan
> ---
> Changes in v2:
> * Provide a more detailed commit message
> * adxl345_trigger_handler()
>   * if using external trigger, place a adxl345_data_ready() call before
>     performing a bulk read
>   * Since get_triple() is scrapped, place a direct bulk read here
>   * Move mutex unlocking below goto label
> * Switch to devm_iio_triggered_buffer_setup()
> * Remove i2c_check_functionality() that could introduce regression
> 
>  drivers/iio/accel/Kconfig        |   2 +
>  drivers/iio/accel/adxl345_core.c | 101 ++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 102 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> index 15de262..45092da 100644
> --- a/drivers/iio/accel/Kconfig
> +++ b/drivers/iio/accel/Kconfig
> @@ -7,6 +7,8 @@ menu "Accelerometers"
>  
>  config ADXL345
>  	tristate
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
>  
>  config ADXL345_I2C
>  	tristate "Analog Devices ADXL345 3-Axis Digital Accelerometer I2C Driver"
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index b8be0d7..40dbdce 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -14,8 +14,11 @@
>  #include <linux/of_irq.h>
>  #include <linux/regmap.h>
>  
> +#include <linux/iio/buffer.h>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/trigger.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
>  
>  #include "adxl345.h"
>  
> @@ -55,12 +58,20 @@
>   */
>  static const int adxl345_uscale = 38300;
>  
> +enum adxl345_scan_index {
> +	ADXL345_IDX_X,
> +	ADXL345_IDX_Y,
> +	ADXL345_IDX_Z,
> +	ADXL345_IDX_TSTAMP,
> +};
> +
>  struct adxl345_data {
>  	struct iio_trigger *data_ready_trig;
>  	bool data_ready_trig_on;
>  	struct regmap *regmap;
>  	struct mutex lock; /* protect this data structure */
>  	u8 data_range;
> +	s16 buffer[8]; /* 3 x 16-bit channels + padding + 64-bit timestamp */
>  };
>  
>  static int adxl345_set_mode(struct adxl345_data *data, u8 mode)
> @@ -109,12 +120,25 @@ static int adxl345_data_ready(struct adxl345_data *data)
>  	.address = reg,							\
>  	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
>  	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
> +	.scan_index = ADXL345_IDX_##axis,				\
> +	.scan_type = {							\
> +		.sign = 's',						\
> +		.realbits = 13,						\
> +		.storagebits = 16,					\
> +		.endianness = IIO_LE,					\
> +	},								\
>  }
>  
>  static const struct iio_chan_spec adxl345_channels[] = {
>  	ADXL345_CHANNEL(ADXL345_REG_DATAX0, X),
>  	ADXL345_CHANNEL(ADXL345_REG_DATAY0, Y),
>  	ADXL345_CHANNEL(ADXL345_REG_DATAZ0, Z),
> +	IIO_CHAN_SOFT_TIMESTAMP(ADXL345_IDX_TSTAMP),
> +};
> +
> +static const unsigned long adxl345_scan_masks[] = {
> +	BIT(ADXL345_IDX_X) | BIT(ADXL345_IDX_Y) | BIT(ADXL345_IDX_Z),
> +	0
>  };
>  
>  static int adxl345_read_raw(struct iio_dev *indio_dev,
> @@ -127,6 +151,10 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> +		ret = iio_device_claim_direct_mode(indio_dev);
> +		if (ret)
> +			return ret;
> +
>  		mutex_lock(&data->lock);
>  		ret = adxl345_set_mode(data, ADXL345_POWER_CTL_MEASURE);
>  		if (ret < 0) {
> @@ -148,12 +176,14 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
>  		ret = regmap_bulk_read(data->regmap, chan->address, &regval,
>  				       sizeof(regval));
>  		mutex_unlock(&data->lock);
> +		iio_device_release_direct_mode(indio_dev);
>  		if (ret < 0) {
>  			adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
>  			return ret;
>  		}
>  
> -		*val = sign_extend32(le16_to_cpu(regval), 12);
> +		*val = sign_extend32(le16_to_cpu(regval),
> +				     chan->scan_type.realbits - 1)
This change isn't really needed, but I suppose it does little harm...

>  		adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
>  
>  		return IIO_VAL_INT;
> @@ -186,6 +216,64 @@ static irqreturn_t adxl345_irq(int irq, void *p)
>  	return IRQ_NONE;
>  }
>  
> +static irqreturn_t adxl345_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct adxl345_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	mutex_lock(&data->lock);
> +	/* Make sure data is ready when using external trigger */
I 'think' this is only really relevant for the very first one.
After that general rule of thumb is that if an external trigger
is too quick - bad luck you'll get repeated data.

One of the reasons we would want to use another trigger is to
support capture in parallel from several sensors - if we 'hold'
like this we'll get out of sync.

As such I wonder if a better strategy would be to 'hold' for the
first reading in the buffer enable - thus guaranteeing valid
data before we start.  After that we wouldn't need to check this
here.

What do others think?

> +	if (!data->data_ready_trig_on) {
> +		ret = adxl345_data_ready(data);
> +		if (ret < 0)
> +			goto error;
> +	}
> +
> +	ret = regmap_bulk_read(data->regmap, ADXL345_REG_DATAX0, data->buffer,
> +			       sizeof(__le16) * 3);
> +	if (ret < 0)
> +		goto error;
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
> +					   pf->timestamp);
> +error:
> +	mutex_unlock(&data->lock);
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int adxl345_triggered_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct adxl345_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = iio_triggered_buffer_postenable(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	return adxl345_set_mode(data, ADXL345_POWER_CTL_MEASURE);
> +}
> +
> +static int adxl345_triggered_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct adxl345_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
> +	if (ret)
> +		return ret;
> +
> +	return iio_triggered_buffer_predisable(indio_dev);
> +}
> +
> +static const struct iio_buffer_setup_ops adxl345_buffer_setup_ops = {
> +	.postenable = adxl345_triggered_buffer_postenable,
> +	.predisable = adxl345_triggered_buffer_predisable,
> +};
> +
>  static int adxl345_drdy_trigger_set_state(struct iio_trigger *trig, bool state)
>  {
>  	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> @@ -278,6 +366,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap, int irq,
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->channels = adxl345_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(adxl345_channels);
> +	indio_dev->available_scan_masks = adxl345_scan_masks;
>  
>  	if (irq > 0) {
>  		ret = devm_request_threaded_irq(dev,
> @@ -311,6 +400,16 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap, int irq,
>  		}
>  	}
>  
> +	ret = devm_iio_triggered_buffer_setup(dev,
> +					      indio_dev,
> +					      iio_pollfunc_store_time,
> +					      adxl345_trigger_handler,
> +					      &adxl345_buffer_setup_ops);
> +	if (ret < 0) {
> +		dev_err(dev, "iio_triggered_buffer_setup failed: %d\n", ret);
> +		return ret;
> +	}
> +
>  	ret = iio_device_register(indio_dev);
>  	if (ret < 0)
>  		dev_err(dev, "iio_device_register failed: %d\n", ret);
> 

[toc] | [prev] | [next] | [standalone]

#1634373 — Re: [PATCH v2 4/4] iio: accel: adxl345: Add support for triggered buffer

On Mon, May 01, 2017 at 01:42:29AM +0100, Jonathan Cameron wrote:
[...]
> Few minor bits inline...  I'm a little bit in two minds about the 
> holding up waiting for new data when using another trigger...
> 
> Jonathan
[...]
> >  static int adxl345_read_raw(struct iio_dev *indio_dev,
> > @@ -127,6 +151,10 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
> >  
> >  	switch (mask) {
> >  	case IIO_CHAN_INFO_RAW:
> > +		ret = iio_device_claim_direct_mode(indio_dev);
> > +		if (ret)
> > +			return ret;
> > +
> >  		mutex_lock(&data->lock);
> >  		ret = adxl345_set_mode(data, ADXL345_POWER_CTL_MEASURE);
> >  		if (ret < 0) {
> > @@ -148,12 +176,14 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
> >  		ret = regmap_bulk_read(data->regmap, chan->address, &regval,
> >  				       sizeof(regval));
> >  		mutex_unlock(&data->lock);
> > +		iio_device_release_direct_mode(indio_dev);
> >  		if (ret < 0) {
> >  			adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
> >  			return ret;
> >  		}
> >  
> > -		*val = sign_extend32(le16_to_cpu(regval), 12);
> > +		*val = sign_extend32(le16_to_cpu(regval),
> > +				     chan->scan_type.realbits - 1)
> This change isn't really needed, but I suppose it does little harm...
> 
> >  		adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
> >  
> >  		return IIO_VAL_INT;
> > @@ -186,6 +216,64 @@ static irqreturn_t adxl345_irq(int irq, void *p)
> >  	return IRQ_NONE;
> >  }
> >  
> > +static irqreturn_t adxl345_trigger_handler(int irq, void *p)
> > +{
> > +	struct iio_poll_func *pf = p;
> > +	struct iio_dev *indio_dev = pf->indio_dev;
> > +	struct adxl345_data *data = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	mutex_lock(&data->lock);
> > +	/* Make sure data is ready when using external trigger */
> I 'think' this is only really relevant for the very first one.
> After that general rule of thumb is that if an external trigger
> is too quick - bad luck you'll get repeated data.
> 
> One of the reasons we would want to use another trigger is to
> support capture in parallel from several sensors - if we 'hold'
> like this we'll get out of sync.
> 
> As such I wonder if a better strategy would be to 'hold' for the
> first reading in the buffer enable - thus guaranteeing valid
> data before we start.  After that we wouldn't need to check this
> here.
> 

Thanks for the explanation. If we are to go with this one, where to put
it, preenable or postenable? I'm assuming the latter but would like to
confirm.

> What do others think?
> 

Any other inputs are greatly appreciated.

Eva

[toc] | [prev] | [next] | [standalone]

#1634495 — Re: [PATCH v2 4/4] iio: accel: adxl345: Add support for triggered buffer

On 02/05/17 13:23, Eva Rachel Retuya wrote:
> On Mon, May 01, 2017 at 01:42:29AM +0100, Jonathan Cameron wrote:
> [...]
>> Few minor bits inline...  I'm a little bit in two minds about the 
>> holding up waiting for new data when using another trigger...
>>
>> Jonathan
> [...]
>>>  static int adxl345_read_raw(struct iio_dev *indio_dev,
>>> @@ -127,6 +151,10 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
>>>  
>>>  	switch (mask) {
>>>  	case IIO_CHAN_INFO_RAW:
>>> +		ret = iio_device_claim_direct_mode(indio_dev);
>>> +		if (ret)
>>> +			return ret;
>>> +
>>>  		mutex_lock(&data->lock);
>>>  		ret = adxl345_set_mode(data, ADXL345_POWER_CTL_MEASURE);
>>>  		if (ret < 0) {
>>> @@ -148,12 +176,14 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
>>>  		ret = regmap_bulk_read(data->regmap, chan->address, &regval,
>>>  				       sizeof(regval));
>>>  		mutex_unlock(&data->lock);
>>> +		iio_device_release_direct_mode(indio_dev);
>>>  		if (ret < 0) {
>>>  			adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
>>>  			return ret;
>>>  		}
>>>  
>>> -		*val = sign_extend32(le16_to_cpu(regval), 12);
>>> +		*val = sign_extend32(le16_to_cpu(regval),
>>> +				     chan->scan_type.realbits - 1)
>> This change isn't really needed, but I suppose it does little harm...
>>
>>>  		adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
>>>  
>>>  		return IIO_VAL_INT;
>>> @@ -186,6 +216,64 @@ static irqreturn_t adxl345_irq(int irq, void *p)
>>>  	return IRQ_NONE;
>>>  }
>>>  
>>> +static irqreturn_t adxl345_trigger_handler(int irq, void *p)
>>> +{
>>> +	struct iio_poll_func *pf = p;
>>> +	struct iio_dev *indio_dev = pf->indio_dev;
>>> +	struct adxl345_data *data = iio_priv(indio_dev);
>>> +	int ret;
>>> +
>>> +	mutex_lock(&data->lock);
>>> +	/* Make sure data is ready when using external trigger */
>> I 'think' this is only really relevant for the very first one.
>> After that general rule of thumb is that if an external trigger
>> is too quick - bad luck you'll get repeated data.
>>
>> One of the reasons we would want to use another trigger is to
>> support capture in parallel from several sensors - if we 'hold'
>> like this we'll get out of sync.
>>
>> As such I wonder if a better strategy would be to 'hold' for the
>> first reading in the buffer enable - thus guaranteeing valid
>> data before we start.  After that we wouldn't need to check this
>> here.
>>
> 
> Thanks for the explanation. If we are to go with this one, where to put
> it, preenable or postenable? I'm assuming the latter but would like to
> confirm.

postenable.  It could in theory be effected by a future use of the
update_scan_mode callback so should be after that.

J
> 
>> What do others think?
>>
> 
> Any other inputs are greatly appreciated.
> 
> Eva
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[toc] | [prev] | [next] | [standalone]

#1633605 — Re: [PATCH v2 4/4] iio: accel: adxl345: Add support for triggered buffer

On Sat, Apr 29, 2017 at 10:49 AM, Eva Rachel Retuya <eraretuya@gmail.com> wrote:
> Previously, the only way to capture data is to read the exposed sysfs
> files in_accel_[x/y/z]_raw and applying the scale from in_accel_scale.
> Provide a way for continuous data capture that allows multiple data
> channels to be read at once by setting up buffer support.
>
> Initialize scan_type fields that describe the buffer and make sure to
> claim and release direct mode in read_raw. The latter is done to ensure
> no raw reads are attempted when utilizing the buffer and vice versa.
>
> Lastly, add triggered buffer support that allows utilization of any
> given trigger such as DATA_READY, hrtimer, etc. to initiate capturing of
> data and placing said data in the buffer. The trigger handler performs
> an all-axes read with timestamp.

One nit below.
FWIW:
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> +static irqreturn_t adxl345_trigger_handler(int irq, void *p)
> +{
> +       struct iio_poll_func *pf = p;
> +       struct iio_dev *indio_dev = pf->indio_dev;
> +       struct adxl345_data *data = iio_priv(indio_dev);
> +       int ret;
> +
> +       mutex_lock(&data->lock);
> +       /* Make sure data is ready when using external trigger */
> +       if (!data->data_ready_trig_on) {
> +               ret = adxl345_data_ready(data);
> +               if (ret < 0)
> +                       goto error;
> +       }
> +
> +       ret = regmap_bulk_read(data->regmap, ADXL345_REG_DATAX0, data->buffer,
> +                              sizeof(__le16) * 3);
> +       if (ret < 0)
> +               goto error;
> +
> +       iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
> +                                          pf->timestamp);

> +error:

I would call it 'err_unlock_notify'.

> +       mutex_unlock(&data->lock);
> +       iio_trigger_notify_done(indio_dev->trig);
> +
> +       return IRQ_HANDLED;
> +}

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1634374 — Re: [PATCH v2 4/4] iio: accel: adxl345: Add support for triggered buffer

On Mon, May 01, 2017 at 02:24:27PM +0300, Andy Shevchenko wrote:
[...]
> One nit below.
> FWIW:
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
> > +static irqreturn_t adxl345_trigger_handler(int irq, void *p)
> > +{
> > +       struct iio_poll_func *pf = p;
> > +       struct iio_dev *indio_dev = pf->indio_dev;
> > +       struct adxl345_data *data = iio_priv(indio_dev);
> > +       int ret;
> > +
> > +       mutex_lock(&data->lock);
> > +       /* Make sure data is ready when using external trigger */
> > +       if (!data->data_ready_trig_on) {
> > +               ret = adxl345_data_ready(data);
> > +               if (ret < 0)
> > +                       goto error;
> > +       }
> > +
> > +       ret = regmap_bulk_read(data->regmap, ADXL345_REG_DATAX0, data->buffer,
> > +                              sizeof(__le16) * 3);
> > +       if (ret < 0)
> > +               goto error;
> > +
> > +       iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
> > +                                          pf->timestamp);
> 
> > +error:
> 
> I would call it 'err_unlock_notify'.
> 

OK.

Thanks for the review,
Eva

> > +       mutex_unlock(&data->lock);
> > +       iio_trigger_notify_done(indio_dev->trig);
> > +
> > +       return IRQ_HANDLED;
> > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1633276 — [PATCH v2 1/4] dt-bindings: iio: accel: adxl345: Add optional interrupt-names support

Add interrupt-names property in order to specify interrupt pin in use.

Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
Acked-by: Rob Herring <robh@kernel.org>
---
Change in v2:
* Add Rob's Acked-by tag

 Documentation/devicetree/bindings/iio/accel/adxl345.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/accel/adxl345.txt b/Documentation/devicetree/bindings/iio/accel/adxl345.txt
index e7111b0..a7c9022 100644
--- a/Documentation/devicetree/bindings/iio/accel/adxl345.txt
+++ b/Documentation/devicetree/bindings/iio/accel/adxl345.txt
@@ -15,6 +15,8 @@ Optional properties:
    in Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
  - interrupts: interrupt mapping for IRQ as documented in
    Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
+ - interrupt-names: specify INTx pin in use. Should be "INT1" for INT1 pin or
+   "INT2" for INT2 pin.
 
 Example for a I2C device node:
 
@@ -23,6 +25,7 @@ Example for a I2C device node:
 		reg = <0x53>;
 		interrupt-parent = <&gpio1>;
 		interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-names = "INT1";
 	};
 
 Example for a SPI device node:
@@ -35,4 +38,5 @@ Example for a SPI device node:
 		spi-cpha;
 		interrupt-parent = <&gpio1>;
 		interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-names = "INT1";
 	};
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1633277 — [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger

The ADXL345 provides a DATA_READY interrupt function to signal
availability of new data. This interrupt function is latched and can be
cleared by reading the data registers. The polarity is set to active
high by default.

Support this functionality by setting it up as an IIO trigger.

In addition, two output pins INT1 and INT2 are available for driving
interrupts. Allow mapping to either pins by specifying the
interrupt-names property in device tree.

Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
---
Changes in v2:
* Provide a detailed commit message
* Move the of_irq_get_byname() check in core file in order to avoid
  introducing another parameter in probe()
* adxl345_irq():
  * return values directly
  * switch from iio_trigger_poll() to iio_trigger_poll_chained(), the former
    should only be called at the top-half not at the bottom-half.
* adxl345_drdy_trigger_set_state():
  * move regmap_get_device() to definition block
  * regmap_update_bits(): line splitting - one parameter per line, remove extra
    parenthesis
* probe()
  * use variable 'regval' to hold value to be written to the register and call
    regmap_write() unconditionally
  * fix line splitting in devm_request_threaded_irq() and devm_iio_trigger_alloc()
  * Switch to devm_iio_trigger_register()

 drivers/iio/accel/adxl345.h      |   2 +-
 drivers/iio/accel/adxl345_core.c | 104 ++++++++++++++++++++++++++++++++++++++-
 drivers/iio/accel/adxl345_i2c.c  |   3 +-
 drivers/iio/accel/adxl345_spi.c  |   2 +-
 4 files changed, 107 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
index c1ddf39..d2fa806 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -11,7 +11,7 @@
 #ifndef _ADXL345_H_
 #define _ADXL345_H_
 
-int adxl345_core_probe(struct device *dev, struct regmap *regmap,
+int adxl345_core_probe(struct device *dev, struct regmap *regmap, int irq,
 		       const char *name);
 int adxl345_core_remove(struct device *dev);
 
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index b8a212c..b8be0d7 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -9,15 +9,20 @@
  */
 
 #include <linux/delay.h>
+#include <linux/interrupt.h>
 #include <linux/module.h>
+#include <linux/of_irq.h>
 #include <linux/regmap.h>
 
 #include <linux/iio/iio.h>
+#include <linux/iio/trigger.h>
 
 #include "adxl345.h"
 
 #define ADXL345_REG_DEVID		0x00
 #define ADXL345_REG_POWER_CTL		0x2D
+#define ADXL345_REG_INT_ENABLE		0x2E
+#define ADXL345_REG_INT_MAP		0x2F
 #define ADXL345_REG_INT_SOURCE		0x30
 #define ADXL345_REG_DATA_FORMAT		0x31
 #define ADXL345_REG_DATAX0		0x32
@@ -39,6 +44,8 @@
 
 #define ADXL345_DEVID			0xE5
 
+#define ADXL345_IRQ_NAME		"adxl345_event"
+
 /*
  * In full-resolution mode, scale factor is maintained at ~4 mg/LSB
  * in all g ranges.
@@ -49,6 +56,8 @@
 static const int adxl345_uscale = 38300;
 
 struct adxl345_data {
+	struct iio_trigger *data_ready_trig;
+	bool data_ready_trig_on;
 	struct regmap *regmap;
 	struct mutex lock; /* protect this data structure */
 	u8 data_range;
@@ -158,17 +167,62 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
 	return -EINVAL;
 }
 
+static irqreturn_t adxl345_irq(int irq, void *p)
+{
+	struct iio_dev *indio_dev = p;
+	struct adxl345_data *data = iio_priv(indio_dev);
+	int ret;
+	u32 int_stat;
+
+	ret = regmap_read(data->regmap, ADXL345_REG_INT_SOURCE, &int_stat);
+	if (ret < 0)
+		return IRQ_HANDLED;
+
+	if (int_stat & ADXL345_INT_DATA_READY) {
+		iio_trigger_poll_chained(data->data_ready_trig);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+static int adxl345_drdy_trigger_set_state(struct iio_trigger *trig, bool state)
+{
+	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+	struct adxl345_data *data = iio_priv(indio_dev);
+	struct device *dev = regmap_get_device(data->regmap);
+	int ret;
+
+	ret = regmap_update_bits(data->regmap,
+				 ADXL345_REG_INT_ENABLE,
+				 ADXL345_INT_DATA_READY,
+				 state ? ADXL345_INT_DATA_READY : 0);
+	if (ret < 0) {
+		dev_err(dev, "Failed to update INT_ENABLE bits\n");
+		return ret;
+	}
+	data->data_ready_trig_on = state;
+
+	return ret;
+}
+
+static const struct iio_trigger_ops adxl345_trigger_ops = {
+	.owner = THIS_MODULE,
+	.set_trigger_state = adxl345_drdy_trigger_set_state,
+};
+
 static const struct iio_info adxl345_info = {
 	.driver_module	= THIS_MODULE,
 	.read_raw	= adxl345_read_raw,
 };
 
-int adxl345_core_probe(struct device *dev, struct regmap *regmap,
+int adxl345_core_probe(struct device *dev, struct regmap *regmap, int irq,
 		       const char *name)
 {
 	struct adxl345_data *data;
 	struct iio_dev *indio_dev;
 	u32 regval;
+	int of_irq;
 	int ret;
 
 	ret = regmap_read(regmap, ADXL345_REG_DEVID, &regval);
@@ -199,6 +253,22 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 		dev_err(dev, "Failed to set data range: %d\n", ret);
 		return ret;
 	}
+	/*
+	 * Any bits set to 0 send their respective interrupts to the INT1 pin,
+	 * whereas bits set to 1 send their respective interrupts to the INT2
+	 * pin. Map all interrupts to the specified pin.
+	 */
+	of_irq = of_irq_get_byname(dev->of_node, "INT2");
+	if (of_irq == irq)
+		regval = 0xFF;
+	else
+		regval = 0x00;
+
+	ret = regmap_write(data->regmap, ADXL345_REG_INT_MAP, regval);
+	if (ret < 0) {
+		dev_err(dev, "Failed to set up interrupts: %d\n", ret);
+		return ret;
+	}
 
 	mutex_init(&data->lock);
 
@@ -209,6 +279,38 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	indio_dev->channels = adxl345_channels;
 	indio_dev->num_channels = ARRAY_SIZE(adxl345_channels);
 
+	if (irq > 0) {
+		ret = devm_request_threaded_irq(dev,
+						irq,
+						NULL,
+						adxl345_irq,
+						IRQF_TRIGGER_HIGH |
+						IRQF_ONESHOT,
+						ADXL345_IRQ_NAME,
+						indio_dev);
+		if (ret < 0) {
+			dev_err(dev, "Failed to request irq: %d\n", irq);
+			return ret;
+		}
+
+		data->data_ready_trig = devm_iio_trigger_alloc(dev,
+							       "%s-dev%d",
+							       indio_dev->name,
+							       indio_dev->id);
+		if (!data->data_ready_trig)
+			return -ENOMEM;
+
+		data->data_ready_trig->dev.parent = dev;
+		data->data_ready_trig->ops = &adxl345_trigger_ops;
+		iio_trigger_set_drvdata(data->data_ready_trig, indio_dev);
+
+		ret = devm_iio_trigger_register(dev, data->data_ready_trig);
+		if (ret) {
+			dev_err(dev, "Failed to register trigger: %d\n", ret);
+			return ret;
+		}
+	}
+
 	ret = iio_device_register(indio_dev);
 	if (ret < 0)
 		dev_err(dev, "iio_device_register failed: %d\n", ret);
diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c
index 05e1ec4..31af702 100644
--- a/drivers/iio/accel/adxl345_i2c.c
+++ b/drivers/iio/accel/adxl345_i2c.c
@@ -34,7 +34,8 @@ static int adxl345_i2c_probe(struct i2c_client *client,
 		return PTR_ERR(regmap);
 	}
 
-	return adxl345_core_probe(&client->dev, regmap, id ? id->name : NULL);
+	return adxl345_core_probe(&client->dev, regmap, client->irq,
+				  id ? id->name : NULL);
 }
 
 static int adxl345_i2c_remove(struct i2c_client *client)
diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
index 6d65819..75a8c12 100644
--- a/drivers/iio/accel/adxl345_spi.c
+++ b/drivers/iio/accel/adxl345_spi.c
@@ -42,7 +42,7 @@ static int adxl345_spi_probe(struct spi_device *spi)
 		return PTR_ERR(regmap);
 	}
 
-	return adxl345_core_probe(&spi->dev, regmap, id->name);
+	return adxl345_core_probe(&spi->dev, regmap, spi->irq, id->name);
 }
 
 static int adxl345_spi_remove(struct spi_device *spi)
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web