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

[PATCH 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, drdy and get_triple,
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 10 -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.421300 1.034100 9.613300 1489394979215985178 
0.421300 0.995800 9.230300 1489394979226027141 
0.421300 1.034100 9.575000 1489394979236031983 
0.383000 1.072400 9.575000 1489394979245992337 
0.421300 1.072400 9.575000 1489394979256031062 
0.383000 1.110700 9.498400 1489394979266012473 
0.421300 1.072400 9.460100 1489394979276021743 
0.421300 1.034100 9.575000 1489394979286025189 
0.383000 1.072400 9.536700 1489394979295988380 
0.421300 1.072400 9.613300 1489394979306036861 
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 10 -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.383000 1.072400 9.575000 1489395043824672808 
0.459600 1.072400 9.575000 1489395043864264458 
0.421300 0.995800 9.575000 1489395043883851974 
0.383000 1.072400 9.536700 1489395043905000622 
0.459600 1.034100 9.575000 1489395043929645868 
0.421300 1.072400 9.498400 1489395043946881648 
0.459600 1.034100 9.613300 1489395043967234777 
0.459600 1.034100 9.575000 1489395043987596905 
0.383000 1.034100 9.613300 1489395044005969376 
0.383000 1.110700 9.575000 1489395044026535007 
Disabling: in_accel_y_en
Disabling: in_accel_x_en
Disabling: in_timestamp_en
Disabling: in_accel_z_en
~ # 

Eva Rachel Retuya (4):
  dt-bindings: iio: accel: adxl345: Add optional interrupt-names support
  iio: accel: adxl345_core: Introduce set_mode, drdy & get_triple
    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                        |   4 +-
 drivers/iio/accel/adxl345_core.c                   | 313 +++++++++++++++++++--
 drivers/iio/accel/adxl345_i2c.c                    |  14 +-
 drivers/iio/accel/adxl345_spi.c                    |  10 +-
 6 files changed, 321 insertions(+), 26 deletions(-)

-- 
2.7.4

[PATCH 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>
---
 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 2/4] iio: accel: adxl345_core: Introduce set_mode, drdy & get_triple 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 drdy function that monitors the INT_SOURCE register of
this bit. This is done to ensure consistent readings.

Make use of drdy in read_raw and also refactor the read to perform an
all-axes read instead through get_triple. This function will come in
handy when buffered reading is introduced.

Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
---
 drivers/iio/accel/adxl345_core.c | 116 +++++++++++++++++++++++++++++----------
 1 file changed, 88 insertions(+), 28 deletions(-)

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 9ccb582..7eee31d 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
@@ -47,6 +53,58 @@ struct adxl345_data {
 	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_get_triple(struct adxl345_data *data, void *buf)
+{
+	struct device *dev = regmap_get_device(data->regmap);
+	int ret;
+
+	ret = regmap_bulk_read(data->regmap, ADXL345_REG_DATAX0, buf,
+			       sizeof(__le16) * 3);
+	if (ret < 0) {
+		dev_err(dev, "Failed to read the data registers, %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int adxl345_drdy(struct adxl345_data *data)
+{
+	struct device *dev = regmap_get_device(data->regmap);
+	int tries = 5;
+	u32 regval;
+	int ret;
+
+	while (tries--) {
+		ret = regmap_read(data->regmap, ADXL345_REG_INT_SOURCE,
+				  &regval);
+		if (ret < 0)
+			return ret;
+		if ((regval & ADXL345_INT_DATA_READY) ==
+		    ADXL345_INT_DATA_READY)
+			return 0;
+
+		msleep(20);
+	}
+	dev_err(dev, "Data is not yet ready\n");
+
+	return -EIO;
+}
+
 #define ADXL345_CHANNEL(reg, axis) {					\
 	.type = IIO_ACCEL,						\
 	.modified = 1,							\
@@ -67,22 +125,37 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
 			    int *val, int *val2, long mask)
 {
 	struct adxl345_data *data = iio_priv(indio_dev);
-	__le16 regval;
+	s16 regval[3];
 	int ret;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		/*
-		 * Data is stored in adjacent registers:
-		 * ADXL345_REG_DATA(X0/Y0/Z0) contain the least significant byte
-		 * and ADXL345_REG_DATA(X0/Y0/Z0) + 1 the most significant byte
-		 */
-		ret = regmap_bulk_read(data->regmap, chan->address, &regval,
-				       sizeof(regval));
+		ret = adxl345_set_mode(data, ADXL345_POWER_CTL_MEASURE);
+		if (ret < 0)
+			return ret;
+		ret = adxl345_drdy(data);
+		if (ret < 0)
+			return ret;
+
+		ret = adxl345_get_triple(data, &regval);
 		if (ret < 0)
 			return ret;
 
-		*val = sign_extend32(le16_to_cpu(regval), 12);
+		switch (chan->address) {
+		case ADXL345_REG_DATAX0:
+			*val = regval[0];
+			break;
+		case ADXL345_REG_DATAY0:
+			*val = regval[1];
+			break;
+		case ADXL345_REG_DATAZ0:
+			*val = regval[2];
+			break;
+		default:
+			return -EINVAL;
+		}
+		adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
+
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
 		*val = 0;
@@ -126,9 +199,12 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	data = iio_priv(indio_dev);
 	dev_set_drvdata(dev, indio_dev);
 	data->regmap = regmap;
+	/* Make sure sensor is in standby mode before configuring registers */
+	ret = adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
+	if (ret < 0)
+		return ret;
 	/* Enable full-resolution mode */
 	data->data_range = ADXL345_DATA_FORMAT_FULL_RES;
-
 	ret = regmap_write(data->regmap, ADXL345_REG_DATA_FORMAT,
 			   data->data_range);
 	if (ret < 0) {
@@ -143,22 +219,7 @@ 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) {
-		dev_err(dev, "iio_device_register failed: %d\n", ret);
-		regmap_write(data->regmap, ADXL345_REG_POWER_CTL,
-			     ADXL345_POWER_CTL_STANDBY);
-	}
-
-	return ret;
+	return iio_device_register(indio_dev);
 }
 EXPORT_SYMBOL_GPL(adxl345_core_probe);
 
@@ -169,8 +230,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 3/4] iio: accel: adxl345: Setup DATA_READY trigger

[Eva Rachel Retuya]

Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
---
 drivers/iio/accel/adxl345.h      |   4 +-
 drivers/iio/accel/adxl345_core.c | 113 +++++++++++++++++++++++++++++++++++++--
 drivers/iio/accel/adxl345_i2c.c  |  10 +++-
 drivers/iio/accel/adxl345_spi.c  |  10 +++-
 4 files changed, 130 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
index c1ddf39..f28a503 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -11,8 +11,8 @@
 #ifndef _ADXL345_H_
 #define _ADXL345_H_
 
-int adxl345_core_probe(struct device *dev, struct regmap *regmap,
-		       const char *name);
+int adxl345_core_probe(struct device *dev, struct regmap *regmap, int irq,
+		       const char *name, bool use_int2);
 int adxl345_core_remove(struct device *dev);
 
 #endif /* _ADXL345_H_ */
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 7eee31d..4b7055c 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -9,15 +9,19 @@
  */
 
 #include <linux/delay.h>
+#include <linux/interrupt.h>
 #include <linux/module.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 +43,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,7 +55,9 @@
 static const int adxl345_uscale = 38300;
 
 struct adxl345_data {
+	struct iio_trigger *drdy_trig;
 	struct regmap *regmap;
+	bool drdy_trig_on;
 	u8 data_range;
 };
 
@@ -167,13 +175,57 @@ 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 = IRQ_NONE;
+	u32 int_stat;
+
+	ret = regmap_read(data->regmap, ADXL345_REG_INT_SOURCE, &int_stat);
+	if (ret < 0)
+		return ret;
+
+	if (int_stat & ADXL345_INT_DATA_READY) {
+		iio_trigger_poll(data->drdy_trig);
+		ret = IRQ_HANDLED;
+	}
+
+	return ret;
+}
+
+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;
+	int ret;
+
+	dev = regmap_get_device(data->regmap);
+	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->drdy_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,
-		       const char *name)
+int adxl345_core_probe(struct device *dev, struct regmap *regmap, int irq,
+		       const char *name, bool use_int2)
 {
 	struct adxl345_data *data;
 	struct iio_dev *indio_dev;
@@ -212,6 +264,20 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 		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.
+	 */
+	if (!use_int2)
+		ret = regmap_write(data->regmap, ADXL345_REG_INT_MAP, 0x00);
+	else
+		ret = regmap_write(data->regmap, ADXL345_REG_INT_MAP, 0xFF);
+	if (ret < 0) {
+		dev_err(dev, "Failed to set up interrupts: %d\n", ret);
+		return ret;
+	}
+
 	indio_dev->dev.parent = dev;
 	indio_dev->name = name;
 	indio_dev->info = &adxl345_info;
@@ -219,7 +285,46 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 	indio_dev->channels = adxl345_channels;
 	indio_dev->num_channels = ARRAY_SIZE(adxl345_channels);
 
-	return iio_device_register(indio_dev);
+	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->drdy_trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
+							 indio_dev->name,
+							 indio_dev->id);
+		if (!data->drdy_trig)
+			return -ENOMEM;
+
+		data->drdy_trig->dev.parent = dev;
+		data->drdy_trig->ops = &adxl345_trigger_ops;
+		iio_trigger_set_drvdata(data->drdy_trig, indio_dev);
+
+		ret = iio_trigger_register(data->drdy_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);
+		goto err_trigger_unregister;
+	}
+
+	return ret;
+
+err_trigger_unregister:
+	if (data->drdy_trig)
+		iio_trigger_unregister(data->drdy_trig);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(adxl345_core_probe);
 
@@ -229,6 +334,8 @@ int adxl345_core_remove(struct device *dev)
 	struct adxl345_data *data = iio_priv(indio_dev);
 
 	iio_device_unregister(indio_dev);
+	if (data->drdy_trig)
+		iio_trigger_unregister(data->drdy_trig);
 
 	return adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
 }
diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c
index 05e1ec4..8c791b8 100644
--- a/drivers/iio/accel/adxl345_i2c.c
+++ b/drivers/iio/accel/adxl345_i2c.c
@@ -13,6 +13,7 @@
 
 #include <linux/i2c.h>
 #include <linux/module.h>
+#include <linux/of_irq.h>
 #include <linux/regmap.h>
 
 #include "adxl345.h"
@@ -26,6 +27,8 @@ static int adxl345_i2c_probe(struct i2c_client *client,
 			     const struct i2c_device_id *id)
 {
 	struct regmap *regmap;
+	bool use_int2 = false;
+	int irq;
 
 	regmap = devm_regmap_init_i2c(client, &adxl345_i2c_regmap_config);
 	if (IS_ERR(regmap)) {
@@ -34,7 +37,12 @@ static int adxl345_i2c_probe(struct i2c_client *client,
 		return PTR_ERR(regmap);
 	}
 
-	return adxl345_core_probe(&client->dev, regmap, id ? id->name : NULL);
+	irq = of_irq_get_byname(client->dev.of_node, "INT2");
+	if (irq == client->irq)
+		use_int2 = true;
+
+	return adxl345_core_probe(&client->dev, regmap, client->irq,
+				  id ? id->name : NULL, use_int2);
 }
 
 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..731d675 100644
--- a/drivers/iio/accel/adxl345_spi.c
+++ b/drivers/iio/accel/adxl345_spi.c
@@ -9,6 +9,7 @@
  */
 
 #include <linux/module.h>
+#include <linux/of_irq.h>
 #include <linux/regmap.h>
 #include <linux/spi/spi.h>
 
@@ -27,6 +28,8 @@ static int adxl345_spi_probe(struct spi_device *spi)
 {
 	const struct spi_device_id *id = spi_get_device_id(spi);
 	struct regmap *regmap;
+	bool use_int2 = false;
+	int irq;
 
 	/* Bail out if max_speed_hz exceeds 5 MHz */
 	if (spi->max_speed_hz > ADXL345_MAX_SPI_FREQ_HZ) {
@@ -42,7 +45,12 @@ static int adxl345_spi_probe(struct spi_device *spi)
 		return PTR_ERR(regmap);
 	}
 
-	return adxl345_core_probe(&spi->dev, regmap, id->name);
+	irq = of_irq_get_byname(spi->dev.of_node, "INT2");
+	if (irq == spi->irq)
+		use_int2 = true;
+
+	return adxl345_core_probe(&spi->dev, regmap, spi->irq, id->name,
+				  use_int2);
 }
 
 static int adxl345_spi_remove(struct spi_device *spi)
-- 
2.7.4

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

[Eva Rachel Retuya]

Provide an all-axes read for triggered buffering.

Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
---
 drivers/iio/accel/Kconfig        |   2 +
 drivers/iio/accel/adxl345_core.c | 124 +++++++++++++++++++++++++++++++++++----
 drivers/iio/accel/adxl345_i2c.c  |   4 ++
 3 files changed, 119 insertions(+), 11 deletions(-)

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 4b7055c..5df1e73 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -13,8 +13,11 @@
 #include <linux/module.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"
 
@@ -54,11 +57,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 *drdy_trig;
 	struct regmap *regmap;
+	struct mutex lock; /* protect this data structure */
 	bool drdy_trig_on;
 	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)
