Aw: Re: [PATCH 1/2] mfd: rk808: add reboot support to rk808 pmic

[Frank Wunderlich]

Hi Robin

thanks for your review

> Gesendet: Donnerstag, 16. Dezember 2021 um 14:17 Uhr
> Von: "Robin Murphy" <robin.murphy at arm.com>

> > +	dev_err(&rk808_i2c_client->dev, "poweroff device!\n");
>
> This is not an error.

ack, we can change to dev_dbg/dev_info or drop completely

> > @@ -552,6 +554,7 @@ static void rk808_pm_power_off(void)
> >   		bit = DEV_OFF;
> >   		break;
> >   	default:
> > +		dev_err(&rk808_i2c_client->dev, "poweroff device not supported!\n");
>
> If we really don't support power off for the present PMIC then we should
> avoid registering the power off handler in the first place. These
> default cases should mostly just serve to keep static checkers happy.

currently we had added all device-ids supported by probe. Before my Patch (Part2) rk809 was missing.
It is not registered for (currently) unsupported IDs as probe exits with -EINVAL for unsupported IDs before.

If anyone adds init-code in future but not poweroff/restart part we ran into this. So i would leave this message.

registering handler depending on supported devices (for poweroff/restart function) imho makes code less readable.

> > @@ -559,6 +562,44 @@ static void rk808_pm_power_off(void)
> >   		dev_err(&rk808_i2c_client->dev, "Failed to shutdown device!\n");
> >   }
> >
> > +static int rk808_restart_notify(struct notifier_block *this, unsigned long mode, void *cmd)
> > +{
> > +	int ret;
> > +	unsigned int reg, bit;
> > +	struct rk808 *rk808 = i2c_get_clientdata(rk808_i2c_client);
> > +
> > +	switch (rk808->variant) {
> > +	case RK805_ID:
> > +		reg = RK805_DEV_CTRL_REG;
> > +		bit = DEV_OFF_RST;
> > +		break;
> > +	case RK808_ID:
> > +		reg = RK808_DEVCTRL_REG,
> > +		bit = DEV_OFF;
>
> Is that right? The RK808 datasheet does say that this bit resets itself
> once the OFF state is reached, but there doesn't seem to be any obvious
> guarantee of a power-on condition being present to automatically
> transition back to ACTIVE.

i think you're right...imho it should be DEV_OFF_RST in restart_notify and DEV_OFF in poweroff

> > @@ -727,6 +768,13 @@ static int rk808_probe(struct i2c_client *client,
> >   	if (of_property_read_bool(np, "rockchip,system-power-controller")) {
> >   		rk808_i2c_client = client;
> >   		pm_power_off = rk808_pm_power_off;
> > +		rk808->nb = &rk808_restart_handler;
>
> The handler just relies on the global rk808_i2c_client anyway, so does
> this serve any purpose?

i guess

ret = register_restart_handler(&rk808_restart_handler);

is better here too.

i cannot figure out why storing the function-pointer in rk808->nb too as this seems not to be used anywhere else

> > +
> > +		dev_warn(&client->dev, "register restart handler\n");
>
> Don't scream a warning about normal and expected operation.

ack

> Thanks,
> Robin.

@Peter, what do you think?

regards Frank