iio: accel: adxl345: Add support for buffered readings

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 <[email protected]>

---
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(-)

drivers/iio/accel/adxl345_core.c +7 -0

index 9ccb582..b8a212c 100644
@@ -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;
 };

drivers/iio/accel/adxl345_core.c +4 -0

+	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);

drivers/iio/accel/adxl345_core.c +11 -13

+			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;
 }

drivers/iio/accel/adxl345_core.c +0 -0

 }
 EXPORT_SYMBOL_GPL(adxl345_core_remove);
 

drivers/iio/accel/adxl345_core.c static const int adxl345_uscale = 38300; · line 65

Quoted 9 lines · Eva Rachel Retuya

drivers/iio/accel/adxl345_core.c +9 -0

+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.

Quoted 4 lines

drivers/iio/accel/adxl345_core.c +4 -0

+     }
+
+     return 0;
+}

wrote:

Quoted 12 lines · Jonathan Cameron

drivers/iio/accel/adxl345_core.c static const int adxl345_uscale = 38300; · line 65

Quoted 9 lines · Eva Rachel Retuya

drivers/iio/accel/adxl345_core.c +9 -0

+static int adxl345_set_mode(struct adxl345_data *data, u8 mode)
+{
+     struct device *dev =3D regmap_get_device(data->regmap);
+     int ret;
+
+     ret =3D 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=2E

Quoted 1 line

One of the static checkers will probably moan about this otherwise=2E

But this will be not equivalent!

Quoted 5 lines

drivers/iio/accel/adxl345_core.c static const int adxl345_uscale = 38300; · line 69

Quoted 5 lines

drivers/iio/accel/adxl345_core.c +4 -0

+     }
+
+     return 0;
+}

Quoted 14 lines · Andy Shevchenko

Quoted 12 lines · Jonathan Cameron

drivers/iio/accel/adxl345_core.c static const int adxl345_uscale = 38300; · line 65

Quoted 9 lines · Eva Rachel Retuya

drivers/iio/accel/adxl345_core.c +9 -0

+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

Quoted 4 lines

function.

Quoted 1 line

One of the static checkers will probably moan about this otherwise.

But this will be not equivalent!

Why? Regmap_write returns 0 for success.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

drivers/iio/accel/adxl345_core.c +4 -0

+     }
+
+     return 0;
+}

Quoted 1 line · Jonathan Cameron

On 1 May 2017 20:42:08 BST, Andy Shevchenko

Quoted 10 lines · <[email protected]> wrote:

Quoted 10 lines

Quoted 8 lines · Jonathan Cameron

drivers/iio/accel/adxl345_core.c static const int adxl345_uscale = 38300;

Quoted 6 lines · Eva Rachel Retuya

drivers/iio/accel/adxl345_core.c +6 -0

+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);

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

drivers/iio/accel/adxl345_core.c static const int adxl345_uscale = 38300; · line 65

Quoted 3 lines

drivers/iio/accel/adxl345_core.c +3 -0

+     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

Quoted 5 lines

Quoted 4 lines

function.

Quoted 1 line

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.

http://lxr.free-electrons.com/source/drivers/base/regmap/regmap.c#L1693

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

drivers/iio/accel/adxl345_core.c +4 -0

+     }
+
+     return 0;
+}

drivers/iio/accel/adxl345_core.c static const int adxl345_uscale = 38300; · line 65

Quoted 9 lines

drivers/iio/accel/adxl345_core.c +9 -0

+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.

drivers/iio/accel/adxl345_core.c static const int adxl345_uscale = 38300; · line 72

Quoted 7 lines

drivers/iio/accel/adxl345_core.c +7 -0

+	}
+
+	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.

Quoted 5 lines

drivers/iio/accel/adxl345_core.c +5 -0

+	struct device *dev = regmap_get_device(data->regmap);
+	int tries = 5;
+	u32 val;
+	int ret;
+

drivers/iio/accel/adxl345_core.c static const int adxl345_uscale = 38300; · line 83

Quoted 6 lines

drivers/iio/accel/adxl345_core.c +6 -0

+	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?

Quoted 4 lines

drivers/iio/accel/adxl345_core.c +4 -0

+
+		ret = regmap_read(data->regmap, ADXL345_REG_INT_SOURCE, &val);
+		if (ret < 0)
+			return ret;

drivers/iio/accel/adxl345_core.c static const int adxl345_uscale = 38300; · line 92

Quoted 5 lines

drivers/iio/accel/adxl345_core.c +5 -0

+		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.

Quoted 17 lines

drivers/iio/accel/adxl345_core.c +10 -0

+	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;
+		}
+

drivers/iio/accel/adxl345_core.c static int adxl345_read_raw(struct iio_dev *indio_dev, · line 131

Quoted 4 lines

drivers/iio/accel/adxl345_core.c +4 -0

+		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!

Quoted 22 lines

drivers/iio/accel/adxl345_core.c +8 -1

+			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;

drivers/iio/accel/adxl345_core.c int adxl345_core_remove(struct device *dev) · line 227

Quoted 7 lines

drivers/iio/accel/adxl345_core.c +1 -2

@@ -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.

Quoted 4 lines

drivers/iio/accel/adxl345_core.c +0 -0

 }
 EXPORT_SYMBOL_GPL(adxl345_core_remove);
 

On Mon, May 01, 2017 at 01:22:52AM +0100, Jonathan Cameron wrote:
Hello Jonathan,
[...]

Quoted 11 lines

drivers/iio/accel/adxl345_core.c static const int adxl345_uscale = 38300; · line 65

Quoted 9 lines

drivers/iio/accel/adxl345_core.c +9 -0

+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.

Quoted 10 lines

drivers/iio/accel/adxl345_core.c static const int adxl345_uscale = 38300; · line 72

Quoted 7 lines

drivers/iio/accel/adxl345_core.c +7 -0

+	}
+
+	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.

Quoted 5 lines

Quoted 5 lines

drivers/iio/accel/adxl345_core.c +5 -0

+	struct device *dev = regmap_get_device(data->regmap);
+	int tries = 5;
+	u32 val;
+	int ret;
+

Quoted 9 lines

drivers/iio/accel/adxl345_core.c static const int adxl345_uscale = 38300; · line 83

Quoted 6 lines

drivers/iio/accel/adxl345_core.c +6 -0

+	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.

Quoted 13 lines

drivers/iio/accel/adxl345_core.c static const int adxl345_uscale = 38300; · line 92

Quoted 10 lines

drivers/iio/accel/adxl345_core.c +9 -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.

OK, will change it to -EIO then.

Quoted 17 lines

Quoted 17 lines

drivers/iio/accel/adxl345_core.c +10 -0

+	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;
+		}
+

Quoted 8 lines

drivers/iio/accel/adxl345_core.c static int adxl345_read_raw(struct iio_dev *indio_dev, · line 131

Quoted 4 lines

drivers/iio/accel/adxl345_core.c +4 -0

+		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.

Quoted 22 lines

Quoted 22 lines

drivers/iio/accel/adxl345_core.c +8 -1

+			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;

[...]

Quoted 9 lines

drivers/iio/accel/adxl345_core.c int adxl345_core_remove(struct device *dev) · line 227

Quoted 7 lines

drivers/iio/accel/adxl345_core.c +1 -2

@@ -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

Quoted 5 lines

drivers/iio/accel/adxl345_core.c int adxl345_core_remove(struct device *dev) · line 230

Quoted 4 lines

drivers/iio/accel/adxl345_core.c +0 -0

 }
 EXPORT_SYMBOL_GPL(adxl345_core_remove);
 

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 45 lines · Eva Rachel Retuya

On Mon, May 01, 2017 at 01:22:52AM +0100, Jonathan Cameron wrote:
Hello Jonathan,
[...]

Quoted 11 lines

drivers/iio/accel/adxl345_core.c static const int adxl345_uscale = 38300; · line 65

Quoted 9 lines

drivers/iio/accel/adxl345_core.c +9 -0

+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.

Quoted 10 lines

drivers/iio/accel/adxl345_core.c static const int adxl345_uscale = 38300; · line 72

Quoted 7 lines

drivers/iio/accel/adxl345_core.c +7 -0

+	}
+
+	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.

Quoted 5 lines

Quoted 5 lines

drivers/iio/accel/adxl345_core.c +5 -0

+	struct device *dev = regmap_get_device(data->regmap);
+	int tries = 5;
+	u32 val;
+	int ret;
+

Quoted 9 lines

drivers/iio/accel/adxl345_core.c static const int adxl345_uscale = 38300; · line 83

Quoted 6 lines

drivers/iio/accel/adxl345_core.c +6 -0

+	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?

Quoted 4 lines

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.

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.

drivers/iio/accel/adxl345_core.c +9 -0

+       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

drivers/iio/accel/adxl345_core.c +11 -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");
+
+       return -EAGAIN;
+}

Couple of minors below.
Otherwise, FWIW:

Reviewed-by Andy Shevchenko <[email protected]>

drivers/iio/accel/adxl345_core.c static const int adxl345_uscale = 38300; · line 74

Quoted 5 lines

drivers/iio/accel/adxl345_core.c +4 -0

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

unsigned int tries = 5;

Quoted 4 lines

drivers/iio/accel/adxl345_core.c +4 -0

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

drivers/iio/accel/adxl345_core.c static const int adxl345_uscale = 38300; · line 83

Quoted 5 lines

drivers/iio/accel/adxl345_core.c +5 -0

+               /*
+                * 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.

Quoted 11 lines

drivers/iio/accel/adxl345_core.c +11 -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");
+
+       return -EAGAIN;
+}

Signature · 2 lines

With Best Regards,
Andy Shevchenko

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 <[email protected]>

---
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(-)

drivers/iio/accel/adxl345.h +1 -1

index c1ddf39..d2fa806 100644
@@ -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);
 

drivers/iio/accel/adxl345_core.c +5 -0

index b8a212c..b8be0d7 100644
@@ -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

drivers/iio/accel/adxl345_core.c +49 -1

+
 /*
  * 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);

drivers/iio/accel/adxl345_core.c +44 -0

+	 */
+	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);

drivers/iio/accel/adxl345_i2c.c +2 -1

index 05e1ec4..31af702 100644
@@ -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)

drivers/iio/accel/adxl345_spi.c +1 -1

index 6d65819..75a8c12 100644
@@ -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)

Quoted 5 lines · Eva Rachel Retuya

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.

Quoted 7 lines

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 <[email protected]>

Coming together nicely, but a few more bits and pieces inline...

One slight worry is that the irq names stuff is to restrictive as we want to direct different interrupts to different pins if both are supported!

drivers/iio/accel/adxl345_core.c int adxl345_core_probe(struct device *dev, struct regmap *regmap, · line 259

Quoted 8 lines

drivers/iio/accel/adxl345_core.c +4 -0

@@ -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.

This is an interesting comment. The usual reason for dual interrupt pins is precisely to not map all functions to the same one. That allows for a saving in querying which interrupt it is by having just the data ready on one pin and just the events on the other...

Perhaps the current approach won't support that mode of operation? Clearly we can't merge a binding that enforces them all being the same and then change it later as it'll be incompatible.

I'm not quite sure how one should do this sort of stuff in DT though.

Rob?

Quoted 6 lines

drivers/iio/accel/adxl345_core.c +6 -0

+      */
+     of_irq = of_irq_get_byname(dev->of_node, "INT2");
+     if (of_irq == irq)
+             regval = 0xFF;
+     else
+             regval = 0x00;

Quoted 18 lines · Jonathan Cameron

Quoted 5 lines · Eva Rachel Retuya

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.

Quoted 7 lines

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 <[email protected]>

Coming together nicely, but a few more bits and pieces inline...

One slight worry is that the irq names stuff is to restrictive as we want to direct different interrupts to different pins if both are supported!

[...]

Quoted 17 lines

drivers/iio/accel/adxl345_core.c int adxl345_core_probe(struct device *dev, struct regmap *regmap, · line 259

Quoted 8 lines

drivers/iio/accel/adxl345_core.c +4 -0

@@ -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.

This is an interesting comment. The usual reason for dual interrupt pins is precisely to not map all functions to the same one. That allows for a saving in querying which interrupt it is by having just the data ready on one pin and just the events on the other...

Perhaps the current approach won't support that mode of operation? Clearly we can't merge a binding that enforces them all being the same and then change it later as it'll be incompatible.

Quoted 3 lines

I'm not quite sure how one should do this sort of stuff in DT though.

Rob?

DT should just describe what is connected which I gather here could be either one or both IRQs. We generally distinguish the IRQs with the interrupt-names property and then retrieve it as below.

I've thought about this before since to me that's the better approach than one or the other. I'm in a time crunch before hence I went with this way. The input driver does this as well and what I just did is to match what it does. If you could point me some drivers for reference, I'll gladly analyze those and present something better on the next revision.

Quoted 7 lines

drivers/iio/accel/adxl345_core.c int adxl345_core_probe(struct device *dev, struct regmap *regmap, · line 265

Quoted 7 lines

drivers/iio/accel/adxl345_core.c +6 -0

+      */
+     of_irq = of_irq_get_byname(dev->of_node, "INT2");
+     if (of_irq == irq)
+             regval = 0xFF;
+     else
+             regval = 0x00;

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 39 lines · Rob Herring

Quoted 18 lines · Jonathan Cameron

Quoted 5 lines · Eva Rachel Retuya

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.

Quoted 7 lines

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 <[email protected]>

Coming together nicely, but a few more bits and pieces inline...

One slight worry is that the irq names stuff is to restrictive as we want to direct different interrupts to different pins if both are supported!

[...]

Quoted 17 lines

drivers/iio/accel/adxl345_core.c int adxl345_core_probe(struct device *dev, struct regmap *regmap, · line 259

Quoted 8 lines

drivers/iio/accel/adxl345_core.c +4 -0

@@ -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.

This is an interesting comment. The usual reason for dual interrupt pins is precisely to not map all functions to the same one. That allows for a saving in querying which interrupt it is by having just the data ready on one pin and just the events on the other...

Perhaps the current approach won't support that mode of operation? Clearly we can't merge a binding that enforces them all being the same and then change it later as it'll be incompatible.

Quoted 7 lines

Quoted 3 lines

I'm not quite sure how one should do this sort of stuff in DT though.

Rob?

DT should just describe what is connected which I gather here could be either one or both IRQs. We generally distinguish the IRQs with the interrupt-names property and then retrieve it as below.

Picking this branch to continue on I'll grab Eva's replay as well.

Quoted 6 lines · Eva

I've thought about this before since to me that's the better approach than one or the other. I'm in a time crunch before hence I went with this way. The input driver does this as well and what I just did is to match what it does. If you could point me some drivers for reference, I'll gladly analyze those and present something better on the next revision.

So taking both of these and having thought about it a bit more in my current jet lagged state (I hate travelling - particularly with the added amusement of a flat tyre on the plane).

To my mind we need to describe what interrupts at there as Rob says. It's all obvious if there is only one interrupt connected (often the case I suspect as pins are in short supply on many SoCs).

If we allow the binding to specify both pins (using names to do the matching to which pin they are on the chip), then we could allow the driver itself to optimize the usage according to what is enabled. Note though that this can come later - for now we just need to allow the specification of both interrupts if they are present.

So lets talk about the ideal ;)
Probably makes sense to separate dataready and the events if possible. Ideal would be to even allow individual events to have there own pins as long as there are only two available. So we need a heuristic to work out what interrupts to put where. It doesn't work well as a lookup table (I tried it)

#define ADXL345_OVERRUN = BIT(0)
#define ADXL345_WATERMARK = BIT(1)
#define ADXL345_FREEFALL = BIT(2)
#define ADXL345_INACTIVITY = BIT(3)
#define ADXL345_ACTIVITY = BIT(4)
#define ADXL345_DOUBLE_TAP = BIT(5)
#define ADXL345_SINGLE_TAP = BIT(6)
#define ADXL345_DATA_READY = BIT(7)

So some function that takes the bitmap of what is enabled and tries to divide it sensibly.

int adxl345_int_heuristic(u8 input, u8 *output)

{
	long bounce;
	switch (hweight8(&input))
	{
	case 0 ... 1:
		*output = input;
		break;
	case 2:
		*output = BIT(ffs(&input)); //this will put one on each interrupt.
		break;
	case 3 ... 7: //now it gets tricky. Perhaps always have dataready and watermark on own interrupt if set?
		
		if (input & (ADXL345_DATA_READY | ADXL345_WATERMARK)) 
			output = input & (ADXL345_DATA_READY | ADXL345_WATERMARK);
		else // taps always on same one etc...
	}
}

Then your interrupt handler will need to look at the incoming and work out if it needs to read the status register to know what it has. If it doesn't need to then it doesn't do so. Be careful to only clear the right interrupts though in that case as it is always possible both are set.

Anyhow, right now all that needs to be there is the binding to allow two interrupts. Absolutely fine if for now the driver only uses the first one.

Jonathan

Quoted 12 lines

Quoted 7 lines

drivers/iio/accel/adxl345_core.c int adxl345_core_probe(struct device *dev, struct regmap *regmap, · line 265

Quoted 7 lines

drivers/iio/accel/adxl345_core.c +6 -0

+      */
+     of_irq = of_irq_get_byname(dev->of_node, "INT2");
+     if (of_irq == irq)
+             regval = 0xFF;
+     else
+             regval = 0x00;

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Coming together nicely, but a few more bits and pieces inline...

One slight worry is that the irq names stuff is to restrictive as we want to direct different interrupts to different pins if both are supported!

Jonathan

Quoted 1 line

+#define ADXL345_IRQ_NAME "adxl345_event"

I'd just put this inline. It doesn't really give any benefit to have this defined at the top.

Quoted 17 lines

drivers/iio/accel/adxl345_core.c +3 -0

+
 /*
  * 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;
 }
 

Quoted 12 lines

drivers/iio/accel/adxl345_core.c +2 -1

-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);

drivers/iio/accel/adxl345_core.c int adxl345_core_probe(struct device *dev, struct regmap *regmap, · line 259

Quoted 7 lines

drivers/iio/accel/adxl345_core.c +4 -0

@@ -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.

This is an interesting comment. The usual reason for dual interrupt pins is precisely to not map all functions to the same one. That allows for a saving in querying which interrupt it is by having just the data ready on one pin and just the events on the other...

Perhaps the current approach won't support that mode of operation? Clearly we can't merge a binding that enforces them all being the same and then change it later as it'll be incompatible.

I'm not quite sure how one should do this sort of stuff in DT though.

Rob?

Quoted 15 lines

drivers/iio/accel/adxl345_core.c +12 -0

+	 */
+	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);
 

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.

drivers/iio/accel/adxl345_core.c +6 -0

+
+       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;
+       }

drivers/iio/accel/adxl345.h line 15

Quoted 3 lines

drivers/iio/accel/adxl345.h +1 -1

-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);

I think I commented this once. Instead of increasing parameters, please introduce a new struct (as separate preparatory patch) which will hold current parameters. Let's call it
strut adxl345_chip {
struct device *dev;
struct regmap *regmap;
const char *name;
};

I insisnt in this chage.

drivers/iio/accel/adxl345_core.c

Quoted 6 lines

#include <linux/delay.h>

drivers/iio/accel/adxl345_core.c +2 -0

+#include <linux/interrupt.h>
 #include <linux/module.h>
 
+#include <linux/of_irq.h>

Can we get rid of gnostic resource providers?

drivers/iio/accel/adxl345_core.c static int adxl345_read_raw(struct iio_dev *indio_dev,

Quoted 3 lines

drivers/iio/accel/adxl345_core.c +2 -0

+static const struct iio_trigger_ops adxl345_trigger_ops = {
 
+       .owner = THIS_MODULE,

Do we still need this kind of lines?

drivers/iio/accel/adxl345_core.c static int adxl345_read_raw(struct iio_dev *indio_dev,

Quoted 6 lines

drivers/iio/accel/adxl345_core.c +2 -0

+       .set_trigger_state = adxl345_drdy_trigger_set_state,
+};
 
 static const struct iio_info adxl345_info = {
 
        .driver_module  = THIS_MODULE,

Ditto, though it's in the current code.

drivers/iio/accel/adxl345_core.c int adxl345_core_probe(struct device *dev, struct regmap *regmap, · line 261

Quoted 9 lines

        .read_raw       = adxl345_read_raw,

};

drivers/iio/accel/adxl345_core.c +6 -0

+       /*
+        * 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");

So, can we get it in resourse provider agnostic way?

drivers/iio/accel/adxl345_core.c int adxl345_core_probe(struct device *dev, struct regmap *regmap, · line 265

Quoted 4 lines

drivers/iio/accel/adxl345_core.c +4 -0

+       if (of_irq == irq)
+               regval = 0xFF;
+       else
+               regval = 0x00;

regval = of_irq == irq ? 0xff : 0x00; ?

Quoted 6 lines

drivers/iio/accel/adxl345_core.c +6 -0

+
+       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;
+       }

Signature · 2 lines

With Best Regards,
Andy Shevchenko

On Mon, May 01, 2017 at 02:31:00PM +0300, Andy Shevchenko wrote:
[...]

Quoted 13 lines

drivers/iio/accel/adxl345.h line 15

Quoted 3 lines

drivers/iio/accel/adxl345.h +1 -1

-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);

I think I commented this once. Instead of increasing parameters, please introduce a new struct (as separate preparatory patch) which will hold current parameters. Let's call it
strut adxl345_chip {
struct device *dev;
struct regmap *regmap;
const char *name;
};

Quoted 1 line

I insisnt in this chage.

I'm not sure if what you want is more simpler, is it something like what this driver does?

http://lxr.free-electrons.com/source/drivers/iio/gyro/mpu3050.h#L41
http://lxr.free-electrons.com/source/drivers/iio/gyro/mpu3050-i2c.c#L34

Quoted 4 lines

Quoted 1 line

+#include <linux/of_irq.h>

Can we get rid of gnostic resource providers?

I'm uninformed and still learning. Please let me know how to approach this in some other way.

Quoted 15 lines · Eva Rachel Retuya

On Mon, May 01, 2017 at 02:31:00PM +0300, Andy Shevchenko wrote:
[...]

Quoted 13 lines

drivers/iio/accel/adxl345.h line 15

Quoted 3 lines

drivers/iio/accel/adxl345.h +1 -1

-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);

I think I commented this once. Instead of increasing parameters, please introduce a new struct (as separate preparatory patch) which will hold current parameters. Let's call it
strut adxl345_chip {
struct device *dev;
struct regmap *regmap;
const char *name;
};

Quoted 4 lines

Quoted 1 line

I insisnt in this chage.

I'm not sure if what you want is more simpler, is it something like what this driver does?

Nope. The driver you were referring to does the same you did.

I'm proposing the above struct to be introduced along with changing prototype like:

-int adxl345_core_probe(struct device *dev, struct regmap *regmap,
const char *name);
+int adxl345_core_probe(struct adxl345_chip *chip);

In next patch adding interrupt would not touch prototypes at all!

Quoted 6 lines

http://lxr.free-electrons.com/source/drivers/iio/gyro/mpu3050.h#L41
http://lxr.free-electrons.com/source/drivers/iio/gyro/mpu3050-i2c.c#L34

Quoted 2 lines

Quoted 1 line

+#include <linux/of_irq.h>

Quoted 5 lines

Quoted 2 lines

Can we get rid of gnostic resource providers?

I'm uninformed and still learning. Please let me know how to approach this in some other way.

I suppose something like platform_get_irq(); to use. But it would be nice to you to investigate more.

Signature · 7 lines

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 23 lines · Andy Shevchenko

Quoted 15 lines · Eva Rachel Retuya

On Mon, May 01, 2017 at 02:31:00PM +0300, Andy Shevchenko wrote:
[...]

Quoted 13 lines

drivers/iio/accel/adxl345.h line 15

Quoted 3 lines

drivers/iio/accel/adxl345.h +1 -1

-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);

I think I commented this once. Instead of increasing parameters, please introduce a new struct (as separate preparatory patch) which will hold current parameters. Let's call it
strut adxl345_chip {
struct device *dev;
struct regmap *regmap;
const char *name;
};

Quoted 4 lines

Quoted 1 line

I insisnt in this chage.

I'm not sure if what you want is more simpler, is it something like what this driver does?

Nope. The driver you were referring to does the same you did.

Quoted 9 lines

I'm proposing the above struct to be introduced along with changing prototype like:

-int adxl345_core_probe(struct device *dev, struct regmap *regmap,
const char *name);
+int adxl345_core_probe(struct adxl345_chip *chip);

In next patch adding interrupt would not touch prototypes at all!

OK, got it. Thanks for clarifying.

Quoted 9 lines

Quoted 9 lines

http://lxr.free-electrons.com/source/drivers/iio/gyro/mpu3050.h#L41
http://lxr.free-electrons.com/source/drivers/iio/gyro/mpu3050-i2c.c#L34

Quoted 4 lines

Quoted 1 line

+#include <linux/of_irq.h>

Can we get rid of gnostic resource providers?

Quoted 5 lines

Quoted 2 lines

I'm uninformed and still learning. Please let me know how to approach this in some other way.

I suppose something like platform_get_irq(); to use. But it would be nice to you to investigate more.

I had a look and it seems I have to convert to platform_driver in order to make use of that function. Is this correct?

Eva

Quoted 8 lines

Signature · 7 lines

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 45 lines · Eva Rachel Retuya

Quoted 23 lines · Andy Shevchenko

Quoted 15 lines · Eva Rachel Retuya

On Mon, May 01, 2017 at 02:31:00PM +0300, Andy Shevchenko wrote:
[...]

Quoted 13 lines

drivers/iio/accel/adxl345.h line 15

Quoted 3 lines

drivers/iio/accel/adxl345.h +1 -1

-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);

I think I commented this once. Instead of increasing parameters, please introduce a new struct (as separate preparatory patch) which will hold current parameters. Let's call it
strut adxl345_chip {
struct device *dev;
struct regmap *regmap;
const char *name;
};

Quoted 4 lines

Quoted 1 line

I insisnt in this chage.

I'm not sure if what you want is more simpler, is it something like what this driver does?

Nope. The driver you were referring to does the same you did.

Quoted 9 lines

I'm proposing the above struct to be introduced along with changing prototype like:

-int adxl345_core_probe(struct device *dev, struct regmap *regmap,
const char *name);
+int adxl345_core_probe(struct adxl345_chip *chip);

In next patch adding interrupt would not touch prototypes at all!

OK, got it. Thanks for clarifying.

Quoted 9 lines

Quoted 9 lines

http://lxr.free-electrons.com/source/drivers/iio/gyro/mpu3050.h#L41
http://lxr.free-electrons.com/source/drivers/iio/gyro/mpu3050-i2c.c#L34

Quoted 4 lines

Quoted 1 line

+#include <linux/of_irq.h>

Can we get rid of gnostic resource providers?

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

I'm uninformed and still learning. Please let me know how to approach this in some other way.

I suppose something like platform_get_irq(); to use. But it would be nice to you to investigate more.

I had a look and it seems I have to convert to platform_driver in order to make use of that function. Is this correct?

I believe Andy was suggesting a function 'similar to' that one rather than actually using platform_get_irq.

It's not an area I know all that much about either, but there are moves to try and move the boiler plat needed to get the same parameters from devicetree and acpi into core library code so that a single function can be called to get the parameter in either case.

On Mon, May 01, 2017 at 02:31:00PM +0300, Andy Shevchenko wrote:
[...]

Quoted 13 lines

drivers/iio/accel/adxl345.h line 15

Quoted 3 lines

drivers/iio/accel/adxl345.h +1 -1

-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);

I think I commented this once. Instead of increasing parameters, please introduce a new struct (as separate preparatory patch) which will hold current parameters. Let's call it
strut adxl345_chip {
struct device *dev;
struct regmap *regmap;
const char *name;
};

Quoted 1 line

I insisnt in this chage.

I'm not sure if what you want is more simpler, is it something like what this driver does?

http://lxr.free-electrons.com/source/drivers/iio/gyro/mpu3050.h#L41
http://lxr.free-electrons.com/source/drivers/iio/gyro/mpu3050-i2c.c#L34

Quoted 9 lines

drivers/iio/accel/adxl345_core.c

Quoted 6 lines

#include <linux/delay.h>

drivers/iio/accel/adxl345_core.c +2 -0

+#include <linux/interrupt.h>
 #include <linux/module.h>
 
+#include <linux/of_irq.h>

Can we get rid of gnostic resource providers?

I'm uninformed and still learning. Please let me know how to approach this in some other way.

Quoted 2 lines

Quoted 1 line

+static const struct iio_trigger_ops adxl345_trigger_ops = {

Quoted 4 lines

Quoted 1 line

+ .owner = THIS_MODULE,

Do we still need this kind of lines?

I'm not sure either.
Jonathan, is it OK to omit this and also the one below?

Quoted 9 lines

drivers/iio/accel/adxl345_core.c static int adxl345_read_raw(struct iio_dev *indio_dev,

Quoted 7 lines

drivers/iio/accel/adxl345_core.c +2 -0

+       .set_trigger_state = adxl345_drdy_trigger_set_state,
+};
 
 static const struct iio_info adxl345_info = {
 
        .driver_module  = THIS_MODULE,

Ditto, though it's in the current code.

Quoted 10 lines

drivers/iio/accel/adxl345_core.c int adxl345_core_probe(struct device *dev, struct regmap *regmap, · line 261

Quoted 9 lines

        .read_raw       = adxl345_read_raw,

};

drivers/iio/accel/adxl345_core.c +6 -0

+       /*
+        * 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");

Quoted 9 lines

So, can we get it in resourse provider agnostic way?

drivers/iio/accel/adxl345_core.c int adxl345_core_probe(struct device *dev, struct regmap *regmap, · line 265

Quoted 4 lines

drivers/iio/accel/adxl345_core.c +4 -0

+       if (of_irq == irq)
+               regval = 0xFF;
+       else
+               regval = 0x00;

regval = of_irq == irq ? 0xff : 0x00; ?

OK.

Thanks,
Eva

Quoted 10 lines

Quoted 6 lines

drivers/iio/accel/adxl345_core.c +6 -0

+
+       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;
+       }

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Quoted 38 lines · Eva Rachel Retuya

On Mon, May 01, 2017 at 02:31:00PM +0300, Andy Shevchenko wrote:
[...]

Quoted 13 lines

drivers/iio/accel/adxl345.h line 15

Quoted 3 lines

drivers/iio/accel/adxl345.h +1 -1

-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);

I think I commented this once. Instead of increasing parameters, please introduce a new struct (as separate preparatory patch) which will hold current parameters. Let's call it
strut adxl345_chip {
struct device *dev;
struct regmap *regmap;
const char *name;
};

Quoted 1 line

I insisnt in this chage.

I'm not sure if what you want is more simpler, is it something like what this driver does?

http://lxr.free-electrons.com/source/drivers/iio/gyro/mpu3050.h#L41
http://lxr.free-electrons.com/source/drivers/iio/gyro/mpu3050-i2c.c#L34

Quoted 9 lines

drivers/iio/accel/adxl345_core.c

Quoted 6 lines

#include <linux/delay.h>

drivers/iio/accel/adxl345_core.c +2 -0

+#include <linux/interrupt.h>
 #include <linux/module.h>
 
+#include <linux/of_irq.h>

Can we get rid of gnostic resource providers?

I'm uninformed and still learning. Please let me know how to approach this in some other way.

Quoted 2 lines

Quoted 1 line

+static const struct iio_trigger_ops adxl345_trigger_ops = {

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

+ .owner = THIS_MODULE,

Do we still need this kind of lines?

I'm not sure either.
Jonathan, is it OK to omit this and also the one below?

No it's not. There are ways avoiding the necessity of specifying this via some macro magic. It could be done easily enough but hasn't been yet.

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

drivers/iio/accel/adxl345_core.c +2 -0

+       .set_trigger_state = adxl345_drdy_trigger_set_state,
+};

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

static const struct iio_info adxl345_info = {

.driver_module = THIS_MODULE,

Ditto, though it's in the current code.

Same issue. Could be fixed, but right now you need them.

Patches welcome ;) Basic eventual aim would be to drop these fields from the structures entirely but obviously there would have to be some intermediate steps.>

Quoted 17 lines

Quoted 17 lines

drivers/iio/accel/adxl345_core.c int adxl345_core_probe(struct device *dev, struct regmap *regmap, · line 261

Quoted 9 lines

        .read_raw       = adxl345_read_raw,

};

drivers/iio/accel/adxl345_core.c +6 -0

+       /*
+        * 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");

So, can we get it in resourse provider agnostic way?

Quoted 4 lines

drivers/iio/accel/adxl345_core.c +4 -0

+       if (of_irq == irq)
+               regval = 0xFF;
+       else
+               regval = 0x00;

Quoted 18 lines

Quoted 2 lines

regval = of_irq == irq ? 0xff : 0x00; ?

OK.

Thanks,
Eva

Quoted 10 lines

Quoted 6 lines

drivers/iio/accel/adxl345_core.c +6 -0

+
+       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;
+       }

Signature · 2 lines

With Best Regards,
Andy Shevchenko

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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 <[email protected]>

---
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(-)

drivers/iio/accel/Kconfig +2 -0

index 15de262..45092da 100644
@@ -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"

drivers/iio/accel/adxl345_core.c +28 -0

index b8be0d7..40dbdce 100644
@@ -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));

drivers/iio/accel/adxl345_core.c +0 -0

 		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;
 }

drivers/iio/accel/adxl345_core.c +60 -0

+	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);

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

Quoted 15 lines

static int adxl345_read_raw(struct iio_dev *indio_dev,

drivers/iio/accel/adxl345_core.c +4 -0

@@ -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));

drivers/iio/accel/adxl345_core.c static int adxl345_read_raw(struct iio_dev *indio_dev, · line 186

Quoted 10 lines

drivers/iio/accel/adxl345_core.c +3 -1

 		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...

Quoted 6 lines

drivers/iio/accel/adxl345_core.c +0 -0

 		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;
 }

drivers/iio/accel/adxl345_core.c static irqreturn_t adxl345_irq(int irq, void *p) · line 227

Quoted 10 lines

drivers/iio/accel/adxl345_core.c +9 -0

 
+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?

On Mon, May 01, 2017 at 01:42:29AM +0100, Jonathan Cameron wrote:
[...]

Quoted 4 lines

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

[...]

Quoted 47 lines

Quoted 15 lines

static int adxl345_read_raw(struct iio_dev *indio_dev,

drivers/iio/accel/adxl345_core.c +4 -0

@@ -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));

drivers/iio/accel/adxl345_core.c static int adxl345_read_raw(struct iio_dev *indio_dev, · line 186

Quoted 10 lines

drivers/iio/accel/adxl345_core.c +3 -1

 		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...

Quoted 6 lines

drivers/iio/accel/adxl345_core.c +0 -0

 		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;
 }

drivers/iio/accel/adxl345_core.c static irqreturn_t adxl345_irq(int irq, void *p) · line 227

Quoted 10 lines

drivers/iio/accel/adxl345_core.c +9 -0

 
+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.

Quoted 9 lines

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.

Quoted 3 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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.

drivers/iio/accel/adxl345_core.c +14 -0

+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;
+       }

drivers/iio/accel/adxl345_core.c +5 -0

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

One nit below.
FWIW:

Reviewed-by Andy Shevchenko <[email protected]>

Quoted 22 lines

drivers/iio/accel/adxl345_core.c +22 -0

+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);

Quoted 1 line

+error:

I would call it 'err_unlock_notify'.

Quoted 5 lines

drivers/iio/accel/adxl345_core.c +5 -0

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

Signature · 2 lines

With Best Regards,
Andy Shevchenko