| Message ID | 20231203222903.343711-3-andreas@kemnade.info (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mfd: twl: system-power-controller | expand |
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 <andreas@kemnade.info> > --- > 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 >
On Thu, 7 Dec 2023 17:11:55 +0000 Lee Jones <lee@kernel.org> 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 <andreas@kemnade.info> > > --- > > 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
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 */
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 <andreas@kemnade.info> --- drivers/mfd/twl-core.c | 28 ++++++++++++++++++++++++++++ include/linux/mfd/twl.h | 1 + 2 files changed, 29 insertions(+)