LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v2] iio: accel: adxl355: Add triggered buffer support
@ 2021-08-19 18:11 Puranjay Mohan
  2021-08-30 15:14 ` Jonathan Cameron
  0 siblings, 1 reply; 4+ messages in thread
From: Puranjay Mohan @ 2021-08-19 18:11 UTC (permalink / raw)
  To: jic23, Michael.Hennerich, linux-iio, linux-kernel, lars,
	Dragos.Bogdan, Darius.Berghe, andy.shevchenko
  Cc: 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>
---
Changes since v1:
- Fix an indentation issue.
- Add comments on layout of data in buffer.
- Zero the buffer before using it in trigger handler.
- Use if(ret) in place of if(ret < 0)
---
drivers/iio/accel/Kconfig        |   4 +
 drivers/iio/accel/adxl355_core.c | 162 ++++++++++++++++++++++++++++++-
 2 files changed, 164 insertions(+), 2 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..d499403dc 100644
--- a/drivers/iio/accel/adxl355_core.c
+++ b/drivers/iio/accel/adxl355_core.c
@@ -9,11 +9,16 @@
 
 #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 +51,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
 
@@ -165,7 +171,15 @@ 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;
+	int irq;
+	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 +200,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 +277,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 +536,74 @@ 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);
+
+	/*
+	 * data->buffer is used both for triggered buffer support
+	 * and read/write_raw(), hence, it has to be zeroed here before usage.
+	 */
+	data->buffer.buf[0] = 0;
+
+	/*
+	 * The accelaration data is 24 bits and big endian. It has to be saved
+	 * in 32 bits, hence, it is saved in the 2nd byte of the 4 byte buffer.
+	 * The buf array is 14 bytes as it includes 3x4=12 bytes for
+	 * accelaration data of x, y, and z axis. It also includes 2 bytes for
+	 * temperature data.
+	 */
+	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 +617,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 +637,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 +645,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)
+		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 +702,37 @@ int adxl355_core_probe(struct device *dev, struct regmap *regmap,
 	data->op_mode = ADXL355_STANDBY;
 	mutex_init(&data->lock);
 
+	/*
+	 * TODO: 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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] iio: accel: adxl355: Add triggered buffer support
  2021-08-19 18:11 [PATCH v2] iio: accel: adxl355: Add triggered buffer support Puranjay Mohan
@ 2021-08-30 15:14 ` Jonathan Cameron
  2021-08-31 17:09   ` Puranjay Mohan
  0 siblings, 1 reply; 4+ messages in thread
From: Jonathan Cameron @ 2021-08-30 15:14 UTC (permalink / raw)
  To: Puranjay Mohan
  Cc: Michael.Hennerich, linux-iio, linux-kernel, lars, Dragos.Bogdan,
	Darius.Berghe, andy.shevchenko

On Thu, 19 Aug 2021 23:41:33 +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 Puranjay,

A fresh read and I noticed a few additional things.
All simple stuff.

I do have a slight concern that we might be setting ourselves up for problems
once the fifo is enabled.  Hopefully not, but if you have looked at that and
verified it will drop in nicely then that's great. We might be fine.

Jonathan


> ---
> Changes since v1:
> - Fix an indentation issue.
> - Add comments on layout of data in buffer.
> - Zero the buffer before using it in trigger handler.
> - Use if(ret) in place of if(ret < 0)
> ---
> drivers/iio/accel/Kconfig        |   4 +
>  drivers/iio/accel/adxl355_core.c | 162 ++++++++++++++++++++++++++++++-
>  2 files changed, 164 insertions(+), 2 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..d499403dc 100644
> --- a/drivers/iio/accel/adxl355_core.c
> +++ b/drivers/iio/accel/adxl355_core.c
> @@ -9,11 +9,16 @@
>  
>  #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 +51,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
>  
> @@ -165,7 +171,15 @@ 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;
> +	int irq;
> +	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 +200,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);

Why take the lock?  What are you protecting here?

> +	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 +277,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 +536,74 @@ 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,

This is fine for now, but I'd like you to think about whether you can ultimately
drop this validation and allow other devices to be triggered from this signal.
It can be very helpful when doing sensor fusion to grab data at (approximately)
the same time from a set of different sensors.

IIRC this device has a fifo though which can complicate that option.

> +};
> +
> +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);
> +
> +	/*
> +	 * data->buffer is used both for triggered buffer support
> +	 * and read/write_raw(), hence, it has to be zeroed here before usage.
> +	 */
> +	data->buffer.buf[0] = 0;
> +
> +	/*
> +	 * The accelaration data is 24 bits and big endian. It has to be saved

Spell check.  acceleration

> +	 * in 32 bits, hence, it is saved in the 2nd byte of the 4 byte buffer.
> +	 * The buf array is 14 bytes as it includes 3x4=12 bytes for
> +	 * accelaration data of x, y, and z axis. It also includes 2 bytes for
> +	 * temperature data.
> +	 */
> +	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 +617,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 +637,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 +645,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");

I would move this check externally so that we never probe if it's
not wired up.  It's also easy to tell (no trigger available) so
the dev_info is probably unnecessary noise in the log.


> +		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)
> +		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 +702,37 @@ int adxl355_core_probe(struct device *dev, struct regmap *regmap,
>  	data->op_mode = ADXL355_STANDBY;
>  	mutex_init(&data->lock);
>  
> +	/*
> +	 * TODO: 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;

If you do this get down near the probe_trigger() call then you should be able to
avoid keeping a copy of irq in data.  Just pass it into that function as
a parameter.

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

This looks to be an unrelated cleanup.  Please move to a precursor patch as
it adds noise in this one which is fiddly enough without 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);


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] iio: accel: adxl355: Add triggered buffer support
  2021-08-30 15:14 ` Jonathan Cameron
