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.
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 <[email protected]>
Reported-by: Aleksander Mazur <[email protected]>
Suggested-by: Guenter Roeck <[email protected]>
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...
---
include/linux/regulator/consumer.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index 4660582a3302..71232fb7dda3 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -320,7 +320,7 @@ devm_regulator_get_exclusive(struct device *dev, const char *id)
static inline int devm_regulator_get_enable(struct device *dev, const char *id)
{
- return -ENODEV;
+ return 0;
}
static inline int devm_regulator_get_enable_optional(struct device *dev,
base-commit: 68adb581a39ae63a0ed082c47f01fbbe515efa0e
--
2.43.2
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]
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 <[email protected]>
> Reported-by: Aleksander Mazur <[email protected]>
> Suggested-by: Guenter Roeck <[email protected]>
> 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.
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.
Anyway, for this patch:
Reviewed-by: Guenter Roeck <[email protected]>
Thanks,
Guenter
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 <[email protected]>
>> Reported-by: Aleksander Mazur <[email protected]>
>> Suggested-by: Guenter Roeck <[email protected]>
>> 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! ~~
On Mon, 22 Apr 2024 09:38:33 +0300, 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.
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
Thanks!
[1/1] regulator: change stubbed devm_regulator_get_enable to return Ok
commit: 96e20adc43c4f81e9163a5188cee75a6dd393e09
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark