[PATCH v1 05/12] iio: accel: adxl345: improve access to the interrupt enable register

Lothar Rubusch posted 12 patches 10 months, 3 weeks ago
There is a newer version of this series
[PATCH v1 05/12] iio: accel: adxl345: improve access to the interrupt enable register
Posted by Lothar Rubusch 10 months, 3 weeks ago
Split the current set_interrupts() functionality. Separate writing the
interrupt map from writing the interrupt enable register.

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.

Also, add the missing initial reset of the interrupt enable register.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 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);
 }
 
 static int adxl345_read_raw(struct iio_dev *indio_dev,
@@ -464,7 +448,7 @@ static int adxl345_buffer_postenable(struct iio_dev *indio_dev)
 	struct adxl345_state *st = iio_priv(indio_dev);
 	int ret;
 
-	ret = adxl345_set_interrupts(st);
+	ret = adxl345_write_interrupts(st);
 	if (ret < 0)
 		return ret;
 
@@ -483,7 +467,7 @@ static int adxl345_buffer_predisable(struct iio_dev *indio_dev)
 		return ret;
 
 	st->int_map = 0x00;
-	return adxl345_set_interrupts(st);
+	return adxl345_write_interrupts(st);
 }
 
 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;
+
+		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)
-- 
2.39.5
Re: [PATCH v1 05/12] iio: accel: adxl345: improve access to the interrupt enable register
Posted by Jonathan Cameron 10 months, 3 weeks ago
On Tue, 28 Jan 2025 12:00:53 +0000
Lothar Rubusch <l.rubusch@gmail.com> 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@gmail.com>
> ---
>  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)