[3/4] ARM: dts: imx28-tx28: fix interrupt flags and use interrupts-extended property

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
Related show

Commit Message

Lothar Waßmann Oct. 11, 2017, 11:05 a.m.
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(-)

Comments

Uwe Kleine-König Oct. 16, 2017, 7:17 a.m. | #1
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
Lothar Waßmann Oct. 16, 2017, 8:56 a.m. | #2
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
Uwe Kleine-König Oct. 16, 2017, 9:03 a.m. | #3
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
Rob Herring Oct. 16, 2017, 9:29 p.m. | #4
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
Shawn Guo Oct. 18, 2017, 2:13 a.m. | #5
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
Lothar Waßmann Oct. 18, 2017, 6:40 a.m. | #6
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
Shawn Guo Oct. 23, 2017, 12:20 a.m. | #7
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

Patch

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>;
 	};