[PATCH 0/3] mfd: twl603x: add power button

[PATCH 0/3] mfd: twl603x: add power button

[akemnade]

Like the TWL4030, the TWL603x also has a power button feature, so add
a driver for it.

Signed-off-by: Andreas Kemnade <akemnade@kernel.org>
---
Andreas Kemnade (3):
      dt-bindings: mfd: twl: enable power button also for twl603x
      Input: add TWL603x power button
      ARM: dts: ti/omap: omap4-epson-embt2ws: add powerbutton

 Documentation/devicetree/bindings/mfd/ti,twl.yaml |  40 ++++++--
 arch/arm/boot/dts/ti/omap/omap4-epson-embt2ws.dts |   5 +
 drivers/input/misc/Kconfig                        |  10 ++
 drivers/input/misc/Makefile                       |   1 +
 drivers/input/misc/twl6030-pwrbutton.c            | 111 ++++++++++++++++++++++
 5 files changed, 159 insertions(+), 8 deletions(-)
---
base-commit: 3a8660878839faadb4f1a6dd72c3179c1df56787
change-id: 20251020-twl6030-button-83d759b060e6

Best regards,
--  
Andreas Kemnade <akemnade@kernel.org>

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

[akemnade]

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.3

[PATCH 2/3] Input: add TWL603x power button

[akemnade]

From: Andreas Kemnade <andreas@kemnade.info>

Like the TWL4030, these PMICs also have a power button feature, so add
a driver for it.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 drivers/input/misc/Kconfig             |  10 +++
 drivers/input/misc/Makefile            |   1 +
 drivers/input/misc/twl6030-pwrbutton.c | 111 +++++++++++++++++++++++++++++++++
 3 files changed, 122 insertions(+)

diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index cc2558630797e..5e2d1a5116502 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -569,6 +569,16 @@ config INPUT_TWL4030_VIBRA
 	  To compile this driver as a module, choose M here. The module will
 	  be called twl4030_vibra.
 
+config INPUT_TWL6030_PWRBUTTON
+	tristate "TWL6030 Power button Driver"
+	depends on TWL4030_CORE
+	help
+	  Say Y here if you want to enable power key reporting via the
+	  TWL6030 family of chips.
+
+	  To compile this driver as a module, choose M here. The module will
+	  be called twl4030_pwrbutton.
+
 config INPUT_TWL6040_VIBRA
 	tristate "Support for TWL6040 Vibrator"
 	depends on TWL6040_CORE
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index f5ebfa9d99831..596c013261f44 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -88,6 +88,7 @@ obj-$(CONFIG_INPUT_TPS65219_PWRBUTTON)	+= tps65219-pwrbutton.o
 obj-$(CONFIG_INPUT_TPS6594_PWRBUTTON)	+= tps6594-pwrbutton.o
 obj-$(CONFIG_INPUT_TWL4030_PWRBUTTON)	+= twl4030-pwrbutton.o
 obj-$(CONFIG_INPUT_TWL4030_VIBRA)	+= twl4030-vibra.o
+obj-$(CONFIG_INPUT_TWL6030_PWRBUTTON)	+= twl6030-pwrbutton.o
 obj-$(CONFIG_INPUT_TWL6040_VIBRA)	+= twl6040-vibra.o
 obj-$(CONFIG_INPUT_UINPUT)		+= uinput.o
 obj-$(CONFIG_INPUT_WISTRON_BTNS)	+= wistron_btns.o
diff --git a/drivers/input/misc/twl6030-pwrbutton.c b/drivers/input/misc/twl6030-pwrbutton.c
new file mode 100644
index 0000000000000..fb3d8dcc9088a
--- /dev/null
+++ b/drivers/input/misc/twl6030-pwrbutton.c
@@ -0,0 +1,111 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/**
+ * TWL603x power button input driver
+ *
+ * Copyright (C) 2024 Andreas Kemnade <andreas@kemnade.info>
+ *
+ * based on older 6030 driver found in a v3.0 vendor kernel
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/input.h>
+#include <linux/bits.h>
+#include <linux/interrupt.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/mfd/twl.h>
+
+#define PWR_PWRON_IRQ BIT(0)
+
+#define STS_HW_CONDITIONS 0x2
+
+static irqreturn_t powerbutton_irq(int irq, void *_pwr)
+{
+	struct input_dev *pwr = _pwr;
+	int err;
+	u8 value;
+
+	err = twl_i2c_read_u8(TWL_MODULE_PM_MASTER, &value, STS_HW_CONDITIONS);
+	if (err)
+		return IRQ_HANDLED;
+
+	pm_wakeup_event(pwr->dev.parent, 0);
+	input_report_key(pwr, KEY_POWER, value & PWR_PWRON_IRQ);
+	input_sync(pwr);
+
+	return IRQ_HANDLED;
+}
+
+static int twl6030_pwrbutton_probe(struct platform_device *pdev)
+{
+	struct input_dev *pwr;
+	int irq = platform_get_irq(pdev, 0);
+	int err;
+
+	pwr = devm_input_allocate_device(&pdev->dev);
+	if (!pwr) {
+		dev_err(&pdev->dev, "Can't allocate power button\n");
+		return -ENOMEM;
+	}
+
+	input_set_capability(pwr, EV_KEY, KEY_POWER);
+	pwr->name = "twl6030_pwrbutton";
+	pwr->phys = "twl6030_pwrbutton/input0";
+	pwr->dev.parent = &pdev->dev;
+
+	err = devm_request_threaded_irq(&pdev->dev, irq, NULL, powerbutton_irq,
+			IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING |
+			IRQF_ONESHOT,
+			"twl6030_pwrbutton", pwr);
+	if (err < 0) {
+		dev_err(&pdev->dev, "Can't get IRQ for pwrbutton: %d\n", err);
+		return err;
+	}
+
+	err = input_register_device(pwr);
+	if (err) {
+		dev_err(&pdev->dev, "Can't register power button: %d\n", err);
+		return err;
+	}
+
+	err = twl6030_interrupt_unmask(0x01, REG_INT_MSK_LINE_A);
+	if (err < 0)
+		return err;
+
+	err = twl6030_interrupt_unmask(0x01, REG_INT_MSK_STS_A);
+	if (err < 0)
+		return err;
+
+	device_init_wakeup(&pdev->dev, true);
+
+	return 0;
+}
+
+static void twl6030_pwrbutton_remove(struct platform_device *pdev)
+{
+	twl6030_interrupt_mask(0x01, REG_INT_MSK_LINE_A);
+	twl6030_interrupt_mask(0x01, REG_INT_MSK_STS_A);
+}
+
+static const struct of_device_id twl6030_pwrbutton_dt_match_table[] = {
+	{ .compatible = "ti,twl6030-pwrbutton" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, twl6030_pwrbutton_dt_match_table);
+
+static struct platform_driver twl6030_pwrbutton_driver = {
+	.probe		= twl6030_pwrbutton_probe,
+	.remove		= twl6030_pwrbutton_remove,
+	.driver		= {
+		.name	= "twl6030_pwrbutton",
+		.of_match_table = of_match_ptr(twl6030_pwrbutton_dt_match_table),
+	},
+};
+module_platform_driver(twl6030_pwrbutton_driver);
+
+MODULE_ALIAS("platform:twl6030_pwrbutton");
+MODULE_DESCRIPTION("Phoenix Power Button");
+MODULE_LICENSE("GPL");

-- 
2.47.3

[PATCH 3/3] ARM: dts: ti/omap: omap4-epson-embt2ws: add powerbutton

[akemnade]

From: Andreas Kemnade <andreas@kemnade.info>

There is a power button connected to the PMIC, so describe it to be able
to power off the device in a convenient manner.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 arch/arm/boot/dts/ti/omap/omap4-epson-embt2ws.dts | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/ti/omap/omap4-epson-embt2ws.dts b/arch/arm/boot/dts/ti/omap/omap4-epson-embt2ws.dts
index c90f43cc2fae9..673df1b693f28 100644
--- a/arch/arm/boot/dts/ti/omap/omap4-epson-embt2ws.dts
+++ b/arch/arm/boot/dts/ti/omap/omap4-epson-embt2ws.dts
@@ -229,6 +229,11 @@ rtc {
 			interrupts = <11>;
 		};
 
+		pwrbutton {
+			compatible = "ti,twl6030-pwrbutton";
+			interrupts = <0>;
+		};
+
 		ldo2: regulator-ldo2 {
 			compatible = "ti,twl6032-ldo2";
 			regulator-min-microvolt = <1000000>;

-- 
2.47.3

Re: [PATCH 2/3] Input: add TWL603x power button

[Krzysztof Kozlowski]

On 20/10/2025 14:31, akemnade@kernel.org wrote:
> +static const struct of_device_id twl6030_pwrbutton_dt_match_table[] = {
> +	{ .compatible = "ti,twl6030-pwrbutton" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, twl6030_pwrbutton_dt_match_table);
> +
> +static struct platform_driver twl6030_pwrbutton_driver = {
> +	.probe		= twl6030_pwrbutton_probe,
> +	.remove		= twl6030_pwrbutton_remove,
> +	.driver		= {
> +		.name	= "twl6030_pwrbutton",
> +		.of_match_table = of_match_ptr(twl6030_pwrbutton_dt_match_table),

Drop of match ptr, you have a warning here.

> +	},
> +};
> +module_platform_driver(twl6030_pwrbutton_driver);
> +
> +MODULE_ALIAS("platform:twl6030_pwrbutton");

You should not need MODULE_ALIAS() in normal cases. If you need it,
usually it means your device ID table is wrong (e.g. misses either
entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
for incomplete ID table.


> +MODULE_DESCRIPTION("Phoenix Power Button");
> +MODULE_LICENSE("GPL");
> 


Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

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.

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

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

[Andreas Kemnade]

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

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

[Krzysztof Kozlowski]

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

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

[Andreas Kemnade]

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

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

[Krzysztof Kozlowski]

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

Re: [PATCH 2/3] Input: add TWL603x power button

[Dmitry Torokhov]

On Mon, Oct 20, 2025 at 02:31:59PM +0200, akemnade@kernel.org wrote:
> From: Andreas Kemnade <andreas@kemnade.info>
> 
> Like the TWL4030, these PMICs also have a power button feature, so add
> a driver for it.

Could it be integrated into twl4030-pwrbutton.c? I think the differences
can be accounted for via a "chip" structure attached to a compatible...

Thanks.

-- 
Dmitry

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

[Andreas Kemnade]

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

Re: [PATCH 2/3] Input: add TWL603x power button

[Andreas Kemnade]

On Tue, 21 Oct 2025 10:58:35 -0700
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> On Mon, Oct 20, 2025 at 02:31:59PM +0200, akemnade@kernel.org wrote:
> > From: Andreas Kemnade <andreas@kemnade.info>
> > 
> > Like the TWL4030, these PMICs also have a power button feature, so add
> > a driver for it.  
> 
> Could it be integrated into twl4030-pwrbutton.c? I think the differences
> can be accounted for via a "chip" structure attached to a compatible...
> 
So what is different:
- different register (but same bit)
- some custom irq stuff for 603x (so if (is_603x) needed)
- different name for the button (can be neglected I think) 

Besides of adding a chip structure  we can do it the same way
as rtc-twl.c is doing: using twl_class_is_xxxx() which derives
its return from the parents compatible. It is simplier, but
I think the chip structure does not hurt much either.

Regards,
Andreas

Re: [PATCH 2/3] Input: add TWL603x power button

[Dmitry Torokhov]

On Wed, Oct 22, 2025 at 02:44:22PM +0200, Andreas Kemnade wrote:
> On Tue, 21 Oct 2025 10:58:35 -0700
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
> > On Mon, Oct 20, 2025 at 02:31:59PM +0200, akemnade@kernel.org wrote:
> > > From: Andreas Kemnade <andreas@kemnade.info>
> > > 
> > > Like the TWL4030, these PMICs also have a power button feature, so add
> > > a driver for it.  
> > 
> > Could it be integrated into twl4030-pwrbutton.c? I think the differences
> > can be accounted for via a "chip" structure attached to a compatible...
> > 
> So what is different:
> - different register (but same bit)
> - some custom irq stuff for 603x (so if (is_603x) needed)

Right, why do we need to unmask the interrupt by hand for 6030? I'd
expect this handled in the core, when we request the interrupt, not in
the button driver..in the core, when we request the interrupt, not in
the button driver...

Thanks.

-- 
Dmitry

Re: [PATCH 2/3] Input: add TWL603x power button

[Andreas Kemnade]

On Wed, 22 Oct 2025 11:48:59 -0700
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> On Wed, Oct 22, 2025 at 02:44:22PM +0200, Andreas Kemnade wrote:
> > On Tue, 21 Oct 2025 10:58:35 -0700
> > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> >   
> > > On Mon, Oct 20, 2025 at 02:31:59PM +0200, akemnade@kernel.org wrote:  
> > > > From: Andreas Kemnade <andreas@kemnade.info>
> > > > 
> > > > Like the TWL4030, these PMICs also have a power button feature, so add
> > > > a driver for it.    
> > > 
> > > Could it be integrated into twl4030-pwrbutton.c? I think the differences
> > > can be accounted for via a "chip" structure attached to a compatible...
> > >   
> > So what is different:
> > - different register (but same bit)
> > - some custom irq stuff for 603x (so if (is_603x) needed)  
> 
> Right, why do we need to unmask the interrupt by hand for 6030? I'd
> expect this handled in the core, when we request the interrupt, not in
> the button driver..in the core, when we request the interrupt, not in
> the button driver...
> 
Short answer: irqchip ops do not provide mask/unmask for 6030.

Why... Interrupts are merged. There are 3 irq registers for the whole chip
and some of these 24bits are merged to provide one interrupt per submodule.
Apparently these custom calls are there to enable the merged interrupts
individually or multiple together. That is at least my theory I derived
from my archeology session.

The thing is implemented differently for the twl4030, there you have multiple
irqs per module.

But why two calls here? With one of them the interrupt pad of the pmic reacts
to the interrupt, with the other call, the irq status register does.

So can it all be implemented differently? Probably yes, but things
need to be done carefully or with one cross-subsystem patch changing
everything in lockstep. This somehow itches me for several reasons.

Regards,
Andreas