@ 2021-08-31 17:09   ` Puranjay Mohan
  2021-09-05  9:45     ` Jonathan Cameron
  0 siblings, 1 reply; 4+ messages in thread
From: Puranjay Mohan @ 2021-08-31 17:09 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Hennerich, Michael, linux-iio, Linux Kernel Mailing List,
	Lars-Peter Clausen, Bogdan, Dragos, Berghe, Darius,
	Andy Shevchenko

On Mon, Aug 30, 2021 at 8:40 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 19 Aug 2021 23:41:33 +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 Puranjay,
>
> A fresh read and I noticed a few additional things.
> All simple stuff.
>
> I do have a slight concern that we might be setting ourselves up for problems
> once the fifo is enabled.  Hopefully not, but if you have looked at that and
> verified it will drop in nicely then that's great. We might be fine.

Yes, I will work on FIFO and make sure I fix all issues if any arise.

>
> Jonathan
>
>
> > ---
> > Changes since v1:
> > - Fix an indentation issue.
> > - Add comments on layout of data in buffer.
> > - Zero the buffer before using it in trigger handler.
> > - Use if(ret) in place of if(ret < 0)
> > ---
> > drivers/iio/accel/Kconfig        |   4 +
> >  drivers/iio/accel/adxl355_core.c | 162 ++++++++++++++++++++++++++++++-
> >  2 files changed, 164 insertions(+), 2 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..d499403dc 100644
> > --- a/drivers/iio/accel/adxl355_core.c
> > +++ b/drivers/iio/accel/adxl355_core.c
> > @@ -9,11 +9,16 @@
> >
> >  #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 +51,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
> >
> > @@ -165,7 +171,15 @@ 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;
> > +     int irq;
> > +     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 +200,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);
>
> Why take the lock?  What are you protecting here?

Power Control Register is the same register that is used to put the
device in measurement mode, so it has to be protected.

>
> > +     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 +277,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 +536,74 @@ 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,
>
> This is fine for now, but I'd like you to think about whether you can ultimately
> drop this validation and allow other devices to be triggered from this signal.
> It can be very helpful when doing sensor fusion to grab data at (approximately)
> the same time from a set of different sensors.

I will remove this line in the next patch, I will read about this more.

>
> IIRC this device has a fifo though which can complicate that option.
>
> > +};
> > +
> > +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);
> > +
> > +     /*
> > +      * data->buffer is used both for triggered buffer support
> > +      * and read/write_raw(), hence, it has to be zeroed here before usage.
> > +      */
> > +     data->buffer.buf[0] = 0;
> > +
> > +     /*
> > +      * The accelaration data is 24 bits and big endian. It has to be saved
>
> Spell check.  acceleration

Will change in next version.

>
> > +      * in 32 bits, hence, it is saved in the 2nd byte of the 4 byte buffer.
> > +      * The buf array is 14 bytes as it includes 3x4=12 bytes for
> > +      * accelaration data of x, y, and z axis. It also includes 2 bytes for
> > +      * temperature data.
> > +      */
> > +     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 +617,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 +637,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 +645,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");
>
> I would move this check externally so that we never probe if it's
> not wired up.  It's also easy to tell (no trigger available) so
> the dev_info is probably unnecessary noise in the log.

I will remove this in next version.

