Messages in this thread | | | Date | Fri, 3 Feb 2017 13:51:20 -0800 | From | Dmitry Torokhov <> | Subject | Re: regulator_get_optional() no longer returning NULL? |
| |
On Fri, Feb 03, 2017 at 12:20:01PM +0100, Mark Brown wrote: > On Wed, Feb 01, 2017 at 02:23:31PM -0800, Dmitry Torokhov wrote: > > > It appears that [devm_]regulator_get_optional() and > > [devm_]gpiod_get_optional() behave irritatingly differently. While the > > latter returns NULL for non-existing GPIOs, the former started returning > > -ENODEV instead of NULL, starting with commit below. > > > Why did we do that? It is much more convenient to write: > > As the commit description for that commit says it's fixing a bug with us > not returning the error code we get from regulator_dev_lookup(). > > > I.e. it is nice to treat *all* codes returned by > > devm_regulator_get_optional() as fatal and NULL as special instead of > > vetting by hand (and having chance that list of vetted codes will bit > > rot). > > There's no real risk of bitrot I can see here, the only thing something > using optional regulator is looking for is that the regulator does not > exist and we have the specific error code -ENODEV for that. > > > Can we please revert this patch? > > That won't do what you want so I don't know why you're asking for it - > the default value of regulator is ERR_PTR(-EPROBE_DEFER) so if we skip > assigning the actual return code you'll get that and not NULL. I don't > recall that behaviour ever working, it's not what's documented so it > doesn't look like something broke here. If you wanted to implement it > you'd need to add new code to do it (eg, squash down -ENODEV in > _get_optional()).
Yes, you are right. The _regulator_get() is a bit confusing, I have a patch series that hopefully makes it a bit more straightforward.
Unfortunately it seems some of the drivers are not ready for regulator_get_optional() to return NULL, I'll have to prepare them first.
> > However I am a bit dubious about doing that as I'm not thrilled with > adding in more things for people to check. Even if you check for NULL > with an optional regulator you'd still need to also go and check that > you didn't get an error code like -EPROBE_DEFER, you'd only get a NULL > in cases where the regulator does not exist. Given that I'm not sure > that it's actually simplifying things.
Like I said, I want people to simply check for error/!error and have all errors be fatal, and the rest of the checks (when trying to use said regulator) should be in form of:
if (blah->supply) regulator_enable(supply).
Or even teach regulator_enable() to like NULLs.
Allowing regulator_get_optional() return NULL will make it work similarly to gpiod_get_optional() which I think is a good thing.
By the way, I see quite a few drivers using regulator_get_optional() for cases when regulator is mandatory, but it might be handled by the firmware. I.e. it seems they can be converted to regulator_get() with the expectation that it will give us dummy supply. Would such conversions be OK/accepted?
Thanks.
-- Dmitry
| |