Re: [PATCH v4 3/4] iio: accel: adxl372: factor out buffer and trigger setup

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 21 Mar 2026 12:04:58 +0200
Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:

> Extract the triggered buffer, trigger allocation, and IRQ request
> logic from adxl372_probe() into a dedicated adxl372_buffer_setup()
> helper. This reduces the probe function complexity and prepares for
> conditionally disabling buffer support on device variants with
> known FIFO issues.
> 
> No functional change intended.
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
A couple of comments, but all about stuff you've simply moved, so not
asking for changes (though if you respin for other reasons, good
to tidy up the line wrap).

Jonathan

> ---
> Changes in v4:
>  - Use 'if (ret)' instead of 'if (ret < 0)' for
>    devm_iio_trigger_register() and devm_iio_triggered_buffer_setup_ext()
>    return checks in adxl372_buffer_setup().
> 
>  drivers/iio/accel/adxl372.c | 94 ++++++++++++++++++++-----------------
>  1 file changed, 51 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl372.c b/drivers/iio/accel/adxl372.c
> index b34b91cae753..6fd4fe0ec1d9 100644
> --- a/drivers/iio/accel/adxl372.c
> +++ b/drivers/iio/accel/adxl372.c
> @@ -1193,6 +1193,56 @@ bool adxl372_readable_noinc_reg(struct device *dev, unsigned int reg)
>  }
>  EXPORT_SYMBOL_NS_GPL(adxl372_readable_noinc_reg, "IIO_ADXL372");
>  
> +static int adxl372_buffer_setup(struct iio_dev *indio_dev)
> +{
> +	struct adxl372_state *st = iio_priv(indio_dev);
> +	struct device *dev = st->dev;
> +	int ret;
> +
> +	ret = devm_iio_triggered_buffer_setup_ext(dev,
> +						  indio_dev, NULL,
Not sure why this wrapping (was in original code too)
I might move these two up a line whilst applying.

> +						  adxl372_trigger_handler,
> +						  IIO_BUFFER_DIRECTION_IN,
> +						  &adxl372_buffer_ops,
> +						  adxl372_fifo_attributes);
> +	if (ret)
> +		return ret;
> +
> +	if (!st->irq)
> +		return 0;
> +
> +	st->dready_trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
> +						 indio_dev->name,
> +						 iio_device_id(indio_dev));
> +	if (!st->dready_trig)
> +		return -ENOMEM;
> +
> +	st->peak_datardy_trig = devm_iio_trigger_alloc(dev, "%s-dev%d-peak",
> +						       indio_dev->name,
> +						       iio_device_id(indio_dev));
> +	if (!st->peak_datardy_trig)
> +		return -ENOMEM;
> +
> +	st->dready_trig->ops = &adxl372_trigger_ops;
> +	st->peak_datardy_trig->ops = &adxl372_peak_data_trigger_ops;

In original code so not something to fix in this patch, but odd mix
of dready and datardy naming. I think they are both related to data
being ready.  Either form is fine, but having both used in adjacent
lines of code not so much!  If you have time to make that consistent
in a follow up series that would be good.

> +	iio_trigger_set_drvdata(st->dready_trig, indio_dev);
> +	iio_trigger_set_drvdata(st->peak_datardy_trig, indio_dev);
> +	ret = devm_iio_trigger_register(dev, st->dready_trig);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_iio_trigger_register(dev, st->peak_datardy_trig);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->trig = iio_trigger_get(st->dready_trig);
> +
> +	return devm_request_irq(dev, st->irq,
> +				iio_trigger_generic_data_rdy_poll,
> +				IRQF_TRIGGER_RISING | IRQF_NO_THREAD,
> +				indio_dev->name, st->dready_trig);
> +}
> +
>  int adxl372_probe(struct device *dev, struct regmap *regmap,
>  		  int irq, const struct adxl372_chip_info *chip_info)
>  {
> @@ -1227,52 +1277,10 @@ int adxl372_probe(struct device *dev, struct regmap *regmap,
>  		return ret;
>  	}
>  
> -	ret = devm_iio_triggered_buffer_setup_ext(dev,
> -						  indio_dev, NULL,
> -						  adxl372_trigger_handler,
> -						  IIO_BUFFER_DIRECTION_IN,
> -						  &adxl372_buffer_ops,
> -						  adxl372_fifo_attributes);
> +	ret = adxl372_buffer_setup(indio_dev);
>  	if (ret < 0)
>  		return ret;
>  
> -	if (st->irq) {
> -		st->dready_trig = devm_iio_trigger_alloc(dev,
> -							 "%s-dev%d",
> -							 indio_dev->name,
> -							 iio_device_id(indio_dev));
> -		if (st->dready_trig == NULL)
> -			return -ENOMEM;
> -
> -		st->peak_datardy_trig = devm_iio_trigger_alloc(dev,
> -							       "%s-dev%d-peak",
> -							       indio_dev->name,
> -							       iio_device_id(indio_dev));
> -		if (!st->peak_datardy_trig)
> -			return -ENOMEM;
> -
> -		st->dready_trig->ops = &adxl372_trigger_ops;
> -		st->peak_datardy_trig->ops = &adxl372_peak_data_trigger_ops;
> -		iio_trigger_set_drvdata(st->dready_trig, indio_dev);
> -		iio_trigger_set_drvdata(st->peak_datardy_trig, indio_dev);
> -		ret = devm_iio_trigger_register(dev, st->dready_trig);
> -		if (ret < 0)
> -			return ret;
> -
> -		ret = devm_iio_trigger_register(dev, st->peak_datardy_trig);
> -		if (ret < 0)
> -			return ret;
> -
> -		indio_dev->trig = iio_trigger_get(st->dready_trig);
> -
> -		ret = devm_request_irq(dev, st->irq,
> -				       iio_trigger_generic_data_rdy_poll,
> -				       IRQF_TRIGGER_RISING | IRQF_NO_THREAD,
> -				       indio_dev->name, st->dready_trig);
> -		if (ret < 0)
> -			return ret;
> -	}
> -
>  	return devm_iio_device_register(dev, indio_dev);
>  }
>  EXPORT_SYMBOL_NS_GPL(adxl372_probe, "IIO_ADXL372");