On Tue, 28 Jan 2025 12:00:53 +0000
Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote:
> Split the current set_interrupts() functionality. Separate writing the
> interrupt map from writing the interrupt enable register.
Makes sense.
>
> Move writing the interrupt map into the probe(). The interrupt map will
> setup which event finally will go over the INT line. Thus, all events
> are mapped to this interrupt line now once at the beginning.
>
> On the other side the function set_interrupts() will now be focussed on
> enabling interrupts for event features. Thus it will be renamed to
> write_interrupts() to better distinguish from further usage of get/set
> in the conext of the sensor features.
For this, I'm not yet seeing why we need a function to do this. May
become clear later.
>
> Also, add the missing initial reset of the interrupt enable register.
>
> Signed-off-by: Lothar Rubusch <l.rubusch@xxxxxxxxx>
> ---
> drivers/iio/accel/adxl345_core.c | 43 +++++++++++++++++---------------
> 1 file changed, 23 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 7ee50a0b23ea..b55f6774b1e9 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -190,25 +190,9 @@ static void adxl345_powerdown(void *ptr)
> adxl345_set_measure_en(st, false);
> }
>
> -static int adxl345_set_interrupts(struct adxl345_state *st)
> +static inline int adxl345_write_interrupts(struct adxl345_state *st)
> {
> - int ret;
> - unsigned int int_enable = st->int_map;
> - unsigned int int_map;
> -
> - /*
> - * Any bits set to 0 in the INT map register send their respective
> - * 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);
> -
> - ret = regmap_write(st->regmap, ADXL345_REG_INT_MAP, int_map);
> - if (ret)
> - return ret;
> -
> - return regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, int_enable);
> + return regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, st->int_map);
Not sure the function adds much. Maybe it will later, but my gut
feeling on what is here would be to just do the regamp_write() inline
where this function is currently called.
> }
>
>
> static const struct iio_buffer_setup_ops adxl345_buffer_ops = {
> @@ -602,6 +586,8 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> return -ENODEV;
> st->fifo_delay = fifo_delay_default;
>
> + st->int_map = 0x00; /* reset interrupts */
> +
> indio_dev->name = st->info->name;
> indio_dev->info = &adxl345_info;
> indio_dev->modes = INDIO_DIRECT_MODE;
> @@ -609,6 +595,11 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> indio_dev->num_channels = ARRAY_SIZE(adxl345_channels);
> indio_dev->available_scan_masks = adxl345_scan_masks;
>
> + /* Reset interrupts at start up */
> + ret = adxl345_write_interrupts(st);
> + if (ret)
> + return ret;
> +
> if (setup) {
> /* Perform optional initial bus specific configuration */
> ret = setup(dev, st->regmap);
> @@ -659,6 +650,18 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> }
>
> if (st->intio != ADXL345_INT_NONE) {
> + /*
> + * Any bits set to 0 in the INT map register send their respective
> + * interrupts to the INT1 pin, whereas bits set to 1 send their respective
> + * interrupts to the INT2 pin. The intio shall convert this accordingly.
> + */
> + regval = st->intio ? ADXL345_REG_INT_SOURCE_MSK
> + : ~ADXL345_REG_INT_SOURCE_MSK;
This mask is another slightly odd one as it just means all bits in the register.
Maybe better just using values here? 0x00 vs 0xFF
> +
> + ret = regmap_write(st->regmap, ADXL345_REG_INT_MAP, regval);
> + if (ret)
> + return ret;
> +
> /* FIFO_STREAM mode is going to be activated later */
> ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, &adxl345_buffer_ops);
> if (ret)
