| Message ID | 20230911133435.14061-5-m.majewski2@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve Exynos thermal driver | expand |
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
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
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; } ??
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
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
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
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 --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; }
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(-)