Messages in this thread | | | From | Peter Geis <> | Date | Fri, 17 Dec 2021 13:16:05 -0500 | Subject | Re: [PATCH v2] mfd: rk808: add reboot support to rk808.c |
| |
On Fri, Dec 17, 2021 at 10:29 AM Dmitry Osipenko <digetx@gmail.com> wrote: > > 17.12.2021 17:55, Peter Geis пишет: > > This adds reboot support to the rk808o pmic driver and enables it for > > the rk809 and rk817 devices. > > This only enables if the rockchip,system-power-controller flag is set. > > > > Signed-off-by: Peter Geis <pgwipeout@gmail.com> > > Signed-off-by: Frank Wunderlich <frank-w@public-files.de> > > --- > > This patch was created to address issues with psci-reset on rk356x > > chips. Until the rk356x series ATF open source code is released so we > > can fix the issue at the source, this is the only way to ensure reliable > > resetting on devices using these chips. > > > > After testing the rk808 (thanks Robin!), it was found DEV_OFF_RST does > > not reset the PMIC to a power on state. Since the rk805 and rk818 match > > this register layout, I'm removing support for all three in the v2. > > It may be possible to add support to them using an RTC wakeup, but that > > has not been explored and is outside the scope of this patch. > > > > Changelog: > > V2: > > - Squash the patch from Frank Wunderlich for rk809 support. > > - Remove support for the rk805, rk808, and rk818 devices. > > - Only register the reset handler for supported devices. > > - Remove unnecessary dev_err and dev_warn statements. > > - Register the reset handler directly > > > > drivers/mfd/rk808.c | 43 +++++++++++++++++++++++++++++++++++++++ > > include/linux/mfd/rk808.h | 1 + > > 2 files changed, 44 insertions(+) > ... > > +static struct notifier_block rk808_restart_handler = { > > + .notifier_call = rk808_restart_notify, > > + .priority = 255, > > +}; > > Hello! > > Please use the default 128 priority if there are no other conflicting > handlers on this RK.
Unfortunately the psci-reboot handler is set to 129, I'll adjust this to 192 which is in line with other PMIC reboot drivers.
> > > static void rk8xx_shutdown(struct i2c_client *client) > > { > > struct rk808 *rk808 = i2c_get_clientdata(client); > > @@ -727,6 +757,19 @@ 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; > > + > > + switch (rk808->variant) { > > + case RK809_ID: > > + case RK817_ID: > > + ret = register_restart_handler(&rk808_restart_handler); > > There is no corresponding unregister_restart_handler(), which you should > add to rk808_remove(). Otherwise kernel will crash on reboot if you'll > unload this driver module.
Thanks, added!
> > > + break; > > + default: > > + dev_info(&client->dev, "pmic controlled board reset not supported\n"); > > I'd set ret=0 explicitly here. Later on somebody may change the code and > ret won't be zero anymore, this is not an uncommon trouble in kernel.
It took me a moment to see the logic here, but I understand it now.
> > > + break; > > + } > > + > > + if (ret) > > + dev_err(&client->dev, "failed to register restart handler, %d\n", ret); > > >
Thank you for the review.
| |