* [PATCH] dt-bindings: mfd: Correct interrupt flags in examples
@ 2020-09-08 14:59 Krzysztof Kozlowski
2020-09-09 6:29 ` Lee Jones
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Krzysztof Kozlowski @ 2020-09-08 14:59 UTC (permalink / raw)
To: Lee Jones, Rob Herring, Tim Harvey, Robert Jones,
Matti Vaittinen, devicetree, linux-kernel, linux-power
Cc: Krzysztof Kozlowski
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@kernel.org>
---
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/act8945a.txt b/Documentation/devicetree/bindings/mfd/act8945a.txt
index e6f168db6c72..5ca75d888b4a 100644
--- a/Documentation/devicetree/bindings/mfd/act8945a.txt
+++ b/Documentation/devicetree/bindings/mfd/act8945a.txt
@@ -71,7 +71,7 @@ Example:
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_charger_chglev &pinctrl_charger_lbo &pinctrl_charger_irq>;
interrupt-parent = <&pioA>;
- interrupts = <45 GPIO_ACTIVE_LOW>;
+ interrupts = <45 IRQ_TYPE_LEVEL_LOW>;
active-semi,chglev-gpios = <&pioA 12 GPIO_ACTIVE_HIGH>;
active-semi,lbo-gpios = <&pioA 72 GPIO_ACTIVE_LOW>;
diff --git a/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml b/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
index 9b6eb50606e8..e541eb0d02ed 100644
--- a/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
+++ b/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
@@ -147,6 +147,7 @@ required:
examples:
- |
#include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
i2c {
#address-cells = <1>;
#size-cells = <0>;
@@ -155,7 +156,7 @@ examples:
compatible = "gw,gsc";
reg = <0x20>;
interrupt-parent = <&gpio1>;
- interrupts = <4 GPIO_ACTIVE_LOW>;
+ interrupts = <4 IRQ_TYPE_LEVEL_LOW>;
interrupt-controller;
#interrupt-cells = <1>;
#address-cells = <1>;
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>;
clocks = <&osc 0>;
#clock-cells = <0>;
clock-output-names = "bd70528-32k-out";
--
2.17.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] dt-bindings: mfd: Correct interrupt flags in examples
2020-09-08 14:59 [PATCH] dt-bindings: mfd: Correct interrupt flags in examples Krzysztof Kozlowski
@ 2020-09-09 6:29 ` Lee Jones
2020-09-09 6:30 ` Vaittinen, Matti
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Lee Jones @ 2020-09-09 6:29 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Rob Herring, Tim Harvey, Robert Jones, Matti Vaittinen,
devicetree, linux-kernel, linux-power
On Tue, 08 Sep 2020, 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@kernel.org>
> ---
> 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(-)
Looks good, but I'd like the author to review.
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dt-bindings: mfd: Correct interrupt flags in examples
2020-09-08 14:59 [PATCH] dt-bindings: mfd: Correct interrupt flags in examples Krzysztof Kozlowski
2020-09-09 6:29 ` Lee Jones
@ 2020-09-09 6:30 ` Vaittinen, Matti
2020-09-09 8:17 ` krzk
2020-09-09 15:22 ` Tim Harvey
2020-09-15 16:53 ` Rob Herring
3 siblings, 1 reply; 9+ messages in thread
From: Vaittinen, Matti @ 2020-09-09 6:30 UTC (permalink / raw)
To: krzk, lee.jones, linux-power, linux-kernel, devicetree, tharvey,
rjones, robh+dt
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@kernel.org>
For BD70528:
Acked-By: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---
> 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!
Br,
Matti
--
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)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dt-bindings: mfd: Correct interrupt flags in examples
2020-09-09 6:30 ` Vaittinen, Matti
@ 2020-09-09 8:17 ` krzk
2020-09-09 8:57 ` Vaittinen, Matti
0 siblings, 1 reply; 9+ messages in thread
From: krzk @ 2020-09-09 8:17 UTC (permalink / raw)
To: Vaittinen, Matti
Cc: lee.jones, linux-power, linux-kernel, devicetree, tharvey,
rjones, robh+dt
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@kernel.org>
>
> For BD70528:
> Acked-By: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
>
> > ---
> > 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?
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dt-bindings: mfd: Correct interrupt flags in examples
2020-09-09 8:17 ` krzk
@ 2020-09-09 8:57 ` Vaittinen, Matti
2020-09-09 9:20 ` krzk
0 siblings, 1 reply; 9+ messages in thread
From: Vaittinen, Matti @ 2020-09-09 8:57 UTC (permalink / raw)
To: krzk
Cc: robh+dt, linux-power, devicetree, linux-kernel, tharvey,
lee.jones, rjones
Hello Krzysztof,
On Wed, 2020-09-09 at 10:17 +0200, krzk@kernel.org 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@kernel.org>
> >
> > For BD70528:
> > Acked-By: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> >
> > > ---
> > > 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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dt-bindings: mfd: Correct interrupt flags in examples
2020-09-09 8:57 ` Vaittinen, Matti
@ 2020-09-09 9:20 ` krzk
2020-09-09 10:19 ` Vaittinen, Matti
0 siblings, 1 reply; 9+ messages in thread
From: krzk @ 2020-09-09 9:20 UTC (permalink / raw)
To: Vaittinen, Matti
Cc: robh+dt, linux-power, devicetree, linux-kernel, tharvey,
lee.jones, rjones
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@kernel.org 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@kernel.org>
> > >
> > > For BD70528:
> > > Acked-By: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > >
> > > > ---
> > > > 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...
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dt-bindings: mfd: Correct interrupt flags in examples
2020-09-09 9:20 ` krzk
@ 2020-09-09 10:19 ` Vaittinen, Matti
0 siblings, 0 replies; 9+ messages in thread
From: Vaittinen, Matti @ 2020-09-09 10:19 UTC (permalink / raw)
To: krzk
Cc: rjones, linux-power, devicetree, linux-kernel, tharvey, robh+dt,
lee.jones
On Wed, 2020-09-09 at 11:20 +0200, krzk@kernel.org 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@kernel.org 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@kernel.org>
> > > >
> > > > For BD70528:
> > > > Acked-By: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > > >
> > > > > ---
> > > > > 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)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dt-bindings: mfd: Correct interrupt flags in examples
2020-09-08 14:59 [PATCH] dt-bindings: mfd: Correct interrupt flags in examples Krzysztof Kozlowski
2020-09-09 6:29 ` Lee Jones
2020-09-09 6:30 ` Vaittinen, Matti
@ 2020-09-09 15:22 ` Tim Harvey
2020-09-15 16:53 ` Rob Herring
3 siblings, 0 replies; 9+ messages in thread
From: Tim Harvey @ 2020-09-09 15:22 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Lee Jones, Rob Herring, Robert Jones, Matti Vaittinen,
Device Tree Mailing List, open list, linux-power
On Tue, Sep 8, 2020 at 7:59 AM Krzysztof Kozlowski <krzk@kernel.org> 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@kernel.org>
> ---
> 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/act8945a.txt b/Documentation/devicetree/bindings/mfd/act8945a.txt
> index e6f168db6c72..5ca75d888b4a 100644
> --- a/Documentation/devicetree/bindings/mfd/act8945a.txt
> +++ b/Documentation/devicetree/bindings/mfd/act8945a.txt
> @@ -71,7 +71,7 @@ Example:
> pinctrl-names = "default";
> pinctrl-0 = <&pinctrl_charger_chglev &pinctrl_charger_lbo &pinctrl_charger_irq>;
> interrupt-parent = <&pioA>;
> - interrupts = <45 GPIO_ACTIVE_LOW>;
> + interrupts = <45 IRQ_TYPE_LEVEL_LOW>;
>
> active-semi,chglev-gpios = <&pioA 12 GPIO_ACTIVE_HIGH>;
> active-semi,lbo-gpios = <&pioA 72 GPIO_ACTIVE_LOW>;
> diff --git a/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml b/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
> index 9b6eb50606e8..e541eb0d02ed 100644
> --- a/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
> +++ b/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
> @@ -147,6 +147,7 @@ required:
> examples:
> - |
> #include <dt-bindings/gpio/gpio.h>
> + #include <dt-bindings/interrupt-controller/irq.h>
> i2c {
> #address-cells = <1>;
> #size-cells = <0>;
> @@ -155,7 +156,7 @@ examples:
> compatible = "gw,gsc";
> reg = <0x20>;
> interrupt-parent = <&gpio1>;
> - interrupts = <4 GPIO_ACTIVE_LOW>;
> + interrupts = <4 IRQ_TYPE_LEVEL_LOW>;
> interrupt-controller;
> #interrupt-cells = <1>;
> #address-cells = <1>;
For gateworks-gsc.yaml:
Acked-By: Tim Harvey <tharvey@gateworks.com>
Thanks for finding this!
Tim
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dt-bindings: mfd: Correct interrupt flags in examples
2020-09-08 14:59 [PATCH] dt-bindings: mfd: Correct interrupt flags in examples Krzysztof Kozlowski
` (2 preceding siblings ...)
2020-09-09 15:22 ` Tim Harvey
@ 2020-09-15 16:53 ` Rob Herring
3 siblings, 0 replies; 9+ messages in thread
From: Rob Herring @ 2020-09-15 16:53 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: linux-power, Rob Herring, Matti Vaittinen, Robert Jones,
devicetree, Tim Harvey, linux-kernel, Lee Jones
On Tue, 08 Sep 2020 16:59:00 +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@kernel.org>
> ---
> 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(-)
>
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-09-15 18:53 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-08 14:59 [PATCH] dt-bindings: mfd: Correct interrupt flags in examples Krzysztof Kozlowski
2020-09-09 6:29 ` Lee Jones
2020-09-09 6:30 ` Vaittinen, Matti
2020-09-09 8:17 ` krzk
2020-09-09 8:57 ` Vaittinen, Matti
2020-09-09 9:20 ` krzk
2020-09-09 10:19 ` Vaittinen, Matti
2020-09-09 15:22 ` Tim Harvey
2020-09-15 16:53 ` Rob Herring
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).