@@ -75,12 +87,12 @@ static int adxl345_set_mode(struct adxl345_data *data, u8 mode)
 	return 0;
 }
 
-static int adxl345_get_triple(struct adxl345_data *data, void *buf)
+static int adxl345_get_triple(struct adxl345_data *data)
 {
 	struct device *dev = regmap_get_device(data->regmap);
 	int ret;
 
-	ret = regmap_bulk_read(data->regmap, ADXL345_REG_DATAX0, buf,
+	ret = regmap_bulk_read(data->regmap, ADXL345_REG_DATAX0, data->buffer,
 			       sizeof(__le16) * 3);
 	if (ret < 0) {
 		dev_err(dev, "Failed to read the data registers, %d\n", ret);
@@ -120,12 +132,25 @@ static int adxl345_drdy(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,
@@ -133,31 +158,44 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
 			    int *val, int *val2, long mask)
 {
 	struct adxl345_data *data = iio_priv(indio_dev);
-	s16 regval[3];
 	int ret;
 
 	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)
+		if (ret < 0) {
+			mutex_unlock(&data->lock);
 			return ret;
+		}
 		ret = adxl345_drdy(data);
-		if (ret < 0)
+		if (ret < 0) {
+			adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
+			mutex_unlock(&data->lock);
 			return ret;
+		}
 
-		ret = adxl345_get_triple(data, &regval);
-		if (ret < 0)
+		ret = adxl345_get_triple(data);
+		mutex_unlock(&data->lock);
+		iio_device_release_direct_mode(indio_dev);
+		if (ret < 0) {
+			adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
 			return ret;
+		}
 
 		switch (chan->address) {
 		case ADXL345_REG_DATAX0:
-			*val = regval[0];
+			*val = data->buffer[ADXL345_IDX_X];
 			break;
 		case ADXL345_REG_DATAY0:
-			*val = regval[1];
+			*val = data->buffer[ADXL345_IDX_Y];
 			break;
 		case ADXL345_REG_DATAZ0:
-			*val = regval[2];
+			*val = data->buffer[ADXL345_IDX_Z];
 			break;
 		default:
 			return -EINVAL;
@@ -194,6 +232,56 @@ static irqreturn_t adxl345_irq(int irq, void *p)
 	return ret;
 }
 
+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);
+	ret = adxl345_get_triple(data);
+	mutex_unlock(&data->lock);
+	if (ret < 0)
+		goto error;
+
+	iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
+					   pf->timestamp);
+error:
+	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,12 +366,15 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap, int irq,
 		return ret;
 	}
 
+	mutex_init(&data->lock);
+
 	indio_dev->dev.parent = dev;
 	indio_dev->name = name;
 	indio_dev->info = &adxl345_info;
 	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 =
@@ -312,14 +403,24 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap, int irq,
 		}
 	}
 
+	ret = iio_triggered_buffer_setup(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);
+		goto err_trigger_unregister;
+	}
+
 	ret = iio_device_register(indio_dev);
 	if (ret < 0) {
 		dev_err(dev, "iio_device_register failed: %d\n", ret);
-		goto err_trigger_unregister;
+		goto err_buffer_cleanup;
 	}
 
 	return ret;
 
+err_buffer_cleanup:
+	iio_triggered_buffer_cleanup(indio_dev);
 err_trigger_unregister:
 	if (data->drdy_trig)
 		iio_trigger_unregister(data->drdy_trig);
@@ -334,6 +435,7 @@ int adxl345_core_remove(struct device *dev)
 	struct adxl345_data *data = iio_priv(indio_dev);
 
 	iio_device_unregister(indio_dev);
+	iio_triggered_buffer_cleanup(indio_dev);
 	if (data->drdy_trig)
 		iio_trigger_unregister(data->drdy_trig);
 
diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c
index 8c791b8..1e0f071 100644
--- a/drivers/iio/accel/adxl345_i2c.c
+++ b/drivers/iio/accel/adxl345_i2c.c
@@ -30,6 +30,10 @@ static int adxl345_i2c_probe(struct i2c_client *client,
 	bool use_int2 = false;
 	int irq;
 
+	if (!i2c_check_functionality(client->adapter,
+				     I2C_FUNC_SMBUS_READ_I2C_BLOCK))
+		return -EOPNOTSUPP;
+
 	regmap = devm_regmap_init_i2c(client, &adxl345_i2c_regmap_config);
 	if (IS_ERR(regmap)) {
 		dev_err(&client->dev, "Error initializing i2c regmap: %ld\n",
-- 
2.7.4

Re: [PATCH 2/4] iio: accel: adxl345_core: Introduce set_mode, drdy & get_triple functions

[Andy Shevchenko]

On Mon, Mar 13, 2017 at 1:11 PM, 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 drdy function that monitors the INT_SOURCE register of
> this bit. This is done to ensure consistent readings.
>
> Make use of drdy in read_raw and also refactor the read to perform an
> all-axes read instead through get_triple. This function will come in
> handy when buffered reading is introduced.

> +static int adxl345_get_triple(struct adxl345_data *data, void *buf)
> +{
> +       struct device *dev = regmap_get_device(data->regmap);
> +       int ret;
> +

> +       ret = regmap_bulk_read(data->regmap, ADXL345_REG_DATAX0, buf,
> +                              sizeof(__le16) * 3);

Magic.

> +static int adxl345_drdy(struct adxl345_data *data)

Name is not speaking for itself.

Perhaps
_data_ready(...)

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

> +       while (tries--) {

do {} while pattern a bit better here, since it shows explicitly that
you run the code at least once.

> +               ret = regmap_read(data->regmap, ADXL345_REG_INT_SOURCE,
> +                                 &regval);

If you rename regval to val (or v) it would fit one line I guess.

> +               if (ret < 0)
> +                       return ret;
> +               if ((regval & ADXL345_INT_DATA_READY) ==
> +                   ADXL345_INT_DATA_READY)
> +                       return 0;
> +

> +               msleep(20);

This needs explanation. (It's too long for usual IO delays)

> +       }
> +       dev_err(dev, "Data is not yet ready\n");
> +
> +       return -EIO;

Hmm... Not sure which error code is better here, though EIO is possible.

> @@ -67,22 +125,37 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
>                             int *val, int *val2, long mask)
>  {
>         struct adxl345_data *data = iio_priv(indio_dev);

> -       __le16 regval;
> +       s16 regval[3];

I'm not sure this is equivalent change. Have you tried to test this
code on different endianess?

>         int ret;
>
>         switch (mask) {
>         case IIO_CHAN_INFO_RAW:
> -               /*
> -                * Data is stored in adjacent registers:
> -                * ADXL345_REG_DATA(X0/Y0/Z0) contain the least significant byte
> -                * and ADXL345_REG_DATA(X0/Y0/Z0) + 1 the most significant byte
> -                */
> -               ret = regmap_bulk_read(data->regmap, chan->address, &regval,
> -                                      sizeof(regval));
> +               ret = adxl345_set_mode(data, ADXL345_POWER_CTL_MEASURE);
> +               if (ret < 0)
> +                       return ret;

+ empty line here.

> +               ret = adxl345_drdy(data);
> +               if (ret < 0)
> +                       return ret;
> +
> +               ret = adxl345_get_triple(data, &regval);
>                 if (ret < 0)
>                         return ret;
>

> -               *val = sign_extend32(le16_to_cpu(regval), 12);
> +               switch (chan->address) {
> +               case ADXL345_REG_DATAX0:
> +                       *val = regval[0];
> +                       break;
> +               case ADXL345_REG_DATAY0:
> +                       *val = regval[1];
> +                       break;
> +               case ADXL345_REG_DATAZ0:
> +                       *val = regval[2];
> +                       break;

Yes, I'm worrying about endianess here.

> @@ -126,9 +199,12 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>         data = iio_priv(indio_dev);
>         dev_set_drvdata(dev, indio_dev);
>         data->regmap = regmap;
> +       /* Make sure sensor is in standby mode before configuring registers */
> +       ret = adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
> +       if (ret < 0)
> +               return ret;
>         /* Enable full-resolution mode */
>         data->data_range = ADXL345_DATA_FORMAT_FULL_RES;

> -

This doesn't belong to the patch, and perhaps you may put _set_mode()
call here surrounded by empty lines.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Mon, Mar 13, 2017 at 1:11 PM, Eva Rachel Retuya <eraretuya@gmail.com> wrote:

Missed commit message is no-no!

> Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>

> -int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> -                      const char *name);
> +int adxl345_core_probe(struct device *dev, struct regmap *regmap, int irq,
> +                      const char *name, bool use_int2);

Hmm... And tomorrow you will add another flag and another.

No, consider to use something like

struct adxl345_chip {
 struct device *dev;
 struct regmap *regmap;
 const char *name;
}

Convert your probe to use it, and after extend for your needs.

>  #define ADXL345_DEVID                  0xE5
>
> +#define ADXL345_IRQ_NAME               "adxl345_event"


>  struct adxl345_data {
> +       struct iio_trigger *drdy_trig;
>         struct regmap *regmap;
> +       bool drdy_trig_on;
>         u8 data_range;

drdy -> data_ready

> +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 = IRQ_NONE;
> +       u32 int_stat;
> +

> +       ret = regmap_read(data->regmap, ADXL345_REG_INT_SOURCE, &int_stat);

> +       if (ret < 0)
> +               return ret;

It makes little sense AFAIU.

> +
> +       if (int_stat & ADXL345_INT_DATA_READY) {
> +               iio_trigger_poll(data->drdy_trig);
> +               ret = IRQ_HANDLED;
> +       }
> +
> +       return ret;

Useless variable ret. You may return values directly.

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

> +       dev = regmap_get_device(data->regmap);

This may be moved to definition block.

> +       ret = regmap_update_bits(data->regmap, ADXL345_REG_INT_ENABLE,

> +                                ADXL345_INT_DATA_READY, (state ?
> +                                ADXL345_INT_DATA_READY : 0));

No way:
 Don't split lines like this.
 Remove extra parens.

> +static const struct iio_trigger_ops adxl345_trigger_ops = {

> +       .owner = THIS_MODULE,

I dunno if we still need this.

>  static const struct iio_info adxl345_info = {

>         .driver_module  = THIS_MODULE,

Ditto.

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

> +       if (!use_int2)
> +               ret = regmap_write(data->regmap, ADXL345_REG_INT_MAP, 0x00);
> +       else
> +               ret = regmap_write(data->regmap, ADXL345_REG_INT_MAP, 0xFF);

I would create a temporary variable to hold the value and call
regmap_write() unconditionally.


> -       return iio_device_register(indio_dev);

You are not supposed to ping-pong changes in your series. Make clear
your goal either you do like above or like below. If you choose
latter, don't alter it in previous patch.

> +       if (irq > 0) {

> +               ret =
> +               devm_request_threaded_irq(dev, irq, NULL, adxl345_irq,

Don't split lines like this.

> +                                         IRQF_TRIGGER_HIGH | IRQF_ONESHOT,

Are you sure you have threaded IRQ handler?

> +       ret = iio_device_register(indio_dev);
> +       if (ret < 0) {
> +               dev_err(dev, "iio_device_register failed: %d\n", ret);
> +               goto err_trigger_unregister;
> +       }
> +
> +       return ret;

> +err_trigger_unregister:
> +       if (data->drdy_trig)
> +               iio_trigger_unregister(data->drdy_trig);
> +
> +       return ret;

So, doesn't devm_iio_*() provide a facility to avoid this?

> @@ -229,6 +334,8 @@ int adxl345_core_remove(struct device *dev)
>         struct adxl345_data *data = iio_priv(indio_dev);
>
>         iio_device_unregister(indio_dev);
> +       if (data->drdy_trig)
> +               iio_trigger_unregister(data->drdy_trig);

Ditto.

> --- a/drivers/iio/accel/adxl345_i2c.c
> +++ b/drivers/iio/accel/adxl345_i2c.c

> -       return adxl345_core_probe(&client->dev, regmap, id ? id->name : NULL);
> +       irq = of_irq_get_byname(client->dev.of_node, "INT2");
> +       if (irq == client->irq)
> +               use_int2 = true;

Can't you use platform_get_irq() instead?


> -       return adxl345_core_probe(&spi->dev, regmap, id->name);
> +       irq = of_irq_get_byname(spi->dev.of_node, "INT2");
> +       if (irq == spi->irq)
> +               use_int2 = true;

Ditto.

P.S. Are you doing this stuff on your own or you are working for some
company? If the latter applies, please, consider to do *internal*
review first.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Mon, Mar 13, 2017 at 1:11 PM, Eva Rachel Retuya <eraretuya@gmail.com> wrote:
> Provide an all-axes read for triggered buffering.

Better description is needed.

> -static int adxl345_get_triple(struct adxl345_data *data, void *buf)
> +static int adxl345_get_triple(struct adxl345_data *data)

Ping-ponging again. This should be essentially a change in the patch
where you introduce a helper.

> +err_buffer_cleanup:
> +       iio_triggered_buffer_cleanup(indio_dev);
>  err_trigger_unregister:
>         if (data->drdy_trig)
>                 iio_trigger_unregister(data->drdy_trig);

devm_iio_*() ?

> @@ -334,6 +435,7 @@ int adxl345_core_remove(struct device *dev)
>         struct adxl345_data *data = iio_priv(indio_dev);
>
>         iio_device_unregister(indio_dev);
> +       iio_triggered_buffer_cleanup(indio_dev);
>         if (data->drdy_trig)
>                 iio_trigger_unregister(data->drdy_trig);

Ditto.

> diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c
> index 8c791b8..1e0f071 100644
> --- a/drivers/iio/accel/adxl345_i2c.c
> +++ b/drivers/iio/accel/adxl345_i2c.c
> @@ -30,6 +30,10 @@ static int adxl345_i2c_probe(struct i2c_client *client,
>         bool use_int2 = false;
>         int irq;
>
> +       if (!i2c_check_functionality(client->adapter,
> +                                    I2C_FUNC_SMBUS_READ_I2C_BLOCK))
> +               return -EOPNOTSUPP;

And if driver works before, you make a regression here.

-- 
With Best Regards,
Andy Shevchenko

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

[Eva Rachel Retuya]

On Mon, Mar 13, 2017 at 02:12:54PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 13, 2017 at 1:11 PM, Eva Rachel Retuya <eraretuya@gmail.com> wrote:
> 

Hello Andy,
Thanks for the review.

> Missed commit message is no-no!
> 
> > Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
> 
> > -int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> > -                      const char *name);
> > +int adxl345_core_probe(struct device *dev, struct regmap *regmap, int irq,
> > +                      const char *name, bool use_int2);
> 
> Hmm... And tomorrow you will add another flag and another.
> 
> No, consider to use something like
> 
> struct adxl345_chip {
>  struct device *dev;
>  struct regmap *regmap;
>  const char *name;
> }
> 
> Convert your probe to use it, and after extend for your needs.
> 
> >  #define ADXL345_DEVID                  0xE5
> >
> > +#define ADXL345_IRQ_NAME               "adxl345_event"
> 
> 
> >  struct adxl345_data {
> > +       struct iio_trigger *drdy_trig;
> >         struct regmap *regmap;
> > +       bool drdy_trig_on;
> >         u8 data_range;
> 
> drdy -> data_ready
> 
> > +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 = IRQ_NONE;
> > +       u32 int_stat;
> > +
> 
> > +       ret = regmap_read(data->regmap, ADXL345_REG_INT_SOURCE, &int_stat);
> 
> > +       if (ret < 0)
> > +               return ret;
> 
> It makes little sense AFAIU.
> 

Can you please elaborate further your comment regarding this?

> > +
> > +       if (int_stat & ADXL345_INT_DATA_READY) {
> > +               iio_trigger_poll(data->drdy_trig);
> > +               ret = IRQ_HANDLED;
> > +       }
> > +
> > +       return ret;
> 
> Useless variable ret. You may return values directly.
> 
> > +}
> > +
> > +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;
> > +       int ret;
> > +
> 
> > +       dev = regmap_get_device(data->regmap);
> 
> This may be moved to definition block.
> 
> > +       ret = regmap_update_bits(data->regmap, ADXL345_REG_INT_ENABLE,
> 
> > +                                ADXL345_INT_DATA_READY, (state ?
> > +                                ADXL345_INT_DATA_READY : 0));
> 
> No way:
>  Don't split lines like this.
>  Remove extra parens.
> 
> > +static const struct iio_trigger_ops adxl345_trigger_ops = {
> 
> > +       .owner = THIS_MODULE,
> 
> I dunno if we still need this.
> 
> >  static const struct iio_info adxl345_info = {
> 
> >         .driver_module  = THIS_MODULE,
> 
> Ditto.
> 
> >         .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.
> > +        */
> 
> > +       if (!use_int2)
> > +               ret = regmap_write(data->regmap, ADXL345_REG_INT_MAP, 0x00);
> > +       else
> > +               ret = regmap_write(data->regmap, ADXL345_REG_INT_MAP, 0xFF);
> 
> I would create a temporary variable to hold the value and call
> regmap_write() unconditionally.
> 
> 
> > -       return iio_device_register(indio_dev);
> 
> You are not supposed to ping-pong changes in your series. Make clear
> your goal either you do like above or like below. If you choose
> latter, don't alter it in previous patch.
> 

Ack. Will revise carefully with consideration to your comments in this
whole series.

> > +       if (irq > 0) {
> 
> > +               ret =
> > +               devm_request_threaded_irq(dev, irq, NULL, adxl345_irq,
> 
> Don't split lines like this.
> 
> > +                                         IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> 
> Are you sure you have threaded IRQ handler?
> 
> > +       ret = iio_device_register(indio_dev);
> > +       if (ret < 0) {
> > +               dev_err(dev, "iio_device_register failed: %d\n", ret);
> > +               goto err_trigger_unregister;
> > +       }
> > +
> > +       return ret;
> 
> > +err_trigger_unregister:
> > +       if (data->drdy_trig)
> > +               iio_trigger_unregister(data->drdy_trig);
> > +
> > +       return ret;
> 
> So, doesn't devm_iio_*() provide a facility to avoid this?
> 
> > @@ -229,6 +334,8 @@ int adxl345_core_remove(struct device *dev)
> >         struct adxl345_data *data = iio_priv(indio_dev);
> >
> >         iio_device_unregister(indio_dev);
> > +       if (data->drdy_trig)
> > +               iio_trigger_unregister(data->drdy_trig);
> 
> Ditto.
> 
> > --- a/drivers/iio/accel/adxl345_i2c.c
> > +++ b/drivers/iio/accel/adxl345_i2c.c
> 
> > -       return adxl345_core_probe(&client->dev, regmap, id ? id->name : NULL);
> > +       irq = of_irq_get_byname(client->dev.of_node, "INT2");
> > +       if (irq == client->irq)
> > +               use_int2 = true;
> 
> Can't you use platform_get_irq() instead?
> 
> 
> > -       return adxl345_core_probe(&spi->dev, regmap, id->name);
> > +       irq = of_irq_get_byname(spi->dev.of_node, "INT2");
> > +       if (irq == spi->irq)
> > +               use_int2 = true;
> 
> Ditto.
> 
> P.S. Are you doing this stuff on your own or you are working for some
> company? If the latter applies, please, consider to do *internal*
> review first.
> 

I'm doing it on my own as a project for Outreachy and have mentors to ask
for advice and/or questions.

Eva

> -- 
> With Best Regards,
> Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Mar 15, 2017 at 11:49 AM, Eva Rachel Retuya <eraretuya@gmail.com> wrote:
> On Mon, Mar 13, 2017 at 02:12:54PM +0200, Andy Shevchenko wrote:
>> On Mon, Mar 13, 2017 at 1:11 PM, Eva Rachel Retuya <eraretuya@gmail.com> wrote:

> Hello Andy,
> Thanks for the review.

You are welcome!

>> > +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 = IRQ_NONE;
>> > +       u32 int_stat;
>> > +
>>
>> > +       ret = regmap_read(data->regmap, ADXL345_REG_INT_SOURCE, &int_stat);
>>
>> > +       if (ret < 0)
>> > +               return ret;
>>
>> It makes little sense AFAIU.
>>
>
> Can you please elaborate further your comment regarding this?

It's defined as enum and returned value is not in a range.

Also it would be useful to look at kernel/irq/manage.c for the
implementation details.

>> P.S. Are you doing this stuff on your own or you are working for some
>> company? If the latter applies, please, consider to do *internal*
>> review first.

> I'm doing it on my own as a project for Outreachy and have mentors to ask
> for advice and/or questions.

Ah, okay, that is fine then!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/4] iio: accel: adxl345_core: Introduce set_mode, drdy & get_triple functions

[Jonathan Cameron]

On 13/03/17 11:11, 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 drdy function that monitors the INT_SOURCE register of
> this bit. This is done to ensure consistent readings.
> 
> Make use of drdy in read_raw and also refactor the read to perform an
> all-axes read instead through get_triple. This function will come in
> handy when buffered reading is introduced.
> 
> Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
A few bits from me to add to Andy's review.
> ---
>  drivers/iio/accel/adxl345_core.c | 116 +++++++++++++++++++++++++++++----------
>  1 file changed, 88 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 9ccb582..7eee31d 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
> @@ -47,6 +53,58 @@ struct adxl345_data {
>  	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_get_triple(struct adxl345_data *data, void *buf)
> +{
> +	struct device *dev = regmap_get_device(data->regmap);
> +	int ret;
> +
> +	ret = regmap_bulk_read(data->regmap, ADXL345_REG_DATAX0, buf,
> +			       sizeof(__le16) * 3);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to read the data registers, %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int adxl345_drdy(struct adxl345_data *data)
> +{
> +	struct device *dev = regmap_get_device(data->regmap);
> +	int tries = 5;
> +	u32 regval;
> +	int ret;
> +
> +	while (tries--) {
> +		ret = regmap_read(data->regmap, ADXL345_REG_INT_SOURCE,
> +				  &regval);
> +		if (ret < 0)
> +			return ret;
> +		if ((regval & ADXL345_INT_DATA_READY) ==
> +		    ADXL345_INT_DATA_READY)
> +			return 0;
> +
> +		msleep(20);
> +	}
> +	dev_err(dev, "Data is not yet ready\n");
> +
Hmm. Does it definitely indicate a hardware failure?  If so fair enough with
EIO. However, the message should perhaps say this.  Sounds like an EBUSY
from that error message.
> +	return -EIO;
> +}
> +
>  #define ADXL345_CHANNEL(reg, axis) {					\
>  	.type = IIO_ACCEL,						\
>  	.modified = 1,							\
> @@ -67,22 +125,37 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
>  			    int *val, int *val2, long mask)
>  {
>  	struct adxl345_data *data = iio_priv(indio_dev);
> -	__le16 regval;
> +	s16 regval[3];
>  	int ret;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> -		/*
> -		 * Data is stored in adjacent registers:
> -		 * ADXL345_REG_DATA(X0/Y0/Z0) contain the least significant byte
> -		 * and ADXL345_REG_DATA(X0/Y0/Z0) + 1 the most significant byte
> -		 */
> -		ret = regmap_bulk_read(data->regmap, chan->address, &regval,
> -				       sizeof(regval));
> +		ret = adxl345_set_mode(data, ADXL345_POWER_CTL_MEASURE);
> +		if (ret < 0)
> +			return ret;
> +		ret = adxl345_drdy(data);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = adxl345_get_triple(data, &regval);
>  		if (ret < 0)
>  			return ret;
>  
> -		*val = sign_extend32(le16_to_cpu(regval), 12);
This function should as Andy pointed out still be returning in
cpu endianness.  So you can't just drop it like this.
> +		switch (chan->address) {
> +		case ADXL345_REG_DATAX0:
> +			*val = regval[0];
> +			break;
> +		case ADXL345_REG_DATAY0:
> +			*val = regval[1];
> +			break;
> +		case ADXL345_REG_DATAZ0:
> +			*val = regval[2];
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
> +
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_SCALE:
>  		*val = 0;
> @@ -126,9 +199,12 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  	data = iio_priv(indio_dev);
>  	dev_set_drvdata(dev, indio_dev);
>  	data->regmap = regmap;
> +	/* Make sure sensor is in standby mode before configuring registers */
> +	ret = adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
> +	if (ret < 0)
> +		return ret;
Is this one specific to the buffered support?  If not, please break it out
as a precursor patch.
>  	/* Enable full-resolution mode */
>  	data->data_range = ADXL345_DATA_FORMAT_FULL_RES;
Should be seeing random white space changes in a patch doing other things.
> -
>  	ret = regmap_write(data->regmap, ADXL345_REG_DATA_FORMAT,
>  			   data->data_range);
>  	if (ret < 0) {
> @@ -143,22 +219,7 @@ 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) {
> -		dev_err(dev, "iio_device_register failed: %d\n", ret);
> -		regmap_write(data->regmap, ADXL345_REG_POWER_CTL,
> -			     ADXL345_POWER_CTL_STANDBY);
> -	}
> -
> -	return ret;
> +	return iio_device_register(indio_dev);
>  }
>  EXPORT_SYMBOL_GPL(adxl345_core_probe);
>  
> @@ -169,8 +230,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);
>  
> 

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

[Jonathan Cameron]

On 13/03/17 12:16, Andy Shevchenko wrote:
> On Mon, Mar 13, 2017 at 1:11 PM, Eva Rachel Retuya <eraretuya@gmail.com> wrote:
>> Provide an all-axes read for triggered buffering.
> 
> Better description is needed.
> 
>> -static int adxl345_get_triple(struct adxl345_data *data, void *buf)
>> +static int adxl345_get_triple(struct adxl345_data *data)
> 
> Ping-ponging again. This should be essentially a change in the patch
> where you introduce a helper.
> 
>> +err_buffer_cleanup:
>> +       iio_triggered_buffer_cleanup(indio_dev);
>>  err_trigger_unregister:
>>         if (data->drdy_trig)
>>                 iio_trigger_unregister(data->drdy_trig);
> 
> devm_iio_*() ?
> 
>> @@ -334,6 +435,7 @@ int adxl345_core_remove(struct device *dev)
>>         struct adxl345_data *data = iio_priv(indio_dev);
>>
>>         iio_device_unregister(indio_dev);
>> +       iio_triggered_buffer_cleanup(indio_dev);
>>         if (data->drdy_trig)
>>                 iio_trigger_unregister(data->drdy_trig);
> 
> Ditto.
> 
>> diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c
>> index 8c791b8..1e0f071 100644
>> --- a/drivers/iio/accel/adxl345_i2c.c
>> +++ b/drivers/iio/accel/adxl345_i2c.c
>> @@ -30,6 +30,10 @@ static int adxl345_i2c_probe(struct i2c_client *client,
>>         bool use_int2 = false;
>>         int irq;
>>
>> +       if (!i2c_check_functionality(client->adapter,
>> +                                    I2C_FUNC_SMBUS_READ_I2C_BLOCK))
>> +               return -EOPNOTSUPP;
> 
> And if driver works before, you make a regression here.
Absolutely. Need a fallback if it's not supported...

Jonathan
> 

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

[Rob Herring]

On Mon, Mar 13, 2017 at 07:11:34PM +0800, Eva Rachel Retuya wrote:
> Add interrupt-names property in order to specify interrupt pin in use.
> 
> Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
> ---
>  Documentation/devicetree/bindings/iio/accel/adxl345.txt | 4 ++++
>  1 file changed, 4 insertions(+)

Acked-by: Rob Herring <robh@kernel.org>

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

[Lars-Peter Clausen]

On 03/15/2017 10:50 PM, Jonathan Cameron wrote:
> On 13/03/17 12:16, Andy Shevchenko wrote:
>> On Mon, Mar 13, 2017 at 1:11 PM, Eva Rachel Retuya <eraretuya@gmail.com> wrote:
>>> Provide an all-axes read for triggered buffering.
>>
>> Better description is needed.
>>
>>> -static int adxl345_get_triple(struct adxl345_data *data, void *buf)
>>> +static int adxl345_get_triple(struct adxl345_data *data)
>>
>> Ping-ponging again. This should be essentially a change in the patch
>> where you introduce a helper.
>>
>>> +err_buffer_cleanup:
>>> +       iio_triggered_buffer_cleanup(indio_dev);
>>>  err_trigger_unregister:
>>>         if (data->drdy_trig)
>>>                 iio_trigger_unregister(data->drdy_trig);
>>
>> devm_iio_*() ?
>>
>>> @@ -334,6 +435,7 @@ int adxl345_core_remove(struct device *dev)
>>>         struct adxl345_data *data = iio_priv(indio_dev);
>>>
>>>         iio_device_unregister(indio_dev);
>>> +       iio_triggered_buffer_cleanup(indio_dev);
>>>         if (data->drdy_trig)
>>>                 iio_trigger_unregister(data->drdy_trig);
>>
>> Ditto.
>>
>>> diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c
>>> index 8c791b8..1e0f071 100644
>>> --- a/drivers/iio/accel/adxl345_i2c.c
>>> +++ b/drivers/iio/accel/adxl345_i2c.c
>>> @@ -30,6 +30,10 @@ static int adxl345_i2c_probe(struct i2c_client *client,
>>>         bool use_int2 = false;
>>>         int irq;
>>>
>>> +       if (!i2c_check_functionality(client->adapter,
>>> +                                    I2C_FUNC_SMBUS_READ_I2C_BLOCK))
>>> +               return -EOPNOTSUPP;
>>
>> And if driver works before, you make a regression here.
> Absolutely. Need a fallback if it's not supported...

I believe regmap has a fallback, so that check could simply be removed, I guess.

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

[Eva Rachel Retuya]

On Mon, Mar 20, 2017 at 08:46:11PM +0100, Lars-Peter Clausen wrote:
> On 03/15/2017 10:50 PM, Jonathan Cameron wrote:
> > On 13/03/17 12:16, Andy Shevchenko wrote:
> >> On Mon, Mar 13, 2017 at 1:11 PM, Eva Rachel Retuya <eraretuya@gmail.com> wrote:
> >>> Provide an all-axes read for triggered buffering.
> >>
> >> Better description is needed.
> >>
> >>> -static int adxl345_get_triple(struct adxl345_data *data, void *buf)
> >>> +static int adxl345_get_triple(struct adxl345_data *data)
> >>
> >> Ping-ponging again. This should be essentially a change in the patch
> >> where you introduce a helper.
> >>
> >>> +err_buffer_cleanup:
> >>> +       iio_triggered_buffer_cleanup(indio_dev);
> >>>  err_trigger_unregister:
> >>>         if (data->drdy_trig)
> >>>                 iio_trigger_unregister(data->drdy_trig);
> >>
> >> devm_iio_*() ?
> >>
> >>> @@ -334,6 +435,7 @@ int adxl345_core_remove(struct device *dev)
> >>>         struct adxl345_data *data = iio_priv(indio_dev);
> >>>
> >>>         iio_device_unregister(indio_dev);
> >>> +       iio_triggered_buffer_cleanup(indio_dev);
> >>>         if (data->drdy_trig)
> >>>                 iio_trigger_unregister(data->drdy_trig);
> >>
> >> Ditto.
> >>
> >>> diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c
> >>> index 8c791b8..1e0f071 100644
> >>> --- a/drivers/iio/accel/adxl345_i2c.c
> >>> +++ b/drivers/iio/accel/adxl345_i2c.c
> >>> @@ -30,6 +30,10 @@ static int adxl345_i2c_probe(struct i2c_client *client,
> >>>         bool use_int2 = false;
> >>>         int irq;
> >>>
> >>> +       if (!i2c_check_functionality(client->adapter,
> >>> +                                    I2C_FUNC_SMBUS_READ_I2C_BLOCK))
> >>> +               return -EOPNOTSUPP;
> >>
> >> And if driver works before, you make a regression here.
> > Absolutely. Need a fallback if it's not supported...
> 
> I believe regmap has a fallback, so that check could simply be removed, I guess.
> 

Yes, regmap does this check. I've seen it while working on the patchset.
I checked other drivers doing regmap and some have this check_functionality
check so I chose to include it because I'm not sure.

Will remove it on the next version.

Thanks,
Eva