Re: [PATCH 4/4] mfd: rk808: Convert RK805 to syscore/PM ops

From: Anand Moon
Date: Sun Dec 15 2019 - 13:52:10 EST

Hi Robin

On Tue, 10 Dec 2019 at 18:54, Robin Murphy <robin.murphy@xxxxxxx> wrote:
>
> RK805 has the same kind of dual-role sleep/shutdown pin as RK809/RK817,
> so it makes little sense for the driver to have to have two completely
> different mechanisms to handle essentially the same thing. Bring RK805
> in line with the RK809/RK817 flow to clean things up.
>
> Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx>
> ---
> drivers/mfd/rk808.c | 58 +++++++++++++++++----------------------
> include/linux/mfd/rk808.h | 1 -
> 2 files changed, 25 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/mfd/rk808.c b/drivers/mfd/rk808.c
> index 657b8baa3b8a..e88bdb889d3a 100644
> --- a/drivers/mfd/rk808.c
> +++ b/drivers/mfd/rk808.c
> @@ -186,7 +186,6 @@ static const struct rk808_reg_data rk805_pre_init_reg[] = {
> {RK805_BUCK4_CONFIG_REG, RK805_BUCK3_4_ILMAX_MASK,
> RK805_BUCK4_ILMAX_3500MA},
> {RK805_BUCK4_CONFIG_REG, BUCK_ILMIN_MASK, BUCK_ILMIN_400MA},
> - {RK805_GPIO_IO_POL_REG, SLP_SD_MSK, SLEEP_FUN},
> {RK805_THERMAL_REG, TEMP_HOTDIE_MSK, TEMP115C},
> };
>
> @@ -449,21 +448,6 @@ static const struct regmap_irq_chip rk818_irq_chip = {
>
> static struct i2c_client *rk808_i2c_client;
>
> -static void rk805_device_shutdown_prepare(void)
> -{
> - int ret;
> - struct rk808 *rk808 = i2c_get_clientdata(rk808_i2c_client);
> -
> - if (!rk808)
> - return;
> -
> - ret = regmap_update_bits(rk808->regmap,
> - RK805_GPIO_IO_POL_REG,
> - SLP_SD_MSK, SHUTDOWN_FUN);
> - if (ret)
> - dev_err(&rk808_i2c_client->dev, "Failed to shutdown device!\n");
> -}
> -
> static void rk808_device_shutdown(void)
> {
> int ret;
> @@ -499,17 +483,29 @@ static void rk8xx_syscore_shutdown(void)
> struct rk808 *rk808 = i2c_get_clientdata(rk808_i2c_client);
> int ret;
>
> - if (system_state == SYSTEM_POWER_OFF &&
> - (rk808->variant == RK809_ID || rk808->variant == RK817_ID)) {
> + if (system_state != SYSTEM_POWER_OFF)
> + return;
> +
> + switch (rk808->variant) {
> + case RK805_ID:
> + ret = regmap_update_bits(rk808->regmap,
> + RK805_GPIO_IO_POL_REG,
> + SLP_SD_MSK,
> + SHUTDOWN_FUN);
> + break;
> + case RK809_ID:
> + case RK817_ID:
> ret = regmap_update_bits(rk808->regmap,
> RK817_SYS_CFG(3),
> RK817_SLPPIN_FUNC_MSK,
> SLPPIN_DN_FUN);
> - if (ret) {
> - dev_warn(&rk808_i2c_client->dev,
> - "Cannot switch to power down function\n");
> - }
> + break;
> + default:
> + return;
> }
> + if (ret)
> + dev_warn(&rk808_i2c_client->dev,
> + "Cannot switch to power down function\n");
> }
>
> static struct syscore_ops rk808_syscore_ops = {
> @@ -579,7 +575,6 @@ static int rk808_probe(struct i2c_client *client,
> nr_pre_init_regs = ARRAY_SIZE(rk805_pre_init_reg);
> cells = rk805s;
> nr_cells = ARRAY_SIZE(rk805s);
> - rk808->pm_pwroff_prep_fn = rk805_device_shutdown_prepare;
> break;
> case RK808_ID:
> rk808->regmap_cfg = &rk808_regmap_config;
> @@ -658,10 +653,8 @@ static int rk808_probe(struct i2c_client *client,
> goto err_irq;
> }
>
> - if (of_property_read_bool(np, "rockchip,system-power-controller")) {
> + if (of_property_read_bool(np, "rockchip,system-power-controller"))
> pm_power_off = rk808_device_shutdown;
> - pm_power_off_prepare = rk808->pm_pwroff_prep_fn;
> - }
>
> return 0;
>
> @@ -686,13 +679,6 @@ static int rk808_remove(struct i2c_client *client)
> if (pm_power_off == rk808_device_shutdown)
> pm_power_off = NULL;
>
> - /**
> - * As above, check if the pointer is set by us before overwrite.
> - */
> - if (rk808->pm_pwroff_prep_fn &&
> - pm_power_off_prepare == rk808->pm_pwroff_prep_fn)
> - pm_power_off_prepare = NULL;
> -
> return 0;
> }
>
> @@ -702,6 +688,12 @@ static int __maybe_unused rk8xx_suspend(struct device *dev)
> int ret = 0;
>
> switch (rk808->variant) {
> + case RK805_ID:
> + ret = regmap_update_bits(rk808->regmap,
> + RK805_GPIO_IO_POL_REG,
> + SLP_SD_MSK,
> + SLEEP_FUN);
> + break;
> case RK809_ID:
> case RK817_ID:
> ret = regmap_update_bits(rk808->regmap,
> diff --git a/include/linux/mfd/rk808.h b/include/linux/mfd/rk808.h
> index b038653fa87e..e07f6e61cd38 100644
> --- a/include/linux/mfd/rk808.h
> +++ b/include/linux/mfd/rk808.h
> @@ -620,6 +620,5 @@ struct rk808 {
> long variant;
> const struct regmap_config *regmap_cfg;
> const struct regmap_irq_chip *regmap_irq_chip;
> - void (*pm_pwroff_prep_fn)(void);
> };
> #endif /* __LINUX_REGULATOR_RK808_H */
> --
> 2.17.1
>

I am sill getting the kernel warning on issue poweroff see below.
on my Rock960 Model A
I feel the reason for this is we now have two poweroff callback
1 pm_power_off = rk808_device_shutdown
2 rk8xx_syscore_shutdown

In my investigation earlier common function for shutdown solve
the issue of clean shutdown.

for *rockchip,system-power-controller* dts property
we can used flags if check if this property support clean shutdown
for that device.

[ 565.009291] xhci-hcd xhci-hcd.0.auto: USB bus 5 deregistered
[ 565.010179] reboot: Power down
[ 565.010536] ------------[ cut here ]------------
[ 565.010940] No atomic I2C transfer handler for 'i2c-0'
[ 565.011437] WARNING: CPU: 0 PID: 1 at drivers/i2c/i2c-core.h:40
i2c_transfer+0xe4/0xf8
[ 565.012126] Modules linked in: snd_soc_hdmi_codec dw_hdmi_i2s_audio
rockchipdrm nvme analogix_dp nvme_core brcmfmac hci_uart dw_mipi_dsi
dw_hdmi btbcm cec panfrost bluetooth drm_kms_helper brcmutil gpu_sched
cfg80211 crct10dif_ce snd_soc_rockchip_i2s snd_soc_simple_card drm
ecdh_generic snd_soc_rockchip_pcm snd_soc_simple_card_utils
phy_rockchip_pcie ecc rtc_rk808 rfkill rockchip_thermal
pcie_rockchip_host ip_tables x_tables ipv6 nf_defrag_ipv6
[ 565.015578] CPU: 0 PID: 1 Comm: shutdown Not tainted
5.5.0-rc1-00292-gd46dd6369c55 #7
[ 565.016260] Hardware name: 96boards Rock960 (DT)
[ 565.016666] pstate: 60000085 (nZCv daIf -PAN -UAO)
[ 565.017087] pc : i2c_transfer+0xe4/0xf8
[ 565.017425] lr : i2c_transfer+0xe4/0xf8
[ 565.017762] sp : ffff80001004baf0
[ 565.018052] x29: ffff80001004baf0 x28: ffff00007d208000
[ 565.018517] x27: 0000000000000000 x26: 0000000000000000
[ 565.018982] x25: 0000000000000008 x24: 0000000000000000
[ 565.019447] x23: ffff00007d208000 x22: ffff80001004bc64
[ 565.019912] x21: ffff80001004bb48 x20: 0000000000000002
[ 565.020377] x19: ffff000078502080 x18: 0000000000000010
[ 565.020842] x17: 0000000000000001 x16: 0000000000000019
[ 565.021307] x15: ffff00007d208470 x14: ffffffffffffffff
[ 565.021772] x13: ffff80009004b857 x12: ffff80001004b860
[ 565.022237] x11: ffff800011841000 x10: ffff800011a10658
[ 565.022702] x9 : 0000000000000000 x8 : ffff800011a11000
[ 565.023167] x7 : ffff800010697c78 x6 : 0000000000000262
[ 565.023632] x5 : 0000000000000000 x4 : 0000000000000000
[ 565.024096] x3 : 00000000ffffffff x2 : ffff800011841ab8
[ 565.024561] x1 : 7b11701b0ae78800 x0 : 0000000000000000
[ 565.025027] Call trace:
[ 565.025246] i2c_transfer+0xe4/0xf8
[ 565.025556] regmap_i2c_read+0x5c/0xa0
[ 565.025886] _regmap_raw_read+0xcc/0x138
[ 565.026230] _regmap_bus_read+0x3c/0x70
[ 565.026568] _regmap_read+0x60/0xe0
[ 565.026875] _regmap_update_bits+0xc8/0x108
[ 565.027241] regmap_update_bits_base+0x60/0x90
[ 565.027633] rk808_device_shutdown+0x6c/0x88
[ 565.028010] machine_power_off+0x24/0x30
[ 565.028356] kernel_power_off+0x64/0x70
[ 565.028693] __do_sys_reboot+0x15c/0x240
[ 565.029038] __arm64_sys_reboot+0x20/0x28
[ 565.029390] el0_svc_common.constprop.0+0x68/0x160
[ 565.029811] el0_svc_handler+0x20/0x80
[ 565.030141] el0_sync_handler+0x10c/0x180
[ 565.030493] el0_sync+0x140/0x180
[ 565.030785] ---[ end trace 5167e842ce15f686 ]---

-Anand