diff mbox series

[v2,4/7] thermal: exynos: simplify regulator (de)initialization

Message ID 20230911133435.14061-5-m.majewski2@samsung.com
State New
Headers show
Series [v2,1/7] thermal: exynos: remove an unnecessary field description | expand

Commit Message

Mateusz Majewski Sept. 11, 2023, 1:34 p.m. UTC
This does reduce the error granularity a bit, but the code
simplification seems to be worth it.

Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
---
 drivers/thermal/samsung/exynos_tmu.c | 33 +++++++---------------------
 1 file changed, 8 insertions(+), 25 deletions(-)

Comments

Krzysztof Kozlowski Sept. 13, 2023, 8:11 a.m. UTC | #1
On 11/09/2023 15:34, Mateusz Majewski wrote:
> This does reduce the error granularity a bit, but the code
> simplification seems to be worth it.
> 
> Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
> ---
>  drivers/thermal/samsung/exynos_tmu.c | 33 +++++++---------------------
>  1 file changed, 8 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> index ba9414b419ef..8451deb65f43 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c
> @@ -157,7 +157,6 @@ enum soc_type {
>   * @reference_voltage: reference voltage of amplifier
>   *	in the positive-TC generator block
>   *	0 < reference_voltage <= 31
> - * @regulator: pointer to the TMU regulator structure.
>   * @tzd: pointer to thermal_zone_device structure
>   * @ntrip: number of supported trip points.
>   * @enabled: current status of TMU device
> @@ -183,7 +182,6 @@ struct exynos_tmu_data {
>  	u16 temp_error1, temp_error2;
>  	u8 gain;
>  	u8 reference_voltage;
> -	struct regulator *regulator;
>  	struct thermal_zone_device *tzd;
>  	unsigned int ntrip;
>  	bool enabled;
> @@ -994,42 +992,34 @@ static int exynos_tmu_probe(struct platform_device *pdev)
>  	 * TODO: Add regulator as an SOC feature, so that regulator enable
>  	 * is a compulsory call.
>  	 */
> -	data->regulator = devm_regulator_get_optional(&pdev->dev, "vtmu");
> -	if (!IS_ERR(data->regulator)) {
> -		ret = regulator_enable(data->regulator);
> -		if (ret) {
> -			dev_err(&pdev->dev, "failed to enable vtmu\n");
> -			return ret;
> -		}
> -	} else {
> -		if (PTR_ERR(data->regulator) == -EPROBE_DEFER)
> +	ret = devm_regulator_get_enable_optional(&pdev->dev, "vtmu");
> +	if (ret) {
> +		if (ret == -EPROBE_DEFER)
>  			return -EPROBE_DEFER;
> -		dev_info(&pdev->dev, "Regulator node (vtmu) not found\n");
> +		dev_info(&pdev->dev, "Failed to get regulator node (vtmu)\n");

This is not equivalent. If regulator is provided and enable fails, the
old code is nicely returning error. Now, it will print misleading
message - failed to get regulator - and continue.

While this simplifies the code, it ignores important running condition -
having regulator enabled.

Best regards,
Krzysztof
Mateusz Majewski Sept. 26, 2023, 11:02 a.m. UTC | #2
Hi,

> This is not equivalent. If regulator is provided and enable fails, the
> old code is nicely returning error. Now, it will print misleading
> message - failed to get regulator - and continue.
>
> While this simplifies the code, it ignores important running condition -
> having regulator enabled.

Would doing this be correct?

ret = devm_regulator_get_enable_optional(&pdev->dev, "vtmu");
switch (ret) {
case 0:
case -ENODEV:
	break;
case -EPROBE_DEFER:
	return -EPROBE_DEFER;
default:
	dev_err(&pdev->dev, "Failed to get enabled regulator: %d\n",
		ret);
	return ret;
}

I understand that we would get -ENODEV in case of the regulator being
unavailable, which we would ignore (this is the "equivalent" of
devm_regulator_get_optional failing in the original code). And in case
of enable failing, we would get some other error, which we would handle.

Thanks for being patient with me by the way, hopefully I will learn quickly :)

Best regards,
Mateusz
Daniel Lezcano Sept. 29, 2023, 10:46 a.m. UTC | #3
On 26/09/2023 13:02, Mateusz Majewski wrote:
> Hi,
> 
>> This is not equivalent. If regulator is provided and enable fails, the
>> old code is nicely returning error. Now, it will print misleading
>> message - failed to get regulator - and continue.
>>
>> While this simplifies the code, it ignores important running condition -
>> having regulator enabled.
> 
> Would doing this be correct?
> 
> ret = devm_regulator_get_enable_optional(&pdev->dev, "vtmu");
> switch (ret) {
> case 0:
> case -ENODEV:

Not sure to understand why -NODEV is not an error

> 	break;
> case -EPROBE_DEFER:
> 	return -EPROBE_DEFER;
> default:
> 	dev_err(&pdev->dev, "Failed to get enabled regulator: %d\n",
> 		ret);
> 	return ret;
> }

ret = devm_regulator_get_enable_optional(&pdev->dev, "vtmu");
if (ret < 0) {
	if (ret != EPROBE_DEFER)
		dev_err(&pdev->dev, "Failed to get enabled regulator: %d\n", ret);
	return ret;
}

??
Marek Szyprowski Sept. 29, 2023, 11:03 a.m. UTC | #4
On 29.09.2023 12:46, Daniel Lezcano wrote:
> On 26/09/2023 13:02, Mateusz Majewski wrote:
>> Hi,
>>
>>> This is not equivalent. If regulator is provided and enable fails, the
>>> old code is nicely returning error. Now, it will print misleading
>>> message - failed to get regulator - and continue.
>>>
>>> While this simplifies the code, it ignores important running 
>>> condition -
>>> having regulator enabled.
>>
>> Would doing this be correct?
>>
>> ret = devm_regulator_get_enable_optional(&pdev->dev, "vtmu");
>> switch (ret) {
>> case 0:
>> case -ENODEV:
>
> Not sure to understand why -NODEV is not an error


Because this what devm_regulator_get_enable_optional() returns if no 
regulator is defined. I also got confused by this a few times.


>
>>     break;
>> case -EPROBE_DEFER:
>>     return -EPROBE_DEFER;
>> default:
>>     dev_err(&pdev->dev, "Failed to get enabled regulator: %d\n",
>>         ret);
>>     return ret;
>> }
>
> ret = devm_regulator_get_enable_optional(&pdev->dev, "vtmu");
> if (ret < 0) {
>     if (ret != EPROBE_DEFER)
>         dev_err(&pdev->dev, "Failed to get enabled regulator: %d\n", 
> ret);
>     return ret;
> }
>
> ??
>
Best regards
Daniel Lezcano Sept. 29, 2023, 11:45 a.m. UTC | #5
On 29/09/2023 13:03, Marek Szyprowski wrote:
> On 29.09.2023 12:46, Daniel Lezcano wrote:
>> On 26/09/2023 13:02, Mateusz Majewski wrote:
>>> Hi,
>>>
>>>> This is not equivalent. If regulator is provided and enable fails, the
>>>> old code is nicely returning error. Now, it will print misleading
>>>> message - failed to get regulator - and continue.
>>>>
>>>> While this simplifies the code, it ignores important running
>>>> condition -
>>>> having regulator enabled.
>>>
>>> Would doing this be correct?
>>>
>>> ret = devm_regulator_get_enable_optional(&pdev->dev, "vtmu");
>>> switch (ret) {
>>> case 0:
>>> case -ENODEV:
>>
>> Not sure to understand why -NODEV is not an error
> 
> 
> Because this what devm_regulator_get_enable_optional() returns if no
> regulator is defined. I also got confused by this a few times.

The code before this change calls devm_regulator_get_optional() which 
returns -ENODEV too, right ? But there is no special case for this error.

So this change uses devm_regulator_get_enable_optional() and handle the 
ENODEV as a non-error, so there is a change in the behavior.


>>>      break;
>>> case -EPROBE_DEFER:
>>>      return -EPROBE_DEFER;
>>> default:
>>>      dev_err(&pdev->dev, "Failed to get enabled regulator: %d\n",
>>>          ret);
>>>      return ret;
>>> }
>>
>> ret = devm_regulator_get_enable_optional(&pdev->dev, "vtmu");
>> if (ret < 0) {
>>      if (ret != EPROBE_DEFER)
>>          dev_err(&pdev->dev, "Failed to get enabled regulator: %d\n",
>> ret);
>>      return ret;
>> }
>>
>> ??
>>
> Best regards
Marek Szyprowski Sept. 29, 2023, noon UTC | #6
On 29.09.2023 13:45, Daniel Lezcano wrote:
> On 29/09/2023 13:03, Marek Szyprowski wrote:
>> On 29.09.2023 12:46, Daniel Lezcano wrote:
>>> On 26/09/2023 13:02, Mateusz Majewski wrote:
>>>> Hi,
>>>>
>>>>> This is not equivalent. If regulator is provided and enable fails, 
>>>>> the
>>>>> old code is nicely returning error. Now, it will print misleading
>>>>> message - failed to get regulator - and continue.
>>>>>
>>>>> While this simplifies the code, it ignores important running
>>>>> condition -
>>>>> having regulator enabled.
>>>>
>>>> Would doing this be correct?
>>>>
>>>> ret = devm_regulator_get_enable_optional(&pdev->dev, "vtmu");
>>>> switch (ret) {
>>>> case 0:
>>>> case -ENODEV:
>>>
>>> Not sure to understand why -NODEV is not an error
>>
>>
>> Because this what devm_regulator_get_enable_optional() returns if no
>> regulator is defined. I also got confused by this a few times.
>
> The code before this change calls devm_regulator_get_optional() which 
> returns -ENODEV too, right ? But there is no special case for this error.
>
> So this change uses devm_regulator_get_enable_optional() and handle 
> the ENODEV as a non-error, so there is a change in the behavior.


It looks that the original code ignores any non-EPROBE_DEFER errors from 
devm_regulator_get_optional(). That's a bug, indeed.


Best regards
Krzysztof Kozlowski Oct. 3, 2023, 9:06 a.m. UTC | #7
On 29/09/2023 14:00, Marek Szyprowski wrote:
> On 29.09.2023 13:45, Daniel Lezcano wrote:
>> On 29/09/2023 13:03, Marek Szyprowski wrote:
>>> On 29.09.2023 12:46, Daniel Lezcano wrote:
>>>> On 26/09/2023 13:02, Mateusz Majewski wrote:
>>>>> Hi,
>>>>>
>>>>>> This is not equivalent. If regulator is provided and enable fails, 
>>>>>> the
>>>>>> old code is nicely returning error. Now, it will print misleading
>>>>>> message - failed to get regulator - and continue.
>>>>>>
>>>>>> While this simplifies the code, it ignores important running
>>>>>> condition -
>>>>>> having regulator enabled.
>>>>>
>>>>> Would doing this be correct?
>>>>>
>>>>> ret = devm_regulator_get_enable_optional(&pdev->dev, "vtmu");
>>>>> switch (ret) {
>>>>> case 0:
>>>>> case -ENODEV:
>>>>
>>>> Not sure to understand why -NODEV is not an error
>>>
>>>
>>> Because this what devm_regulator_get_enable_optional() returns if no
>>> regulator is defined. I also got confused by this a few times.
>>
>> The code before this change calls devm_regulator_get_optional() which 
>> returns -ENODEV too, right ? But there is no special case for this error.
>>
>> So this change uses devm_regulator_get_enable_optional() and handle 
>> the ENODEV as a non-error, so there is a change in the behavior.
> 
> 
> It looks that the original code ignores any non-EPROBE_DEFER errors from 
> devm_regulator_get_optional(). That's a bug, indeed.

How about separate change fixing it? I know the same code will be
changed twice, but it will be easier to backport and analyze in case of
issues.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index ba9414b419ef..8451deb65f43 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -157,7 +157,6 @@  enum soc_type {
  * @reference_voltage: reference voltage of amplifier
  *	in the positive-TC generator block
  *	0 < reference_voltage <= 31
- * @regulator: pointer to the TMU regulator structure.
  * @tzd: pointer to thermal_zone_device structure
  * @ntrip: number of supported trip points.
  * @enabled: current status of TMU device
@@ -183,7 +182,6 @@  struct exynos_tmu_data {
 	u16 temp_error1, temp_error2;
 	u8 gain;
 	u8 reference_voltage;
-	struct regulator *regulator;
 	struct thermal_zone_device *tzd;
 	unsigned int ntrip;
 	bool enabled;
@@ -994,42 +992,34 @@  static int exynos_tmu_probe(struct platform_device *pdev)
 	 * TODO: Add regulator as an SOC feature, so that regulator enable
 	 * is a compulsory call.
 	 */
-	data->regulator = devm_regulator_get_optional(&pdev->dev, "vtmu");
-	if (!IS_ERR(data->regulator)) {
-		ret = regulator_enable(data->regulator);
-		if (ret) {
-			dev_err(&pdev->dev, "failed to enable vtmu\n");
-			return ret;
-		}
-	} else {
-		if (PTR_ERR(data->regulator) == -EPROBE_DEFER)
+	ret = devm_regulator_get_enable_optional(&pdev->dev, "vtmu");
+	if (ret) {
+		if (ret == -EPROBE_DEFER)
 			return -EPROBE_DEFER;
-		dev_info(&pdev->dev, "Regulator node (vtmu) not found\n");
+		dev_info(&pdev->dev, "Failed to get regulator node (vtmu)\n");
 	}
 
 	ret = exynos_map_dt_data(pdev);
 	if (ret)
-		goto err_sensor;
+		return ret;
 
 	data->clk = devm_clk_get(&pdev->dev, "tmu_apbif");
 	if (IS_ERR(data->clk)) {
 		dev_err(&pdev->dev, "Failed to get clock\n");
-		ret = PTR_ERR(data->clk);
-		goto err_sensor;
+		return PTR_ERR(data->clk);
 	}
 
 	data->clk_sec = devm_clk_get(&pdev->dev, "tmu_triminfo_apbif");
 	if (IS_ERR(data->clk_sec)) {
 		if (data->soc == SOC_ARCH_EXYNOS5420_TRIMINFO) {
 			dev_err(&pdev->dev, "Failed to get triminfo clock\n");
-			ret = PTR_ERR(data->clk_sec);
-			goto err_sensor;
+			return PTR_ERR(data->clk_sec);
 		}
 	} else {
 		ret = clk_prepare(data->clk_sec);
 		if (ret) {
 			dev_err(&pdev->dev, "Failed to get clock\n");
-			goto err_sensor;
+			return ret;
 		}
 	}
 
@@ -1099,10 +1089,6 @@  static int exynos_tmu_probe(struct platform_device *pdev)
 err_clk_sec:
 	if (!IS_ERR(data->clk_sec))
 		clk_unprepare(data->clk_sec);
-err_sensor:
-	if (!IS_ERR(data->regulator))
-		regulator_disable(data->regulator);
-
 	return ret;
 }
 
@@ -1117,9 +1103,6 @@  static int exynos_tmu_remove(struct platform_device *pdev)
 	if (!IS_ERR(data->clk_sec))
 		clk_unprepare(data->clk_sec);
 
-	if (!IS_ERR(data->regulator))
-		regulator_disable(data->regulator);
-
 	return 0;
 }