Hi Lokesh, On Sun, 30 Dec 2018 at 23:26, Lokesh Vutla <lokeshvu...@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 <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.
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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot