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

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

[Eva Rachel Retuya]

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

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

[Eva Rachel Retuya]

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

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

[Eva Rachel Retuya]

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

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

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

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

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

[Eva Rachel Retuya]

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

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

[Jonathan Cameron]

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

Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger

[Jonathan Cameron]

On 29/04/17 08:49, Eva Rachel Retuya wrote:
> 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>
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
> ---
> 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"
I'd just put this inline.  It doesn't really give any benefit to
have this defined at the top.
> +
>  /*
>   * 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.
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?
> +	 */
> +	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)
> 

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

[Jonathan Cameron]

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

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

[Andy Shevchenko]

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

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

[Andy Shevchenko]

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

Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger

[Andy Shevchenko]

On Sat, Apr 29, 2017 at 10:49 AM, Eva Rachel Retuya <eraretuya@gmail.com> wrote:
> 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.

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

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

> +#include <linux/of_irq.h>

Can we get rid of gnostic resource providers?

> +static const struct iio_trigger_ops adxl345_trigger_ops = {

> +       .owner = THIS_MODULE,

Do we still need this kind of lines?

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

>         .read_raw       = adxl345_read_raw,
>  };

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

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

regval = of_irq == irq ? 0xff : 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;
> +       }

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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

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

[Jonathan Cameron]

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.

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

[Andy Shevchenko]

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

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

[Jonathan Cameron]

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.

Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger

[Rob Herring]

On Sun, Apr 30, 2017 at 7:32 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 29/04/17 08:49, Eva Rachel Retuya wrote:
>> 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>
> 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!

[...]

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

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.

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

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

[Eva Rachel Retuya]

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

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

[Eva Rachel Retuya]

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

Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger

[Eva Rachel Retuya]

On Mon, May 01, 2017 at 01:32:00AM +0100, Jonathan Cameron wrote:
[...]
> 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
[...]
> > +#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.

Ack.

> > +
> >  /*
> >   * 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;
> >  }
> >  
[...]
> > -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.
> 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'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.

Thanks,
Eva

> I'm not quite sure how one should do this sort of stuff in DT though.
> 
> Rob?
> > +	 */
> > +	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);
> >  

Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger

[Eva Rachel Retuya]

On Mon, May 01, 2017 at 02:31:00PM +0300, Andy Shevchenko wrote:
[...]
> > -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.

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

> 
> >  #include <linux/delay.h>
> > +#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.

> > +static const struct iio_trigger_ops adxl345_trigger_ops = {
> 
> > +       .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?

> > +       .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.
> 
> >         .read_raw       = adxl345_read_raw,
> >  };
> 
> > +       /*
> > +        * 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?
> 
> > +       if (of_irq == irq)
> > +               regval = 0xFF;
> > +       else
> > +               regval = 0x00;
> 
> regval = of_irq == irq ? 0xff : 0x00; ?
> 

OK.

Thanks,
Eva

> > +
> > +       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;
> > +       }
> 
> -- 
> With Best Regards,
> Andy Shevchenko

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

[Eva Rachel Retuya]

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

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

[Eva Rachel Retuya]

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

Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger

[Jonathan Cameron]

On 02/05/17 04:01, Rob Herring wrote:
> On Sun, Apr 30, 2017 at 7:32 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 29/04/17 08:49, Eva Rachel Retuya wrote:
>>> 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>
>> 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!
> 
> [...]
> 
>>> @@ -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?
> 
> 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.

Eva said:
> 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
> 
>>> +      */
>>> +     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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Jonathan Cameron]

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
> 

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

[Jonathan Cameron]

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
> 

Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger

[Andy Shevchenko]

On Tue, May 2, 2017 at 3:15 PM, Eva Rachel Retuya <eraretuya@gmail.com> wrote:
> On Mon, May 01, 2017 at 02:31:00PM +0300, Andy Shevchenko wrote:
> [...]
>> > -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.
>
> 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!

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

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

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger

[Jonathan Cameron]

On 02/05/17 13:15, Eva Rachel Retuya wrote:
> On Mon, May 01, 2017 at 02:31:00PM +0300, Andy Shevchenko wrote:
> [...]
>>> -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.
> 
> 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
> 
>>
>>>  #include <linux/delay.h>
>>> +#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.
> 
>>> +static const struct iio_trigger_ops adxl345_trigger_ops = {
>>
>>> +       .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.

> 
>>> +       .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.
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.>
>>>         .read_raw       = adxl345_read_raw,
>>>  };
>>
>>> +       /*
>>> +        * 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?
>>
>>> +       if (of_irq == irq)
>>> +               regval = 0xFF;
>>> +       else
>>> +               regval = 0x00;
>>
>> regval = of_irq == irq ? 0xff : 0x00; ?
>>
> 
> OK.
> 
> Thanks,
> Eva
> 
>>> +
>>> +       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;
>>> +       }
>>
>> -- 
>> With Best Regards,
>> Andy Shevchenko

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

[Eva Rachel Retuya]

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

Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger

[Eva Rachel Retuya]

On Wed, May 03, 2017 at 12:05:00AM +0300, Andy Shevchenko wrote:
> On Tue, May 2, 2017 at 3:15 PM, Eva Rachel Retuya <eraretuya@gmail.com> wrote:
> > On Mon, May 01, 2017 at 02:31:00PM +0300, Andy Shevchenko wrote:
> > [...]
> >> > -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.
> >
> > 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!
> 

OK, got it. Thanks for clarifying.

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

> 
> -- 
> With Best Regards,
> Andy Shevchenko
> --
> 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

Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger

[Eva Rachel Retuya]

On Fri, May 05, 2017 at 07:26:06PM +0100, Jonathan Cameron wrote:
> On 02/05/17 13:15, Eva Rachel Retuya wrote:
> > On Mon, May 01, 2017 at 02:31:00PM +0300, Andy Shevchenko wrote:
> > [...]
> >>> -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.
> > 
> > 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
> > 
> >>
> >>>  #include <linux/delay.h>
> >>> +#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.
> > 
> >>> +static const struct iio_trigger_ops adxl345_trigger_ops = {
> >>
> >>> +       .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.
> 
> > 
> >>> +       .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.
> Same issue.  Could be fixed, but right now you need them.

Noted, I will leave them as-is.

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

I'll suggest this as a coding task for Outreachy.

Thanks,
Eva

> >>>         .read_raw       = adxl345_read_raw,
> >>>  };
> >>
> >>> +       /*
> >>> +        * 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?
> >>
> >>> +       if (of_irq == irq)
> >>> +               regval = 0xFF;
> >>> +       else
> >>> +               regval = 0x00;
> >>
> >> regval = of_irq == irq ? 0xff : 0x00; ?
> >>
> > 
> > OK.
> > 
> > Thanks,
> > Eva
> > 
> >>> +
> >>> +       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;
> >>> +       }
> >>
> >> -- 
> >> With Best Regards,
> >> Andy Shevchenko
> 
> --
> 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

Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger

[Eva Rachel Retuya]

On Tue, May 02, 2017 at 04:59:12PM +0100, Jonathan Cameron wrote:
> On 02/05/17 04:01, Rob Herring wrote:
> > On Sun, Apr 30, 2017 at 7:32 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> >> On 29/04/17 08:49, Eva Rachel Retuya wrote:
> >>> 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>
> >> 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!
> > 
> > [...]
> > 
> >>> @@ -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?
> > 
> > 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.
> 
> Eva said:
> > 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

Thank you for explaining it well. I'll refer to this while working with
the issue.

Eva

> > 
> >>> +      */
> >>> +     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 majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 

Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger

[Jonathan Cameron]

On 10/05/17 14:24, Eva Rachel Retuya wrote:
> On Wed, May 03, 2017 at 12:05:00AM +0300, Andy Shevchenko wrote:
>> On Tue, May 2, 2017 at 3:15 PM, Eva Rachel Retuya <eraretuya@gmail.com> wrote:
>>> On Mon, May 01, 2017 at 02:31:00PM +0300, Andy Shevchenko wrote:
>>> [...]
>>>>> -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.
>>>
>>> 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!
>>
> 
> OK, got it. Thanks for clarifying.
> 
>>>
>>> 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
>>
>>>>> +#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.
>>
>> 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.

Jonathan
> 
> Eva
> 
>>
>> -- 
>> With Best Regards,
>> Andy Shevchenko
>> --
>> 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
> --
> 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
> 

Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger

[Dmitry Torokhov]

On Sun, May 14, 2017 at 04:15:37PM +0100, Jonathan Cameron wrote:
> On 10/05/17 14:24, Eva Rachel Retuya wrote:
> >On Wed, May 03, 2017 at 12:05:00AM +0300, Andy Shevchenko wrote:
> >>On Tue, May 2, 2017 at 3:15 PM, Eva Rachel Retuya <eraretuya@gmail.com> wrote:
> >>>On Mon, May 01, 2017 at 02:31:00PM +0300, Andy Shevchenko wrote:
> >>>[...]
> >>>>>-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.
> >>>
> >>>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!
> >>
> >
> >OK, got it. Thanks for clarifying.
> >
> >>>
> >>>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
> >>
> >>>>>+#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.
> >>
> >>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.

Well, we of course could look into moving bulk of platform_get_irq()
into generic device_get_irq(), but I think the main problem is that ACPI
(as far as I know) does not have notion of interrupt names...

Thanks.

-- 
Dmitry