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 <[email protected]>
> ---
> 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;
}
??
--
<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
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
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland