lkml.org 
[lkml]   [2021]   [Dec]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectAw: Re: [PATCH 1/2] mfd: rk808: add reboot support to rk808 pmic
Date
Hi Robin

thanks for your review

> Gesendet: Donnerstag, 16. Dezember 2021 um 14:17 Uhr
> Von: "Robin Murphy" <robin.murphy@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

\
 
 \ /
  Last update: 2021-12-16 17:36    [W:0.114 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site