[PATCH] iio: accel: adxl355: Add triggered buffer support

[PATCH] iio: accel: adxl355: Add triggered buffer support

[Puranjay Mohan]

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

Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
---
 drivers/iio/accel/Kconfig        |   4 +
 drivers/iio/accel/adxl355_core.c | 154 ++++++++++++++++++++++++++++++-
 2 files changed, 155 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
index d0c45c809..9c16c1841 100644
--- a/drivers/iio/accel/Kconfig
+++ b/drivers/iio/accel/Kconfig
@@ -69,6 +69,8 @@ config ADXL355_I2C
 	depends on I2C
 	select ADXL355
 	select REGMAP_I2C
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
 	help
 	  Say Y here if you want to build i2c support for the Analog Devices
 	  ADXL355 3-axis digital accelerometer.
@@ -82,6 +84,8 @@ config ADXL355_SPI
 	depends on SPI
 	select ADXL355
 	select REGMAP_SPI
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
 	help
 	  Say Y here if you want to build spi support for the Analog Devices
 	  ADXL355 3-axis digital accelerometer.
diff --git a/drivers/iio/accel/adxl355_core.c b/drivers/iio/accel/adxl355_core.c
index c91d2254c..bd5304bcf 100644
--- a/drivers/iio/accel/adxl355_core.c
+++ b/drivers/iio/accel/adxl355_core.c
@@ -6,14 +6,18 @@
  *
  * Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adxl354_adxl355.pdf
  */
-
 #include <linux/bits.h>
 #include <linux/bitfield.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 <linux/limits.h>
 #include <linux/math64.h>
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
+#include <linux/of_irq.h>
 #include <linux/regmap.h>
 #include <asm/unaligned.h>
 
@@ -46,6 +50,7 @@
 #define ADXL355_RANGE_REG		0x2C
 #define ADXL355_POWER_CTL_REG		0x2D
 #define  ADXL355_POWER_CTL_MODE_MSK	GENMASK(1, 0)
+#define  ADXL355_POWER_CTL_DRDY_MSK	BIT(2)
 #define ADXL355_SELF_TEST_REG		0x2E
 #define ADXL355_RESET_REG		0x2F
 
