Re: [PATCH] dt-bindings: mfd: Correct interrupt flags in examples

[Vaittinen, Matti]

Hello Krzysztof,

On Wed, 2020-09-09 at 10:17 +0200, krzk@xxxxxxxxxx wrote:
> On Wed, Sep 09, 2020 at 06:30:44AM +0000, Vaittinen, Matti wrote:
> > On Tue, 2020-09-08 at 16:59 +0200, Krzysztof Kozlowski wrote:
> > > GPIO_ACTIVE_x flags are not correct in the context of interrupt
> > > flags.
> > > These are simple defines so they could be used in DTS but they
> > > will
> > > not
> > > have the same meaning:
> > > 1. GPIO_ACTIVE_HIGH = 0 = IRQ_TYPE_NONE
> > > 2. GPIO_ACTIVE_LOW  = 1 = IRQ_TYPE_EDGE_RISING
> > > 
> > > Correct the interrupt flags, assuming the author of the code
> > > wanted
> > > some
> > > logical behavior behind the name "ACTIVE_xxx", this is:
> > >   ACTIVE_LOW => IRQ_TYPE_LEVEL_LOW
> > > 
> > > Signed-off-by: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
> > 
> > For BD70528:
> > Acked-By: Matti Vaittinen <matti.vaittinen@xxxxxxxxxxxxxxxxx>
> > 
> > > ---
> > >  Documentation/devicetree/bindings/mfd/act8945a.txt          | 2
> > > +-
> > >  Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml    | 3
> > > ++-
> > >  Documentation/devicetree/bindings/mfd/rohm,bd70528-pmic.txt | 2
> > > +-
> > >  3 files changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > 
> > > diff --git a/Documentation/devicetree/bindings/mfd/rohm,bd70528-
> > > pmic.txt b/Documentation/devicetree/bindings/mfd/rohm,bd70528-
> > > pmic.txt
> > > index c3c02ce73cde..386eec06cf08 100644
> > > --- a/Documentation/devicetree/bindings/mfd/rohm,bd70528-pmic.txt
> > > +++ b/Documentation/devicetree/bindings/mfd/rohm,bd70528-pmic.txt
> > > @@ -39,7 +39,7 @@ pmic: pmic@4b {
> > >  	compatible = "rohm,bd70528";
> > >  	reg = <0x4b>;
> > >  	interrupt-parent = <&gpio1>;
> > > -	interrupts = <29 GPIO_ACTIVE_LOW>;
> > > +	interrupts = <29 IRQ_TYPE_LEVEL_LOW>;
> > 
> > This is how it should have been from the beginning :) Thanks!
> 
> I start to wonder now. It seems some boards do not configure a pull
> up
> there, so IRQ_TYPE_LEVEL_LOW is wrong - causes the line to stay in
> low
> state.  But actually this maybe is a problem of missing pull up, not
> the
> IRQ flag?

The BD70528 is designed so that it will use level active interrupts -
and line is pulled down when IRQ is active. Thus the example should
have IRQ_TYPE_LEVEL_LOW - and your fix is correct.

After that being said - I can't comment on actual board using BD70528
(or other ROHM ICs) - even less I can comment boards using other ICs.

After that being said - it's not a rare mistake to configure level
active IRQs to be triggered at edge - it actually works most of the
time - untill they deadlock at the race of generating new IRQ between
reading the status and acking the line... I've debugged way too many
such cases...

Anyways, for BD70528 DTS example your fix looks correct. Thanks.

> 
> Best regards,
> Krzysztof
>