Re: [PATCH v2 03/14] iio: accel: adxl345: reorganize irq handler

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 10 Feb 2025 11:01:08 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Reorganize the IRQ handler. Move the overrun handling to the bottom.
> Overrun leads to reset the interrupt register. This also happens at
> evaluation of a particular interrupt event. First evaluate an event
> if possible, then fall back to overrun handling. Additionally simplify
> fetching the interrupt status function.
> 
> Both is in preparation to build interrupt handling up for the handling
> of different detected events, implemented in follow up patches.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
Hi Lothar,

Changes are fine, but I think we should reevaluate if the little
helper to get_status is actually useful any more.  Probably best
to just drop it as part of this patch.

Jonathan

> ---
>  drivers/iio/accel/adxl345.h      |  1 -
>  drivers/iio/accel/adxl345_core.c | 26 +++++++++-----------------
>  2 files changed, 9 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> index 517e494ba555..bc6d634bd85c 100644
> --- a/drivers/iio/accel/adxl345.h
> +++ b/drivers/iio/accel/adxl345.h
> @@ -43,7 +43,6 @@
>  #define ADXL345_REG_INT_ENABLE		0x2E
>  #define ADXL345_REG_INT_MAP		0x2F
>  #define ADXL345_REG_INT_SOURCE		0x30
> -#define ADXL345_REG_INT_SOURCE_MSK	0xFF
>  #define ADXL345_REG_DATA_FORMAT		0x31
>  #define ADXL345_REG_XYZ_BASE		0x32
>  #define ADXL345_REG_DATA_AXIS(index)				\
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 468d562de227..2928c1c0760f 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -107,8 +107,7 @@ static int adxl345_set_interrupts(struct adxl345_state *st)
>  	 * interrupts to the INT1 pin, whereas bits set to 1 send their respective
>  	 * interrupts to the INT2 pin. The intio shall convert this accordingly.
>  	 */
> -	int_map = FIELD_GET(ADXL345_REG_INT_SOURCE_MSK,
> -			    st->intio ? st->int_map : ~st->int_map);
> +	int_map = st->intio ? st->int_map : ~st->int_map;
>  
>  	ret = regmap_write(st->regmap, ADXL345_REG_INT_MAP, int_map);
>  	if (ret)
> @@ -404,16 +403,9 @@ static const struct iio_buffer_setup_ops adxl345_buffer_ops = {
>  	.predisable = adxl345_buffer_predisable,
>  };
>  
> -static int adxl345_get_status(struct adxl345_state *st)
> +static int adxl345_get_status(struct adxl345_state *st, unsigned int *int_stat)
>  {
> -	int ret;
> -	unsigned int regval;
> -
> -	ret = regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, &regval);
> -	if (ret < 0)
> -		return ret;
> -
> -	return FIELD_GET(ADXL345_REG_INT_SOURCE_MSK, regval);
> +	return regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, int_stat);

After this cleanup/refactoring this function no longer has a reason to exist.
Just call regmap_read() directly inline.

>  }
>  
>  static int adxl345_fifo_push(struct iio_dev *indio_dev,
> @@ -449,14 +441,10 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
>  	int int_stat;
>  	int samples;
>  
> -	int_stat = adxl345_get_status(st);
> -	if (int_stat <= 0)
> +	if (adxl345_get_status(st, &int_stat))

	if (regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, &int_stat))

>  		return IRQ_NONE;
>  
> -	if (int_stat & ADXL345_INT_OVERRUN)
> -		goto err;
> -
> -	if (int_stat & ADXL345_INT_WATERMARK) {
> +	if (FIELD_GET(ADXL345_INT_WATERMARK, int_stat)) {
>  		samples = adxl345_get_samples(st);
>  		if (samples < 0)
>  			goto err;
> @@ -464,6 +452,10 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
>  		if (adxl345_fifo_push(indio_dev, samples) < 0)
>  			goto err;
>  	}
> +
> +	if (FIELD_GET(ADXL345_INT_OVERRUN, int_stat))
> +		goto err;
> +
>  	return IRQ_HANDLED;
>  
>  err: