[PATCH] dt-bindings: mfd: Improve readability for TPS65217 interrupt sources

[PATCH] dt-bindings: mfd: Improve readability for TPS65217 interrupt sources

[Milo Kim]

AC and USB interrupts are related with external power input.
PB interrupt means push button pressed or released event.
Use better human readable definitions.

Signed-off-by: Milo Kim <woogyom.kim@gmail.com>
---
 arch/arm/boot/dts/am335x-bone-common.dtsi | 4 ++--
 include/dt-bindings/mfd/tps65217.h        | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm/boot/dts/am335x-bone-common.dtsi b/arch/arm/boot/dts/am335x-bone-common.dtsi
index dc561d5..1848d58 100644
--- a/arch/arm/boot/dts/am335x-bone-common.dtsi
+++ b/arch/arm/boot/dts/am335x-bone-common.dtsi
@@ -319,13 +319,13 @@
 	ti,pmic-shutdown-controller;
 
 	charger {
-		interrupts = <TPS65217_IRQ_AC>, <TPS65217_IRQ_USB>;
+		interrupts = <TPS65217_IRQ_AC_POWER>, <TPS65217_IRQ_USB_POWER>;
 		interrupts-names = "AC", "USB";
 		status = "okay";
 	};
 
 	pwrbutton {
-		interrupts = <TPS65217_IRQ_PB>;
+		interrupts = <TPS65217_IRQ_PUSHBUTTON>;
 		status = "okay";
 	};
 
diff --git a/include/dt-bindings/mfd/tps65217.h b/include/dt-bindings/mfd/tps65217.h
index cafb9e6..0293fdd 100644
--- a/include/dt-bindings/mfd/tps65217.h
+++ b/include/dt-bindings/mfd/tps65217.h
@@ -19,8 +19,8 @@
 #ifndef __DT_BINDINGS_TPS65217_H__
 #define __DT_BINDINGS_TPS65217_H__
 
-#define TPS65217_IRQ_USB	0
-#define TPS65217_IRQ_AC		1
-#define TPS65217_IRQ_PB		2
+#define TPS65217_IRQ_USB_POWER		0	/* USB power state change */
+#define TPS65217_IRQ_AC_POWER		1	/* AC power state change */
+#define TPS65217_IRQ_PUSHBUTTON		2	/* Push button state change */
 
 #endif
-- 
2.9.3

Re: [PATCH] dt-bindings: mfd: Improve readability for TPS65217 interrupt sources

[Lee Jones]

On Mon, 21 Nov 2016, Milo Kim wrote:

> AC and USB interrupts are related with external power input.
> PB interrupt means push button pressed or released event.
> Use better human readable definitions.
> 
> Signed-off-by: Milo Kim <woogyom.kim@gmail.com>
> ---
>  arch/arm/boot/dts/am335x-bone-common.dtsi | 4 ++--
>  include/dt-bindings/mfd/tps65217.h        | 6 +++---
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/am335x-bone-common.dtsi b/arch/arm/boot/dts/am335x-bone-common.dtsi
> index dc561d5..1848d58 100644
> --- a/arch/arm/boot/dts/am335x-bone-common.dtsi
> +++ b/arch/arm/boot/dts/am335x-bone-common.dtsi
> @@ -319,13 +319,13 @@
>  	ti,pmic-shutdown-controller;
>  
>  	charger {
> -		interrupts = <TPS65217_IRQ_AC>, <TPS65217_IRQ_USB>;
> +		interrupts = <TPS65217_IRQ_AC_POWER>, <TPS65217_IRQ_USB_POWER>;
>  		interrupts-names = "AC", "USB";
>  		status = "okay";
>  	};
>  
>  	pwrbutton {
> -		interrupts = <TPS65217_IRQ_PB>;
> +		interrupts = <TPS65217_IRQ_PUSHBUTTON>;

Push button or power button?

>  		status = "okay";
>  	};
>  
> diff --git a/include/dt-bindings/mfd/tps65217.h b/include/dt-bindings/mfd/tps65217.h
> index cafb9e6..0293fdd 100644
> --- a/include/dt-bindings/mfd/tps65217.h
> +++ b/include/dt-bindings/mfd/tps65217.h
> @@ -19,8 +19,8 @@
>  #ifndef __DT_BINDINGS_TPS65217_H__
>  #define __DT_BINDINGS_TPS65217_H__
>  
> -#define TPS65217_IRQ_USB	0
> -#define TPS65217_IRQ_AC		1
> -#define TPS65217_IRQ_PB		2
> +#define TPS65217_IRQ_USB_POWER		0	/* USB power state change */
> +#define TPS65217_IRQ_AC_POWER		1	/* AC power state change */
> +#define TPS65217_IRQ_PUSHBUTTON		2	/* Push button state change */

