Re: [PATCH v1 1/3] thermal: rockchip: add pinctrl control

[Daniel Lezcano]

Hi Elaine,

On 11/04/2019 09:46, elaine.zhang wrote:
> hi,
> 
> å 2019/4/4 äå11:03, Daniel Lezcano åé:
>> On 01/04/2019 08:43, Elaine Zhang wrote:
>>> Based on the TSADC Tshut mode to select pinctrl,
>>> instead of setting pinctrl based on architecture
>>> (Not depends on pinctrl setting by "init" or "default").
>>> And it requires setting the tshut polarity before select pinctrl.
>> I'm not sure to fully read the description. Can you rephrase/elaborate
>> the changelog?
> Example:
> ÂÂÂÂÂÂÂ tsadc: tsadc@ff250000 {
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ compatible = "rockchip,rk3328-tsadc";
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ .....
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pinctrl-names = "init", "default", "sleep";
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pinctrl-0 = <&otp_gpio>;
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pinctrl-1 = <&otp_out>;
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pinctrl-2 = <&otp_gpio>;
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ status = "disabled";
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ .....
> ÂÂÂÂÂÂÂÂÂÂÂ };
> ÂÂÂÂÂÂÂ &tsadc {
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ rockchip,hw-tshut-mode = <0>; /* tshut mode 0:CRU 1:GPIO */
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ status = "okay";
> ÂÂÂÂÂÂÂ };
> ÂÂÂ Application on the product, hope tsadc overtemperature reset cru to
> restart.
> ÂÂÂ But when pinctrl is initialized, it will setting pinctrl by "init"
> and "default".
> ÂÂÂ So the pinctrl will set iomux to "otp_out", which may be make system
> crash.

why?

> ÂÂÂ tsadc gpio iomux:
> ÂÂÂ "otp_gpio" : just normal gpio, keep default level.
> ÂÂÂ "otp_out" : tsadc shut down io, when overtemperature,this io may be
> trigger high
> ÂÂÂ level or low level(depend on the tsadc polarity).
> 
> ÂÂÂ After correction:
> ÂÂÂ if rockchip,hw-tshut-mode = <0>; (tsadc overtemperature reset cru to
> restart)
> ÂÂÂ select pinctrl to otp_gpio
> ÂÂÂ if rockchip,hw-tshut-mode = <1>;(tsadc overtemperature IO triggers
> output at high or low levels)
> ÂÂÂ select pinctrl to otp_out.
> ÂÂÂ Actively select iomux based on rockchip,hw-tshut-mode,
> ÂÂÂ rather than relying on the pinctrl framework to select iomux.

Let me rephrase it and tell me if I understood correctly:

"When the temperature sensor mode is set to 0, it will automatically
reset the board via the Clock-Reset-Unit (CRU) if the over temperature
threshold is reached. However, when the pinctrl initializes, it does a
transition to "otp_out" which may lead the SoC to crash (why?)

Explicitly use the pinctrl to set/unset the right mode instead of
relying on the pinctrl init mode"

So this patch is a fix and it must contain the Fixes: ... tag.


