Re: [PATCH v2 20/22] iio: accel: adxl345: use FIFO with watermark IRQ

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 17 Nov 2024 18:26:49 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Enable the watermark feature of the adxl345 sensor. The feature uses a
> FIFO to be configured by the IIO fifo sysfs handles. The sensor
> configures the FIFO to streaming mode for measurements, or bypass for
> configuration. The sensor issues an interrupt on the configured line to
> signal several signals (data available, overrun or watermark reached).
> The setup allows for extension of further features based on the
> interrupt handler and the FIFO setup.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345_core.c | 39 ++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index f7b937fb16..07c336211f 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -870,6 +870,45 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  		if (ret)
>  			return dev_err_probe(dev, ret, "Failed to setup triggered buffer\n");
>  
> +		dev_dbg(dev, "IRQ: allocating data ready trigger\n");
> +		st->trig_dready = devm_iio_trigger_alloc(dev,
> +							 "%s-dev%d-dready",
> +							 indio_dev->name,
> +							 iio_device_id(indio_dev));
> +		if (!st->trig_dready)
> +			return dev_err_probe(dev, -ENOMEM,
> +					     "Failed to setup triggered buffer\n");
> +		dev_dbg(dev, "IRQ: allocating data ready trigger ok\n");
Drop these debug prints from code you want to go upstream. Mostly it is easy
to find out what they provide via other means and they hurt code redability.
> +
> +		st->trig_dready->ops = &adxl34x_trig_dready_ops;
> +		dev_dbg(dev, "IRQ: Setting data ready trigger ops ok\n");
> +
> +		iio_trigger_set_drvdata(st->trig_dready, indio_dev);
> +		dev_dbg(dev, "IRQ: Setting up data ready trigger as driver data ok\n");
> +
> +		ret = devm_iio_trigger_register(dev, st->trig_dready);
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to register dready trigger\n");
> +		dev_dbg(dev, "Registering data ready trigger ok\n");
> +
> +		indio_dev->trig = iio_trigger_get(st->trig_dready);
you want to set it as immutable as this code doesn't currently work with other
triggers or with this removed.  This is subject to just getting rid of the trigger
in general a suggested earlier.
> +
> +		dev_dbg(dev, "Requesting threaded IRQ, indio_dev->name '%s'\n",
> +			indio_dev->name);
> +
> +		ret = devm_request_threaded_irq(dev, st->irq,
> +						iio_trigger_generic_data_rdy_poll,
It's not. It's on a watermark interrupt. That's what strongly implies treating this
as a trigger is a bad idea.
> +						NULL,
> +						IRQF_TRIGGER_RISING | IRQF_ONESHOT,

Direction should be coming from firmware so not set here.

Note there are some older drivers where we do set it in the driver, and we can't
'fix' those because there may be hardware with broken firmware relying on that.
However no new cases of this.

> +						indio_dev->name, st->trig_dready);
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret, "Failed to request IRQ handler\n");
> +		dev_dbg(dev, "Requesting threaded IRQ handler ok\n");
> +
> +		/* Cleanup */
> +		adxl345_empty_fifo(st);
> +
>  	} else { /* Initialization to prepare for FIFO_BYPASS mode (fallback) */
>  
>  		/* The following defaults to 0x00, anyway */