Add system-power-controller property in the bindings and
the corresponding implementation and use it where
appropriate.
Not all cases are hit yet, there has probably to be a
separate series after going through with a brush.
Changes in v3:
- twl-core:
- remove repetitive verbose error messages
- placed constants at top part of function
- minor cleanups
Changes in v2:
- add A-By
- fix compiler warning
Andreas Kemnade (6):
dt-bindings: mfd: ti,twl: Document system-power-controller
twl-core: add power off implementation for twl603x
ARM: dts: omap-embt2ws: system-power-controller for bt200
ARM: dts: omap4-panda-common: Enable powering off the device
mfd: twl4030-power: accept standard property for power controller
ARM: dts: omap: gta04: standardize system-power-controller
.../devicetree/bindings/mfd/ti,twl.yaml | 2 ++
arch/arm/boot/dts/ti/omap/omap3-gta04.dtsi | 2 +-
.../boot/dts/ti/omap/omap4-epson-embt2ws.dts | 1 +
.../boot/dts/ti/omap/omap4-panda-common.dtsi | 1 +
drivers/mfd/twl-core.c | 28 +++++++++++++++++++
drivers/mfd/twl4030-power.c | 3 ++
include/linux/mfd/twl.h | 1 +
7 files changed, 37 insertions(+), 1 deletion(-)
--
2.39.2
Add system-power-controller property because these chips
can power off the device.
Signed-off-by: Andreas Kemnade <[email protected]>
Acked-by: Conor Dooley <[email protected]>
---
Documentation/devicetree/bindings/mfd/ti,twl.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/mfd/ti,twl.yaml b/Documentation/devicetree/bindings/mfd/ti,twl.yaml
index c04d57ba22b49..52ed228fb1e7e 100644
--- a/Documentation/devicetree/bindings/mfd/ti,twl.yaml
+++ b/Documentation/devicetree/bindings/mfd/ti,twl.yaml
@@ -34,6 +34,8 @@ properties:
interrupt-controller: true
+ system-power-controller: true
+
"#interrupt-cells":
const: 1
--
2.39.2
If the system-power-controller property is there, enable power off.
Implementation is based on a Linux v3.0 vendor kernel.
Signed-off-by: Andreas Kemnade <[email protected]>
---
drivers/mfd/twl-core.c | 28 ++++++++++++++++++++++++++++
include/linux/mfd/twl.h | 1 +
2 files changed, 29 insertions(+)
diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
index 6e384a79e3418..f3982d18008d1 100644
--- a/drivers/mfd/twl-core.c
+++ b/drivers/mfd/twl-core.c
@@ -124,6 +124,11 @@
#define TWL6030_BASEADD_RSV 0x0000
#define TWL6030_BASEADD_ZERO 0x0000
+/* some fields in TWL6030_PHOENIX_DEV_ON */
+#define TWL6030_APP_DEVOFF BIT(0)
+#define TWL6030_CON_DEVOFF BIT(1)
+#define TWL6030_MOD_DEVOFF BIT(2)
+
/* Few power values */
#define R_CFG_BOOT 0x05
@@ -687,6 +692,20 @@ static void twl_remove(struct i2c_client *client)
twl_priv->ready = false;
}
+static void twl6030_power_off(void)
+{
+ int err;
+ u8 val;
+
+ err = twl_i2c_read_u8(TWL_MODULE_PM_MASTER, &val, TWL6030_PHOENIX_DEV_ON);
+ if (err)
+ return;
+
+ val |= TWL6030_APP_DEVOFF | TWL6030_CON_DEVOFF | TWL6030_MOD_DEVOFF;
+ twl_i2c_write_u8(TWL_MODULE_PM_MASTER, val, TWL6030_PHOENIX_DEV_ON);
+}
+
+
static struct of_dev_auxdata twl_auxdata_lookup[] = {
OF_DEV_AUXDATA("ti,twl4030-gpio", 0, "twl4030-gpio", NULL),
{ /* sentinel */ },
@@ -852,6 +871,15 @@ twl_probe(struct i2c_client *client)
goto free;
}
+ if (twl_class_is_6030()) {
+ if (of_device_is_system_power_controller(node)) {
+ if (!pm_power_off)
+ pm_power_off = twl6030_power_off;
+ else
+ dev_warn(&client->dev, "Poweroff callback already assigned\n");
+ }
+ }
+
status = of_platform_populate(node, NULL, twl_auxdata_lookup,
&client->dev);
diff --git a/include/linux/mfd/twl.h b/include/linux/mfd/twl.h
index c062d91a67d92..85dc406173dba 100644
--- a/include/linux/mfd/twl.h
+++ b/include/linux/mfd/twl.h
@@ -461,6 +461,7 @@ static inline int twl6030_mmc_card_detect(struct device *dev, int slot)
#define TWL4030_PM_MASTER_GLOBAL_TST 0xb6
+#define TWL6030_PHOENIX_DEV_ON 0x06
/*----------------------------------------------------------------------*/
/* Power bus message definitions */
--
2.39.2
Replace TI-specific property by generic one.
Signed-off-by: Andreas Kemnade <[email protected]>
---
cannot be applied independently of the other ones, so maybe simply delay
it.
arch/arm/boot/dts/ti/omap/omap3-gta04.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/ti/omap/omap3-gta04.dtsi b/arch/arm/boot/dts/ti/omap/omap3-gta04.dtsi
index 3661340009e7a..5001c4ea35658 100644
--- a/arch/arm/boot/dts/ti/omap/omap3-gta04.dtsi
+++ b/arch/arm/boot/dts/ti/omap/omap3-gta04.dtsi
@@ -476,6 +476,7 @@ twl: twl@48 {
reg = <0x48>;
interrupts = <7>; /* SYS_NIRQ cascaded to intc */
interrupt-parent = <&intc>;
+ system-power-controller;
clocks = <&hfclk_26m>;
clock-names = "fck";
@@ -490,7 +491,6 @@ codec {
twl_power: power {
compatible = "ti,twl4030-power-idle";
- ti,system-power-controller;
};
};
};
--
2.39.2
Instead of only accepting the ti specific properties accept also
the standard property. For uniformity, search in the parent node
for the tag. The code for powering of is also isolated from the
rest in this file. So it is a pure Linux design decision to put it
here.
Signed-off-by: Andreas Kemnade <[email protected]>
---
drivers/mfd/twl4030-power.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/mfd/twl4030-power.c b/drivers/mfd/twl4030-power.c
index e35b0f788c504..3ef892e63b88f 100644
--- a/drivers/mfd/twl4030-power.c
+++ b/drivers/mfd/twl4030-power.c
@@ -686,6 +686,9 @@ static bool twl4030_power_use_poweroff(const struct twl4030_power_data *pdata,
if (of_property_read_bool(node, "ti,use_poweroff"))
return true;
+ if (of_device_is_system_power_controller(node->parent))
+ return true;
+
return false;
}
--
2.39.2
As the TWL6030 chip is the main power controller here, declare
it as system-power-controller
Signed-off-by: Andreas Kemnade <[email protected]>
---
arch/arm/boot/dts/ti/omap/omap4-panda-common.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/boot/dts/ti/omap/omap4-panda-common.dtsi b/arch/arm/boot/dts/ti/omap/omap4-panda-common.dtsi
index f528511c2537b..97706d6296a68 100644
--- a/arch/arm/boot/dts/ti/omap/omap4-panda-common.dtsi
+++ b/arch/arm/boot/dts/ti/omap/omap4-panda-common.dtsi
@@ -408,6 +408,7 @@ twl: twl@48 {
reg = <0x48>;
/* IRQ# = 7 */
interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>; /* IRQ_SYS_1N cascaded to gic */
+ system-power-controller;
};
twl6040: twl@4b {
--
2.39.2
On Sun, Dec 3, 2023 at 4:29 PM Andreas Kemnade <[email protected]> wrote:
>
> Replace TI-specific property by generic one.
>
> Signed-off-by: Andreas Kemnade <[email protected]>
> ---
> cannot be applied independently of the other ones, so maybe simply delay
> it.
>
> arch/arm/boot/dts/ti/omap/omap3-gta04.dtsi | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/ti/omap/omap3-gta04.dtsi b/arch/arm/boot/dts/ti/omap/omap3-gta04.dtsi
> index 3661340009e7a..5001c4ea35658 100644
> --- a/arch/arm/boot/dts/ti/omap/omap3-gta04.dtsi
> +++ b/arch/arm/boot/dts/ti/omap/omap3-gta04.dtsi
> @@ -476,6 +476,7 @@ twl: twl@48 {
> reg = <0x48>;
> interrupts = <7>; /* SYS_NIRQ cascaded to intc */
> interrupt-parent = <&intc>;
> + system-power-controller;
Could this go into the twl4030.dtsi file so we don't have every omap3
board with this part duplicating this line?
adam
>
> clocks = <&hfclk_26m>;
> clock-names = "fck";
> @@ -490,7 +491,6 @@ codec {
>
> twl_power: power {
> compatible = "ti,twl4030-power-idle";
> - ti,system-power-controller;
> };
> };
> };
> --
> 2.39.2
>
>
On Sun, 3 Dec 2023 16:46:20 -0600
Adam Ford <[email protected]> wrote:
> On Sun, Dec 3, 2023 at 4:29 PM Andreas Kemnade <[email protected]> wrote:
> >
> > Replace TI-specific property by generic one.
> >
> > Signed-off-by: Andreas Kemnade <[email protected]>
> > ---
> > cannot be applied independently of the other ones, so maybe simply delay
> > it.
> >
> > arch/arm/boot/dts/ti/omap/omap3-gta04.dtsi | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/boot/dts/ti/omap/omap3-gta04.dtsi b/arch/arm/boot/dts/ti/omap/omap3-gta04.dtsi
> > index 3661340009e7a..5001c4ea35658 100644
> > --- a/arch/arm/boot/dts/ti/omap/omap3-gta04.dtsi
> > +++ b/arch/arm/boot/dts/ti/omap/omap3-gta04.dtsi
> > @@ -476,6 +476,7 @@ twl: twl@48 {
> > reg = <0x48>;
> > interrupts = <7>; /* SYS_NIRQ cascaded to intc */
> > interrupt-parent = <&intc>;
> > + system-power-controller;
>
> Could this go into the twl4030.dtsi file so we don't have every omap3
> board with this part duplicating this line?
>
Well, that is a board-specific issue, so I do not think it belongs there,
although quite common to have the twl4030 as the system-power-controller.
Regards,
Andreas
* Adam Ford <[email protected]> [231203 22:46]:
> On Sun, Dec 3, 2023 at 4:29 PM Andreas Kemnade <[email protected]> wrote:
> >
> > Replace TI-specific property by generic one.
> >
> > Signed-off-by: Andreas Kemnade <[email protected]>
> > ---
> > cannot be applied independently of the other ones, so maybe simply delay
> > it.
> >
> > arch/arm/boot/dts/ti/omap/omap3-gta04.dtsi | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/boot/dts/ti/omap/omap3-gta04.dtsi b/arch/arm/boot/dts/ti/omap/omap3-gta04.dtsi
> > index 3661340009e7a..5001c4ea35658 100644
> > --- a/arch/arm/boot/dts/ti/omap/omap3-gta04.dtsi
> > +++ b/arch/arm/boot/dts/ti/omap/omap3-gta04.dtsi
> > @@ -476,6 +476,7 @@ twl: twl@48 {
> > reg = <0x48>;
> > interrupts = <7>; /* SYS_NIRQ cascaded to intc */
> > interrupt-parent = <&intc>;
> > + system-power-controller;
>
> Could this go into the twl4030.dtsi file so we don't have every omap3
> board with this part duplicating this line?
Sounds good to me.
Regards,
Tony
On Sun, 03 Dec 2023, Andreas Kemnade wrote:
> If the system-power-controller property is there, enable power off.
> Implementation is based on a Linux v3.0 vendor kernel.
>
> Signed-off-by: Andreas Kemnade <[email protected]>
> ---
> drivers/mfd/twl-core.c | 28 ++++++++++++++++++++++++++++
> include/linux/mfd/twl.h | 1 +
> 2 files changed, 29 insertions(+)
>
> diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
> index 6e384a79e3418..f3982d18008d1 100644
> --- a/drivers/mfd/twl-core.c
> +++ b/drivers/mfd/twl-core.c
> @@ -124,6 +124,11 @@
> #define TWL6030_BASEADD_RSV 0x0000
> #define TWL6030_BASEADD_ZERO 0x0000
>
> +/* some fields in TWL6030_PHOENIX_DEV_ON */
My preference is for proper grammar in comments please.
"Some"
What is TWL6030_PHOENIX_DEV_ON? A register?
> +#define TWL6030_APP_DEVOFF BIT(0)
> +#define TWL6030_CON_DEVOFF BIT(1)
> +#define TWL6030_MOD_DEVOFF BIT(2)
> +
> /* Few power values */
> #define R_CFG_BOOT 0x05
>
> @@ -687,6 +692,20 @@ static void twl_remove(struct i2c_client *client)
> twl_priv->ready = false;
> }
>
> +static void twl6030_power_off(void)
> +{
> + int err;
> + u8 val;
> +
> + err = twl_i2c_read_u8(TWL_MODULE_PM_MASTER, &val, TWL6030_PHOENIX_DEV_ON);
> + if (err)
> + return;
> +
> + val |= TWL6030_APP_DEVOFF | TWL6030_CON_DEVOFF | TWL6030_MOD_DEVOFF;
> + twl_i2c_write_u8(TWL_MODULE_PM_MASTER, val, TWL6030_PHOENIX_DEV_ON);
> +}
> +
> +
> static struct of_dev_auxdata twl_auxdata_lookup[] = {
> OF_DEV_AUXDATA("ti,twl4030-gpio", 0, "twl4030-gpio", NULL),
> { /* sentinel */ },
> @@ -852,6 +871,15 @@ twl_probe(struct i2c_client *client)
> goto free;
> }
>
> + if (twl_class_is_6030()) {
Is this check required?
> + if (of_device_is_system_power_controller(node)) {
Shouldn't this cover it?
> + if (!pm_power_off)
> + pm_power_off = twl6030_power_off;
> + else
> + dev_warn(&client->dev, "Poweroff callback already assigned\n");
Can this happen? Why would anyone care if it did?
> + }
> + }
> +
> status = of_platform_populate(node, NULL, twl_auxdata_lookup,
> &client->dev);
>
> diff --git a/include/linux/mfd/twl.h b/include/linux/mfd/twl.h
> index c062d91a67d92..85dc406173dba 100644
> --- a/include/linux/mfd/twl.h
> +++ b/include/linux/mfd/twl.h
> @@ -461,6 +461,7 @@ static inline int twl6030_mmc_card_detect(struct device *dev, int slot)
>
> #define TWL4030_PM_MASTER_GLOBAL_TST 0xb6
>
> +#define TWL6030_PHOENIX_DEV_ON 0x06
> /*----------------------------------------------------------------------*/
>
> /* Power bus message definitions */
> --
> 2.39.2
>
--
Lee Jones [李琼斯]
On Sun, 03 Dec 2023, Andreas Kemnade wrote:
> Instead of only accepting the ti specific properties accept also
> the standard property. For uniformity, search in the parent node
Search 'in' the parent node or 'from' the parent node?
Where is the property?
> for the tag. The code for powering of is also isolated from the
Should this be "off"?
> rest in this file. So it is a pure Linux design decision to put it
> here.
>
> Signed-off-by: Andreas Kemnade <[email protected]>
> ---
> drivers/mfd/twl4030-power.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/mfd/twl4030-power.c b/drivers/mfd/twl4030-power.c
> index e35b0f788c504..3ef892e63b88f 100644
> --- a/drivers/mfd/twl4030-power.c
> +++ b/drivers/mfd/twl4030-power.c
> @@ -686,6 +686,9 @@ static bool twl4030_power_use_poweroff(const struct twl4030_power_data *pdata,
> if (of_property_read_bool(node, "ti,use_poweroff"))
> return true;
>
> + if (of_device_is_system_power_controller(node->parent))
> + return true;
> +
> return false;
> }
>
> --
> 2.39.2
>
--
Lee Jones [李琼斯]
On Thu, 7 Dec 2023 17:11:55 +0000
Lee Jones <[email protected]> wrote:
> On Sun, 03 Dec 2023, Andreas Kemnade wrote:
>
> > If the system-power-controller property is there, enable power off.
> > Implementation is based on a Linux v3.0 vendor kernel.
> >
> > Signed-off-by: Andreas Kemnade <[email protected]>
> > ---
> > drivers/mfd/twl-core.c | 28 ++++++++++++++++++++++++++++
> > include/linux/mfd/twl.h | 1 +
> > 2 files changed, 29 insertions(+)
> >
> > diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
> > index 6e384a79e3418..f3982d18008d1 100644
> > --- a/drivers/mfd/twl-core.c
> > +++ b/drivers/mfd/twl-core.c
> > @@ -124,6 +124,11 @@
> > #define TWL6030_BASEADD_RSV 0x0000
> > #define TWL6030_BASEADD_ZERO 0x0000
> >
> > +/* some fields in TWL6030_PHOENIX_DEV_ON */
>
> My preference is for proper grammar in comments please.
>
> "Some"
>
> What is TWL6030_PHOENIX_DEV_ON? A register?
>
yes, a register.
> > +#define TWL6030_APP_DEVOFF BIT(0)
> > +#define TWL6030_CON_DEVOFF BIT(1)
> > +#define TWL6030_MOD_DEVOFF BIT(2)
> > +
> > /* Few power values */
> > #define R_CFG_BOOT 0x05
> >
> > @@ -687,6 +692,20 @@ static void twl_remove(struct i2c_client *client)
> > twl_priv->ready = false;
> > }
> >
> > +static void twl6030_power_off(void)
> > +{
> > + int err;
> > + u8 val;
> > +
> > + err = twl_i2c_read_u8(TWL_MODULE_PM_MASTER, &val, TWL6030_PHOENIX_DEV_ON);
> > + if (err)
> > + return;
> > +
> > + val |= TWL6030_APP_DEVOFF | TWL6030_CON_DEVOFF | TWL6030_MOD_DEVOFF;
> > + twl_i2c_write_u8(TWL_MODULE_PM_MASTER, val, TWL6030_PHOENIX_DEV_ON);
> > +}
> > +
> > +
> > static struct of_dev_auxdata twl_auxdata_lookup[] = {
> > OF_DEV_AUXDATA("ti,twl4030-gpio", 0, "twl4030-gpio", NULL),
> > { /* sentinel */ },
> > @@ -852,6 +871,15 @@ twl_probe(struct i2c_client *client)
> > goto free;
> > }
> >
> > + if (twl_class_is_6030()) {
>
> Is this check required?
>
Well, as we want to do something specific to 603x we need the check.
> > + if (of_device_is_system_power_controller(node)) {
>
> Shouldn't this cover it?
>
Well, in the twl4030 case there is another power off routine required.
> > + if (!pm_power_off)
> > + pm_power_off = twl6030_power_off;
> > + else
> > + dev_warn(&client->dev, "Poweroff callback already assigned\n");
>
> Can this happen? Why would anyone care if it did?
>
If system is correctly configured, then not. Well, I do not know about others
but I personally expect my devices to turn on after having them turned off
without significant battery loss and really turn off.
But if it is not the case, then having such warning messages would be an
early indication that something is really wrong.
I personally have been in a situation where two devices wanted to register
a power off handler. Order of device probing is random, so having such a
message is a real good idea I think.
Regards,
Andreas
On Thu, 7 Dec 2023 17:13:41 +0000
Lee Jones <[email protected]> wrote:
> On Sun, 03 Dec 2023, Andreas Kemnade wrote:
>
> > Instead of only accepting the ti specific properties accept also
> > the standard property. For uniformity, search in the parent node
>
> Search 'in' the parent node or 'from' the parent node?
>
> Where is the property?
>
The idea was to have the device tree property at the same place for all
twl family devices. So no distinction for these devices needed
in the bindings. It is in the main node.
I guess today this twl4030-power subnode would not be accepted nowadays
and the compatible would be some kind of flag in the parent.
> > for the tag. The code for powering of is also isolated from the
>
> Should this be "off"?
>
yes, of course.
Regards,
Andreas