[PATCH 0/3] iio: gyro: adxrs290: Add triggered buffer & debugfs support

[PATCH 0/3] iio: gyro: adxrs290: Add triggered buffer & debugfs support

[Nishant Malpani]

Introduce DATA_RDY trigger for triggered buffer setup; this enables continuous
data capture. Additionally, add support for direct register access using the debugfs 
iio interface. 

The device-tree bindings documentation illustrates an example of using a GPIO irq
line to trigger a data capture.

Nishant Malpani (3):
  iio: gyro: adxrs290: Add triggered buffer support
  dt-bindings: iio: gyro: adxrs290: Add required interrupts property
  iio: gyro: adxrs290: Add debugfs register access support

 .../bindings/iio/gyroscope/adi,adxrs290.yaml  |   8 +
 drivers/iio/gyro/Kconfig                      |   2 +
 drivers/iio/gyro/adxrs290.c                   | 291 +++++++++++++++++-
 3 files changed, 287 insertions(+), 14 deletions(-)

-- 
2.20.1

[PATCH 1/3] iio: gyro: adxrs290: Add triggered buffer support

[Nishant Malpani]

Provide a way for continuous data capture by setting up buffer support. The
data ready signal exposed at the SYNC pin of the ADXRS290 is exploited as
a hardware interrupt which triggers to fill the buffer.

Triggered buffer setup was tested with both hardware trigger (DATA_RDY) and
software triggers (sysfs-trig & hrtimer).

Signed-off-by: Nishant Malpani <nish.malpani25@gmail.com>
---
 drivers/iio/gyro/Kconfig    |   2 +
 drivers/iio/gyro/adxrs290.c | 272 ++++++++++++++++++++++++++++++++++--
 2 files changed, 260 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/gyro/Kconfig b/drivers/iio/gyro/Kconfig
index 024a34139875..5824f2edf975 100644
--- a/drivers/iio/gyro/Kconfig
+++ b/drivers/iio/gyro/Kconfig
@@ -44,6 +44,8 @@ config ADIS16260
 config ADXRS290
 	tristate "Analog Devices ADXRS290 Dual-Axis MEMS Gyroscope SPI driver"
 	depends on SPI
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
 	help
 	  Say yes here to build support for Analog Devices ADXRS290 programmable
 	  digital output gyroscope.
diff --git a/drivers/iio/gyro/adxrs290.c b/drivers/iio/gyro/adxrs290.c
index ff989536d2fb..25046590761e 100644
--- a/drivers/iio/gyro/adxrs290.c
+++ b/drivers/iio/gyro/adxrs290.c
@@ -13,8 +13,12 @@
 #include <linux/module.h>
 #include <linux/spi/spi.h>
 
+#include <linux/iio/buffer.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
 
 #define ADXRS290_ADI_ID		0xAD
 #define ADXRS290_MEMS_ID	0x1D
@@ -35,7 +39,9 @@
 #define ADXRS290_READ		BIT(7)
 #define ADXRS290_TSM		BIT(0)
 #define ADXRS290_MEASUREMENT	BIT(1)
-#define ADXRS290_SYNC		GENMASK(1, 0)
+#define ADXRS290_DATA_RDY_OUT	BIT(0)
+#define ADXRS290_SYNC_MASK	GENMASK(1, 0)
+#define ADXRS290_SYNC(x)	FIELD_PREP(ADXRS290_SYNC_MASK, x)
 #define ADXRS290_LPF_MASK	GENMASK(2, 0)
 #define ADXRS290_LPF(x)		FIELD_PREP(ADXRS290_LPF_MASK, x)
 #define ADXRS290_HPF_MASK	GENMASK(7, 4)
@@ -50,6 +56,13 @@ enum adxrs290_mode {
 	ADXRS290_MODE_MEASUREMENT,
 };
 
+enum adxrs290_scan_index {
+	ADXRS290_IDX_X,
+	ADXRS290_IDX_Y,
+	ADXRS290_IDX_TEMP,
+	ADXRS290_IDX_TS,
+};
+
 struct adxrs290_state {
 	struct spi_device	*spi;
 	/* Serialize reads and their subsequent processing */
@@ -57,6 +70,12 @@ struct adxrs290_state {
 	enum adxrs290_mode	mode;
 	unsigned int		lpf_3db_freq_idx;
 	unsigned int		hpf_3db_freq_idx;
+	struct iio_trigger      *dready_trig;
+	/* Ensure correct alignment of timestamp when present */
+	struct {
+		s16 channels[3];
+		s64 ts __aligned(8);
+	}                       buffer;
 };
 
 /*
@@ -192,6 +211,52 @@ static int adxrs290_set_filter_freq(struct iio_dev *indio_dev,
 	return adxrs290_spi_write_reg(st->spi, ADXRS290_REG_FILTER, val);
 }
 
+static int adxrs290_set_mode(struct iio_dev *indio_dev, enum adxrs290_mode mode)
+{
+	struct adxrs290_state *st = iio_priv(indio_dev);
+	int val, ret;
+
+	mutex_lock(&st->lock);
+
+	if (st->mode == mode) {
+		ret = 0;
+		goto done;
+	}
+
+	val = spi_w8r8(st->spi, ADXRS290_READ_REG(ADXRS290_REG_POWER_CTL));
+	if (val < 0) {
+		ret = val;
+		goto done;
+	}
+
+	switch (mode) {
+	case ADXRS290_MODE_STANDBY:
+		val &= ~ADXRS290_MEASUREMENT;
+		break;
+	case ADXRS290_MODE_MEASUREMENT:
+		val |= ADXRS290_MEASUREMENT;
+		break;
+	default:
+		ret = -EINVAL;
+		goto done;
+	}
+
+	ret = adxrs290_spi_write_reg(st->spi,
+				     ADXRS290_REG_POWER_CTL,
+				     val);
+	if (ret < 0) {
+		dev_err(&st->spi->dev, "unable to set mode: %d\n", ret);
+		goto done;
+	}
+
+	/* update cached mode */
+	st->mode = mode;
+
+done:
+	mutex_unlock(&st->lock);
+	return ret;
+}
+
 static int adxrs290_initial_setup(struct iio_dev *indio_dev)
 {
 	struct adxrs290_state *st = iio_priv(indio_dev);
@@ -203,6 +268,13 @@ static int adxrs290_initial_setup(struct iio_dev *indio_dev)
 				      ADXRS290_MEASUREMENT | ADXRS290_TSM);
 }
 
+static void adxrs290_chip_off_action(void *data)
+{
+	struct iio_dev *indio_dev = data;
+
+	adxrs290_set_mode(indio_dev, ADXRS290_MODE_STANDBY);
+}
+
 static int adxrs290_read_raw(struct iio_dev *indio_dev,
 			     struct iio_chan_spec const *chan,
 			     int *val,
@@ -215,24 +287,34 @@ static int adxrs290_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;
+
 		switch (chan->type) {
 		case IIO_ANGL_VEL:
 			ret = adxrs290_get_rate_data(indio_dev,
 						     ADXRS290_READ_REG(chan->address),
 						     val);
 			if (ret < 0)
-				return ret;
+				goto err_release;
 
-			return IIO_VAL_INT;
+			ret = IIO_VAL_INT;
+			break;
 		case IIO_TEMP:
 			ret = adxrs290_get_temp_data(indio_dev, val);
 			if (ret < 0)
-				return ret;
+				goto err_release;
 
-			return IIO_VAL_INT;
+			ret = IIO_VAL_INT;
+			break;
 		default:
-			return -EINVAL;
+			ret = -EINVAL;
+			break;
 		}
+err_release:
+		iio_device_release_direct_mode(indio_dev);
+		return ret;
 	case IIO_CHAN_INFO_SCALE:
 		switch (chan->type) {
 		case IIO_ANGL_VEL:
@@ -279,34 +361,53 @@ static int adxrs290_write_raw(struct iio_dev *indio_dev,
 			      long mask)
 {
 	struct adxrs290_state *st = iio_priv(indio_dev);
-	int lpf_idx, hpf_idx;
+	int ret, lpf_idx, hpf_idx;
+
+	ret = iio_device_claim_direct_mode(indio_dev);
+	if (ret)
+		return ret;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
 		lpf_idx = adxrs290_find_match(adxrs290_lpf_3db_freq_hz_table,
 					      ARRAY_SIZE(adxrs290_lpf_3db_freq_hz_table),
 					      val, val2);
-		if (lpf_idx < 0)
-			return -EINVAL;
+		if (lpf_idx < 0) {
+			ret = -EINVAL;
+			goto err_release;
+		}
+
 		/* caching the updated state of the low-pass filter */
 		st->lpf_3db_freq_idx = lpf_idx;
 		/* retrieving the current state of the high-pass filter */
 		hpf_idx = st->hpf_3db_freq_idx;
-		return adxrs290_set_filter_freq(indio_dev, lpf_idx, hpf_idx);
+		ret = adxrs290_set_filter_freq(indio_dev, lpf_idx, hpf_idx);
+		break;
+
 	case IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY:
 		hpf_idx = adxrs290_find_match(adxrs290_hpf_3db_freq_hz_table,
 					      ARRAY_SIZE(adxrs290_hpf_3db_freq_hz_table),
 					      val, val2);
-		if (hpf_idx < 0)
-			return -EINVAL;
+		if (hpf_idx < 0) {
+			ret = -EINVAL;
+			goto err_release;
+		}
+
 		/* caching the updated state of the high-pass filter */
 		st->hpf_3db_freq_idx = hpf_idx;
 		/* retrieving the current state of the low-pass filter */
 		lpf_idx = st->lpf_3db_freq_idx;
-		return adxrs290_set_filter_freq(indio_dev, lpf_idx, hpf_idx);
+		ret = adxrs290_set_filter_freq(indio_dev, lpf_idx, hpf_idx);
+		break;
+
+	default:
+		ret = -EINVAL;
+		break;
 	}
 
-	return -EINVAL;
+err_release:
+	iio_device_release_direct_mode(indio_dev);
+	return ret;
 }
 
 static int adxrs290_read_avail(struct iio_dev *indio_dev,
@@ -334,6 +435,68 @@ static int adxrs290_read_avail(struct iio_dev *indio_dev,
 	}
 }
 
+static int adxrs290_data_rdy_trigger_set_state(struct iio_trigger *trig, bool state)
+{
+	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+	struct adxrs290_state *st = iio_priv(indio_dev);
+	int ret;
+	u8 val;
+
+	val = (state ? ADXRS290_SYNC(ADXRS290_DATA_RDY_OUT) : 0);
+
+	ret = adxrs290_spi_write_reg(st->spi, ADXRS290_REG_DATA_RDY, val);
+	if (ret < 0)
+		dev_err(&st->spi->dev, "failed to start data ready interrupt\n");
+
+	return ret;
+}
+
+static int adxrs290_reset_trig(struct iio_trigger *trig)
+{
+	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+	int val;
+
+	/*
+	 * Data ready interrupt is reset after a read of the data registers.
+	 * Here, we only read the 16b DATAY registers as that marks the end of
+	 * a read of the data registers and initiates a reset for the interrupt line.
+	 */
+	return adxrs290_get_rate_data(indio_dev, ADXRS290_READ_REG(ADXRS290_REG_DATAY0), &val);
+}
+
+static const struct iio_trigger_ops adxrs290_trigger_ops = {
+	.set_trigger_state = &adxrs290_data_rdy_trigger_set_state,
+	.validate_device = &iio_trigger_validate_own_device,
+	.try_reenable = &adxrs290_reset_trig,
+};
+
+static irqreturn_t adxrs290_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct adxrs290_state *st = iio_priv(indio_dev);
+	struct spi_device *spi = st->spi;
+	u8 tx = ADXRS290_READ_REG(ADXRS290_REG_DATAX0);
+	int ret;
+
+	mutex_lock(&st->lock);
+
+	/* exercise a bulk data capture starting from reg DATAX0... */
+	ret = spi_write_then_read(spi, &tx, sizeof(tx), st->buffer.channels,
+				  sizeof(st->buffer.channels));
+	if (ret < 0)
+		goto done;
+
+	iio_push_to_buffers_with_timestamp(indio_dev, &st->buffer,
+					   pf->timestamp);
+
+done:
+	mutex_unlock(&st->lock);
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
 #define ADXRS290_ANGL_VEL_CHANNEL(reg, axis) {				\
 	.type = IIO_ANGL_VEL,						\
 	.address = reg,							\
@@ -346,6 +509,13 @@ static int adxrs290_read_avail(struct iio_dev *indio_dev,
 	.info_mask_shared_by_type_available =				\
 	BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) |		\
 	BIT(IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY),		\
+	.scan_index = ADXRS290_IDX_##axis,				\
+	.scan_type = {                                                  \
+		.sign = 's',                                            \
+		.realbits = 16,                                         \
+		.storagebits = 16,                                      \
+		.endianness = IIO_LE,					\
+	},                                                              \
 }
 
 static const struct iio_chan_spec adxrs290_channels[] = {
@@ -356,7 +526,21 @@ static const struct iio_chan_spec adxrs290_channels[] = {
 		.address = ADXRS290_REG_TEMP0,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 		BIT(IIO_CHAN_INFO_SCALE),
+		.scan_index = ADXRS290_IDX_TEMP,
+		.scan_type = {
+			.sign = 's',
+			.realbits = 12,
+			.storagebits = 16,
+			.shift = 0,
+			.endianness = IIO_LE,
+		},
 	},
+	IIO_CHAN_SOFT_TIMESTAMP(ADXRS290_IDX_TS),
+};
+
+static const unsigned long adxrs290_avail_scan_masks[] = {
+	BIT(ADXRS290_IDX_X) | BIT(ADXRS290_IDX_Y) | BIT(ADXRS290_IDX_TEMP),
+	0
 };
 
 static const struct iio_info adxrs290_info = {
@@ -365,6 +549,47 @@ static const struct iio_info adxrs290_info = {
 	.read_avail = &adxrs290_read_avail,
 };
 
+static int adxrs290_probe_trigger(struct iio_dev *indio_dev)
+{
+	struct adxrs290_state *st = iio_priv(indio_dev);
+	int ret;
+
+	if (!st->spi->irq) {
+		dev_info(&st->spi->dev, "no irq, using polling\n");
+		return 0;
+	}
+
+	st->dready_trig = devm_iio_trigger_alloc(&st->spi->dev,
+						 "%s-dev%d",
+						 indio_dev->name,
+						 indio_dev->id);
+	if (!st->dready_trig)
+		return -ENOMEM;
+
+	st->dready_trig->dev.parent = &st->spi->dev;
+	st->dready_trig->ops = &adxrs290_trigger_ops;
+	iio_trigger_set_drvdata(st->dready_trig, indio_dev);
+
+	ret = devm_request_irq(&st->spi->dev, st->spi->irq,
+			       &iio_trigger_generic_data_rdy_poll,
+			       IRQF_ONESHOT,
+			       "adxrs290_irq", st->dready_trig);
+	if (ret < 0) {
+		dev_err(&st->spi->dev, "request irq %d failed\n", st->spi->irq);
+		return ret;
+	}
+
+	ret = devm_iio_trigger_register(&st->spi->dev, st->dready_trig);
+	if (ret) {
+		dev_err(&st->spi->dev, "iio trigger register failed\n");
+		return ret;
+	}
+
+	indio_dev->trig = iio_trigger_get(st->dready_trig);
+
+	return 0;
+}
+
 static int adxrs290_probe(struct spi_device *spi)
 {
 	struct iio_dev *indio_dev;
@@ -384,6 +609,7 @@ static int adxrs290_probe(struct spi_device *spi)
 	indio_dev->channels = adxrs290_channels;
 	indio_dev->num_channels = ARRAY_SIZE(adxrs290_channels);
 	indio_dev->info = &adxrs290_info;
+	indio_dev->available_scan_masks = adxrs290_avail_scan_masks;
 
 	mutex_init(&st->lock);
 
@@ -416,6 +642,12 @@ static int adxrs290_probe(struct spi_device *spi)
 	/* max transition time to measurement mode */
 	msleep(ADXRS290_MAX_TRANSITION_TIME_MS);
 
+	ret = devm_add_action_or_reset(&spi->dev,
+				       adxrs290_chip_off_action,
+				       indio_dev);
+	if (ret < 0)
+		return ret;
+
 	ret = adxrs290_get_3db_freq(indio_dev, &val, &val2);
 	if (ret < 0)
 		return ret;
@@ -423,6 +655,18 @@ static int adxrs290_probe(struct spi_device *spi)
 	st->lpf_3db_freq_idx = val;
 	st->hpf_3db_freq_idx = val2;
 
+	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
+					      &iio_pollfunc_store_time,
+					      &adxrs290_trigger_handler, NULL);
+	if (ret < 0) {
+		dev_err(&spi->dev, "iio triggered buffer setup failed\n");
+		return ret;
+	}
+
+	ret = adxrs290_probe_trigger(indio_dev);
+	if (ret < 0)
+		return ret;
+
 	return devm_iio_device_register(&spi->dev, indio_dev);
 }
 
-- 
2.20.1

[PATCH 2/3] dt-bindings: iio: gyro: adxrs290: Add required interrupts property

[Nishant Malpani]

Append 'interrupts' as a required property and provide a suitable example
for using a GPIO interrupt line.

Signed-off-by: Nishant Malpani <nish.malpani25@gmail.com>
---
 .../devicetree/bindings/iio/gyroscope/adi,adxrs290.yaml   | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/gyroscope/adi,adxrs290.yaml b/Documentation/devicetree/bindings/iio/gyroscope/adi,adxrs290.yaml
index 61adb2c2454b..cae593dd1ba7 100644
--- a/Documentation/devicetree/bindings/iio/gyroscope/adi,adxrs290.yaml
+++ b/Documentation/devicetree/bindings/iio/gyroscope/adi,adxrs290.yaml
@@ -28,17 +28,23 @@ properties:
 
   spi-cpha: true
 
+  interrupts:
+    maxItems: 1
+
 required:
   - compatible
   - reg
   - spi-max-frequency
   - spi-cpol
   - spi-cpha
+  - interrupts
 
 additionalProperties: false
 
 examples:
   - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
     spi {
         #address-cells = <1>;
         #size-cells = <0>;
@@ -48,6 +54,8 @@ examples:
                    spi-max-frequency = <5000000>;
                    spi-cpol;
                    spi-cpha;
+                   interrupt-parent = <&gpio>;
+                   interrupts = <25 IRQ_TYPE_EDGE_RISING>;
         };
     };
 ...
-- 
2.20.1

[PATCH 3/3] iio: gyro: adxrs290: Add debugfs register access support

[Nishant Malpani]

Extend support to read/write byte data from/to the device using
debugfs iio interface.

Signed-off-by: Nishant Malpani <nish.malpani25@gmail.com>
---
 drivers/iio/gyro/adxrs290.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/iio/gyro/adxrs290.c b/drivers/iio/gyro/adxrs290.c
index 25046590761e..b0050cdd2b90 100644
--- a/drivers/iio/gyro/adxrs290.c
+++ b/drivers/iio/gyro/adxrs290.c
@@ -435,6 +435,24 @@ static int adxrs290_read_avail(struct iio_dev *indio_dev,
 	}
 }
 
+static int adxrs290_reg_access(struct iio_dev *indio_dev, unsigned int reg,
+			       unsigned int writeval, unsigned int *readval)
+{
+	struct adxrs290_state *st = iio_priv(indio_dev);
+	int ret;
+
+	if (!readval)
+		return adxrs290_spi_write_reg(st->spi, reg, writeval);
+
+	ret = spi_w8r8(st->spi, ADXRS290_READ_REG(reg));
+	if (ret < 0)
+		return ret;
+
+	*readval = ret;
+
+	return 0;
+}
+
 static int adxrs290_data_rdy_trigger_set_state(struct iio_trigger *trig, bool state)
 {
 	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
@@ -547,6 +565,7 @@ static const struct iio_info adxrs290_info = {
 	.read_raw = &adxrs290_read_raw,
 	.write_raw = &adxrs290_write_raw,
 	.read_avail = &adxrs290_read_avail,
+	.debugfs_reg_access = &adxrs290_reg_access,
 };
 
 static int adxrs290_probe_trigger(struct iio_dev *indio_dev)
-- 
2.20.1

Re: [PATCH 1/3] iio: gyro: adxrs290: Add triggered buffer support

[Andy Shevchenko]

On Tue, Aug 25, 2020 at 4:11 PM Nishant Malpani
<nish.malpani25@gmail.com> wrote:
>
> Provide a way for continuous data capture by setting up buffer support. The
> data ready signal exposed at the SYNC pin of the ADXRS290 is exploited as
> a hardware interrupt which triggers to fill the buffer.
>
> Triggered buffer setup was tested with both hardware trigger (DATA_RDY) and
> software triggers (sysfs-trig & hrtimer).

...

> +static int adxrs290_set_mode(struct iio_dev *indio_dev, enum adxrs290_mode mode)
> +{
> +       struct adxrs290_state *st = iio_priv(indio_dev);
> +       int val, ret;
> +
> +       mutex_lock(&st->lock);
> +
> +       if (st->mode == mode) {

> +               ret = 0;

Can be done outside of mutex.

> +               goto done;
> +       }
> +

> +       val = spi_w8r8(st->spi, ADXRS290_READ_REG(ADXRS290_REG_POWER_CTL));
> +       if (val < 0) {
> +               ret = val;
> +               goto done;
> +       }

Consider other way around
 ret = ...
 ...
 val = ret;

> +       switch (mode) {
> +       case ADXRS290_MODE_STANDBY:
> +               val &= ~ADXRS290_MEASUREMENT;
> +               break;
> +       case ADXRS290_MODE_MEASUREMENT:
> +               val |= ADXRS290_MEASUREMENT;
> +               break;
> +       default:
> +               ret = -EINVAL;
> +               goto done;
> +       }
> +
> +       ret = adxrs290_spi_write_reg(st->spi,
> +                                    ADXRS290_REG_POWER_CTL,
> +                                    val);
> +       if (ret < 0) {
> +               dev_err(&st->spi->dev, "unable to set mode: %d\n", ret);
> +               goto done;
> +       }
> +
> +       /* update cached mode */
> +       st->mode = mode;
> +
> +done:
> +       mutex_unlock(&st->lock);
> +       return ret;
> +}

...

> +                               goto err_release;
>
> -                       return IIO_VAL_INT;
> +                       ret = IIO_VAL_INT;
> +                       break;
>                 default:
> -                       return -EINVAL;
> +                       ret = -EINVAL;
> +                       break;
>                 }

> +err_release:

I didn't get the purpose of this. Wasn't the break statement enough?

> +               iio_device_release_direct_mode(indio_dev);
> +               return ret;
>         case IIO_CHAN_INFO_SCALE:
>                 switch (chan->type) {
>                 case IIO_ANGL_VEL:

...

> +                       goto err_release;

Ditto.

> +               }
> +
>                 /* caching the updated state of the high-pass filter */
>                 st->hpf_3db_freq_idx = hpf_idx;
>                 /* retrieving the current state of the low-pass filter */
>                 lpf_idx = st->lpf_3db_freq_idx;
> -               return adxrs290_set_filter_freq(indio_dev, lpf_idx, hpf_idx);
> +               ret = adxrs290_set_filter_freq(indio_dev, lpf_idx, hpf_idx);
> +               break;
> +
> +       default:
> +               ret = -EINVAL;
> +               break;
>         }
>
> -       return -EINVAL;
> +err_release:
> +       iio_device_release_direct_mode(indio_dev);
> +       return ret;
>  }

...

> +       val = (state ? ADXRS290_SYNC(ADXRS290_DATA_RDY_OUT) : 0);

Purpose of outer parentheses?

...

> +static int adxrs290_probe_trigger(struct iio_dev *indio_dev)
> +{
> +       struct adxrs290_state *st = iio_priv(indio_dev);
> +       int ret;
> +
> +       if (!st->spi->irq) {
> +               dev_info(&st->spi->dev, "no irq, using polling\n");
> +               return 0;
> +       }
> +
> +       st->dready_trig = devm_iio_trigger_alloc(&st->spi->dev,
> +                                                "%s-dev%d",
> +                                                indio_dev->name,
> +                                                indio_dev->id);
> +       if (!st->dready_trig)
> +               return -ENOMEM;
> +
> +       st->dready_trig->dev.parent = &st->spi->dev;
> +       st->dready_trig->ops = &adxrs290_trigger_ops;
> +       iio_trigger_set_drvdata(st->dready_trig, indio_dev);
> +
> +       ret = devm_request_irq(&st->spi->dev, st->spi->irq,
> +                              &iio_trigger_generic_data_rdy_poll,
> +                              IRQF_ONESHOT,
> +                              "adxrs290_irq", st->dready_trig);
> +       if (ret < 0) {

> +               dev_err(&st->spi->dev, "request irq %d failed\n", st->spi->irq);
> +               return ret;

return dev_err_probe(...);

> +       }
> +
> +       ret = devm_iio_trigger_register(&st->spi->dev, st->dready_trig);
> +       if (ret) {

> +               dev_err(&st->spi->dev, "iio trigger register failed\n");
> +               return ret;

return dev_err_probe(...);

> +       }
> +
> +       indio_dev->trig = iio_trigger_get(st->dready_trig);
> +
> +       return 0;
> +}

...

> +       ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
> +                                             &iio_pollfunc_store_time,
> +                                             &adxrs290_trigger_handler, NULL);
> +       if (ret < 0) {

> +               dev_err(&spi->dev, "iio triggered buffer setup failed\n");
> +               return ret;

return dev_err_probe(...);

> +       }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] iio: gyro: adxrs290: Add triggered buffer support

[Jonathan Cameron]

On Tue, 25 Aug 2020 18:17:09 +0530
Nishant Malpani <nish.malpani25@gmail.com> wrote:

> Provide a way for continuous data capture by setting up buffer support. The
> data ready signal exposed at the SYNC pin of the ADXRS290 is exploited as
> a hardware interrupt which triggers to fill the buffer.
> 
> Triggered buffer setup was tested with both hardware trigger (DATA_RDY) and
> software triggers (sysfs-trig & hrtimer).
> 
> Signed-off-by: Nishant Malpani <nish.malpani25@gmail.com>

I've tried not to overlap with Andy's comments but might have hit on a few.
Anyhow, comments inline.

Jonathan


> ---
>  drivers/iio/gyro/Kconfig    |   2 +
>  drivers/iio/gyro/adxrs290.c | 272 ++++++++++++++++++++++++++++++++++--
>  2 files changed, 260 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/gyro/Kconfig b/drivers/iio/gyro/Kconfig
> index 024a34139875..5824f2edf975 100644
> --- a/drivers/iio/gyro/Kconfig
> +++ b/drivers/iio/gyro/Kconfig
> @@ -44,6 +44,8 @@ config ADIS16260
>  config ADXRS290
>  	tristate "Analog Devices ADXRS290 Dual-Axis MEMS Gyroscope SPI driver"
>  	depends on SPI
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
>  	help
>  	  Say yes here to build support for Analog Devices ADXRS290 programmable
>  	  digital output gyroscope.
> diff --git a/drivers/iio/gyro/adxrs290.c b/drivers/iio/gyro/adxrs290.c
> index ff989536d2fb..25046590761e 100644
> --- a/drivers/iio/gyro/adxrs290.c
> +++ b/drivers/iio/gyro/adxrs290.c
> @@ -13,8 +13,12 @@
>  #include <linux/module.h>
>  #include <linux/spi/spi.h>
>  
> +#include <linux/iio/buffer.h>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
>  
>  #define ADXRS290_ADI_ID		0xAD
>  #define ADXRS290_MEMS_ID	0x1D
> @@ -35,7 +39,9 @@
>  #define ADXRS290_READ		BIT(7)
>  #define ADXRS290_TSM		BIT(0)
>  #define ADXRS290_MEASUREMENT	BIT(1)
> -#define ADXRS290_SYNC		GENMASK(1, 0)
> +#define ADXRS290_DATA_RDY_OUT	BIT(0)
> +#define ADXRS290_SYNC_MASK	GENMASK(1, 0)
> +#define ADXRS290_SYNC(x)	FIELD_PREP(ADXRS290_SYNC_MASK, x)
>  #define ADXRS290_LPF_MASK	GENMASK(2, 0)
>  #define ADXRS290_LPF(x)		FIELD_PREP(ADXRS290_LPF_MASK, x)
>  #define ADXRS290_HPF_MASK	GENMASK(7, 4)
> @@ -50,6 +56,13 @@ enum adxrs290_mode {
>  	ADXRS290_MODE_MEASUREMENT,
>  };
>  
> +enum adxrs290_scan_index {
> +	ADXRS290_IDX_X,
> +	ADXRS290_IDX_Y,
> +	ADXRS290_IDX_TEMP,
> +	ADXRS290_IDX_TS,
> +};
> +
>  struct adxrs290_state {
>  	struct spi_device	*spi;
>  	/* Serialize reads and their subsequent processing */
> @@ -57,6 +70,12 @@ struct adxrs290_state {
>  	enum adxrs290_mode	mode;
>  	unsigned int		lpf_3db_freq_idx;
>  	unsigned int		hpf_3db_freq_idx;
> +	struct iio_trigger      *dready_trig;
> +	/* Ensure correct alignment of timestamp when present */
> +	struct {
> +		s16 channels[3];
> +		s64 ts __aligned(8);
> +	}                       buffer;
        } buffer;

I can see why you did the alignment like you have, but it looks really odd
in this case.

>  };
>  

