[U-Boot] [PATCH] power: regulator: Return success on attempt to disable an always-on regulator

[Lokesh Vutla]

Hi Simon,

On 29/12/18 6:58 PM, Simon Glass wrote:
> Hi Lokesh,
> 
> On Thu, 27 Dec 2018 at 22:33, Lokesh Vutla <lokeshvutla at ti.com> wrote:
>>
>> Hi Simon,
>>
>> On 28/12/18 3:57 AM, Simon Glass wrote:
>>> Hi Lokesh,
>>>
>>> On Mon, 24 Dec 2018 at 04:08, Lokesh Vutla <lokeshvutla at ti.com> wrote:
>>>>
>>>> commit 4f86a724e82c0 ("power: regulator: denied disable on always-on
>>>> regulator") throws an error when requested to disable an always-on
>>>> regulator. It is right that an always-on regulator should not be
>>>> attempted to be disabled. But at the same time regulator framework
>>>> should not return an error when such request is received. Instead
>>>> it should just return success without attempting to disable the
>>>> specified regulator. This is because the requesting driver will
>>>> not have the idea if the regulator is always-on or not. The
>>>> requesting driver will always try to enable/disable regulator as
>>>> per the required flow. So it is upto regulator framework to not
>>>> break such scenarios.
>>>
>>> Can the caller not check the error code? It is -EACCES in this case.
>>
>> We considered this an one of the option but I ended up fixing regulator
>> framework due to the following reasons:
>> - If regulator framework returns -EACCES on this scenario then:
>>          - -EACCES should be checked in all the existing usage of the api[1] or else
>> someone else might encounter the same problem.
> 
> Yes. Some already check for -ENOSYS, e.g. omap_hsmmc.c
> 
>>          - Any future usage of the api should take of handling this error.
> 
> Yes, and it should be commented too.
> 
>>          - From a client driver perspective it is not really an error. It is doing the
>> right thing and receiving an error might be confusing.
> 
> The error means that the request was not handled. There is no way to
> find out that requesting this was actually wrong.
> 
>>
>> Hope this is clear. Also just to add one more point, I adapted this error
>> handling from Linux kernel[2].
> 
> The only question for me whether anything would need to detect that
> the request to disable a regulator is not supported.
> 
> Your linux link appears to lead me to regulator_ena_gpio_ctrl(),
> related to regulator GPIOs. Is that right? It's hard for me to
> understand what the code there is doing.

Looks like functions are moving around too fast. I am referring to the function 
_regulator_disable() in the same file[1]. So logic of _regulator_disable() looks 
something like below:

_regulator_disable()
{
	if (use_count == 1 && !always_on_regulator)
		.....
		ret = _regulator_do_disable()
		.....
		use_count = 0;
	else
		use_count--;

	return ret;
}

Obviously there are more things happening in the function but I just mentioned 
the details what we require.

> 
> Once we make this change we will not be able to go back without breaking things.
> 
> I am not really convinced that this patch is the best approach. I do
> understand your point though. It just worries me that we are hiding
> something and it will be hard to unhide it later.
> 
> What do you think about adding something like
> regulator_disable_if_allowed() which silently ignored -ENOSYS and
> -EACCES?

hmm...not sure if this is necessary. But if you feel "detecting the request to 
disable is not supported" might be needed in future, I can make something 
regulator_set_enable_if_allowed() and discard -ENOSYS and -EACCESS as you suggested.

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/regulator/core.c#n2627

Thanks and regards,
Lokesh