This changes the ABI.

It will require a DT Ack.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] dt-bindings: mfd: Improve readability for TPS65217 interrupt sources

[Lee Jones]

On Tue, 22 Nov 2016, Lee Jones wrote:

> On Mon, 21 Nov 2016, Milo Kim wrote:
> 
> > AC and USB interrupts are related with external power input.
> > PB interrupt means push button pressed or released event.
> > Use better human readable definitions.
> > 
> > Signed-off-by: Milo Kim <woogyom.kim-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> >  arch/arm/boot/dts/am335x-bone-common.dtsi | 4 ++--
> >  include/dt-bindings/mfd/tps65217.h        | 6 +++---
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/am335x-bone-common.dtsi b/arch/arm/boot/dts/am335x-bone-common.dtsi
> > index dc561d5..1848d58 100644
> > --- a/arch/arm/boot/dts/am335x-bone-common.dtsi
> > +++ b/arch/arm/boot/dts/am335x-bone-common.dtsi
> > @@ -319,13 +319,13 @@
> >  	ti,pmic-shutdown-controller;
> >  
> >  	charger {
> > -		interrupts = <TPS65217_IRQ_AC>, <TPS65217_IRQ_USB>;
> > +		interrupts = <TPS65217_IRQ_AC_POWER>, <TPS65217_IRQ_USB_POWER>;
> >  		interrupts-names = "AC", "USB";
> >  		status = "okay";
> >  	};
> >  
> >  	pwrbutton {
> > -		interrupts = <TPS65217_IRQ_PB>;
> > +		interrupts = <TPS65217_IRQ_PUSHBUTTON>;
> 
> Push button or power button?
> 
> >  		status = "okay";
> >  	};
> >  
> > diff --git a/include/dt-bindings/mfd/tps65217.h b/include/dt-bindings/mfd/tps65217.h
> > index cafb9e6..0293fdd 100644
> > --- a/include/dt-bindings/mfd/tps65217.h
> > +++ b/include/dt-bindings/mfd/tps65217.h
> > @@ -19,8 +19,8 @@
> >  #ifndef __DT_BINDINGS_TPS65217_H__
> >  #define __DT_BINDINGS_TPS65217_H__
> >  
> > -#define TPS65217_IRQ_USB	0
> > -#define TPS65217_IRQ_AC		1
> > -#define TPS65217_IRQ_PB		2
> > +#define TPS65217_IRQ_USB_POWER		0	/* USB power state change */
> > +#define TPS65217_IRQ_AC_POWER		1	/* AC power state change */
> > +#define TPS65217_IRQ_PUSHBUTTON		2	/* Push button state change */
> 
> This changes the ABI.
> 
> It will require a DT Ack.

Tell a lie.  Sorry, I was getting false positives from my grep.  It
looks like you use the same scheme from within include/linux.  I
suggest that you probable don't want to do that.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dt-bindings: mfd: Improve readability for TPS65217 interrupt sources

[Arnd Bergmann]

On Tuesday, November 22, 2016 4:00:13 PM CET Lee Jones wrote:
> > > diff --git a/include/dt-bindings/mfd/tps65217.h b/include/dt-bindings/mfd/tps65217.h
> > > index cafb9e6..0293fdd 100644
> > > --- a/include/dt-bindings/mfd/tps65217.h
> > > +++ b/include/dt-bindings/mfd/tps65217.h
> > > @@ -19,8 +19,8 @@
> > >  #ifndef __DT_BINDINGS_TPS65217_H__
> > >  #define __DT_BINDINGS_TPS65217_H__
> > >  
> > > -#define TPS65217_IRQ_USB   0
> > > -#define TPS65217_IRQ_AC            1
> > > -#define TPS65217_IRQ_PB            2
> > > +#define TPS65217_IRQ_USB_POWER             0       /* USB power state change */
> > > +#define TPS65217_IRQ_AC_POWER              1       /* AC power state change */
> > > +#define TPS65217_IRQ_PUSHBUTTON            2       /* Push button state change */
> > 
> > This changes the ABI.
> > 
> > It will require a DT Ack.
> 
> Tell a lie.  Sorry, I was getting false positives from my grep.  It
> looks like you use the same scheme from within include/linux.  I
> suggest that you probable don't want to do that.

Doing this change however would cause a bisection problem: you
can't rename just the constants in the header or just the driver
using those constants.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dt-bindings: mfd: Improve readability for TPS65217 interrupt sources

[Milo Kim]

On 11/23/2016 12:57 AM, Lee Jones wrote:
>>  	pwrbutton {
>> > -		interrupts = <TPS65217_IRQ_PB>;
>> > +		interrupts = <TPS65217_IRQ_PUSHBUTTON>;
> Push button or power button?
>

According to the datasheet, push button interrupt is correct.

	http://www.ti.com/lit/ds/symlink/tps65217.pdf

This is used for a power button input in Beaglebone boards. In other 
words, the power button is one of push button usages.

So, I'd like to keep general name for the interrupt.

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dt-bindings: mfd: Improve readability for TPS65217 interrupt sources

[Arnd Bergmann]

On Wednesday, November 23, 2016 8:38:00 PM CET Milo Kim wrote:
> On 11/23/2016 12:57 AM, Lee Jones wrote:
> >>      pwrbutton {
> >> > -          interrupts = <TPS65217_IRQ_PB>;
> >> > +          interrupts = <TPS65217_IRQ_PUSHBUTTON>;
> > Push button or power button?
> >
> 
> According to the datasheet, push button interrupt is correct.
> 
>         http://www.ti.com/lit/ds/symlink/tps65217.pdf
> 
> This is used for a power button input in Beaglebone boards. In other 
> words, the power button is one of push button usages.
> 
> So, I'd like to keep general name for the interrupt.

Ah, the numbers come from the data sheet. Please just remove the
header then, there is no need to keep them as an ABI, in particular
when the driver doesn't even include that header today.

I see you even have names for them in the node:

        charger {
-               interrupts = <TPS65217_IRQ_AC>, <TPS65217_IRQ_USB>;
+               interrupts = <TPS65217_IRQ_AC_POWER>, <TPS65217_IRQ_USB_POWER>;
                interrupts-names = "AC", "USB";
                status = "okay";

What matters here is the binding documentation in
Documentation/devicetree/bindings/power/supply/tps65217_charger.txt

Please fix that instead to mandate that the two interrupts are
required, what their functions are, and what the interrupt-names
(not interrupts-names) property must list.

	Arnd

Re: [PATCH] dt-bindings: mfd: Improve readability for TPS65217 interrupt sources

[Milo Kim]

On 11/23/2016 08:51 PM, Arnd Bergmann wrote:
> Ah, the numbers come from the data sheet. Please just remove the
> header then, there is no need to keep them as an ABI, in particular
> when the driver doesn't even include that header today.

Got it.

> What matters here is the binding documentation in
> Documentation/devicetree/bindings/power/supply/tps65217_charger.txt

OK, in addition to this, the interrupt specifier needs to be added in 
Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt.

> Please fix that instead to mandate that the two interrupts are
> required, what their functions are, and what the interrupt-names
> (not interrupts-names) property must list.

Oops, wrong interrupt name property - my bad.

Thanks for all your feedback.

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html