>
>
> > +             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)
> > +             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 +702,37 @@ int adxl355_core_probe(struct device *dev, struct regmap *regmap,
> >       data->op_mode = ADXL355_STANDBY;
> >       mutex_init(&data->lock);
> >
> > +     /*
> > +      * TODO: 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;
>
> If you do this get down near the probe_trigger() call then you should be able to
> avoid keeping a copy of irq in data.  Just pass it into that function as
> a parameter.

ok, I will change it in next version.

>
> > +
> >       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) {
>
> This looks to be an unrelated cleanup.  Please move to a precursor patch as
> it adds noise in this one which is fiddly enough without it ;)

ok, I will make a separate patch for this and send a series.

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


-- 
Thanks and Regards

Yours Truly,

Puranjay Mohan

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] iio: accel: adxl355: Add triggered buffer support
  2021-08-31 17:09   ` Puranjay Mohan
@ 2021-09-05  9:45     ` Jonathan Cameron
  0 siblings, 0 replies; 4+ messages in thread
From: Jonathan Cameron @ 2021-09-05  9:45 UTC (permalink / raw)
  To: Puranjay Mohan
  Cc: Hennerich, Michael, linux-iio, Linux Kernel Mailing List,
	Lars-Peter Clausen, Bogdan, Dragos, Berghe, Darius,
	Andy Shevchenko

On Tue, 31 Aug 2021 22:39:19 +0530
Puranjay Mohan <puranjay12@gmail.com> wrote:

> On Mon, Aug 30, 2021 at 8:40 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Thu, 19 Aug 2021 23:41:33 +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 Puranjay,
> >
> > A fresh read and I noticed a few additional things.
> > All simple stuff.
> >
> > I do have a slight concern that we might be setting ourselves up for problems
> > once the fifo is enabled.  Hopefully not, but if you have looked at that and
> > verified it will drop in nicely then that's great. We might be fine.  
> 
> Yes, I will work on FIFO and make sure I fix all issues if any arise.
> 
> >
> > Jonathan
> >
> >  
> > > ---
> > > Changes since v1:
> > > - Fix an indentation issue.
> > > - Add comments on layout of data in buffer.
> > > - Zero the buffer before using it in trigger handler.
> > > - Use if(ret) in place of if(ret < 0)
> > > ---
> > > drivers/iio/accel/Kconfig        |   4 +
> > >  drivers/iio/accel/adxl355_core.c | 162 ++++++++++++++++++++++++++++++-
> > >  2 files changed, 164 insertions(+), 2 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..d499403dc 100644
> > > --- a/drivers/iio/accel/adxl355_core.c
> > > +++ b/drivers/iio/accel/adxl355_core.c
> > > @@ -9,11 +9,16 @@
> > >
> > >  #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 +51,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
> > >
> > > @@ -165,7 +171,15 @@ 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;
> > > +     int irq;
> > > +     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 +200,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);  
> >
> > Why take the lock?  What are you protecting here?  
> 
> Power Control Register is the same register that is used to put the
> device in measurement mode, so it has to be protected.
> 

Regmap has an internal lock that will protect the register / cache etc
against concurrent changes - so we'd normally only expect to see
locking needed if one of the paths does an explicit read / modify /write
perhaps because it needs to base the write on what was read.

> >  
> > > +     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 +277,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 +536,74 @@ 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,  
> >
> > This is fine for now, but I'd like you to think about whether you can ultimately
> > drop this validation and allow other devices to be triggered from this signal.
> > It can be very helpful when doing sensor fusion to grab data at (approximately)
> > the same time from a set of different sensors.  
> 
> I will remove this line in the next patch, I will read about this more.
It's a little bit fiddly to do, so I meant this more as a long term target
rather than something to do in this particular patch!

> 
> >
> > IIRC this device has a fifo though which can complicate that option.
> >  
> > > +};
> > > +
> > > +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);
> > > +
> > > +     /*
> > > +      * data->buffer is used both for triggered buffer support
> > > +      * and read/write_raw(), hence, it has to be zeroed here before usage.
> > > +      */
> > > +     data->buffer.buf[0] = 0;
> > > +
> > > +     /*
> > > +      * The accelaration data is 24 bits and big endian. It has to be saved  
> >
> > Spell check.  acceleration  
> 
> Will change in next version.
> 
> >  
> > > +      * in 32 bits, hence, it is saved in the 2nd byte of the 4 byte buffer.
> > > +      * The buf array is 14 bytes as it includes 3x4=12 bytes for
> > > +      * accelaration data of x, y, and z axis. It also includes 2 bytes for
> > > +      * temperature data.
> > > +      */
> > > +     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 +617,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 +637,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 +645,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");  
> >
> > I would move this check externally so that we never probe if it's
> > not wired up.  It's also easy to tell (no trigger available) so
> > the dev_info is probably unnecessary noise in the log.  
> 
> I will remove this in next version.
> 
> >
> >  
> > > +             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)
> > > +             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 +702,37 @@ int adxl355_core_probe(struct device *dev, struct regmap *regmap,
> > >       data->op_mode = ADXL355_STANDBY;
> > >       mutex_init(&data->lock);
> > >
> > > +     /*
> > > +      * TODO: 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;  
> >
> > If you do this get down near the probe_trigger() call then you should be able to
> > avoid keeping a copy of irq in data.  Just pass it into that function as
> > a parameter.  
> 
> ok, I will change it in next version.
> 
> >  
> > > +
> > >       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) {  
> >
> > This looks to be an unrelated cleanup.  Please move to a precursor patch as
> > it adds noise in this one which is fiddly enough without it ;)  
> 
> ok, I will make a separate patch for this and send a series.
> 
> >  
> > >               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);  
> >  
> 
> 


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-09-05  9:42 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-19 18:11 [PATCH v2] iio: accel: adxl355: Add triggered buffer support Puranjay Mohan
2021-08-30 15:14 ` Jonathan Cameron
2021-08-31 17:09   ` Puranjay Mohan
2021-09-05  9:45     ` Jonathan Cameron

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).