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

[Simon Glass]

Hi Lokesh,

On Sun, 30 Dec 2018 at 23:26, Lokesh Vutla <lokeshvutla at ti.com> wrote:
>
> 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.

I think that would be useful. Another concern I have is that errors
that are suppressed at a low level can result in people checking for
success in other ways (e.g. reading data back in a separate
transaction). I think it is better to explicitly ignore specific error
numbers at the level where a decision can be made as to its
importance.

If I say 'please do this' I think it should mean 'return an error if
you cannot'. With driver model there are quite a few places where
errors are returned which might be harmless. For example.
gpio_request_by_name() returns -ENOENT if there is no GPIO by that
name, but returns -EINVAL if the config is invalid. We may want to
ignore the first one (e.g. if the GPIO is optional) but should never
ignore the second.

So a function like 'please do this if allowed' makes sense to me in
this context.

>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/regulator/core.c#n2627
>
> Thanks and regards,
> Lokesh

Regards,
Simon