@@ -157,6 +162,7 @@ static const struct adxl355_chan_info adxl355_chans[] = {
 };
 
 struct adxl355_data {
+	int irq;
 	struct regmap *regmap;
 	struct device *dev;
 	struct mutex lock; /* lock to protect op_mode */
@@ -165,7 +171,14 @@ struct adxl355_data {
 	enum adxl355_hpf_3db hpf_3db;
 	int calibbias[3];
 	int adxl355_hpf_3db_table[7][2];
-	u8 transf_buf[3] ____cacheline_aligned;
+	struct iio_trigger      *dready_trig;
+	union {
+		u8 transf_buf[3];
+		struct {
+			u8 buf[14];
+			s64 ts;
+		} buffer;
+	} ____cacheline_aligned;
 };
 
 static int adxl355_set_op_mode(struct adxl355_data *data,
@@ -186,6 +199,23 @@ static int adxl355_set_op_mode(struct adxl355_data *data,
 	return ret;
 }
 
+static int adxl355_data_rdy_trigger_set_state(struct iio_trigger *trig,
+					      bool state)
+{
+	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+	struct adxl355_data *data = iio_priv(indio_dev);
+	int ret;
+
+	mutex_lock(&data->lock);
+	ret = regmap_update_bits(data->regmap, ADXL355_POWER_CTL_REG,
+				 ADXL355_POWER_CTL_DRDY_MSK,
+				 FIELD_PREP(ADXL355_POWER_CTL_DRDY_MSK,
+					    !state));
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
 static void adxl355_fill_3db_frequency_table(struct adxl355_data *data)
 {
 	u32 multiplier;
@@ -246,6 +276,12 @@ static int adxl355_setup(struct adxl355_data *data)
 	if (ret)
 		return ret;
 
+	ret = regmap_update_bits(data->regmap, ADXL355_POWER_CTL_REG,
+				 ADXL355_POWER_CTL_DRDY_MSK,
+				 FIELD_PREP(ADXL355_POWER_CTL_DRDY_MSK, 1));
+	if (ret)
+		return ret;
+
 	adxl355_fill_3db_frequency_table(data);
 
 	return adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
@@ -499,12 +535,65 @@ static int adxl355_read_avail(struct iio_dev *indio_dev,
 	}
 }
 
+static const unsigned long adxl355_avail_scan_masks[] = {
+	GENMASK(3, 0),
+	0
+};
+
 static const struct iio_info adxl355_info = {
 	.read_raw	= adxl355_read_raw,
 	.write_raw	= adxl355_write_raw,
 	.read_avail	= &adxl355_read_avail,
 };
 
+static const struct iio_trigger_ops adxl355_trigger_ops = {
+	.set_trigger_state = &adxl355_data_rdy_trigger_set_state,
+	.validate_device = &iio_trigger_validate_own_device,
+};
+
+static irqreturn_t adxl355_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct adxl355_data *data = iio_priv(indio_dev);
+	int ret;
+
+	mutex_lock(&data->lock);
+
+	ret = regmap_bulk_read(data->regmap, ADXL355_XDATA3_REG,
+			       &data->buffer.buf[1],
+			       3);
+	if (ret)
+		goto out_unlock_notify;
+
+	ret = regmap_bulk_read(data->regmap, ADXL355_YDATA3_REG,
+			       &data->buffer.buf[5],
+			       3);
+	if (ret)
+		goto out_unlock_notify;
+
+	ret = regmap_bulk_read(data->regmap, ADXL355_ZDATA3_REG,
+			       &data->buffer.buf[9],
+			       3);
+	if (ret)
+		goto out_unlock_notify;
+
+	ret = regmap_bulk_read(data->regmap, ADXL355_TEMP2_REG,
+			       &data->buffer.buf[12],
+			       2);
+	if (ret)
+		goto out_unlock_notify;
+
+	iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer,
+					   pf->timestamp);
+
+out_unlock_notify:
+	mutex_unlock(&data->lock);
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
 #define ADXL355_ACCEL_CHANNEL(index, reg, axis) {			\
 	.type = IIO_ACCEL,						\
 	.address = reg,							\
@@ -518,6 +607,7 @@ static const struct iio_info adxl355_info = {
 	.info_mask_shared_by_type_available =				\
 		BIT(IIO_CHAN_INFO_SAMP_FREQ) |				\
 		BIT(IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY),	\
+	.scan_index = index,						\
 	.scan_type = {							\
 		.sign = 's',						\
 		.realbits = 20,						\
@@ -537,6 +627,7 @@ static const struct iio_chan_spec adxl355_channels[] = {
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 				      BIT(IIO_CHAN_INFO_SCALE) |
 				      BIT(IIO_CHAN_INFO_OFFSET),
+		.scan_index = 3,
 		.scan_type = {
 			.sign = 's',
 			.realbits = 12,
@@ -544,8 +635,46 @@ static const struct iio_chan_spec adxl355_channels[] = {
 			.endianness = IIO_BE,
 		},
 	},
+	IIO_CHAN_SOFT_TIMESTAMP(4),
 };
 
+static int adxl355_probe_trigger(struct iio_dev *indio_dev)
+{
+	struct adxl355_data *data = iio_priv(indio_dev);
+	int ret;
+
+	if (!data->irq) {
+		dev_info(data->dev, "no irq, using polling\n");
+		return 0;
+	}
+
+	data->dready_trig = devm_iio_trigger_alloc(data->dev, "%s-dev%d",
+						   indio_dev->name,
+						   indio_dev->id);
+	if (!data->dready_trig)
+		return -ENOMEM;
+
+	data->dready_trig->ops = &adxl355_trigger_ops;
+	iio_trigger_set_drvdata(data->dready_trig, indio_dev);
+
+	ret = devm_request_irq(data->dev, data->irq,
+			       &iio_trigger_generic_data_rdy_poll,
+			       IRQF_ONESHOT, "adxl355_irq", data->dready_trig);
+	if (ret < 0)
+		return dev_err_probe(data->dev, ret, "request irq %d failed\n",
+				     data->irq);
+
+	ret = devm_iio_trigger_register(data->dev, data->dready_trig);
+	if (ret) {
+		dev_err(data->dev, "iio trigger register failed\n");
+		return ret;
+	}
+
+	indio_dev->trig = iio_trigger_get(data->dready_trig);
+
+	return 0;
+}
+
 int adxl355_core_probe(struct device *dev, struct regmap *regmap,
 		       const char *name)
 {
@@ -563,18 +692,37 @@ int adxl355_core_probe(struct device *dev, struct regmap *regmap,
 	data->op_mode = ADXL355_STANDBY;
 	mutex_init(&data->lock);
 
+	/*
+	 * Would be good to move it to the generic version.
+	 */
+	ret = of_irq_get_byname(dev->of_node, "DRDY");
+	if (ret > 0)
+		data->irq = ret;
+
 	indio_dev->name = name;
 	indio_dev->info = &adxl355_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->channels = adxl355_channels;
 	indio_dev->num_channels = ARRAY_SIZE(adxl355_channels);
+	indio_dev->available_scan_masks = adxl355_avail_scan_masks;
 
 	ret = adxl355_setup(data);
-	if (ret < 0) {
+	if (ret) {
 		dev_err(dev, "ADXL355 setup failed\n");
 		return ret;
 	}
 
+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
+					      &iio_pollfunc_store_time,
+					      &adxl355_trigger_handler, NULL);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "iio triggered buffer setup failed\n");
+
+	ret = adxl355_probe_trigger(indio_dev);
+	if (ret)
+		return ret;
+
 	return devm_iio_device_register(dev, indio_dev);
 }
 EXPORT_SYMBOL_GPL(adxl355_core_probe);
-- 
2.30.1

Re: [PATCH] iio: accel: adxl355: Add triggered buffer support

[Andy Shevchenko]

On Fri, Aug 13, 2021 at 11:35 AM Puranjay Mohan <puranjay12@gmail.com> wrote:
>
> Provide a way for continuous data capture by setting up buffer support. The
> data ready signal exposed at the DRDY pin of the ADXL355 is exploited as
> a hardware interrupt which triggers to fill the buffer.

...

>   *
>   * Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adxl354_adxl355.pdf
>   */
> -

Unrelated change.

...

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

I would rather regroup this like

linux/*

asm/*

linux/iio*

...

> +#include <linux/of_irq.h>

Okay, this is due to the absence of generic fwnode_irq_get_byname() or so.

...

>  struct adxl355_data {

> +       int irq;

Depending on container_of and frequency of usage this is not a good
location for this.
Strating from regmap pointer is much better (no pointer arithmetics involved).

>         struct regmap *regmap;
>         struct device *dev;
>         struct mutex lock; /* lock to protect op_mode */

>  };

...

> +       ret = regmap_bulk_read(data->regmap, ADXL355_XDATA3_REG,
> +                              &data->buffer.buf[1],
> +                              3);

ARRAY_SIZE()? Or put this 3 to the previous line, it will be easier to read.

Ditto for the rest of the similar code.

> +       if (ret)
> +               goto out_unlock_notify;

...

> +       /*
> +        * Would be good to move it to the generic version.

Something like "TODO: Would be..." ?

> +        */
> +       ret = of_irq_get_byname(dev->of_node, "DRDY");
> +       if (ret > 0)
> +               data->irq = ret;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: accel: adxl355: Add triggered buffer support

[Jonathan Cameron]

On Fri, 13 Aug 2021 14:04:55 +0530
Puranjay Mohan <puranjay12@gmail.com> wrote:

> Provide a way for continuous data capture by setting up buffer support. The
> data ready signal exposed at the DRDY pin of the ADXL355 is exploited as
> a hardware interrupt which triggers to fill the buffer.
> 
> Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>

Hi.

A few comments additions to add to what Andy has highlighted.
Other than these details and those Andy highlighted, this looks good to me.

...

> @@ -157,6 +162,7 @@ static const struct adxl355_chan_info adxl355_chans[] = {
>  };
>  
>  struct adxl355_data {
> +	int irq;
>  	struct regmap *regmap;
>  	struct device *dev;
>  	struct mutex lock; /* lock to protect op_mode */
> @@ -165,7 +171,14 @@ struct adxl355_data {
>  	enum adxl355_hpf_3db hpf_3db;
>  	int calibbias[3];
>  	int adxl355_hpf_3db_table[7][2];
> -	u8 transf_buf[3] ____cacheline_aligned;
> +	struct iio_trigger      *dready_trig;

As the rest of the structure doesn't have extra spaces
for alignment of variable names, don't do it here either.

> +	union {
> +		u8 transf_buf[3];
> +		struct {
> +			u8 buf[14];
> +			s64 ts;
> +		} buffer;
> +	} ____cacheline_aligned;
>  };
>  
>  static int adxl355_set_op_mode(struct adxl355_data *data,
> @@ -186,6 +199,23 @@ static int adxl355_set_op_mode(struct adxl355_data *data,
>  	return ret;
>  }
>  


...

> +static const struct iio_trigger_ops adxl355_trigger_ops = {
> +	.set_trigger_state = &adxl355_data_rdy_trigger_set_state,
> +	.validate_device = &iio_trigger_validate_own_device,
> +};
> +
> +static irqreturn_t adxl355_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct adxl355_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	mutex_lock(&data->lock);
> +
> +	ret = regmap_bulk_read(data->regmap, ADXL355_XDATA3_REG,
> +			       &data->buffer.buf[1],

I would add some comments on the data layout here given you are
writing the bottom 24bits of a 32 bit big endian variable and that
may be non obvious.

There is also an issue as a result of the union used for the buffer.
It is (I think) possible that earlier read has put a non 0 value
into transf_buf[0].  Now C doesn't guarantee that will be visible
if accessed via buffer.buf[0], but it's certainly possible.
As such you may get garbage in that top byte.  It's not a 'problem'
from an ABI point of view, but it might well confused someone
who is for example logging the data stream unprocessed.
We have drivers where the hardware puts something in the bits
that will get screened out using the type (often status info and
similar) but we probably don't want it to be from an entirely different
read or write.

Perhaps just set buf[0] = 0 somewhere in this funciton with a comment
on why.

> +			       3);
> +	if (ret)
> +		goto out_unlock_notify;
> +

...

>  
> +static int adxl355_probe_trigger(struct iio_dev *indio_dev)
> +{
> +	struct adxl355_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (!data->irq) {
> +		dev_info(data->dev, "no irq, using polling\n");
> +		return 0;
> +	}
> +
> +	data->dready_trig = devm_iio_trigger_alloc(data->dev, "%s-dev%d",
> +						   indio_dev->name,
> +						   indio_dev->id);
> +	if (!data->dready_trig)
> +		return -ENOMEM;
> +
> +	data->dready_trig->ops = &adxl355_trigger_ops;
> +	iio_trigger_set_drvdata(data->dready_trig, indio_dev);
> +
> +	ret = devm_request_irq(data->dev, data->irq,
> +			       &iio_trigger_generic_data_rdy_poll,
> +			       IRQF_ONESHOT, "adxl355_irq", data->dready_trig);
> +	if (ret < 0)
if (ret)

If you follow through what devm_request_irq calls, you will see
devm_request_threaded_irq internally checks for errors with if (rc) so
we should do the same here.  It would also be more consistent with the
rest of your code in this function.

> +		return dev_err_probe(data->dev, ret, "request irq %d failed\n",
> +				     data->irq);
> +
> +	ret = devm_iio_trigger_register(data->dev, data->dready_trig);
> +	if (ret) {
> +		dev_err(data->dev, "iio trigger register failed\n");
> +		return ret;
> +	}
> +
> +	indio_dev->trig = iio_trigger_get(data->dready_trig);
> +
> +	return 0;
> +}
> +
>  int adxl355_core_probe(struct device *dev, struct regmap *regmap,
>  		       const char *name)
>  {
> @@ -563,18 +692,37 @@ int adxl355_core_probe(struct device *dev, struct regmap *regmap,
>  	data->op_mode = ADXL355_STANDBY;
>  	mutex_init(&data->lock);
>  
> +	/*
> +	 * Would be good to move it to the generic version.
> +	 */
> +	ret = of_irq_get_byname(dev->of_node, "DRDY");
> +	if (ret > 0)
> +		data->irq = ret;
> +
>  	indio_dev->name = name;
>  	indio_dev->info = &adxl355_info;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->channels = adxl355_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(adxl355_channels);
> +	indio_dev->available_scan_masks = adxl355_avail_scan_masks;
>  
>  	ret = adxl355_setup(data);
> -	if (ret < 0) {
> +	if (ret) {

Nitpick. Ideally this would have been in an earlier patch, but it's trivial here so
I'm not that bothered (so don't bother moving it).

>  		dev_err(dev, "ADXL355 setup failed\n");
>  		return ret;
>  	}
>  
> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> +					      &iio_pollfunc_store_time,
> +					      &adxl355_trigger_handler, NULL);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "iio triggered buffer setup failed\n");
> +
> +	ret = adxl355_probe_trigger(indio_dev);
> +	if (ret)
> +		return ret;
> +
>  	return devm_iio_device_register(dev, indio_dev);
>  }
>  EXPORT_SYMBOL_GPL(adxl355_core_probe);