...

>  
> +static void adxrs290_chip_off_action(void *data)
> +{
> +	struct iio_dev *indio_dev = data;
> +
> +	adxrs290_set_mode(indio_dev, ADXRS290_MODE_STANDBY);
> +}
> +
>  static int adxrs290_read_raw(struct iio_dev *indio_dev,
>  			     struct iio_chan_spec const *chan,
>  			     int *val,
> @@ -215,24 +287,34 @@ static int adxrs290_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;
> +
>  		switch (chan->type) {
>  		case IIO_ANGL_VEL:
>  			ret = adxrs290_get_rate_data(indio_dev,
>  						     ADXRS290_READ_REG(chan->address),
>  						     val);
>  			if (ret < 0)
> -				return ret;
> +				goto err_release;
>  
> -			return IIO_VAL_INT;
> +			ret = IIO_VAL_INT;
> +			break;
>  		case IIO_TEMP:
>  			ret = adxrs290_get_temp_data(indio_dev, val);
>  			if (ret < 0)
> -				return ret;
> +				goto err_release;
>  
> -			return IIO_VAL_INT;
> +			ret = IIO_VAL_INT;
> +			break;
>  		default:
> -			return -EINVAL;
> +			ret = -EINVAL;
> +			break;
>  		}
> +err_release:
> +		iio_device_release_direct_mode(indio_dev);
> +		return ret;
>  	case IIO_CHAN_INFO_SCALE:
>  		switch (chan->type) {
>  		case IIO_ANGL_VEL:
> @@ -279,34 +361,53 @@ static int adxrs290_write_raw(struct iio_dev *indio_dev,
>  			      long mask)
>  {
>  	struct adxrs290_state *st = iio_priv(indio_dev);
> -	int lpf_idx, hpf_idx;
> +	int ret, lpf_idx, hpf_idx;
> +
> +	ret = iio_device_claim_direct_mode(indio_dev);

Is there anything in the datasheet that says we can't change the filters
whilst doing captures?  Might get crazy data but if the datasheet doesn't
want against a particular action, then we tend not to try and prevent it.

> +	if (ret)
> +		return ret;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
>  		lpf_idx = adxrs290_find_match(adxrs290_lpf_3db_freq_hz_table,
>  					      ARRAY_SIZE(adxrs290_lpf_3db_freq_hz_table),
>  					      val, val2);
> -		if (lpf_idx < 0)
> -			return -EINVAL;
> +		if (lpf_idx < 0) {
> +			ret = -EINVAL;
> +			goto err_release;
> +		}
> +
>  		/* caching the updated state of the low-pass filter */
>  		st->lpf_3db_freq_idx = lpf_idx;
>  		/* retrieving the current state of the high-pass filter */
>  		hpf_idx = st->hpf_3db_freq_idx;
> -		return adxrs290_set_filter_freq(indio_dev, lpf_idx, hpf_idx);
> +		ret = adxrs290_set_filter_freq(indio_dev, lpf_idx, hpf_idx);
> +		break;
> +
>  	case IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY:
>  		hpf_idx = adxrs290_find_match(adxrs290_hpf_3db_freq_hz_table,
>  					      ARRAY_SIZE(adxrs290_hpf_3db_freq_hz_table),
>  					      val, val2);
> -		if (hpf_idx < 0)
> -			return -EINVAL;
> +		if (hpf_idx < 0) {
> +			ret = -EINVAL;
> +			goto err_release;
> +		}
> +
>  		/* caching the updated state of the high-pass filter */
>  		st->hpf_3db_freq_idx = hpf_idx;
>  		/* retrieving the current state of the low-pass filter */
>  		lpf_idx = st->lpf_3db_freq_idx;
> -		return adxrs290_set_filter_freq(indio_dev, lpf_idx, hpf_idx);
> +		ret = adxrs290_set_filter_freq(indio_dev, lpf_idx, hpf_idx);
> +		break;
> +
> +	default:
> +		ret = -EINVAL;
> +		break;
>  	}
>  
> -	return -EINVAL;
> +err_release:
I think Andy may have mentioned this, but a break would work just as well
to get you here

> +	iio_device_release_direct_mode(indio_dev);
> +	return ret;
>  }
>  
>  static int adxrs290_read_avail(struct iio_dev *indio_dev,
> @@ -334,6 +435,68 @@ static int adxrs290_read_avail(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static int adxrs290_data_rdy_trigger_set_state(struct iio_trigger *trig, bool state)
> +{
> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> +	struct adxrs290_state *st = iio_priv(indio_dev);
> +	int ret;
> +	u8 val;
> +
> +	val = (state ? ADXRS290_SYNC(ADXRS290_DATA_RDY_OUT) : 0);

Why the outer set of brackets?

> +
> +	ret = adxrs290_spi_write_reg(st->spi, ADXRS290_REG_DATA_RDY, val);
> +	if (ret < 0)
> +		dev_err(&st->spi->dev, "failed to start data ready interrupt\n");
> +
> +	return ret;
> +}
> +
> +static int adxrs290_reset_trig(struct iio_trigger *trig)
> +{
> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> +	int val;
> +
> +	/*
> +	 * Data ready interrupt is reset after a read of the data registers.
> +	 * Here, we only read the 16b DATAY registers as that marks the end of
> +	 * a read of the data registers and initiates a reset for the interrupt line.
> +	 */
> +	return adxrs290_get_rate_data(indio_dev, ADXRS290_READ_REG(ADXRS290_REG_DATAY0), &val);

We have a bug around this at the moment btw.

A try_reenable callback shouldn't return an error.  The return value is
meant to indicate that we need to go round again or not
(though that's broken currently anwyay). Hence safer to just not
return errors.

return 0;

> +}
> +
> +static const struct iio_trigger_ops adxrs290_trigger_ops = {
> +	.set_trigger_state = &adxrs290_data_rdy_trigger_set_state,
> +	.validate_device = &iio_trigger_validate_own_device,
> +	.try_reenable = &adxrs290_reset_trig,
> +};
> +
> +static irqreturn_t adxrs290_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct adxrs290_state *st = iio_priv(indio_dev);
> +	struct spi_device *spi = st->spi;

As you only use spi once, I'd just use st->spi inline.

> +	u8 tx = ADXRS290_READ_REG(ADXRS290_REG_DATAX0);
> +	int ret;
> +
> +	mutex_lock(&st->lock);
> +
> +	/* exercise a bulk data capture starting from reg DATAX0... */
> +	ret = spi_write_then_read(spi, &tx, sizeof(tx), st->buffer.channels,
> +				  sizeof(st->buffer.channels));
> +	if (ret < 0)
> +		goto done;
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, &st->buffer,
> +					   pf->timestamp);
> +
> +done:
> +	mutex_unlock(&st->lock);
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  #define ADXRS290_ANGL_VEL_CHANNEL(reg, axis) {				\
>  	.type = IIO_ANGL_VEL,						\
>  	.address = reg,							\
> @@ -346,6 +509,13 @@ static int adxrs290_read_avail(struct iio_dev *indio_dev,
>  	.info_mask_shared_by_type_available =				\
>  	BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) |		\
>  	BIT(IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY),		\
> +	.scan_index = ADXRS290_IDX_##axis,				\
> +	.scan_type = {                                                  \
> +		.sign = 's',                                            \
> +		.realbits = 16,                                         \
> +		.storagebits = 16,                                      \
> +		.endianness = IIO_LE,					\
> +	},                                                              \
>  }
>  
>  static const struct iio_chan_spec adxrs290_channels[] = {
> @@ -356,7 +526,21 @@ static const struct iio_chan_spec adxrs290_channels[] = {
>  		.address = ADXRS290_REG_TEMP0,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>  		BIT(IIO_CHAN_INFO_SCALE),
> +		.scan_index = ADXRS290_IDX_TEMP,
> +		.scan_type = {
> +			.sign = 's',
> +			.realbits = 12,
> +			.storagebits = 16,
> +			.shift = 0,

No need to specify shift if it is 0 as that's the obvious default.

> +			.endianness = IIO_LE,
> +		},
>  	},
> +	IIO_CHAN_SOFT_TIMESTAMP(ADXRS290_IDX_TS),
> +};
> +
> +static const unsigned long adxrs290_avail_scan_masks[] = {
> +	BIT(ADXRS290_IDX_X) | BIT(ADXRS290_IDX_Y) | BIT(ADXRS290_IDX_TEMP),
> +	0
>  };
>  
>  static const struct iio_info adxrs290_info = {
> @@ -365,6 +549,47 @@ static const struct iio_info adxrs290_info = {
>  	.read_avail = &adxrs290_read_avail,
>  };
>  
> +static int adxrs290_probe_trigger(struct iio_dev *indio_dev)
> +{
> +	struct adxrs290_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (!st->spi->irq) {
> +		dev_info(&st->spi->dev, "no irq, using polling\n");
> +		return 0;
> +	}
> +
> +	st->dready_trig = devm_iio_trigger_alloc(&st->spi->dev,
> +						 "%s-dev%d",
> +						 indio_dev->name,
> +						 indio_dev->id);

As below, this could be sensibly done on fewer lines without any real loss
of readability (staying under advised 80 chars)

> +	if (!st->dready_trig)
> +		return -ENOMEM;
> +
> +	st->dready_trig->dev.parent = &st->spi->dev;
> +	st->dready_trig->ops = &adxrs290_trigger_ops;
> +	iio_trigger_set_drvdata(st->dready_trig, indio_dev);
> +
> +	ret = devm_request_irq(&st->spi->dev, st->spi->irq,
> +			       &iio_trigger_generic_data_rdy_poll,
> +			       IRQF_ONESHOT,
> +			       "adxrs290_irq", st->dready_trig);
> +	if (ret < 0) {
> +		dev_err(&st->spi->dev, "request irq %d failed\n", st->spi->irq);
> +		return ret;
> +	}
> +
> +	ret = devm_iio_trigger_register(&st->spi->dev, st->dready_trig);
> +	if (ret) {
> +		dev_err(&st->spi->dev, "iio trigger register failed\n");
> +		return ret;
> +	}
> +
> +	indio_dev->trig = iio_trigger_get(st->dready_trig);
> +
> +	return 0;
> +}
> +
>  static int adxrs290_probe(struct spi_device *spi)
>  {
>  	struct iio_dev *indio_dev;
> @@ -384,6 +609,7 @@ static int adxrs290_probe(struct spi_device *spi)
>  	indio_dev->channels = adxrs290_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(adxrs290_channels);
>  	indio_dev->info = &adxrs290_info;
> +	indio_dev->available_scan_masks = adxrs290_avail_scan_masks;
>  
>  	mutex_init(&st->lock);
>  
> @@ -416,6 +642,12 @@ static int adxrs290_probe(struct spi_device *spi)
>  	/* max transition time to measurement mode */
>  	msleep(ADXRS290_MAX_TRANSITION_TIME_MS);
>  
> +	ret = devm_add_action_or_reset(&spi->dev,
> +				       adxrs290_chip_off_action,
> +				       indio_dev);

What is this unwinding?  We should only be using devm to undo things
that have been 'done' in probe and the association should be clear.

You have been a bit in consistent with wrapping.  Personally I would
go for as many parameters as fit on each line under 80 chars as possible.
The only exception being when there is some clear 'pairing' of parameters
or similar.

> +	if (ret < 0)
> +		return ret;
> +
>  	ret = adxrs290_get_3db_freq(indio_dev, &val, &val2);
>  	if (ret < 0)
>  		return ret;
> @@ -423,6 +655,18 @@ static int adxrs290_probe(struct spi_device *spi)
>  	st->lpf_3db_freq_idx = val;
>  	st->hpf_3db_freq_idx = val2;
>  
> +	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
> +					      &iio_pollfunc_store_time,
> +					      &adxrs290_trigger_handler, NULL);
> +	if (ret < 0) {
> +		dev_err(&spi->dev, "iio triggered buffer setup failed\n");
> +		return ret;
> +	}
> +
> +	ret = adxrs290_probe_trigger(indio_dev);
> +	if (ret < 0)
> +		return ret;
> +
>  	return devm_iio_device_register(&spi->dev, indio_dev);
>  }
>  

Re: [PATCH 2/3] dt-bindings: iio: gyro: adxrs290: Add required interrupts property

[Jonathan Cameron]

On Tue, 25 Aug 2020 18:17:10 +0530
Nishant Malpani <nish.malpani25@gmail.com> wrote:

> Append 'interrupts' as a required property and provide a suitable example
> for using a GPIO interrupt line.
> 
> Signed-off-by: Nishant Malpani <nish.malpani25@gmail.com>
Hi Nishant,

I don't understand why the interrupt is 'required'.  Also note you should
never be adding required properties to an existing binding.  It's possible
someone already used the binding as it stands and shipped a board with
it burnt in a firmware. (bit unlikely but you never know!)

Jonathan

> ---
>  .../devicetree/bindings/iio/gyroscope/adi,adxrs290.yaml   | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/gyroscope/adi,adxrs290.yaml b/Documentation/devicetree/bindings/iio/gyroscope/adi,adxrs290.yaml
> index 61adb2c2454b..cae593dd1ba7 100644
> --- a/Documentation/devicetree/bindings/iio/gyroscope/adi,adxrs290.yaml
> +++ b/Documentation/devicetree/bindings/iio/gyroscope/adi,adxrs290.yaml
> @@ -28,17 +28,23 @@ properties:
>  
>    spi-cpha: true
>  
> +  interrupts:
> +    maxItems: 1
> +
>  required:
>    - compatible
>    - reg
>    - spi-max-frequency
>    - spi-cpol
>    - spi-cpha
> +  - interrupts

Why?  Device works fine without one being supplied. 
It's not uncommon on embedded boards to not wire up interrupts
due to a lack of pins and just rely on polling.

>  
>  additionalProperties: false
>  
>  examples:
>    - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
>      spi {
>          #address-cells = <1>;
>          #size-cells = <0>;
> @@ -48,6 +54,8 @@ examples:
>                     spi-max-frequency = <5000000>;
>                     spi-cpol;
>                     spi-cpha;
> +                   interrupt-parent = <&gpio>;
> +                   interrupts = <25 IRQ_TYPE_EDGE_RISING>;
>          };
>      };
>  ...

Re: [PATCH 3/3] iio: gyro: adxrs290: Add debugfs register access support

[Jonathan Cameron]

On Tue, 25 Aug 2020 18:17:11 +0530
Nishant Malpani <nish.malpani25@gmail.com> wrote:

> Extend support to read/write byte data from/to the device using
> debugfs iio interface.
> 
> Signed-off-by: Nishant Malpani <nish.malpani25@gmail.com>
Hi Nishant,

I'm always a bit unsure on whether I want drivers to provide this
interface, as it isn't something that should be of much use once
initial driver debugging is done.

However, the patch is good so if you want to add it fair enough.
I'll pick it up once patches 1 and 2 are ready.

Thanks,

Jonathan

> ---
>  drivers/iio/gyro/adxrs290.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/iio/gyro/adxrs290.c b/drivers/iio/gyro/adxrs290.c
> index 25046590761e..b0050cdd2b90 100644
> --- a/drivers/iio/gyro/adxrs290.c
> +++ b/drivers/iio/gyro/adxrs290.c
> @@ -435,6 +435,24 @@ static int adxrs290_read_avail(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static int adxrs290_reg_access(struct iio_dev *indio_dev, unsigned int reg,
> +			       unsigned int writeval, unsigned int *readval)
> +{
> +	struct adxrs290_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (!readval)
> +		return adxrs290_spi_write_reg(st->spi, reg, writeval);
> +
> +	ret = spi_w8r8(st->spi, ADXRS290_READ_REG(reg));
> +	if (ret < 0)
> +		return ret;
> +
> +	*readval = ret;
> +
> +	return 0;
> +}
> +
>  static int adxrs290_data_rdy_trigger_set_state(struct iio_trigger *trig, bool state)
>  {
>  	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> @@ -547,6 +565,7 @@ static const struct iio_info adxrs290_info = {
>  	.read_raw = &adxrs290_read_raw,
>  	.write_raw = &adxrs290_write_raw,
>  	.read_avail = &adxrs290_read_avail,
> +	.debugfs_reg_access = &adxrs290_reg_access,
>  };
>  
>  static int adxrs290_probe_trigger(struct iio_dev *indio_dev)

Re: [PATCH 1/3] iio: gyro: adxrs290: Add triggered buffer support

[Nishant Malpani]

Hello,

Thanks for the review, Andy. Comments inline...

On 26/08/20 9:40 pm, Andy Shevchenko wrote:
> On Tue, Aug 25, 2020 at 4:11 PM Nishant Malpani
> <nish.malpani25@gmail.com> wrote:
>>
>> Provide a way for continuous data capture by setting up buffer support. The
>> data ready signal exposed at the SYNC pin of the ADXRS290 is exploited as
>> a hardware interrupt which triggers to fill the buffer.
>>
>> Triggered buffer setup was tested with both hardware trigger (DATA_RDY) and
>> software triggers (sysfs-trig & hrtimer).
> 
> ...
> 
>> +static int adxrs290_set_mode(struct iio_dev *indio_dev, enum adxrs290_mode mode)
>> +{
>> +       struct adxrs290_state *st = iio_priv(indio_dev);
>> +       int val, ret;
>> +
>> +       mutex_lock(&st->lock);
>> +
>> +       if (st->mode == mode) {
> 
>> +               ret = 0;
> 
> Can be done outside of mutex.
> 
Yep, makes sense.
>> +               goto done;
>> +       }
>> +
> 
>> +       val = spi_w8r8(st->spi, ADXRS290_READ_REG(ADXRS290_REG_POWER_CTL));
>> +       if (val < 0) {
>> +               ret = val;
>> +               goto done;
>> +       }
> 
> Consider other way around
>   ret = ...
>   ...
>   val = ret;
> 
I suppose that does make things consistent; will do so in v2.

>> +       switch (mode) {
>> +       case ADXRS290_MODE_STANDBY:
>> +               val &= ~ADXRS290_MEASUREMENT;
>> +               break;
>> +       case ADXRS290_MODE_MEASUREMENT:
>> +               val |= ADXRS290_MEASUREMENT;
>> +               break;
>> +       default:
>> +               ret = -EINVAL;
>> +               goto done;
>> +       }
>> +
>> +       ret = adxrs290_spi_write_reg(st->spi,
>> +                                    ADXRS290_REG_POWER_CTL,
>> +                                    val);
>> +       if (ret < 0) {
>> +               dev_err(&st->spi->dev, "unable to set mode: %d\n", ret);
>> +               goto done;
>> +       }
>> +
>> +       /* update cached mode */
>> +       st->mode = mode;
>> +
>> +done:
>> +       mutex_unlock(&st->lock);
>> +       return ret;
>> +}
> 
> ...
> 
>> +                               goto err_release;
>>
>> -                       return IIO_VAL_INT;
>> +                       ret = IIO_VAL_INT;
>> +                       break;
>>                  default:
>> -                       return -EINVAL;
>> +                       ret = -EINVAL;
>> +                       break;
>>                  }
> 
>> +err_release:
> 
> I didn't get the purpose of this. Wasn't the break statement enough?
> 
It is indeed; I just thought the labeling was a preferred way to jump to 
error handling paths. Will use just the 'break' in v2.

>> +               iio_device_release_direct_mode(indio_dev);
>> +               return ret;
>>          case IIO_CHAN_INFO_SCALE:
>>                  switch (chan->type) {
>>                  case IIO_ANGL_VEL:
> 
> ...
> 
>> +                       goto err_release;
> 
> Ditto.
> 
Got it.

>> +               }
>> +
>>                  /* caching the updated state of the high-pass filter */
>>                  st->hpf_3db_freq_idx = hpf_idx;
>>                  /* retrieving the current state of the low-pass filter */
>>                  lpf_idx = st->lpf_3db_freq_idx;
>> -               return adxrs290_set_filter_freq(indio_dev, lpf_idx, hpf_idx);
>> +               ret = adxrs290_set_filter_freq(indio_dev, lpf_idx, hpf_idx);
>> +               break;
>> +
>> +       default:
>> +               ret = -EINVAL;
>> +               break;
>>          }
>>
>> -       return -EINVAL;
>> +err_release:
>> +       iio_device_release_direct_mode(indio_dev);
>> +       return ret;
>>   }
> 
> ...
> 
>> +       val = (state ? ADXRS290_SYNC(ADXRS290_DATA_RDY_OUT) : 0);
> 
> Purpose of outer parentheses?
> I personally find that more readable but I think I'm violating the 
coding style in the kernel; will remove the parentheses in v2.

