[PATCH] regmap: irq: fix ack-invert

[PATCH] regmap: irq: fix ack-invert

[Tim Harvey]

When acking irqs we need to take into account the ack-invert case. Without
this chips that require 0's to ACK interrupts will never clear the interrupt.

I am working on an mfd driver that will use ack-invert and discovered
this issue. The only user of ack_invert currently appears to be the
motorola-cpcap driver. I'm not clear why that driver doesn't appear affected
so I'm cc'ing those involved with that driver for review and testing.

Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: Tony Lindgren <tony@atomide.com>
Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 drivers/base/regmap/regmap-irq.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index 429ca8e..abfed41 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -355,16 +355,25 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 	 * doing a write per register.
 	 */
 	for (i = 0; i < data->chip->num_regs; i++) {
-		data->status_buf[i] &= ~data->mask_buf[i];
-
-		if (data->status_buf[i] && (chip->ack_base || chip->use_ack)) {
+		if ( (data->status_buf[i] & ~data->mask_buf[i]) &&
+		     (chip->ack_base || chip->use_ack) ) {
 			reg = chip->ack_base +
 				(i * map->reg_stride * data->irq_reg_stride);
-			ret = regmap_write(map, reg, data->status_buf[i]);
+			/* some chips ack by write 0 */
+			if (chip->ack_invert)
+				ret = regmap_write(map, reg,
+						   ~data->status_buf[i] &
+						   ~data->mask_buf[i]);
+
+			else
+				ret = regmap_write(map, reg,
+						   data->status_buf[i] &
+						   ~data->mask_buf[i]);
 			if (ret != 0)
 				dev_err(map->dev, "Failed to ack 0x%x: %d\n",
 					reg, ret);
 		}
+		data->status_buf[i] &= ~data->mask_buf[i];
 	}
 
 	for (i = 0; i < chip->num_irqs; i++) {
-- 
2.7.4

Re: [PATCH] regmap: irq: fix ack-invert

[Tony Lindgren]

* Tim Harvey <tharvey@gateworks.com> [180227 16:07]:
> When acking irqs we need to take into account the ack-invert case. Without
> this chips that require 0's to ACK interrupts will never clear the interrupt.
> 
> I am working on an mfd driver that will use ack-invert and discovered
> this issue. The only user of ack_invert currently appears to be the
> motorola-cpcap driver. I'm not clear why that driver doesn't appear affected
> so I'm cc'ing those involved with that driver for review and testing.

I gave this a quick try and it fails with cpcap. So yeah, you're right,
it seems we still have the cpcap config wrong.

Things do work with the following patch and your patch for cpcap. So
they should both be applied together as a single patch.

Care to fold in the following change and then repost your patch?

Otherwise we might end up breaking things easily for booting or
bisect or stable. Or else the patch below needs to be applied first
to avoid breaking things.

Regards,

Tony

8< -------
--- a/drivers/mfd/motorola-cpcap.c
+++ b/drivers/mfd/motorola-cpcap.c
@@ -100,7 +100,6 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
 		.ack_base = CPCAP_REG_MI1,
 		.mask_base = CPCAP_REG_MIM1,
 		.use_ack = true,
-		.ack_invert = true,
 	},
 	{
 		.name = "cpcap-m2",
@@ -109,7 +108,6 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
 		.ack_base = CPCAP_REG_MI2,
 		.mask_base = CPCAP_REG_MIM2,
 		.use_ack = true,
-		.ack_invert = true,
 	},
 	{
 		.name = "cpcap1-4",
@@ -118,7 +116,6 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
 		.ack_base = CPCAP_REG_INT1,
 		.mask_base = CPCAP_REG_INTM1,
 		.use_ack = true,
-		.ack_invert = true,
 	},
 };
 

Re: [PATCH] regmap: irq: fix ack-invert

[Tim Harvey]

On Tue, Feb 27, 2018 at 10:39 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Tim Harvey <tharvey@gateworks.com> [180227 16:07]:
>> When acking irqs we need to take into account the ack-invert case. Without
>> this chips that require 0's to ACK interrupts will never clear the interrupt.
>>
>> I am working on an mfd driver that will use ack-invert and discovered
>> this issue. The only user of ack_invert currently appears to be the
>> motorola-cpcap driver. I'm not clear why that driver doesn't appear affected
>> so I'm cc'ing those involved with that driver for review and testing.
>
> I gave this a quick try and it fails with cpcap. So yeah, you're right,
> it seems we still have the cpcap config wrong.
>

Tony,

So you would agree with my findings/patch right? I certainly don't
want to break regmap-irq in general :)

Adding Guo Zeng and Barry Song to the thread as they were the authors
of the ack_invert feature (a650fdd9427f1f5236f83d2d8137bea9b452fa53)
and I'm not clear what happened to the chip they were needing it for.

> Things do work with the following patch and your patch for cpcap. So
> they should both be applied together as a single patch.
>
> Care to fold in the following change and then repost your patch?
>
> Otherwise we might end up breaking things easily for booting or
> bisect or stable. Or else the patch below needs to be applied first
> to avoid breaking things.
>

So cpcap needs to write 1's to clear irq's not 0's right?

Yes, I can certainly roll in the fix for cpcap if everyone agrees
that's the right move.

I'll wait for some feedback from Mark Brown as well.

Regards,

Tim

Re: [PATCH] regmap: irq: fix ack-invert

[Tony Lindgren]

* Tim Harvey <tharvey@gateworks.com> [180228 21:18]:
> On Tue, Feb 27, 2018 at 10:39 AM, Tony Lindgren <tony@atomide.com> wrote:
> > * Tim Harvey <tharvey@gateworks.com> [180227 16:07]:
> >> When acking irqs we need to take into account the ack-invert case. Without
> >> this chips that require 0's to ACK interrupts will never clear the interrupt.
> >>
> >> I am working on an mfd driver that will use ack-invert and discovered
> >> this issue. The only user of ack_invert currently appears to be the
> >> motorola-cpcap driver. I'm not clear why that driver doesn't appear affected
> >> so I'm cc'ing those involved with that driver for review and testing.
> >
> > I gave this a quick try and it fails with cpcap. So yeah, you're right,
> > it seems we still have the cpcap config wrong.
> >
> 
> Tony,
> 
> So you would agree with my findings/patch right? I certainly don't
> want to break regmap-irq in general :)

Yes I agree that it breaks now things for me, so if it works for you
it seems we're good to go. But I don't want to ack it yet as I'm
worried that it gets applied without the cpcap changes which would
break things :)

> Adding Guo Zeng and Barry Song to the thread as they were the authors
> of the ack_invert feature (a650fdd9427f1f5236f83d2d8137bea9b452fa53)
> and I'm not clear what happened to the chip they were needing it for.
> 
> > Things do work with the following patch and your patch for cpcap. So
> > they should both be applied together as a single patch.
> >
> > Care to fold in the following change and then repost your patch?
> >
> > Otherwise we might end up breaking things easily for booting or
> > bisect or stable. Or else the patch below needs to be applied first
> > to avoid breaking things.
> >
> 
> So cpcap needs to write 1's to clear irq's not 0's right?

Correct. And I tried to follow what the Motorola kernel tree
was doing to configure things but got the configuration wrong
but it worked so I never had a reason to doubt it before your
patch.

> Yes, I can certainly roll in the fix for cpcap if everyone agrees
> that's the right move.

OK thanks.

> I'll wait for some feedback from Mark Brown as well.

OK

Tony

Re: [PATCH] regmap: irq: fix ack-invert

[Tim Harvey]

On Tue, Feb 27, 2018 at 8:05 AM, Tim Harvey <tharvey@gateworks.com> wrote:
> When acking irqs we need to take into account the ack-invert case. Without
> this chips that require 0's to ACK interrupts will never clear the interrupt.
>
> I am working on an mfd driver that will use ack-invert and discovered
> this issue. The only user of ack_invert currently appears to be the
> motorola-cpcap driver. I'm not clear why that driver doesn't appear affected
> so I'm cc'ing those involved with that driver for review and testing.
>
> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> ---
>  drivers/base/regmap/regmap-irq.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
> index 429ca8e..abfed41 100644
> --- a/drivers/base/regmap/regmap-irq.c
> +++ b/drivers/base/regmap/regmap-irq.c
> @@ -355,16 +355,25 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
>          * doing a write per register.
>          */
>         for (i = 0; i < data->chip->num_regs; i++) {
> -               data->status_buf[i] &= ~data->mask_buf[i];
> -
> -               if (data->status_buf[i] && (chip->ack_base || chip->use_ack)) {
> +               if ( (data->status_buf[i] & ~data->mask_buf[i]) &&
> +                    (chip->ack_base || chip->use_ack) ) {
>                         reg = chip->ack_base +
>                                 (i * map->reg_stride * data->irq_reg_stride);
> -                       ret = regmap_write(map, reg, data->status_buf[i]);
> +                       /* some chips ack by write 0 */
> +                       if (chip->ack_invert)
> +                               ret = regmap_write(map, reg,
> +                                                  ~data->status_buf[i] &
> +                                                  ~data->mask_buf[i]);
> +
> +                       else
> +                               ret = regmap_write(map, reg,
> +                                                  data->status_buf[i] &
> +                                                  ~data->mask_buf[i]);
>                         if (ret != 0)
>                                 dev_err(map->dev, "Failed to ack 0x%x: %d\n",
>                                         reg, ret);
>                 }
> +               data->status_buf[i] &= ~data->mask_buf[i];
>         }
>
>         for (i = 0; i < chip->num_irqs; i++) {
> --
> 2.7.4
>

This still needs some work. For the mask_invert=true and
ack_invert=true case which is the case for the driver I'm writing it
ends up setting bits that didn't even fire. I will revisit on Monday
with a patch that uses update_bits to ensure only the correct bits are
set or cleared on an ack regardless of akc_invert.

Regards,

Tim