On Fri, 23 May 2025 22:35:17 +0000
Lothar Rubusch <l.rubusch@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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);