> ...
> 
>> +static int adxrs290_probe_trigger(struct iio_dev *indio_dev)
>> +{
>> +       struct adxrs290_state *st = iio_priv(indio_dev);
>> +       int ret;
>> +
>> +       if (!st->spi->irq) {
>> +               dev_info(&st->spi->dev, "no irq, using polling\n");
>> +               return 0;
>> +       }
>> +
>> +       st->dready_trig = devm_iio_trigger_alloc(&st->spi->dev,
>> +                                                "%s-dev%d",
>> +                                                indio_dev->name,
>> +                                                indio_dev->id);
>> +       if (!st->dready_trig)
>> +               return -ENOMEM;
>> +
>> +       st->dready_trig->dev.parent = &st->spi->dev;
>> +       st->dready_trig->ops = &adxrs290_trigger_ops;
>> +       iio_trigger_set_drvdata(st->dready_trig, indio_dev);
>> +
>> +       ret = devm_request_irq(&st->spi->dev, st->spi->irq,
>> +                              &iio_trigger_generic_data_rdy_poll,
>> +                              IRQF_ONESHOT,
>> +                              "adxrs290_irq", st->dready_trig);
>> +       if (ret < 0) {
> 
>> +               dev_err(&st->spi->dev, "request irq %d failed\n", st->spi->irq);
>> +               return ret;
> 
> return dev_err_probe(...);
> 
Nice, wasn't aware of this. Thanks. Will use 'dev_err_probe()' in v2 
wherever pointed.

With regards,
Nishant Malpani

>> +       }
>> +
>> +       ret = devm_iio_trigger_register(&st->spi->dev, st->dready_trig);
>> +       if (ret) {
> 
>> +               dev_err(&st->spi->dev, "iio trigger register failed\n");
>> +               return ret;
> 
> return dev_err_probe(...);
> 
>> +       }
>> +
>> +       indio_dev->trig = iio_trigger_get(st->dready_trig);
>> +
>> +       return 0;
>> +}
> 
> ...
> 
>> +       ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
>> +                                             &iio_pollfunc_store_time,
>> +                                             &adxrs290_trigger_handler, NULL);
>> +       if (ret < 0) {
> 
>> +               dev_err(&spi->dev, "iio triggered buffer setup failed\n");
>> +               return ret;
> 
> return dev_err_probe(...);
> 
>> +       }
> 

