Hi Simon,
On 29/12/18 6:58 PM, Simon Glass wrote:
Hi Lokesh,
On Thu, 27 Dec 2018 at 22:33, Lokesh Vutla <lokeshvu...@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 <lokeshvu...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
