Messages in this thread | | | Date | Tue, 23 Apr 2024 08:11:57 +0300 | Subject | Re: [PATCH] regulator: change stubbed devm_regulator_get_enable to return Ok | From | Matti Vaittinen <> |
| |
On 4/22/24 16:23, Guenter Roeck wrote: > On 4/21/24 23:38, Matti Vaittinen wrote: >> The devm_regulator_get_enable() should be a 'call and forget' API, >> meaning, when it is used to enable the regulators, the API does not >> provide a handle to do any further control of the regulators. It gives >> no real benefit to return an error from the stub if CONFIG_REGULATOR is >> not set. >> >> On the contrary, returning and error is causing problems to drivers when >> hardware is such it works out just fine with no regulator control. >> Returning an error forces drivers to specifically handle the case where >> CONFIG_REGULATOR is not set, making the mere existence of the stub >> questionalble. Furthermore, the stub of the regulator_enable() seems to >> be returning Ok. >> > > Yes, that was the reason why the lm90 driver worked pripr to its conversion > to use devm_regulator_get_enable() if CONFIG_REGULATOR=n. > >> Change the stub implementation for the devm_regulator_get_enable() to >> return Ok so drivers do not separately handle the case where the >> CONFIG_REGULATOR is not set. >> >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> >> Reported-by: Aleksander Mazur <deweloper@wp.pl> >> Suggested-by: Guenter Roeck <linux@roeck-us.net> >> Fixes: da279e6965b3 ("regulator: Add devm helpers for get and enable") >> >> --- >> Please find the report by Aleksander from: >> https://lore.kernel.org/all/20240420183427.0d3fda27@mocarz/ >> >> This patch has not received testing. It'd be great to hear if this >> solves the issue. >> >> I see the regulator_get_exclusive() and devm_regulator_get_optional() >> returning errors. I thus leave the >> devm_regulator_get_enable_[optional/exclusive]() to do the same while >> wondering if this is the right thing to do, and why... >> > > At least one of the callers of devm_regulator_get_enable (exc3000) > checks for > -ENODEV and ignores it. I assume we'll see more of those unless this patch > is accepted. Many of the callers of devm_regulator_get_enable_optional() > explicitly check for -ENODEV and ignore it. Others fail if > CONFIG_REGULATOR=n. > My plan for affected hwmon drivers is (was ?) to check for -ENODEV and > ignore > it to match other drivers.
I'd rather fixed the stub than the callers. I suspect same goes with other subsystems.
> Returning ERR_PTR(-ENODEV) for [devm_]regulator_get() made sense because > the returned regulator pointer was often used to obtain a voltage or to > do other regulator operations. I don't really see the point of returning > -ENODEV for the _enable APIs if regulator support is disabled.
I agree. I'll send another one for the devm_regulator_get_enable_[optional/exclusive]() if Mark accepts this one.
Thanks for the heads up!
Yours, -- Matti
-- Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
| |