On Thu, Aug 29, 2019 at 11:09:34AM +0100, Andrew Murray wrote: > On Wed, Aug 28, 2019 at 10:49:01PM +0100, Mark Brown wrote: > > On Wed, Aug 28, 2019 at 10:26:55PM +0100, Andrew Murray wrote: > > > > > I initially thought that you forgot to check for -ENODEV - though I can see > > > that the implementation of devm_phy_optional_get very helpfully does this for > > > us and returns NULL instead of an error. > > > > > What is also confusing is that devm_regulator_get_optional, despite its > > > _optional suffix doesn't do this and returns an error. I wonder if > > > devm_phy_optional_get should be changed to return NULL instead of an error > > > instead of -ENODEV. I've copied Liam/Mark for feedback. > > > > The regulator API has an assumption that people will write bad DTs and > > not describe all the regulators in the system, this is especially likely > > in cases where consumer drivers initially don't have regulator support > > and then get it added since people often only describe supplies actively > > used by drivers. In order to handle this gracefully the API will > > substitute in a dummy regulator if it sees that the regulator just isn't > > drescribed in the system but a consumer requests it, this will ensure > > that for most simple uses the consumer will function fine even if the DT > > is not fully described. Since most devices won't physically work if > > some of their supplies are missing this is a good default assumption. > > Right, if I understand correctly this is the behaviour when regulator_get > is called (e.g. NORMAL_GET) - you get a dummy instead of an error. > > > > > If a consumer could genuinely have some missing supplies (some devices > > do support this for various reasons) then this support would mean that > > the consumer would have to have some extra property to say that the > > regulator is intentionally missing which would be bad. Instead what we > > do is let the consumer say that real systems could actually be missing > > the regulator and that the dummy shouldn't be used so that the consumer > > can handle this. > > And if I understand correctly this is the behaviour when > regulator_get_optional is called (e.g. OPTIONAL_GET) - you get -ENODEV > instead of a dummy. > > But why do we return -ENODEV and not NULL for OPTIONAL_GET? > > 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: > > - some fail their probes and return upon IS_ERR(ret) - for example even > if -ENODEV is returned. > > - some don't fail their probes and assume the regulator isn't present upon > IS_ERR(ret) - yet this may not be correct as the regulator may be present > but -ENOMEM was returned. > > 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. > > (Of course some consumers won't set a regulator pointer to NULL and instead > test it against IS_ERR instead of NULL everywhere (IS_ERR(NULL) is false) - > but such a change may be a reason to not use IS_ERR everywhere). > > As I understand, if a consumer wants to fail upon an absent regulator > it seems the only way they can do this is call regulator_get_optional (which > seems odd) and test for -ENODEV. I'm not sure if there is actually a use-case > for this. > > 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. Agreed. However, this requires a thorough audit of all callers of regulator_get_optional() to make sure they behave in a sane way. To further complicate things, unless we want to convert all ~100 callers in a single patch we need to convert all of them to set the regulator pointer to NULL on -ENODEV. After that we can make the change to regulator_get_optional() and only then can we remove the now obsolete boilerplate from those ~100 callers. Not impossible, but pretty time- consuming. While at it, we could also add optional variants to some of the *phy*_get() functions to convert those as well. Currently there's only optional variants for phy_get() and devm_phy_get(), but a bunch of drivers use of_phy_get() or of_phy_get_by_index(). Though especially the latter isn't very common with optional PHYs, I think. I also noticed a slightly similar pattern for GPIOs. Perhaps this would be a good task for someone with good semantic patch skills. Or perhaps something to add to the janitors' TODO list? Not sure if that's still a thing, though. Thierry
Attachment:
signature.asc
Description: PGP signature
