On Thu, Aug 29, 2019 at 12:17:29PM +0100, Mark Brown wrote:
> On Thu, Aug 29, 2019 at 11:09:34AM +0100, Andrew Murray wrote:
>
> > But why do we return -ENODEV and not NULL for OPTIONAL_GET?
>
> Why would we return NULL? The caller is going to have to check the
> error code anyway since they need to handle -EPROBE_DEFER and NULL is
> never a valid thing to use with the regulator API.
I think the idea is that consumers would do something like this:
supply = regulator_get_optional(dev, "foo");
if (IS_ERR(supply)) {
/* -EPROBE_DEFER handled as part of this */
return PTR_ERR(supply);
}
/*
* Optional supply is NULL here, making it "easier" to check
* whether or not to use it.
*/
I suppose checking using IS_ERR() is equally simple, so this mostly
boils down to taste...
The PHY subsystem, for instance, uses a similar approach but it goes one
step further. All APIs can take NULL as their struct phy * argument and
return success in that case, which makes it really trivial to deal with
optional PHYs. You really don't have to care about them at all after you
get NULL from phy_get_optional().
> > Looking at some of the consumer drivers I can see that lots of them don't
> > correctly handle the return value of regulator_get_optional:
>
> This API is *really* commonly abused, especially in the graphics
> subsystem which for some reason has lots of users even though I don't
> think I've ever seen a sensible use of the API there. As far as I can
> tell the graphics subsystem mostly suffers from people cut'n'pasting
> from the Qualcomm BSP which is full of really bad practice. It's really
> frustrating since I will intermittently go through and clean things up
> but the uses just keep coming back into the subsystem.
The intention here is to make it more difficult to abuse. Obviously if
people keep copying abuses from one driver to another that's a different
story. But if there was no way to abuse the API and if it automatically
did the right thing, even copy/paste abuse would be difficult to pull
off.
> > Given that a common pattern is to set a consumer regulator pointer to NULL
> > upon -ENODEV - if regulator_get_optional did this for them, then it would
> > be more difficult for consumer drivers to get the error handling wrong and
> > would remove some consumer boiler plate code.
>
> No, they'd all still be broken for probe deferral (which is commonly
> caused by off-chip devices) and they'd crash as soon as they try to call
> any further regulator API functions.
I think what Andrew was suggesting here was to make it easier for people
to determine whether or not an optional regulator was in fact requested
successfully or not. Many consumers already set the optional supply to
NULL and then check for that before calling any regulator API.
If regulator_get_optional() returned NULL for absent optional supplies,
this could be unified across all drivers. And it would allow treating
NULL regulators special, if that's something you'd be willing to do.
In either case, the number of abuses shows that people clearly don't
understand how to use this. So there are two options: a) fix abuse every
time we come across it or b) try to change the API to make it more
difficult to abuse.
> > I guess I'm looking here for something that can simplify consumer error
> > handling - it's easy to get wrong and it seems that many drivers may be wrong.
>
> The overwhelming majority of the users just shouldn't be using this API.
> If there weren't devices that absolutely *need* this API it wouldn't be
> there in the first place since it seems like a magent for bad code, it's
> so disappointing how bad the majority of the consumer code is.
Yeah, I guess in many cases this API is used as a cheap way to model
always-on, fixed-voltage regulators. It's pretty hard to change those
after the fact, though, because they're usually happening as part of
device tree bindings implementation, so by the time we notice them,
they've often become ABI...
Thierry
Attachment:
signature.asc
Description: PGP signature
