[PATCH] iio: accel: bma180: Fix dataready interrupt to use INT2 pin

[PATCH] iio: accel: bma180: Fix dataready interrupt to use INT2 pin

[Abhishek Tamboli]

Update the interrupt configuration to use the INT2 pin for
the dataready interrupt.

Address the FIXME: support using the INT2 pin.

Signed-off-by: Abhishek Tamboli <abhishektamboli9@gmail.com>
---
 drivers/iio/accel/bma180.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/accel/bma180.c b/drivers/iio/accel/bma180.c
index 6581772cb0c4..d44409afffbf 100644
--- a/drivers/iio/accel/bma180.c
+++ b/drivers/iio/accel/bma180.c
@@ -126,7 +126,7 @@ struct bma180_part_info {
 #define BMA250_SUSPEND_MASK	BIT(7) /* chip will sleep */
 #define BMA250_LOWPOWER_MASK	BIT(6)
 #define BMA250_DATA_INTEN_MASK	BIT(4)
-#define BMA250_INT1_DATA_MASK	BIT(0)
+#define BMA250_INT2_DATA_MASK	BIT(7)
 #define BMA250_INT_RESET_MASK	BIT(7) /* Reset pending interrupts */
 
 struct bma180_data {
@@ -425,10 +425,9 @@ static int bma250_chip_config(struct bma180_data *data)
 	if (ret)
 		goto err;
 	/*
-	 * This enables dataready interrupt on the INT1 pin
-	 * FIXME: support using the INT2 pin
+	 * This enables dataready interrupt on the INT2 pin
 	 */
-	ret = bma180_set_bits(data, BMA250_INT_MAP_REG, BMA250_INT1_DATA_MASK, 1);
+	ret = bma180_set_bits(data, BMA250_INT_MAP_REG, BMA250_INT2_DATA_MASK, 1);
 	if (ret)
 		goto err;
 
-- 
2.34.1

Re: [PATCH] iio: accel: bma180: Fix dataready interrupt to use INT2 pin

[Jonathan Cameron]

On Mon,  5 Aug 2024 23:02:37 +0530
Abhishek Tamboli <abhishektamboli9@gmail.com> wrote:

> Update the interrupt configuration to use the INT2 pin for
> the dataready interrupt.
> 
> Address the FIXME: support using the INT2 pin.
> 
> Signed-off-by: Abhishek Tamboli <abhishektamboli9@gmail.com>
Hi Abhishek,

That's not the intent of that FIXME.  It's pointing out that the driver
does not currently support the wiring configuration where only INT2 is
connected.  The change you have here breaks the configuration where
only INT1 is wired or both are wired (as it will register the
interrupt handler on the int1 pin, not int2).

I'm guessing that is the situation you have is int2 only?

To handle this you need to add the firmware queries to identify the
interrupt by name. See for example driver/iio/imu/bmi323_core.c
bmi323_trigger_probe()

	fwnode = dev_fwnode(data->dev);
	if (!fwnode)
		return -ENODEV;

	irq = fwnode_irq_get_byname(fwnode, "INT1");
	if (irq > 0) {
		irq_pin = BMI323_IRQ_INT1;
	} else {
		irq = fwnode_irq_get_byname(fwnode, "INT2");
		if (irq < 0)
			return 0;

		irq_pin = BMI323_IRQ_INT2;
	}

This defaults to INT1 if available, but will find INT2
if interrupt-names is provided in DT and specifies that only INT2
is present.

Jonathan

> ---
>  drivers/iio/accel/bma180.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/accel/bma180.c b/drivers/iio/accel/bma180.c
> index 6581772cb0c4..d44409afffbf 100644
> --- a/drivers/iio/accel/bma180.c
> +++ b/drivers/iio/accel/bma180.c
> @@ -126,7 +126,7 @@ struct bma180_part_info {
>  #define BMA250_SUSPEND_MASK	BIT(7) /* chip will sleep */
>  #define BMA250_LOWPOWER_MASK	BIT(6)
>  #define BMA250_DATA_INTEN_MASK	BIT(4)
> -#define BMA250_INT1_DATA_MASK	BIT(0)
> +#define BMA250_INT2_DATA_MASK	BIT(7)
>  #define BMA250_INT_RESET_MASK	BIT(7) /* Reset pending interrupts */
>  
>  struct bma180_data {
> @@ -425,10 +425,9 @@ static int bma250_chip_config(struct bma180_data *data)
>  	if (ret)
>  		goto err;
>  	/*
> -	 * This enables dataready interrupt on the INT1 pin
> -	 * FIXME: support using the INT2 pin
> +	 * This enables dataready interrupt on the INT2 pin
>  	 */
> -	ret = bma180_set_bits(data, BMA250_INT_MAP_REG, BMA250_INT1_DATA_MASK, 1);
> +	ret = bma180_set_bits(data, BMA250_INT_MAP_REG, BMA250_INT2_DATA_MASK, 1);
>  	if (ret)
>  		goto err;
>  

Re: [PATCH] iio: accel: bma180: Fix dataready interrupt to use INT2 pin

[Abhishek Tamboli]

On Tue, Aug 06, 2024 at 05:32:28PM +0100, Jonathan Cameron wrote:
> On Mon,  5 Aug 2024 23:02:37 +0530
> Abhishek Tamboli <abhishektamboli9@gmail.com> wrote:
> 
> > Update the interrupt configuration to use the INT2 pin for
> > the dataready interrupt.
> > 
> > Address the FIXME: support using the INT2 pin.
> > 
> > Signed-off-by: Abhishek Tamboli <abhishektamboli9@gmail.com>
> Hi Abhishek,
> 
> That's not the intent of that FIXME.  It's pointing out that the driver
> does not currently support the wiring configuration where only INT2 is
> connected.  The change you have here breaks the configuration where
> only INT1 is wired or both are wired (as it will register the
> interrupt handler on the int1 pin, not int2).
> 
> I'm guessing that is the situation you have is int2 only?
> 
> To handle this you need to add the firmware queries to identify the
> interrupt by name. See for example driver/iio/imu/bmi323_core.c
> bmi323_trigger_probe()
> 
> 	fwnode = dev_fwnode(data->dev);
> 	if (!fwnode)
> 		return -ENODEV;
> 
> 	irq = fwnode_irq_get_byname(fwnode, "INT1");
> 	if (irq > 0) {
> 		irq_pin = BMI323_IRQ_INT1;
> 	} else {
> 		irq = fwnode_irq_get_byname(fwnode, "INT2");
> 		if (irq < 0)
> 			return 0;
> 
> 		irq_pin = BMI323_IRQ_INT2;
> 	}
> 
> This defaults to INT1 if available, but will find INT2
> if interrupt-names is provided in DT and specifies that only INT2
> is present.
Thank you for the guidance. I see that I misinterpreted the intent of 'FIXME' comment.
I'll work on implementing the logic to query the firmware for the correct interrupt
configuration, as you suggested. I'll refer to the example in bmi323_core.c and update 
the patch accordingly.

Thanks
Abhishek Tamboli
> 
> > ---
> >  drivers/iio/accel/bma180.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/iio/accel/bma180.c b/drivers/iio/accel/bma180.c
> > index 6581772cb0c4..d44409afffbf 100644
> > --- a/drivers/iio/accel/bma180.c
> > +++ b/drivers/iio/accel/bma180.c
> > @@ -126,7 +126,7 @@ struct bma180_part_info {
> >  #define BMA250_SUSPEND_MASK	BIT(7) /* chip will sleep */
> >  #define BMA250_LOWPOWER_MASK	BIT(6)
> >  #define BMA250_DATA_INTEN_MASK	BIT(4)
> > -#define BMA250_INT1_DATA_MASK	BIT(0)
> > +#define BMA250_INT2_DATA_MASK	BIT(7)
> >  #define BMA250_INT_RESET_MASK	BIT(7) /* Reset pending interrupts */
> >  
> >  struct bma180_data {
> > @@ -425,10 +425,9 @@ static int bma250_chip_config(struct bma180_data *data)
> >  	if (ret)
> >  		goto err;
> >  	/*
> > -	 * This enables dataready interrupt on the INT1 pin
> > -	 * FIXME: support using the INT2 pin
> > +	 * This enables dataready interrupt on the INT2 pin
> >  	 */
> > -	ret = bma180_set_bits(data, BMA250_INT_MAP_REG, BMA250_INT1_DATA_MASK, 1);
> > +	ret = bma180_set_bits(data, BMA250_INT_MAP_REG, BMA250_INT2_DATA_MASK, 1);
> >  	if (ret)
> >  		goto err;
> >  
>