| Message ID | 20230911133435.14061-1-m.majewski2@samsung.com |
|---|---|
| Headers | show |
| Series | Improve Exynos thermal driver | expand |
On 11/09/2023 15:34, Mateusz Majewski wrote: > It seems that the field has been removed in one of the previous commits, > but the description has been forgotten. > > Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com> > --- Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On 11/09/2023 15:34, Mateusz Majewski wrote: > The workqueue boilerplate is mostly one-to-one what the threaded > interrupts do. > > Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com> > --- > v1 -> v2: devm_request_threaded_irq call formatting change. > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
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
This work improves Exynos thermal driver in various ways. This is related to the discussion in https://lore.kernel.org/all/97201878-3bb8-eac5-7fac-a690322ac43a@linaro.org/ The primary issue being fixed is a lockdep warning, which is fixed by the thermal: exynos: use set_trips patch. We also simplify the code in general. Changelog: v2: - Added missing field descriptions - Removed an unnecessary field description - Removed the commits that made clock management more fine-grained (need more discussion), and adapted the new code to manage clocks - Removed the devicetree changes (will be uploaded separately), changing the recipient list accordingly - Improved formatting of the devm_request_threaded_irq call Mateusz Majewski (7): thermal: exynos: remove an unnecessary field description thermal: exynos: drop id field thermal: exynos: switch from workqueue-driven interrupt handling to threaded interrupts thermal: exynos: simplify regulator (de)initialization thermal: exynos: stop using the threshold mechanism on Exynos 4210 thermal: exynos: split initialization of TMU and the thermal zone thermal: exynos: use set_trips drivers/thermal/samsung/exynos_tmu.c | 525 ++++++++++++++------------- 1 file changed, 272 insertions(+), 253 deletions(-)