| Message ID | 1507719939-27250-4-git-send-email-LW@KARO-electronics.de |
|---|---|
| State | New |
| Headers | show |
| Series | ARM: dts: imx28: Update Ka-Ro TX28 dts file | expand |
Hello, On Wed, Oct 11, 2017 at 01:05:38PM +0200, Lothar Waßmann wrote: > diff --git a/arch/arm/boot/dts/imx28-tx28.dts b/arch/arm/boot/dts/imx28-tx28.dts > index 211e67d..3c852f7 100644 > --- a/arch/arm/boot/dts/imx28-tx28.dts > +++ b/arch/arm/boot/dts/imx28-tx28.dts > @@ -328,8 +328,7 @@ > reg = <0x20>; > pinctrl-names = "default"; > pinctrl-0 = <&tx28_pca9554_pins>; > - interrupt-parent = <&gpio3>; > - interrupts = <28 0>; > + interrupts-extended = <&gpio3 28 IRQ_TYPE_NONE>; > gpio-controller; > #gpio-cells = <2>; > interrupt-controller; While interrupts-extended looks nice, Documentation/devicetree/bindings/interrupt-controller/interrupts.txt has: "interrupts-extended" should only be used when a device has multiple interrupt parents. If this is still true, this patch is wrong. Best regards Uwe
Hi, On Mon, 16 Oct 2017 09:17:26 +0200 Uwe Kleine-König wrote: > Hello, > > On Wed, Oct 11, 2017 at 01:05:38PM +0200, Lothar Waßmann wrote: > > diff --git a/arch/arm/boot/dts/imx28-tx28.dts b/arch/arm/boot/dts/imx28-tx28.dts > > index 211e67d..3c852f7 100644 > > --- a/arch/arm/boot/dts/imx28-tx28.dts > > +++ b/arch/arm/boot/dts/imx28-tx28.dts > > @@ -328,8 +328,7 @@ > > reg = <0x20>; > > pinctrl-names = "default"; > > pinctrl-0 = <&tx28_pca9554_pins>; > > - interrupt-parent = <&gpio3>; > > - interrupts = <28 0>; > > + interrupts-extended = <&gpio3 28 IRQ_TYPE_NONE>; > > gpio-controller; > > #gpio-cells = <2>; > > interrupt-controller; > > While interrupts-extended looks nice, > Documentation/devicetree/bindings/interrupt-controller/interrupts.txt > has: > > "interrupts-extended" should only be used when a device has > multiple interrupt parents. > > If this is still true, this patch is wrong. > Thanks for the hint. It really helps to read the documentation sometimes, rahter than relying on existing code only... A quick check shows, that more than 100 of the 130 uses of interrupts-extended are wrong. :( Lothar Waßmann
On Mon, Oct 16, 2017 at 10:56:32AM +0200, Lothar Waßmann wrote: > Hi, > > On Mon, 16 Oct 2017 09:17:26 +0200 Uwe Kleine-König wrote: > > Hello, > > > > On Wed, Oct 11, 2017 at 01:05:38PM +0200, Lothar Waßmann wrote: > > > diff --git a/arch/arm/boot/dts/imx28-tx28.dts b/arch/arm/boot/dts/imx28-tx28.dts > > > index 211e67d..3c852f7 100644 > > > --- a/arch/arm/boot/dts/imx28-tx28.dts > > > +++ b/arch/arm/boot/dts/imx28-tx28.dts > > > @@ -328,8 +328,7 @@ > > > reg = <0x20>; > > > pinctrl-names = "default"; > > > pinctrl-0 = <&tx28_pca9554_pins>; > > > - interrupt-parent = <&gpio3>; > > > - interrupts = <28 0>; > > > + interrupts-extended = <&gpio3 28 IRQ_TYPE_NONE>; > > > gpio-controller; > > > #gpio-cells = <2>; > > > interrupt-controller; > > > > While interrupts-extended looks nice, > > Documentation/devicetree/bindings/interrupt-controller/interrupts.txt > > has: > > > > "interrupts-extended" should only be used when a device has > > multiple interrupt parents. > > > > If this is still true, this patch is wrong. > > > Thanks for the hint. It really helps to read the documentation > sometimes, rahter than relying on existing code only... > > A quick check shows, that more than 100 of the 130 uses of > interrupts-extended are wrong. :( That's why I honestly consider that these documentation bits are stale. I adapted the Subject to maybe catch the attention of the devicetree guys. (BTW: The current wording is likely imprecise. I'd expect that it really should mean "Use interrupt-parent + interrupts if possible", but the following still fulfills the documented condition: interrupts-extended = <&gpio3 28 IRQ_TYPE_NONE &gpio3 29 IRQ_TYPE_NONE> while it can be expressed without interrupts-extended.) Best regards Uwe
On Mon, Oct 16, 2017 at 4:03 AM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Mon, Oct 16, 2017 at 10:56:32AM +0200, Lothar Waßmann wrote: >> Hi, >> >> On Mon, 16 Oct 2017 09:17:26 +0200 Uwe Kleine-König wrote: >> > Hello, >> > >> > On Wed, Oct 11, 2017 at 01:05:38PM +0200, Lothar Waßmann wrote: >> > > diff --git a/arch/arm/boot/dts/imx28-tx28.dts b/arch/arm/boot/dts/imx28-tx28.dts >> > > index 211e67d..3c852f7 100644 >> > > --- a/arch/arm/boot/dts/imx28-tx28.dts >> > > +++ b/arch/arm/boot/dts/imx28-tx28.dts >> > > @@ -328,8 +328,7 @@ >> > > reg = <0x20>; >> > > pinctrl-names = "default"; >> > > pinctrl-0 = <&tx28_pca9554_pins>; >> > > - interrupt-parent = <&gpio3>; >> > > - interrupts = <28 0>; >> > > + interrupts-extended = <&gpio3 28 IRQ_TYPE_NONE>; >> > > gpio-controller; >> > > #gpio-cells = <2>; >> > > interrupt-controller; >> > >> > While interrupts-extended looks nice, >> > Documentation/devicetree/bindings/interrupt-controller/interrupts.txt >> > has: >> > >> > "interrupts-extended" should only be used when a device has >> > multiple interrupt parents. >> > >> > If this is still true, this patch is wrong. >> > >> Thanks for the hint. It really helps to read the documentation >> sometimes, rahter than relying on existing code only... >> >> A quick check shows, that more than 100 of the 130 uses of >> interrupts-extended are wrong. :( > > That's why I honestly consider that these documentation bits are stale. > I adapted the Subject to maybe catch the attention of the devicetree > guys. The documentation is correct as that is recommended practice IMO. I wouldn't go fixing the 100 cases found either. > (BTW: The current wording is likely imprecise. I'd expect that it really > should mean "Use interrupt-parent + interrupts if possible", but the > following still fulfills the documented condition: "should" is pretty standard to mean recommended vs. "shall" or "must" meaning required. Rob
On Mon, Oct 16, 2017 at 10:56:32AM +0200, Lothar Waßmann wrote: > Hi, > > On Mon, 16 Oct 2017 09:17:26 +0200 Uwe Kleine-König wrote: > > Hello, > > > > On Wed, Oct 11, 2017 at 01:05:38PM +0200, Lothar Waßmann wrote: > > > diff --git a/arch/arm/boot/dts/imx28-tx28.dts b/arch/arm/boot/dts/imx28-tx28.dts > > > index 211e67d..3c852f7 100644 > > > --- a/arch/arm/boot/dts/imx28-tx28.dts > > > +++ b/arch/arm/boot/dts/imx28-tx28.dts > > > @@ -328,8 +328,7 @@ > > > reg = <0x20>; > > > pinctrl-names = "default"; > > > pinctrl-0 = <&tx28_pca9554_pins>; > > > - interrupt-parent = <&gpio3>; > > > - interrupts = <28 0>; > > > + interrupts-extended = <&gpio3 28 IRQ_TYPE_NONE>; > > > gpio-controller; > > > #gpio-cells = <2>; > > > interrupt-controller; > > > > While interrupts-extended looks nice, > > Documentation/devicetree/bindings/interrupt-controller/interrupts.txt > > has: > > > > "interrupts-extended" should only be used when a device has > > multiple interrupt parents. > > > > If this is still true, this patch is wrong. > > > Thanks for the hint. It really helps to read the documentation > sometimes, rahter than relying on existing code only... > > A quick check shows, that more than 100 of the 130 uses of > interrupts-extended are wrong. :( So should I drop all interrupts-extended patches from you? Shawn
Hi, On Wed, 18 Oct 2017 10:13:32 +0800 Shawn Guo wrote: > On Mon, Oct 16, 2017 at 10:56:32AM +0200, Lothar Waßmann wrote: > > Hi, > > > > On Mon, 16 Oct 2017 09:17:26 +0200 Uwe Kleine-König wrote: > > > Hello, > > > > > > On Wed, Oct 11, 2017 at 01:05:38PM +0200, Lothar Waßmann wrote: > > > > diff --git a/arch/arm/boot/dts/imx28-tx28.dts b/arch/arm/boot/dts/imx28-tx28.dts > > > > index 211e67d..3c852f7 100644 > > > > --- a/arch/arm/boot/dts/imx28-tx28.dts > > > > +++ b/arch/arm/boot/dts/imx28-tx28.dts > > > > @@ -328,8 +328,7 @@ > > > > reg = <0x20>; > > > > pinctrl-names = "default"; > > > > pinctrl-0 = <&tx28_pca9554_pins>; > > > > - interrupt-parent = <&gpio3>; > > > > - interrupts = <28 0>; > > > > + interrupts-extended = <&gpio3 28 IRQ_TYPE_NONE>; > > > > gpio-controller; > > > > #gpio-cells = <2>; > > > > interrupt-controller; > > > > > > While interrupts-extended looks nice, > > > Documentation/devicetree/bindings/interrupt-controller/interrupts.txt > > > has: > > > > > > "interrupts-extended" should only be used when a device has > > > multiple interrupt parents. > > > > > > If this is still true, this patch is wrong. > > > > > Thanks for the hint. It really helps to read the documentation > > sometimes, rahter than relying on existing code only... > > > > A quick check shows, that more than 100 of the 130 uses of > > interrupts-extended are wrong. :( > > So should I drop all interrupts-extended patches from you? > Yes, please. Since this patch, and the corresponding patch for tx53 also fixed the interrupt flags, I will send new patches to only fix the interrupt flags. Lothar Waßmann
On Wed, Oct 18, 2017 at 08:40:59AM +0200, Lothar Waßmann wrote: > > So should I drop all interrupts-extended patches from you? > > > Yes, please. Since this patch, and the corresponding patch for tx53 > also fixed the interrupt flags, I will send new patches to only fix > the interrupt flags. Okay, the following two got dropped from imx/dt branch. ARM: dts: imx28-tx28: fix interrupt flags and use interrupts-extended property ARM: dts: imx53-tx53: fix interrupt flags and use interrupts-extended property Shawn
diff --git a/arch/arm/boot/dts/imx28-tx28.dts b/arch/arm/boot/dts/imx28-tx28.dts index 211e67d..3c852f7 100644 --- a/arch/arm/boot/dts/imx28-tx28.dts +++ b/arch/arm/boot/dts/imx28-tx28.dts @@ -328,8 +328,7 @@ reg = <0x20>; pinctrl-names = "default"; pinctrl-0 = <&tx28_pca9554_pins>; - interrupt-parent = <&gpio3>; - interrupts = <28 0>; + interrupts-extended = <&gpio3 28 IRQ_TYPE_NONE>; gpio-controller; #gpio-cells = <2>; interrupt-controller; @@ -341,8 +340,7 @@ reg = <0x38>; pinctrl-names = "default"; pinctrl-0 = <&tx28_edt_ft5x06_pins>; - interrupt-parent = <&gpio2>; - interrupts = <5 IRQ_TYPE_EDGE_FALLING>; + interrupts-extended = <&gpio2 5 IRQ_TYPE_EDGE_FALLING>; reset-gpios = <&gpio2 6 GPIO_ACTIVE_LOW>; wake-gpios = <&gpio4 9 GPIO_ACTIVE_HIGH>; }; @@ -352,8 +350,7 @@ reg = <0x48>; pinctrl-names = "default"; pinctrl-0 = <&tx28_tsc2007_pins>; - interrupt-parent = <&gpio3>; - interrupts = <20 0>; + interrupts-extended = <&gpio3 20 IRQ_TYPE_EDGE_FALLING>; pendown-gpio = <&gpio3 20 GPIO_ACTIVE_LOW>; ti,x-plate-ohms = /bits/ 16 <660>; };
Use the correct interrupt flags and change the 'interrupt-parent', 'interrupts' property pairs to 'interrupts-extended'. Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> --- arch/arm/boot/dts/imx28-tx28.dts | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)