Re: [PATCH] pinctrl/rockchip: Don't call pinctrl_force_* for nothing

[Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>]

Hi Marc,

Am Samstag, 24. Februar 2018, 21:07:31 CET schrieb Marc Zyngier:
> The rockchip pinctl driver calls pinctrl_force_default and
> pinctrl_force_sleep on suspend resume, but seems to expect
> that the outcome of these calls will be that nothing happens,
> as the core code checks whether we're already in the right
> state or not.
> 
> Or at least, that was what the core code was doing until
> 981ed1bfbc ("pinctrl: Really force states during suspend/resume"),
> which gives the "force" qualifier its actual meaning.
> 
> In turn, this breaks suspend/resume on the rk3399. So let's
> change the rockchip code to do what it should have done from
> the very begining, which is exactly *nothing*.
> 
> We take this opportunity to tidy-up the RK3288 GPIO6_C6 mux
> resume workaround, making it symetrical to the suspend path.
> 
> Tested on a rk3399-based kevin Chromebook.
> 
> Fixes: 9198f509c888 ("pinctrl: rockchip: add suspend/resume functions")
> Signed-off-by: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>

Now that I had time to look at this a bit more, I'm not sure if removing
that is the right solution.

The force_sleep and force_suspend functions are meant for the "hog" pins,
meaning the pins that just need to be configured once but are not claimed
by any actual device - see everything named hog_* in the pinctrl core.
And judging by the other users of them, using them like now is the expected
usage.


On the rk3399-gru boards, the hogged pins only have a default state and
no sleep configuration and contain some pins like the ap_pwroff that
sound somewhat strange :-)
So my guess is that now something bad happens now that the state==oldstate
check is gone.

I've included Doug and Brian who have worked on Gru devices to hopefully
shed some light on this?

@Doug,Brian: In a nutshell, the issue with the commit
"pinctrl: Really force states during suspend/resume" [0] is that now it
really forces the state, instead of first checking of newstate == oldstate
and doing nothing in this case. And as Marc reported this breaks resume
on Kevin with 4.16-rc1.

Any idea what would need to change?


Heiko


[0] https://patchwork.kernel.org/patch/9598969/

>  drivers/pinctrl/pinctrl-rockchip.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
> index 3924779f5578..a3a503e684dc 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.c
> +++ b/drivers/pinctrl/pinctrl-rockchip.c
> @@ -3114,22 +3114,18 @@ static u32 rk3288_grf_gpio6c_iomux;
>  static int __maybe_unused rockchip_pinctrl_suspend(struct device *dev)
>  {
>  	struct rockchip_pinctrl *info = dev_get_drvdata(dev);
> -	int ret = pinctrl_force_sleep(info->pctl_dev);
> -
> -	if (ret)
> -		return ret;
>  
>  	/*
>  	 * RK3288 GPIO6_C6 mux would be modified by Maskrom when resume, so save
>  	 * the setting here, and restore it at resume.
>  	 */
>  	if (info->ctrl->type == RK3288) {
> +		int ret;
> +
>  		ret = regmap_read(info->regmap_base, RK3288_GRF_GPIO6C_IOMUX,
>  				  &rk3288_grf_gpio6c_iomux);
> -		if (ret) {
> -			pinctrl_force_default(info->pctl_dev);
> +		if (ret)
>  			return ret;
> -		}
>  	}
>  
>  	return 0;
> @@ -3138,14 +3134,18 @@ static int __maybe_unused rockchip_pinctrl_suspend(struct device *dev)
>  static int __maybe_unused rockchip_pinctrl_resume(struct device *dev)
>  {
>  	struct rockchip_pinctrl *info = dev_get_drvdata(dev);
> -	int ret = regmap_write(info->regmap_base, RK3288_GRF_GPIO6C_IOMUX,
> -			       rk3288_grf_gpio6c_iomux |
> -			       GPIO6C6_SEL_WRITE_ENABLE);
>  
> -	if (ret)
> -		return ret;
> +	if (info->ctrl->type == RK3288) {
> +		int ret;
>  
> -	return pinctrl_force_default(info->pctl_dev);
> +		ret = regmap_write(info->regmap_base, RK3288_GRF_GPIO6C_IOMUX,
> +				   rk3288_grf_gpio6c_iomux |
> +				   GPIO6C6_SEL_WRITE_ENABLE);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
>  }
>  
>  static SIMPLE_DEV_PM_OPS(rockchip_pinctrl_dev_pm_ops, rockchip_pinctrl_suspend,
>