Re: [PATCH 1/3] iio: gyro: adxrs290: Add triggered buffer support

[Nishant Malpani]

Hello,

Thanks for the review, Jonathan. Comments inline...

On Sat, Aug 29, 2020 at 10:16 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 25 Aug 2020 18:17:09 +0530
> Nishant Malpani <nish.malpani25@gmail.com> wrote:
>
> > Provide a way for continuous data capture by setting up buffer support. The
> > data ready signal exposed at the SYNC pin of the ADXRS290 is exploited as
> > a hardware interrupt which triggers to fill the buffer.
> >
> > Triggered buffer setup was tested with both hardware trigger (DATA_RDY) and
> > software triggers (sysfs-trig & hrtimer).
> >
> > Signed-off-by: Nishant Malpani <nish.malpani25@gmail.com>
>
> I've tried not to overlap with Andy's comments but might have hit on a few.
> Anyhow, comments inline.
>
> Jonathan
>
>
> > ---
> >  drivers/iio/gyro/Kconfig    |   2 +
> >  drivers/iio/gyro/adxrs290.c | 272 ++++++++++++++++++++++++++++++++++--
> >  2 files changed, 260 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/iio/gyro/Kconfig b/drivers/iio/gyro/Kconfig
> > index 024a34139875..5824f2edf975 100644
> > --- a/drivers/iio/gyro/Kconfig
> > +++ b/drivers/iio/gyro/Kconfig
> > @@ -44,6 +44,8 @@ config ADIS16260
> >  config ADXRS290
> >       tristate "Analog Devices ADXRS290 Dual-Axis MEMS Gyroscope SPI driver"
> >       depends on SPI
> > +     select IIO_BUFFER
> > +     select IIO_TRIGGERED_BUFFER
> >       help
> >         Say yes here to build support for Analog Devices ADXRS290 programmable
> >         digital output gyroscope.
> > diff --git a/drivers/iio/gyro/adxrs290.c b/drivers/iio/gyro/adxrs290.c
> > index ff989536d2fb..25046590761e 100644
> > --- a/drivers/iio/gyro/adxrs290.c
> > +++ b/drivers/iio/gyro/adxrs290.c
> > @@ -13,8 +13,12 @@
> >  #include <linux/module.h>
> >  #include <linux/spi/spi.h>
> >
> > +#include <linux/iio/buffer.h>
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/sysfs.h>
> > +#include <linux/iio/trigger.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +#include <linux/iio/trigger_consumer.h>
> >
> >  #define ADXRS290_ADI_ID              0xAD
> >  #define ADXRS290_MEMS_ID     0x1D
> > @@ -35,7 +39,9 @@
> >  #define ADXRS290_READ                BIT(7)
> >  #define ADXRS290_TSM         BIT(0)
> >  #define ADXRS290_MEASUREMENT BIT(1)
> > -#define ADXRS290_SYNC                GENMASK(1, 0)
> > +#define ADXRS290_DATA_RDY_OUT        BIT(0)
> > +#define ADXRS290_SYNC_MASK   GENMASK(1, 0)
> > +#define ADXRS290_SYNC(x)     FIELD_PREP(ADXRS290_SYNC_MASK, x)
> >  #define ADXRS290_LPF_MASK    GENMASK(2, 0)
> >  #define ADXRS290_LPF(x)              FIELD_PREP(ADXRS290_LPF_MASK, x)
> >  #define ADXRS290_HPF_MASK    GENMASK(7, 4)
> > @@ -50,6 +56,13 @@ enum adxrs290_mode {
> >       ADXRS290_MODE_MEASUREMENT,
> >  };
> >
> > +enum adxrs290_scan_index {
> > +     ADXRS290_IDX_X,
> > +     ADXRS290_IDX_Y,
> > +     ADXRS290_IDX_TEMP,
> > +     ADXRS290_IDX_TS,
> > +};
> > +
> >  struct adxrs290_state {
> >       struct spi_device       *spi;
> >       /* Serialize reads and their subsequent processing */
> > @@ -57,6 +70,12 @@ struct adxrs290_state {
> >       enum adxrs290_mode      mode;
> >       unsigned int            lpf_3db_freq_idx;
> >       unsigned int            hpf_3db_freq_idx;
> > +     struct iio_trigger      *dready_trig;
> > +     /* Ensure correct alignment of timestamp when present */
> > +     struct {
> > +             s16 channels[3];
> > +             s64 ts __aligned(8);
> > +     }                       buffer;
>         } buffer;
>
> I can see why you did the alignment like you have, but it looks really odd
> in this case.
>

I thought so too. Will fix it in v2.

> >  };
> >
>
> ...
>
> >
> > +static void adxrs290_chip_off_action(void *data)
> > +{
> > +     struct iio_dev *indio_dev = data;
> > +
> > +     adxrs290_set_mode(indio_dev, ADXRS290_MODE_STANDBY);
> > +}
> > +
> >  static int adxrs290_read_raw(struct iio_dev *indio_dev,
> >                            struct iio_chan_spec const *chan,
> >                            int *val,
> > @@ -215,24 +287,34 @@ static int adxrs290_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;
> > +
> >               switch (chan->type) {
> >               case IIO_ANGL_VEL:
> >                       ret = adxrs290_get_rate_data(indio_dev,
> >                                                    ADXRS290_READ_REG(chan->address),
> >                                                    val);
> >                       if (ret < 0)
> > -                             return ret;
> > +                             goto err_release;
> >
> > -                     return IIO_VAL_INT;
> > +                     ret = IIO_VAL_INT;
> > +                     break;
> >               case IIO_TEMP:
> >                       ret = adxrs290_get_temp_data(indio_dev, val);
> >                       if (ret < 0)
> > -                             return ret;
> > +                             goto err_release;
> >
> > -                     return IIO_VAL_INT;
> > +                     ret = IIO_VAL_INT;
> > +                     break;
> >               default:
> > -                     return -EINVAL;
> > +                     ret = -EINVAL;
> > +                     break;
> >               }
> > +err_release:
> > +             iio_device_release_direct_mode(indio_dev);
> > +             return ret;
> >       case IIO_CHAN_INFO_SCALE:
> >               switch (chan->type) {
> >               case IIO_ANGL_VEL:
> > @@ -279,34 +361,53 @@ static int adxrs290_write_raw(struct iio_dev *indio_dev,
> >                             long mask)
> >  {
> >       struct adxrs290_state *st = iio_priv(indio_dev);
> > -     int lpf_idx, hpf_idx;
> > +     int ret, lpf_idx, hpf_idx;
> > +
> > +     ret = iio_device_claim_direct_mode(indio_dev);
>
> Is there anything in the datasheet that says we can't change the filters
> whilst doing captures?  Might get crazy data but if the datasheet doesn't
> want against a particular action, then we tend not to try and prevent it.
>

The datasheet, to my knowledge, explicitly doesn't mention any
restriction on changing the filter settings during a capture.

Quoting the datasheet, "The group delay of the wideband filter option
is less than 0.5ms" (pg. 11) - during an ongoing capture if one
re-configures the filter, how do we address this?

> > +     if (ret)
> > +             return ret;
> >
> >       switch (mask) {
> >       case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> >               lpf_idx = adxrs290_find_match(adxrs290_lpf_3db_freq_hz_table,
> >                                             ARRAY_SIZE(adxrs290_lpf_3db_freq_hz_table),
> >                                             val, val2);
> > -             if (lpf_idx < 0)
> > -                     return -EINVAL;
> > +             if (lpf_idx < 0) {
> > +                     ret = -EINVAL;
> > +                     goto err_release;
> > +             }
> > +
> >               /* caching the updated state of the low-pass filter */
> >               st->lpf_3db_freq_idx = lpf_idx;
> >               /* retrieving the current state of the high-pass filter */
> >               hpf_idx = st->hpf_3db_freq_idx;
> > -             return adxrs290_set_filter_freq(indio_dev, lpf_idx, hpf_idx);
> > +             ret = adxrs290_set_filter_freq(indio_dev, lpf_idx, hpf_idx);
> > +             break;
> > +
> >       case IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY:
> >               hpf_idx = adxrs290_find_match(adxrs290_hpf_3db_freq_hz_table,
> >                                             ARRAY_SIZE(adxrs290_hpf_3db_freq_hz_table),
> >                                             val, val2);
> > -             if (hpf_idx < 0)
> > -                     return -EINVAL;
> > +             if (hpf_idx < 0) {
> > +                     ret = -EINVAL;
> > +                     goto err_release;
> > +             }
> > +
> >               /* caching the updated state of the high-pass filter */
> >               st->hpf_3db_freq_idx = hpf_idx;
> >               /* retrieving the current state of the low-pass filter */
> >               lpf_idx = st->lpf_3db_freq_idx;
> > -             return adxrs290_set_filter_freq(indio_dev, lpf_idx, hpf_idx);
> > +             ret = adxrs290_set_filter_freq(indio_dev, lpf_idx, hpf_idx);
> > +             break;
> > +
> > +     default:
> > +             ret = -EINVAL;
> > +             break;
> >       }
> >
> > -     return -EINVAL;
> > +err_release:
> I think Andy may have mentioned this, but a break would work just as well
> to get you here
>

Agreed. Will refactor accordingly in v2.

> > +     iio_device_release_direct_mode(indio_dev);
> > +     return ret;
> >  }
> >
> >  static int adxrs290_read_avail(struct iio_dev *indio_dev,
> > @@ -334,6 +435,68 @@ static int adxrs290_read_avail(struct iio_dev *indio_dev,
> >       }
> >  }
> >
> > +static int adxrs290_data_rdy_trigger_set_state(struct iio_trigger *trig, bool state)
> > +{
> > +     struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> > +     struct adxrs290_state *st = iio_priv(indio_dev);
> > +     int ret;
> > +     u8 val;
> > +
> > +     val = (state ? ADXRS290_SYNC(ADXRS290_DATA_RDY_OUT) : 0);
>
> Why the outer set of brackets?
>

I now realize those are unnecessary and probably violate the kernel
coding-style. Will remove those in v2.

> > +
> > +     ret = adxrs290_spi_write_reg(st->spi, ADXRS290_REG_DATA_RDY, val);
> > +     if (ret < 0)
> > +             dev_err(&st->spi->dev, "failed to start data ready interrupt\n");
> > +
> > +     return ret;
> > +}
> > +
> > +static int adxrs290_reset_trig(struct iio_trigger *trig)
> > +{
> > +     struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> > +     int val;
> > +
> > +     /*
> > +      * Data ready interrupt is reset after a read of the data registers.
> > +      * Here, we only read the 16b DATAY registers as that marks the end of
> > +      * a read of the data registers and initiates a reset for the interrupt line.
> > +      */
> > +     return adxrs290_get_rate_data(indio_dev, ADXRS290_READ_REG(ADXRS290_REG_DATAY0), &val);
>
> We have a bug around this at the moment btw.
>
> A try_reenable callback shouldn't return an error.  The return value is
> meant to indicate that we need to go round again or not
> (though that's broken currently anwyay). Hence safer to just not
> return errors.
>
> return 0;
>

Right, I forgot about the recent conversations on the mailing list
about this; thanks for pointing out again. Will fix this in v2.

> > +}
> > +
> > +static const struct iio_trigger_ops adxrs290_trigger_ops = {
> > +     .set_trigger_state = &adxrs290_data_rdy_trigger_set_state,
> > +     .validate_device = &iio_trigger_validate_own_device,
> > +     .try_reenable = &adxrs290_reset_trig,
> > +};
> > +
> > +static irqreturn_t adxrs290_trigger_handler(int irq, void *p)
> > +{
> > +     struct iio_poll_func *pf = p;
> > +     struct iio_dev *indio_dev = pf->indio_dev;
> > +     struct adxrs290_state *st = iio_priv(indio_dev);
> > +     struct spi_device *spi = st->spi;
>
> As you only use spi once, I'd just use st->spi inline.
>

Okay.

> > +     u8 tx = ADXRS290_READ_REG(ADXRS290_REG_DATAX0);
> > +     int ret;
> > +
> > +     mutex_lock(&st->lock);
> > +
> > +     /* exercise a bulk data capture starting from reg DATAX0... */
> > +     ret = spi_write_then_read(spi, &tx, sizeof(tx), st->buffer.channels,
> > +                               sizeof(st->buffer.channels));
> > +     if (ret < 0)
> > +             goto done;
> > +
> > +     iio_push_to_buffers_with_timestamp(indio_dev, &st->buffer,
> > +                                        pf->timestamp);
> > +
> > +done:
> > +     mutex_unlock(&st->lock);
> > +     iio_trigger_notify_done(indio_dev->trig);
> > +
> > +     return IRQ_HANDLED;
> > +}
> > +
> >  #define ADXRS290_ANGL_VEL_CHANNEL(reg, axis) {                               \
> >       .type = IIO_ANGL_VEL,                                           \
> >       .address = reg,                                                 \
> > @@ -346,6 +509,13 @@ static int adxrs290_read_avail(struct iio_dev *indio_dev,
> >       .info_mask_shared_by_type_available =                           \
> >       BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) |              \
> >       BIT(IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY),              \
> > +     .scan_index = ADXRS290_IDX_##axis,                              \
> > +     .scan_type = {                                                  \
> > +             .sign = 's',                                            \
> > +             .realbits = 16,                                         \
> > +             .storagebits = 16,                                      \
> > +             .endianness = IIO_LE,                                   \
> > +     },                                                              \
> >  }
> >
> >  static const struct iio_chan_spec adxrs290_channels[] = {
> > @@ -356,7 +526,21 @@ static const struct iio_chan_spec adxrs290_channels[] = {
> >               .address = ADXRS290_REG_TEMP0,
> >               .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> >               BIT(IIO_CHAN_INFO_SCALE),
> > +             .scan_index = ADXRS290_IDX_TEMP,
> > +             .scan_type = {
> > +                     .sign = 's',
> > +                     .realbits = 12,
> > +                     .storagebits = 16,
> > +                     .shift = 0,
>
> No need to specify shift if it is 0 as that's the obvious default.
>

Got it.

> > +                     .endianness = IIO_LE,
> > +             },
> >       },
> > +     IIO_CHAN_SOFT_TIMESTAMP(ADXRS290_IDX_TS),
> > +};
> > +
> > +static const unsigned long adxrs290_avail_scan_masks[] = {
> > +     BIT(ADXRS290_IDX_X) | BIT(ADXRS290_IDX_Y) | BIT(ADXRS290_IDX_TEMP),
> > +     0
> >  };
> >
> >  static const struct iio_info adxrs290_info = {
> > @@ -365,6 +549,47 @@ static const struct iio_info adxrs290_info = {
> >       .read_avail = &adxrs290_read_avail,
> >  };
> >
> > +static int adxrs290_probe_trigger(struct iio_dev *indio_dev)
> > +{
> > +     struct adxrs290_state *st = iio_priv(indio_dev);
> > +     int ret;
> > +
> > +     if (!st->spi->irq) {
> > +             dev_info(&st->spi->dev, "no irq, using polling\n");
> > +             return 0;
> > +     }
> > +
> > +     st->dready_trig = devm_iio_trigger_alloc(&st->spi->dev,
> > +                                              "%s-dev%d",
> > +                                              indio_dev->name,
> > +                                              indio_dev->id);
>
> As below, this could be sensibly done on fewer lines without any real loss
> of readability (staying under advised 80 chars)
>

Sure, will re-align in v2.

> > +     if (!st->dready_trig)
> > +             return -ENOMEM;
> > +
> > +     st->dready_trig->dev.parent = &st->spi->dev;
> > +     st->dready_trig->ops = &adxrs290_trigger_ops;
> > +     iio_trigger_set_drvdata(st->dready_trig, indio_dev);
> > +
> > +     ret = devm_request_irq(&st->spi->dev, st->spi->irq,
> > +                            &iio_trigger_generic_data_rdy_poll,
> > +                            IRQF_ONESHOT,
> > +                            "adxrs290_irq", st->dready_trig);
> > +     if (ret < 0) {
> > +             dev_err(&st->spi->dev, "request irq %d failed\n", st->spi->irq);
> > +             return ret;
> > +     }
> > +
> > +     ret = devm_iio_trigger_register(&st->spi->dev, st->dready_trig);
> > +     if (ret) {
> > +             dev_err(&st->spi->dev, "iio trigger register failed\n");
> > +             return ret;
> > +     }
> > +
> > +     indio_dev->trig = iio_trigger_get(st->dready_trig);
> > +
> > +     return 0;
> > +}
> > +
> >  static int adxrs290_probe(struct spi_device *spi)
> >  {
> >       struct iio_dev *indio_dev;
> > @@ -384,6 +609,7 @@ static int adxrs290_probe(struct spi_device *spi)
> >       indio_dev->channels = adxrs290_channels;
> >       indio_dev->num_channels = ARRAY_SIZE(adxrs290_channels);
> >       indio_dev->info = &adxrs290_info;
> > +     indio_dev->available_scan_masks = adxrs290_avail_scan_masks;
> >
> >       mutex_init(&st->lock);
> >
> > @@ -416,6 +642,12 @@ static int adxrs290_probe(struct spi_device *spi)
> >       /* max transition time to measurement mode */
> >       msleep(ADXRS290_MAX_TRANSITION_TIME_MS);
> >
> > +     ret = devm_add_action_or_reset(&spi->dev,
> > +                                    adxrs290_chip_off_action,
> > +                                    indio_dev);
>
> What is this unwinding?  We should only be using devm to undo things
> that have been 'done' in probe and the association should be clear.
>

This unwinding is for placing the device **back** to its default
'STANDBY' mode. The device, during the probe, was put to 'MEASUREMENT'
mode during 'adxrs290_initial_setup()' a few lines back.

How else do you suggest I highlight the association?

> You have been a bit in consistent with wrapping.  Personally I would
> go for as many parameters as fit on each line under 80 chars as possible.
> The only exception being when there is some clear 'pairing' of parameters
> or similar.
>

Yes, I realize that, sorry. Will fix wherever necessary in v2.

With regards,
Nishant Malpani

> > +     if (ret < 0)
> > +             return ret;
> > +
> >       ret = adxrs290_get_3db_freq(indio_dev, &val, &val2);
> >       if (ret < 0)
> >               return ret;
> > @@ -423,6 +655,18 @@ static int adxrs290_probe(struct spi_device *spi)
> >       st->lpf_3db_freq_idx = val;
> >       st->hpf_3db_freq_idx = val2;
> >
> > +     ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
> > +                                           &iio_pollfunc_store_time,
> > +                                           &adxrs290_trigger_handler, NULL);
> > +     if (ret < 0) {
> > +             dev_err(&spi->dev, "iio triggered buffer setup failed\n");
> > +             return ret;
> > +     }
> > +
> > +     ret = adxrs290_probe_trigger(indio_dev);
> > +     if (ret < 0)
> > +             return ret;
> > +
> >       return devm_iio_device_register(&spi->dev, indio_dev);
> >  }
> >
>

Re: [PATCH 2/3] dt-bindings: iio: gyro: adxrs290: Add required interrupts property

[Nishant Malpani]

Hello,

On Sat, Aug 29, 2020 at 10:18 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 25 Aug 2020 18:17:10 +0530
> Nishant Malpani <nish.malpani25@gmail.com> wrote:
>
> > Append 'interrupts' as a required property and provide a suitable example
> > for using a GPIO interrupt line.
> >
> > Signed-off-by: Nishant Malpani <nish.malpani25@gmail.com>
> Hi Nishant,
>
> I don't understand why the interrupt is 'required'.  Also note you should
> never be adding required properties to an existing binding.  It's possible
> someone already used the binding as it stands and shipped a board with
> it burnt in a firmware. (bit unlikely but you never know!)
>
> Jonathan
>

You're right; I hadn't thought of it that way. Will remove the
'interrupts' property from the 'required' ist in v2.

> > ---
> >  .../devicetree/bindings/iio/gyroscope/adi,adxrs290.yaml   | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/iio/gyroscope/adi,adxrs290.yaml b/Documentation/devicetree/bindings/iio/gyroscope/adi,adxrs290.yaml
> > index 61adb2c2454b..cae593dd1ba7 100644
> > --- a/Documentation/devicetree/bindings/iio/gyroscope/adi,adxrs290.yaml
> > +++ b/Documentation/devicetree/bindings/iio/gyroscope/adi,adxrs290.yaml
> > @@ -28,17 +28,23 @@ properties:
> >
> >    spi-cpha: true
> >
> > +  interrupts:
> > +    maxItems: 1
> > +
> >  required:
> >    - compatible
> >    - reg
> >    - spi-max-frequency
> >    - spi-cpol
> >    - spi-cpha
> > +  - interrupts
>
> Why?  Device works fine without one being supplied.
> It's not uncommon on embedded boards to not wire up interrupts
> due to a lack of pins and just rely on polling.
>

I was under the impression that the triggered-buffer way of capturing
data would more reasonable to the consumers of a gyroscope. But what
you point out makes total sense. Thanks for pointing out. Will fix it
in v2.

With regards,
Nishant Malpani

> >
> >  additionalProperties: false
> >
> >  examples:
> >    - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> >      spi {
> >          #address-cells = <1>;
> >          #size-cells = <0>;
> > @@ -48,6 +54,8 @@ examples:
> >                     spi-max-frequency = <5000000>;
> >                     spi-cpol;
> >                     spi-cpha;
> > +                   interrupt-parent = <&gpio>;
> > +                   interrupts = <25 IRQ_TYPE_EDGE_RISING>;
> >          };
> >      };
> >  ...
>

Re: [PATCH 1/3] iio: gyro: adxrs290: Add triggered buffer support

[Jonathan Cameron]

On Thu, 3 Sep 2020 18:07:00 +0530
Nishant Malpani <nish.malpani25@gmail.com> wrote:

> Hello,
> 
> Thanks for the review, Jonathan. Comments inline...
> 
...

> > >
> > > +static void adxrs290_chip_off_action(void *data)
> > > +{
> > > +     struct iio_dev *indio_dev = data;
> > > +
> > > +     adxrs290_set_mode(indio_dev, ADXRS290_MODE_STANDBY);
> > > +}
> > > +
> > >  static int adxrs290_read_raw(struct iio_dev *indio_dev,
> > >                            struct iio_chan_spec const *chan,
> > >                            int *val,
> > > @@ -215,24 +287,34 @@ static int adxrs290_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;
> > > +
> > >               switch (chan->type) {
> > >               case IIO_ANGL_VEL:
> > >                       ret = adxrs290_get_rate_data(indio_dev,
> > >                                                    ADXRS290_READ_REG(chan->address),
> > >                                                    val);
> > >                       if (ret < 0)
> > > -                             return ret;
> > > +                             goto err_release;
> > >
> > > -                     return IIO_VAL_INT;
> > > +                     ret = IIO_VAL_INT;
> > > +                     break;
> > >               case IIO_TEMP:
> > >                       ret = adxrs290_get_temp_data(indio_dev, val);
> > >                       if (ret < 0)
> > > -                             return ret;
> > > +                             goto err_release;
> > >
> > > -                     return IIO_VAL_INT;
> > > +                     ret = IIO_VAL_INT;
> > > +                     break;
> > >               default:
> > > -                     return -EINVAL;
> > > +                     ret = -EINVAL;
> > > +                     break;
> > >               }
> > > +err_release:
> > > +             iio_device_release_direct_mode(indio_dev);
> > > +             return ret;
> > >       case IIO_CHAN_INFO_SCALE:
> > >               switch (chan->type) {
> > >               case IIO_ANGL_VEL:
> > > @@ -279,34 +361,53 @@ static int adxrs290_write_raw(struct iio_dev *indio_dev,
> > >                             long mask)
> > >  {
> > >       struct adxrs290_state *st = iio_priv(indio_dev);
> > > -     int lpf_idx, hpf_idx;
> > > +     int ret, lpf_idx, hpf_idx;
> > > +
> > > +     ret = iio_device_claim_direct_mode(indio_dev);  
> >
> > Is there anything in the datasheet that says we can't change the filters
> > whilst doing captures?  Might get crazy data but if the datasheet doesn't
> > want against a particular action, then we tend not to try and prevent it.
> >  
> 
> The datasheet, to my knowledge, explicitly doesn't mention any
> restriction on changing the filter settings during a capture.
> 
> Quoting the datasheet, "The group delay of the wideband filter option
> is less than 0.5ms" (pg. 11) - during an ongoing capture if one
> re-configures the filter, how do we address this?

To be honest, I'm not really sure what that means, so will if you think
it makes sense it is fine to keep protections to not allow a filter update
during buffered capture.

> 
> > > +     if (ret)
> > > +             return ret;
...
> > >  static int adxrs290_probe(struct spi_device *spi)
> > >  {
> > >       struct iio_dev *indio_dev;
> > > @@ -384,6 +609,7 @@ static int adxrs290_probe(struct spi_device *spi)
> > >       indio_dev->channels = adxrs290_channels;
> > >       indio_dev->num_channels = ARRAY_SIZE(adxrs290_channels);
> > >       indio_dev->info = &adxrs290_info;
> > > +     indio_dev->available_scan_masks = adxrs290_avail_scan_masks;
> > >
> > >       mutex_init(&st->lock);
> > >
> > > @@ -416,6 +642,12 @@ static int adxrs290_probe(struct spi_device *spi)
> > >       /* max transition time to measurement mode */
> > >       msleep(ADXRS290_MAX_TRANSITION_TIME_MS);
> > >
> > > +     ret = devm_add_action_or_reset(&spi->dev,
> > > +                                    adxrs290_chip_off_action,
> > > +                                    indio_dev);  
> >
> > What is this unwinding?  We should only be using devm to undo things
> > that have been 'done' in probe and the association should be clear.
> >  
> 
> This unwinding is for placing the device **back** to its default
> 'STANDBY' mode. The device, during the probe, was put to 'MEASUREMENT'
> mode during 'adxrs290_initial_setup()' a few lines back.
> 
> How else do you suggest I highlight the association?

Perhaps put it inside the call to initial_setup() so it is clearly
paired with the 'forwards' action.

Jonathan