Re: [PATCH v3 2/6] twl-core: add power off implementation for twl603x

[Andreas Kemnade <andreas@kemnade.info>]

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