Re: [PATCH v2] mfd: rk808: add reboot support to rk808.c

[Dmitry Osipenko <digetx@gmail.com>]

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.

>  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.

> +			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.

> +			break;
> +		}
> +
> +		if (ret)
> +			dev_err(&client->dev, "failed to register restart handler, %d\n", ret);