Messages in this thread | | | From | Frank Wunderlich <> | Subject | Aw: Re: [PATCH 1/2] mfd: rk808: add reboot support to rk808 pmic | Date | Thu, 16 Dec 2021 17:36:21 +0100 |
| |
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
| |