dt-bindings: mfd: Correct interrupt flags in examples
Subsystems (2)
PATCH 1/1 dt-bindings: mfd: Correct interrupt flags in examples
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
--- 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(-)
Documentation/devicetree/bindings/mfd/act8945a.txt +1 -1
@@ -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>;
Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml +2 -1
@@ -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>;
Documentation/devicetree/bindings/mfd/rohm,bd70528-pmic.txt +1 -1
@@ -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";
Signature · 1 line
2.17.1
Quoted 10 lines · Krzysztof Kozlowski
GPIO_ACTIVE_xflags 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_RISINGCorrect 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
Quoted 6 lines
Signed-off-by Krzysztof Kozlowski <[email protected]>--- 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.
Signature · 4 lines
Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
Quoted 8 lines · Krzysztof Kozlowski
GPIO_ACTIVE_xflags 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
Quoted 6 lines
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_LOWSigned-off-by Krzysztof Kozlowski <[email protected]>
For BD70528:
Quoted 14 lines
--- 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(-)(quoted diff) +0
-0pmic.txt b/Documentation/devicetree/bindings/mfd/rohm,bd70528- pmic.txt
indexc3c02ce73cde..386eec06cf08100644
Documentation/devicetree/bindings/mfd/rohm,bd70528-pmic.txt pmic: pmic@4b { · line 42
Quoted 5 lines
Documentation/devicetree/bindings/mfd/rohm,bd70528-pmic.txt +1
-1@@ -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)
Quoted 27 lines · Matti
Quoted 8 lines · Krzysztof Kozlowski
GPIO_ACTIVE_xflags 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_RISINGQuoted 6 lines
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_LOWSigned-off-by Krzysztof Kozlowski <[email protected]>For BD70528:
Acked-by Matti Vaittinen <[email protected]>Quoted 7 lines
--- 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(-)
Quoted 14 lines
Documentation/devicetree/bindings/mfd/rohm,bd70528-pmic.txt pmic: pmic@4b { · line 42
Quoted 12 lines
(quoted diff) +0
-0pmic.txt b/Documentation/devicetree/bindings/mfd/rohm,bd70528- pmic.txt
indexc3c02ce73cde..386eec06cf08100644Documentation/devicetree/bindings/mfd/rohm,bd70528-pmic.txt +1
-1@@ -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
Hello Krzysztof,
Quoted 47 lines · On Wed, 2020-09-09 at 10:17 +0200, [email protected] wrote:
Quoted 31 lines · Matti
Quoted 9 lines · Krzysztof Kozlowski
GPIO_ACTIVE_xflags 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_RISINGQuoted 7 lines
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_LOWSigned-off-by Krzysztof Kozlowski <[email protected]>For BD70528:
Acked-by Matti Vaittinen <[email protected]>Quoted 10 lines
--- 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(-)Quoted 14 lines
Documentation/devicetree/bindings/mfd/rohm,bd70528-pmic.txt pmic: pmic@4b { · line 42
Quoted 12 lines
(quoted diff) +0
-0pmic.txt b/Documentation/devicetree/bindings/mfd/rohm,bd70528- pmic.txt
indexc3c02ce73cde..386eec06cf08100644Documentation/devicetree/bindings/mfd/rohm,bd70528-pmic.txt +1
-1@@ -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!
Quoted 7 lines
I start to wonder now. It seems some boards do not configure a pull up
there, soIRQ_TYPE_LEVEL_LOWis 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.
Quoted 4 lines
Best regards,
Krzysztof
Quoted 65 lines · Matti
Hello Krzysztof,
Quoted 47 lines · On Wed, 2020-09-09 at 10:17 +0200, [email protected] wrote:
Quoted 31 lines · Matti
Quoted 9 lines · Krzysztof Kozlowski
GPIO_ACTIVE_xflags 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_RISINGQuoted 7 lines
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_LOWSigned-off-by Krzysztof Kozlowski <[email protected]>For BD70528:
Acked-by Matti Vaittinen <[email protected]>Quoted 10 lines
--- 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(-)Quoted 14 lines
Documentation/devicetree/bindings/mfd/rohm,bd70528-pmic.txt pmic: pmic@4b { · line 42
Quoted 12 lines
(quoted diff) +0
-0pmic.txt b/Documentation/devicetree/bindings/mfd/rohm,bd70528- pmic.txt
indexc3c02ce73cde..386eec06cf08100644Documentation/devicetree/bindings/mfd/rohm,bd70528-pmic.txt +1
-1@@ -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!
Quoted 7 lines
I start to wonder now. It seems some boards do not configure a pull up
there, soIRQ_TYPE_LEVEL_LOWis 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.
Quoted 7 lines
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
Quoted 91 lines · On Wed, 2020-09-09 at 11:20 +0200, [email protected] wrote:
Quoted 88 lines · Matti
Hello Krzysztof,
Quoted 56 lines · On Wed, 2020-09-09 at 10:17 +0200, [email protected] wrote:
Quoted 46 lines · Matti
Quoted 11 lines · Krzysztof Kozlowski
GPIO_ACTIVE_xflags 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_RISINGQuoted 7 lines
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_LOWSigned-off-by Krzysztof Kozlowski <[email protected]>For BD70528:
Acked-by Matti Vaittinen <[email protected]>Quoted 23 lines
---
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
indexc3c02ce73cde..386eec06cf08100644
--- a/Documentation/devicetree/bindings/mfd/rohm,bd70528- pmic.txt
+++ b/Documentation/devicetree/bindings/mfd/rohm,bd70528- pmic.txtQuoted 8 lines
Documentation/devicetree/bindings/mfd/rohm,bd70528-pmic.txt pmic: pmic@4b { · line 42
Quoted 6 lines
Documentation/devicetree/bindings/mfd/rohm,bd70528-pmic.txt +1
-1@@ -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!
Quoted 10 lines
I start to wonder now. It seems some boards do not configure a pull
up
there, soIRQ_TYPE_LEVEL_LOWis 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 haveIRQ_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...Quoted 1 line
Anyways, for BD70528 DTS example your fix looks correct. Thanks.
Quoted 8 lines
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)
Quoted 44 lines · Krzysztof Kozlowski
GPIO_ACTIVE_xflags 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_RISINGCorrect 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_LOWSigned-off-by Krzysztof Kozlowski <[email protected]>--- 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(-)Documentation/devicetree/bindings/mfd/act8945a.txt +1
-1@@ -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>;Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml +1
-0@@ -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>;
Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml examples: · line 162
Quoted 8 lines
Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml +1
-1@@ -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:
Thanks for finding this!
Tim
Quoted 10 lines · Krzysztof Kozlowski
GPIO_ACTIVE_xflags 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_RISINGCorrect 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
Quoted 7 lines
Signed-off-by Krzysztof Kozlowski <[email protected]>--- 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(-)