Re: [PATCH 1/3] dt-bindings: mfd: twl: enable power button also for twl603x

[Andreas Kemnade <andreas@kemnade.info>]

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