From: Andreas Kemnade <andreas@kemnade.info>
TWL603x has also a power button, so add the corresponding subnode.
Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
Documentation/devicetree/bindings/mfd/ti,twl.yaml | 40 ++++++++++++++++++-----
1 file changed, 32 insertions(+), 8 deletions(-)
diff --git a/Documentation/devicetree/bindings/mfd/ti,twl.yaml b/Documentation/devicetree/bindings/mfd/ti,twl.yaml
index 776b04e182cb2..3527fee32cb07 100644
--- a/Documentation/devicetree/bindings/mfd/ti,twl.yaml
+++ b/Documentation/devicetree/bindings/mfd/ti,twl.yaml
@@ -55,6 +55,15 @@ allOf:
gpadc: false
+ pwrbutton:
+ properties:
+ compatible:
+ const: ti,twl4030-pwrbutton
+ interrupts:
+ items:
+ - items:
+ const: 8
+
usb-comparator: false
- if:
@@ -95,7 +104,14 @@ allOf:
compatible:
const: ti,twl6030-gpadc
- pwrbutton: false
+ pwrbutton:
+ properties:
+ compatible:
+ const: ti,twl6030-pwrbutton
+ interrupts:
+ items:
+ - items:
+ const: 0
madc: false
@@ -146,7 +162,14 @@ allOf:
compatible:
const: ti,twl6032-gpadc
- pwrbutton: false
+ pwrbutton:
+ properties:
+ compatible:
+ const: ti,twl6030-pwrbutton
+ interrupts:
+ items:
+ - items:
+ const: 0
madc: false
@@ -225,12 +248,8 @@ properties:
additionalProperties: false
properties:
- compatible:
- const: ti,twl4030-pwrbutton
- interrupts:
- items:
- - items:
- const: 8
+ compatible: true
+ interrupts: true
watchdog:
type: object
@@ -459,6 +478,11 @@ examples:
#io-channel-cells = <1>;
};
+ pwrbutton {
+ compatible = "ti,twl6030-pwrbutton";
+ interrupts = <0>;
+ };
+
rtc {
compatible = "ti,twl4030-rtc";
interrupts = <8>;
--
2.47.3On 20/10/2025 14:31, akemnade@kernel.org wrote: > From: Andreas Kemnade <andreas@kemnade.info> > > TWL603x has also a power button, so add the corresponding subnode. No, we don't add subnodes just because there is a power button. This needs broader explanation, see also my further comment. > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > --- > Documentation/devicetree/bindings/mfd/ti,twl.yaml | 40 ++++++++++++++++++----- > 1 file changed, 32 insertions(+), 8 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mfd/ti,twl.yaml b/Documentation/devicetree/bindings/mfd/ti,twl.yaml > index 776b04e182cb2..3527fee32cb07 100644 > --- a/Documentation/devicetree/bindings/mfd/ti,twl.yaml > +++ b/Documentation/devicetree/bindings/mfd/ti,twl.yaml > @@ -55,6 +55,15 @@ allOf: > > gpadc: false > > + pwrbutton: > + properties: > + compatible: > + const: ti,twl4030-pwrbutton > + interrupts: > + items: > + - items: > + const: 8 What is the point of defining const interrupts? If they are const, then it is implied by compatible and defined in the driver. Anyway, double items does not look right here. This is an odd syntax. > + > usb-comparator: false > > - if: > @@ -95,7 +104,14 @@ allOf: > compatible: > const: ti,twl6030-gpadc > > - pwrbutton: false > + pwrbutton: > + properties: > + compatible: > + const: ti,twl6030-pwrbutton > + interrupts: > + items: > + - items: > + const: 0 So everywhere interrupt is defined by parent compatible. BTW, you do not have any resources here, so the child node should be folded into the parent. Best regards, Krzysztof
On Tue, 21 Oct 2025 09:10:28 +0200
Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On 20/10/2025 14:31, akemnade@kernel.org wrote:
> > From: Andreas Kemnade <andreas@kemnade.info>
> >
> > TWL603x has also a power button, so add the corresponding subnode.
>
> No, we don't add subnodes just because there is a power button. This
> needs broader explanation, see also my further comment.
>
Hmm, what is the general pattern to follow if a mfd device has some
functionality which depends on some optional external components?
The might be a power button connected to it or not. I find it ugly
to have non-existent stuff in the system.
In general, yes I understand the argument against the subnode.
> >
> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > ---
> > Documentation/devicetree/bindings/mfd/ti,twl.yaml | 40 ++++++++++++++++++-----
> > 1 file changed, 32 insertions(+), 8 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/ti,twl.yaml b/Documentation/devicetree/bindings/mfd/ti,twl.yaml
> > index 776b04e182cb2..3527fee32cb07 100644
> > --- a/Documentation/devicetree/bindings/mfd/ti,twl.yaml
> > +++ b/Documentation/devicetree/bindings/mfd/ti,twl.yaml
> > @@ -55,6 +55,15 @@ allOf:
> >
> > gpadc: false
> >
> > + pwrbutton:
> > + properties:
> > + compatible:
> > + const: ti,twl4030-pwrbutton
> > + interrupts:
> > + items:
> > + - items:
> > + const: 8
>
> What is the point of defining const interrupts? If they are const, then
> it is implied by compatible and defined in the driver.
>
> Anyway, double items does not look right here. This is an odd syntax.
>
Quoting Rob:
As 'interrupts' is a matrix, this needs to be:
interrupts:
items:
- items:
- const: 8
https://lore.kernel.org/linux-omap/20240318150750.GA4000895-robh@kernel.org/
Regards,
Andreas
On 21/10/2025 10:45, Andreas Kemnade wrote: > On Tue, 21 Oct 2025 09:10:28 +0200 > Krzysztof Kozlowski <krzk@kernel.org> wrote: > >> On 20/10/2025 14:31, akemnade@kernel.org wrote: >>> From: Andreas Kemnade <andreas@kemnade.info> >>> >>> TWL603x has also a power button, so add the corresponding subnode. >> >> No, we don't add subnodes just because there is a power button. This >> needs broader explanation, see also my further comment. >> > Hmm, what is the general pattern to follow if a mfd device has some > functionality which depends on some optional external components? Please describe it better - how these nodes depend on external component? The power button logic/IC is in this device always. It is not optional. > The might be a power button connected to it or not. I find it ugly > to have non-existent stuff in the system. > In general, yes I understand the argument against the subnode. > >>> >>> Signed-off-by: Andreas Kemnade <andreas@kemnade.info> >>> --- >>> Documentation/devicetree/bindings/mfd/ti,twl.yaml | 40 ++++++++++++++++++----- >>> 1 file changed, 32 insertions(+), 8 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/mfd/ti,twl.yaml b/Documentation/devicetree/bindings/mfd/ti,twl.yaml >>> index 776b04e182cb2..3527fee32cb07 100644 >>> --- a/Documentation/devicetree/bindings/mfd/ti,twl.yaml >>> +++ b/Documentation/devicetree/bindings/mfd/ti,twl.yaml >>> @@ -55,6 +55,15 @@ allOf: >>> >>> gpadc: false >>> >>> + pwrbutton: >>> + properties: >>> + compatible: >>> + const: ti,twl4030-pwrbutton >>> + interrupts: >>> + items: >>> + - items: >>> + const: 8 >> >> What is the point of defining const interrupts? If they are const, then >> it is implied by compatible and defined in the driver. >> >> Anyway, double items does not look right here. This is an odd syntax. >> > Quoting Rob: > As 'interrupts' is a matrix, this needs to be: > > interrupts: > items: > - items: > - const: 8 > > https://lore.kernel.org/linux-omap/20240318150750.GA4000895-robh@kernel.org/ OK, this answers second part but I don't understand why even having this in DT. If this is fixed, should be implied by the compatible? Best regards, Krzysztof
On Tue, 21 Oct 2025 11:58:49 +0200 Krzysztof Kozlowski <krzk@kernel.org> wrote: > On 21/10/2025 10:45, Andreas Kemnade wrote: > > On Tue, 21 Oct 2025 09:10:28 +0200 > > Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > >> On 20/10/2025 14:31, akemnade@kernel.org wrote: > >>> From: Andreas Kemnade <andreas@kemnade.info> > >>> > >>> TWL603x has also a power button, so add the corresponding subnode. > >> > >> No, we don't add subnodes just because there is a power button. This > >> needs broader explanation, see also my further comment. > >> > > Hmm, what is the general pattern to follow if a mfd device has some > > functionality which depends on some optional external components? > > Please describe it better - how these nodes depend on external > component? The power button logic/IC is in this device always. It is not > optional. > The power button logic is always there, yes, but it depends on an optional actual mechanical button connected to a pad of this device, which is not always there. The logic will not work if I just put my finger on the PMIC, but it will work if there is a mechanical button which I can press connected to the PMIC. > > The might be a power button connected to it or not. I find it ugly > > to have non-existent stuff in the system. > > In general, yes I understand the argument against the subnode. > > > >>> > >>> Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > >>> --- > >>> Documentation/devicetree/bindings/mfd/ti,twl.yaml | 40 ++++++++++++++++++----- > >>> 1 file changed, 32 insertions(+), 8 deletions(-) > >>> > >>> diff --git a/Documentation/devicetree/bindings/mfd/ti,twl.yaml b/Documentation/devicetree/bindings/mfd/ti,twl.yaml > >>> index 776b04e182cb2..3527fee32cb07 100644 > >>> --- a/Documentation/devicetree/bindings/mfd/ti,twl.yaml > >>> +++ b/Documentation/devicetree/bindings/mfd/ti,twl.yaml > >>> @@ -55,6 +55,15 @@ allOf: > >>> > >>> gpadc: false > >>> > >>> + pwrbutton: > >>> + properties: > >>> + compatible: > >>> + const: ti,twl4030-pwrbutton > >>> + interrupts: > >>> + items: > >>> + - items: > >>> + const: 8 > >> > >> What is the point of defining const interrupts? If they are const, then > >> it is implied by compatible and defined in the driver. > >> > >> Anyway, double items does not look right here. This is an odd syntax. > >> > > Quoting Rob: > > As 'interrupts' is a matrix, this needs to be: > > > > interrupts: > > items: > > - items: > > - const: 8 > > > > https://lore.kernel.org/linux-omap/20240318150750.GA4000895-robh@kernel.org/ > > > OK, this answers second part but I don't understand why even having this > in DT. If this is fixed, should be implied by the compatible? > correct, they do not need to come from DT. The same is true for all subnodes of the twl[46]03X. I just followed the usual pattern there, which is of course not recommended for new designs. Regards, Andreas
On 21/10/2025 18:36, Andreas Kemnade wrote: > On Tue, 21 Oct 2025 11:58:49 +0200 > Krzysztof Kozlowski <krzk@kernel.org> wrote: > >> On 21/10/2025 10:45, Andreas Kemnade wrote: >>> On Tue, 21 Oct 2025 09:10:28 +0200 >>> Krzysztof Kozlowski <krzk@kernel.org> wrote: >>> >>>> On 20/10/2025 14:31, akemnade@kernel.org wrote: >>>>> From: Andreas Kemnade <andreas@kemnade.info> >>>>> >>>>> TWL603x has also a power button, so add the corresponding subnode. >>>> >>>> No, we don't add subnodes just because there is a power button. This >>>> needs broader explanation, see also my further comment. >>>> >>> Hmm, what is the general pattern to follow if a mfd device has some >>> functionality which depends on some optional external components? >> >> Please describe it better - how these nodes depend on external >> component? The power button logic/IC is in this device always. It is not >> optional. >> > The power button logic is always there, yes, but it depends on an optional > actual mechanical button connected to a pad of this device, which is > not always there. The logic will not work if I just put my finger on the PMIC, > but it will work if there is a mechanical button which I can press connected to > the PMIC. Hm... how do you represent this logic now? By adding status=disabled to the pwrbutton node? > >>> The might be a power button connected to it or not. I find it ugly >>> to have non-existent stuff in the system. >>> In general, yes I understand the argument against the subnode. >>> >>>>> >>>>> Signed-off-by: Andreas Kemnade <andreas@kemnade.info> >>>>> --- >>>>> Documentation/devicetree/bindings/mfd/ti,twl.yaml | 40 ++++++++++++++++++----- >>>>> 1 file changed, 32 insertions(+), 8 deletions(-) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/mfd/ti,twl.yaml b/Documentation/devicetree/bindings/mfd/ti,twl.yaml >>>>> index 776b04e182cb2..3527fee32cb07 100644 >>>>> --- a/Documentation/devicetree/bindings/mfd/ti,twl.yaml >>>>> +++ b/Documentation/devicetree/bindings/mfd/ti,twl.yaml >>>>> @@ -55,6 +55,15 @@ allOf: >>>>> >>>>> gpadc: false >>>>> >>>>> + pwrbutton: >>>>> + properties: >>>>> + compatible: >>>>> + const: ti,twl4030-pwrbutton >>>>> + interrupts: >>>>> + items: >>>>> + - items: >>>>> + const: 8 >>>> >>>> What is the point of defining const interrupts? If they are const, then >>>> it is implied by compatible and defined in the driver. >>>> >>>> Anyway, double items does not look right here. This is an odd syntax. >>>> >>> Quoting Rob: >>> As 'interrupts' is a matrix, this needs to be: >>> >>> interrupts: >>> items: >>> - items: >>> - const: 8 >>> >>> https://lore.kernel.org/linux-omap/20240318150750.GA4000895-robh@kernel.org/ >> >> >> OK, this answers second part but I don't understand why even having this >> in DT. If this is fixed, should be implied by the compatible? >> > correct, they do not need to come from DT. The same is true for all > subnodes of the twl[46]03X. I just followed the usual > pattern there, which is of course not recommended for new designs. OK, please mention the reasons (so following established pattern for this legacy device) in commit msg. Best regards, Krzysztof
On Tue, 21 Oct 2025 19:18:25 +0200 Krzysztof Kozlowski <krzk@kernel.org> wrote: > On 21/10/2025 18:36, Andreas Kemnade wrote: > > On Tue, 21 Oct 2025 11:58:49 +0200 > > Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > >> On 21/10/2025 10:45, Andreas Kemnade wrote: > >>> On Tue, 21 Oct 2025 09:10:28 +0200 > >>> Krzysztof Kozlowski <krzk@kernel.org> wrote: > >>> > >>>> On 20/10/2025 14:31, akemnade@kernel.org wrote: > >>>>> From: Andreas Kemnade <andreas@kemnade.info> > >>>>> > >>>>> TWL603x has also a power button, so add the corresponding subnode. > >>>> > >>>> No, we don't add subnodes just because there is a power button. This > >>>> needs broader explanation, see also my further comment. > >>>> > >>> Hmm, what is the general pattern to follow if a mfd device has some > >>> functionality which depends on some optional external components? > >> > >> Please describe it better - how these nodes depend on external > >> component? The power button logic/IC is in this device always. It is not > >> optional. > >> > > The power button logic is always there, yes, but it depends on an optional > > actual mechanical button connected to a pad of this device, which is > > not always there. The logic will not work if I just put my finger on the PMIC, > > but it will work if there is a mechanical button which I can press connected to > > the PMIC. > > > Hm... how do you represent this logic now? By adding status=disabled to > the pwrbutton node? > Yes, or by simply not adding tho pwrbutton node at all. Well, if we break the legacy pattern here, we can probably add a property for this. Regards, Andreas
© 2016 - 2026 Red Hat, Inc.