Re: [PATCH v3 06/12] iio: accel: adxl313: prepare interrupt handling

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 23 May 2025 22:35:17 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Evaluate the devicetree property for an optional interrupt line, and
> configure the interrupt mapping accordingly. When no interrupt line
> is defined in the devicetree, keep the FIFO in bypass mode as before.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl313.h      |  8 ++++++++
>  drivers/iio/accel/adxl313_core.c | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/drivers/iio/accel/adxl313.h b/drivers/iio/accel/adxl313.h
> index 9bf2facdbf87..ab109d1c359e 100644
> --- a/drivers/iio/accel/adxl313.h
> +++ b/drivers/iio/accel/adxl313.h
> @@ -21,7 +21,9 @@
>  #define ADXL313_REG_ACT_INACT_CTL	0x27
>  #define ADXL313_REG_BW_RATE		0x2C
>  #define ADXL313_REG_POWER_CTL		0x2D
> +#define ADXL313_REG_INT_ENABLE		0x2E
>  #define ADXL313_REG_INT_MAP		0x2F
> +#define ADXL313_REG_INT_SOURCE		0x30
>  #define ADXL313_REG_DATA_FORMAT		0x31
>  #define ADXL313_REG_DATA_AXIS(index)	(0x32 + ((index) * 2))
>  #define ADXL313_REG_FIFO_CTL		0x38
> @@ -45,6 +47,11 @@
>  #define ADXL313_SPI_3WIRE		BIT(6)
>  #define ADXL313_I2C_DISABLE		BIT(6)
>  
> +#define ADXL313_REG_FIFO_CTL_MODE_MSK		GENMASK(7, 6)
> +
> +#define ADXL313_FIFO_BYPASS			0
> +#define ADXL313_FIFO_STREAM			2
> +
>  extern const struct regmap_access_table adxl312_readable_regs_table;
>  extern const struct regmap_access_table adxl313_readable_regs_table;
>  extern const struct regmap_access_table adxl314_readable_regs_table;
> @@ -65,6 +72,7 @@ struct adxl313_data {
>  	struct regmap	*regmap;
>  	const struct adxl313_chip_info *chip_info;
>  	struct mutex	lock; /* lock to protect transf_buf */
> +	int irq;

Curious.  Why do we need to keep this around?  Normally we only need
the actual interrupt number in the probe() function.

>  	__le16		transf_buf __aligned(IIO_DMA_MINALIGN);
>  };
>  
> diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
> index 6170c9daa30f..9db318a03eea 100644
> --- a/drivers/iio/accel/adxl313_core.c
> +++ b/drivers/iio/accel/adxl313_core.c
> @@ -8,11 +8,17 @@
>   */
>  
>  #include <linux/bitfield.h>
> +#include <linux/interrupt.h>
>  #include <linux/module.h>
> +#include <linux/property.h>
>  #include <linux/regmap.h>
>  
>  #include "adxl313.h"
>  
> +#define ADXL313_INT_NONE			U8_MAX
> +#define ADXL313_INT1				1
> +#define ADXL313_INT2				2
> +
>  static const struct regmap_range adxl312_readable_reg_range[] = {
>  	regmap_reg_range(ADXL313_REG_DEVID0, ADXL313_REG_DEVID0),
>  	regmap_reg_range(ADXL313_REG_OFS_AXIS(0), ADXL313_REG_OFS_AXIS(2)),
> @@ -436,6 +442,7 @@ int adxl313_core_probe(struct device *dev,
>  {
>  	struct adxl313_data *data;
>  	struct iio_dev *indio_dev;
> +	u8 int_line;
>  	int ret;
>  
>  	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> @@ -461,6 +468,30 @@ int adxl313_core_probe(struct device *dev,
>  		return ret;
>  	}
>  
> +	int_line = ADXL313_INT1;
> +	data->irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT1");
> +	if (data->irq < 0) {
> +		int_line = ADXL313_INT2;
> +		data->irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT2");
> +		if (data->irq < 0)
> +			int_line = ADXL313_INT_NONE;
> +	}
> +
> +	if (int_line == ADXL313_INT1 || int_line == ADXL313_INT2) {

Why not int_line != ADXL313_INT_NONE ?
Or flip the logic so that you do that case first.

> +		/* FIFO_STREAM mode */
> +		ret = regmap_assign_bits(data->regmap, ADXL313_REG_INT_MAP,

A number of bits in this register are give in datasheet as always 0.
As a general rule writing bits documented like that is unwise. Sometimes
they have undocumented side effects.

> +					 0xff, int_line == ADXL313_INT2);
> +		if (ret)
> +			return ret;
> +	} else {
> +		/* FIFO_BYPASSED mode */

I'd like the comment to say why you bypass the fifo in this case.
In theory nothing stops us polling for the watermark. I don't mind
the driver not doing that because all reasonable boards will wire
the interrupt if they want fifo support, but we should talk a little
more about why here.

> +		ret = regmap_write(data->regmap, ADXL313_REG_FIFO_CTL,
> +				   FIELD_PREP(ADXL313_REG_FIFO_CTL_MODE_MSK,
> +					      ADXL313_FIFO_BYPASS));
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return devm_iio_device_register(dev, indio_dev);
>  }
>  EXPORT_SYMBOL_NS_GPL(adxl313_core_probe, IIO_ADXL313);