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

[Vaittinen, Matti]

On Wed, 2020-09-09 at 11:20 +0200, krzk@xxxxxxxxxx wrote:
> On Wed, Sep 09, 2020 at 08:57:36AM +0000, Vaittinen, Matti wrote:
> > 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.
> 
> Thanks. I found this error in multiple DTS files - most probably a
> copy
> paste from example or from evalkit (e.g. imx8mm-evk.dts). The trouble
> is
> that I don't have the schematics for them and at least in one
> hardware
> (Variscite VAR-SOM-MX8M which I am using) it looks like logic got
> reversed...

Hmm. According to the Variscite materials they use the BD71847AMWV -
not the BD70528. BD71847 does also have level active IORQs (active low)
- but misconfiguration may go unnoticed as (AFAIR) the BD71847 IRQs do
mostly inform conditions leading to reset by HW - the power button
short push being an exception. Thus configuring the IRQ to falling edge
is likely to work without deadlocking due to the race I mentioned.
(BD70528 would use IRQs for RTC so it would possibly be more
errorprone). Anyways the board dtses go beyond my area - but the
example fix for BD70528 definitely looks good :) Thanks again.



--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland
SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~

Simon says - in Latin please.
"non cogito me" dixit Rene Descarte, deinde evanescavit

(Thanks for the translation Simon)