>>> Signed-off-by: Elaine Zhang <zhangqing@xxxxxxxxxxxxxx>
>>> ---
>>> Â drivers/thermal/rockchip_thermal.c | 61
>>> +++++++++++++++++++++++++++++++-------
>>> Â 1 file changed, 50 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/thermal/rockchip_thermal.c
>>> b/drivers/thermal/rockchip_thermal.c
>>> index 9c7643d62ed7..faa6c7792155 100644
>>> --- a/drivers/thermal/rockchip_thermal.c
>>> +++ b/drivers/thermal/rockchip_thermal.c
>>> @@ -34,7 +34,7 @@
>>> ÂÂ */
>>> Â enum tshut_mode {
>>> ÂÂÂÂÂ TSHUT_MODE_CRU = 0,
>>> -ÂÂÂ TSHUT_MODE_GPIO,
>>> +ÂÂÂ TSHUT_MODE_OTP,
>> Why do you change the enum name? The impact on the patch is much higher,
>> no ?
> 
> I just want to make it a little bit more intuitive to understand the
> definition of mode.
> 
> TSHUT_MODE_CRU: pinctrl select iomux to otp_gpio,the io is normal io.
> TSHUT_MODE_OTP: pinctrl select iomux to otp_out, the io is tsadc shut io.

I understand, but at the end the changes for this patch are counter
intuitive, so it is preferable to keep it as is so we can review the
important part.

[ ... ]

>>> Â +static void thermal_pinctrl_select_otp(struct
>>> rockchip_thermal_data *thermal)
>>> +{
>>> +ÂÂÂ if (!IS_ERR(thermal->pinctrl) &&
>>> !IS_ERR_OR_NULL(thermal->otp_state))
>>> +ÂÂÂÂÂÂÂ pinctrl_select_state(thermal->pinctrl,
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ thermal->otp_state);
>>> +}
>>> +
>>> +static void thermal_pinctrl_select_gpio(struct rockchip_thermal_data
>>> *thermal)
>>> +{
>>> +ÂÂÂ if (!IS_ERR(thermal->pinctrl) &&
>>> !IS_ERR_OR_NULL(thermal->gpio_state))
>>> +ÂÂÂÂÂÂÂ pinctrl_select_state(thermal->pinctrl,
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ thermal->gpio_state);
>>> +}
>> You should not have to create a couple of specific functions just to
>> check the pinctrl pointers are set. The caller should do that.
> Because there are several places where the call is made,create a couple
> of specific functions reduce a lot of duplicated code.

No, that does not reduce a lot of duplicated code, it hides the fact
there is no control from the caller. See the comments below.

[ ... ]

>>> Â static int rockchip_configure_from_dt(struct device *dev,
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct device_node *np,
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct rockchip_thermal_data *thermal)
>>> @@ -1103,7 +1120,7 @@ static int rockchip_configure_from_dt(struct
>>> device *dev,
>>> ÂÂÂÂÂ if (of_property_read_u32(np, "rockchip,hw-tshut-mode",
>>> &tshut_mode)) {
>>> ÂÂÂÂÂÂÂÂÂ dev_warn(dev,
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "Missing tshut mode property, using default (%s)\n",
>>> -ÂÂÂÂÂÂÂÂÂÂÂÂ thermal->chip->tshut_mode == TSHUT_MODE_GPIO ?
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂ thermal->chip->tshut_mode == TSHUT_MODE_OTP ?
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "gpio" : "cru");
>>> ÂÂÂÂÂÂÂÂÂ thermal->tshut_mode = thermal->chip->tshut_mode;
>>> ÂÂÂÂÂ } else {
>>> @@ -1242,6 +1259,8 @@ static int rockchip_thermal_probe(struct
>>> platform_device *pdev)
>>> ÂÂÂÂÂÂÂÂÂ return error;
>>> ÂÂÂÂÂ }
>>> Â +ÂÂÂ thermal->chip->control(thermal->regs, false);
>>> +
>>> ÂÂÂÂÂ error = clk_prepare_enable(thermal->clk);
>>> ÂÂÂÂÂ if (error) {
>>> ÂÂÂÂÂÂÂÂÂ dev_err(&pdev->dev, "failed to enable converter clock: %d\n",
>>> @@ -1267,6 +1286,24 @@ static int rockchip_thermal_probe(struct
>>> platform_device *pdev)
>>> ÂÂÂÂÂ thermal->chip->initialize(thermal->grf, thermal->regs,
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ thermal->tshut_polarity);
>>> Â +ÂÂÂ if (thermal->tshut_mode == TSHUT_MODE_OTP) {
>>> +ÂÂÂÂÂÂÂ thermal->pinctrl = devm_pinctrl_get(&pdev->dev);
>>> +ÂÂÂÂÂÂÂ if (IS_ERR(thermal->pinctrl))
>>> +ÂÂÂÂÂÂÂÂÂÂÂ dev_err(&pdev->dev, "failed to find thermal pinctrl\n");
>>> +
>>> +ÂÂÂÂÂÂÂ thermal->gpio_state = pinctrl_lookup_state(thermal->pinctrl,
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "gpio");

panic if devm_pinctrl_get fails.

>>> +ÂÂÂÂÂÂÂ if (IS_ERR_OR_NULL(thermal->gpio_state))
>>> +ÂÂÂÂÂÂÂÂÂÂÂ dev_err(&pdev->dev, "failed to find thermal gpio state\n");
>>> +
>>> +ÂÂÂÂÂÂÂ thermal->otp_state = pinctrl_lookup_state(thermal->pinctrl,
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "otpout");
>>> +ÂÂÂÂÂÂÂ if (IS_ERR_OR_NULL(thermal->otp_state))
>>> +ÂÂÂÂÂÂÂÂÂÂÂ dev_err(&pdev->dev, "failed to find thermal otpout
>>> state\n");
>> What is the meaning for the rest of the code if the lookup fails for any
>> of those ?
> If the lookup fails for any of thoseï The pinctrl is no longer set and
> remains in its default state(otp_gpio normal io).
>>
>>> +ÂÂÂÂÂÂÂ thermal_pinctrl_select_otp(thermal);
>>> +ÂÂÂ }
>>> +

It is pointless to have a otp_state and a gpio_state field. Just use one
as the configuration tells you which lookup to do.

In case of error, just bail out. It is pointless to continue with a
broken configuration.

	thermal->pinctrl = devm_pinctrl_get(&pdev->dev);
	if (IS_ERR(thermal->pinctrl)) {
		dev_err(&pdev->dev, "....");
		return PTR_ERR(thermal->pinctrl);
	}

	thermal->pinctrl_state =
		pinctrl_lookup_state(thermal->pinctrl,
			thermal->tshut_mode == TSHUT_MODE_OTP ?
			"otp" : "gpio");

	if (IS_ERR_OR_NULL(thermal->pinctrl_state)) {
		dev_err(&pdev->dev, "....");
		return -EINVAL;
	}


No need to use the thermal_pinctrl_select_otp/gpio wrappers, just
replace them with:

pinctrl_select_state(thermal->pinctrl, thermal->pinctrl_state);

and use it unconditionally.


>>> ÂÂÂÂÂ for (i = 0; i < thermal->chip->chn_num; i++) {
>>> ÂÂÂÂÂÂÂÂÂ error = rockchip_thermal_register_sensor(pdev, thermal,
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &thermal->sensors[i],
>>> @@ -1338,7 +1375,8 @@ static int __maybe_unused
>>> rockchip_thermal_suspend(struct device *dev)
>>> ÂÂÂÂÂ clk_disable(thermal->pclk);
>>> ÂÂÂÂÂ clk_disable(thermal->clk);
>>> Â -ÂÂÂ pinctrl_pm_select_sleep_state(dev);
>>> +ÂÂÂ if (thermal->tshut_mode == TSHUT_MODE_OTP)
>>> +ÂÂÂÂÂÂÂ thermal_pinctrl_select_gpio(thermal);
>>> Â ÂÂÂÂÂ return 0;
>>> Â }
>>> @@ -1383,7 +1421,8 @@ static int __maybe_unused
>>> rockchip_thermal_resume(struct device *dev)
>>> ÂÂÂÂÂ for (i = 0; i < thermal->chip->chn_num; i++)
>>> ÂÂÂÂÂÂÂÂÂ rockchip_thermal_toggle_sensor(&thermal->sensors[i], true);
>>> Â -ÂÂÂ pinctrl_pm_select_default_state(dev);
>>> +ÂÂÂ if (thermal->tshut_mode == TSHUT_MODE_OTP)
>>> +ÂÂÂÂÂÂÂ thermal_pinctrl_select_otp(thermal);
>>> Â ÂÂÂÂÂ return 0;
>>> Â }
>>>
>>
> 
> 


-- 
 <http://www.linaro.org/> Linaro.org â Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog