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

[Jonathan Cameron]

On Wed,  4 Aug 2021 19:33:09 +0530
Puranjay Mohan <puranjay12@xxxxxxxxx> 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@xxxxxxxxx>

Hi Puranjay,

This device has a hardware fifo, so we need to be careful how we add
trigger support such that we can enable that smoothly.

There are a few things in here that suggest this isn't tested as much
as I'd expect for a submission.  If you are posting early, but need
to do more testing, please state that clearly.

Jonathan

> ---
>  drivers/iio/accel/Kconfig        |   4 ++
>  drivers/iio/accel/adxl355.h      |   2 +-
>  drivers/iio/accel/adxl355_core.c | 102 ++++++++++++++++++++++++++++++-
>  drivers/iio/accel/adxl355_i2c.c  |   3 +-
>  drivers/iio/accel/adxl355_spi.c  |   2 +-
>  5 files changed, 108 insertions(+), 5 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.h b/drivers/iio/accel/adxl355.h
> index 322b0abb8..f0a376e6d 100644
> --- a/drivers/iio/accel/adxl355.h
> +++ b/drivers/iio/accel/adxl355.h
> @@ -15,5 +15,5 @@ extern const struct regmap_access_table adxl355_readable_regs_tbl;
>  extern const struct regmap_access_table adxl355_writeable_regs_tbl;
>  
>  int adxl355_core_probe(struct device *dev, struct regmap *regmap,
> -		       const char *name);
> +		       const char *name, int irq);
>  #endif /* _ADXL355_H_ */
> diff --git a/drivers/iio/accel/adxl355_core.c b/drivers/iio/accel/adxl355_core.c
> index 47fbb31bc..f9dc2a530 100644
> --- a/drivers/iio/accel/adxl355_core.c
> +++ b/drivers/iio/accel/adxl355_core.c
> @@ -9,6 +9,10 @@
>  
>  #include <asm/unaligned.h>
>  #include <linux/bitfield.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/iio.h>
>  #include <linux/limits.h>
>  #include <linux/math64.h>
> @@ -172,6 +176,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 */
> @@ -181,6 +186,12 @@ struct adxl355_data {
>  	int calibbias[3];
>  	int adxl355_hpf_3db_table[7][2];
>  	u8 transf_buf[3] ____cacheline_aligned;
> +	struct iio_trigger      *dready_trig;
> +	/* Ensure correct alignment of timestamp when present */
> +	struct {
> +		__be32 channels[3];
> +		s64 ts;
> +	} buffer ____cacheline_aligned;

Interesting quirk here is that we need to ensure that any of the
buffers used for DMA don't share a cacheline with other data, but we don't
need to ensure they don't share with each other.  As such, this can
share fine with transf_buf, but definitely not with dready_trig (which
can't share with transf_buf like it is right now).

So reorder to put the various DMA safe buffers at the end, and ensure
the first one is cacheline aligned.

Even better, given you can only safely access one at a time anyway
(and I think you always hold the lock so that is fine), put them in a union
to make it explicit that only one is used at a time.

struct iio_trigger *dready_trig;
union {
	u8 trans_buf[3];
	struct {
		__be32 channels[3];
		s64 ts;
	} buffer;
} ___cacheline_algned;
or something like that.



>  };
>  
>  static int adxl355_set_op_mode(struct adxl355_data *data,
> @@ -499,12 +510,46 @@ 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 = {
> +	.validate_device = &iio_trigger_validate_own_device,

No means of turning it on?

> +};
> +
> +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.channels,
> +			       9);

That doesn't look right.  you are reading 9 bytes into an array of 3 __be32 channels.
You should check the data is turning up where it should.


> +	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 +563,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,17 +583,56 @@ 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,
>  			.storagebits = 16,
>  			.endianness = IIO_BE,
>  		},
> -	}
> +	},

If you do end up respinning patch 2, add the comma in that patch as it'll reduced
noise here.

> +	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)
> +		       const char *name, int irq)
>  {
>  	struct adxl355_data *data;
>  	struct iio_dev *indio_dev;
> @@ -560,6 +645,7 @@ int adxl355_core_probe(struct device *dev, struct regmap *regmap,
>  	data = iio_priv(indio_dev);
>  	data->regmap = regmap;
>  	data->dev = dev;
> +	data->irq = irq;
>  	data->op_mode = ADXL355_STANDBY;
>  	mutex_init(&data->lock);
>  
> @@ -568,6 +654,7 @@ int adxl355_core_probe(struct device *dev, struct regmap *regmap,
>  	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) {
> @@ -575,6 +662,17 @@ int adxl355_core_probe(struct device *dev, struct regmap *regmap,
>  		return ret;
>  	}
>  
> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> +					      &iio_pollfunc_store_time,
> +					      &adxl355_trigger_handler, NULL);
> +	if (ret < 0)

if (ret) preferred, because it is more consistent with returning devm_iio_device_register()
below (where we are assuming negative or 0 only)

> +		return dev_err_probe(dev, ret,
> +				     "iio triggered buffer setup failed\n");
> +
> +	ret = adxl355_probe_trigger(indio_dev);
> +	if (ret < 0)
> +		return ret;
> +
>  	return devm_iio_device_register(dev, indio_dev);
>  }
>  EXPORT_SYMBOL_GPL(adxl355_core_probe);
> diff --git a/drivers/iio/accel/adxl355_i2c.c b/drivers/iio/accel/adxl355_i2c.c
> index e3070ee81..c18521819 100644
> --- a/drivers/iio/accel/adxl355_i2c.c
> +++ b/drivers/iio/accel/adxl355_i2c.c
> @@ -31,7 +31,8 @@ static int adxl355_i2c_probe(struct i2c_client *client)
>  		return PTR_ERR(regmap);
>  	}
>  
> -	return adxl355_core_probe(&client->dev, regmap, client->name);
> +	return adxl355_core_probe(&client->dev, regmap, client->name,
> +				  client->irq);

We can't just use the first interrupt (which this corresponds to) because
the device supports 3 and who knows if this is the right one?

Now to solve this, we run into a 'fun' issue.  We want to use the generic
firmware property interfaces (include/linux/property.h) but that doesn't
include a fwnode_irq_get_by_name()

We've had some recent discussion about this:
https://lore.kernel.org/linux-iio/20210724190614.3593dcd1@jic23-huawei/

If it is something you would like to take on, that would be great.
If not, for now use the of_irq_get_byname() method but add a comment
saying it would good to move to a generic version.

>  }
>  
>  static const struct i2c_device_id adxl355_i2c_id[] = {
> diff --git a/drivers/iio/accel/adxl355_spi.c b/drivers/iio/accel/adxl355_spi.c
> index a16bd1407..f9ba153f6 100644
> --- a/drivers/iio/accel/adxl355_spi.c
> +++ b/drivers/iio/accel/adxl355_spi.c
> @@ -34,7 +34,7 @@ static int adxl355_spi_probe(struct spi_device *spi)
>  		return PTR_ERR(regmap);
>  	}
>  
> -	return adxl355_core_probe(&spi->dev, regmap, id->name);
> +	return adxl355_core_probe(&spi->dev, regmap, id->name, spi->irq);
>  }
>  
>  static const struct spi_device_id adxl355_spi_id[] = {