Re: [PATCH 5/5] PCI: iproc: Properly handle optional PHYs

[Andrew Murray <andrew.murray@arm.com>]

On Thu, Aug 29, 2019 at 02:16:03PM +0100, Mark Brown wrote:
> On Thu, Aug 29, 2019 at 01:08:35PM +0100, Andrew Murray wrote:
> > On Thu, Aug 29, 2019 at 01:46:03PM +0200, Thierry Reding wrote:
> 
> > > 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.
> 
> > Sure. I think we end up with something like:
> 
> > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> > index e0c0cf462004..67e2a6d7abf6 100644
> > --- a/drivers/regulator/core.c
> > +++ b/drivers/regulator/core.c
> > @@ -1868,6 +1868,9 @@ struct regulator *_regulator_get(struct device *dev, const char *id,
> >                 }
> >  
> >                 switch (get_type) {
> > +               case OPTIONAL_GET:
> > +                       return NULL;
> > +
> 
> Implementing returning NULL is not hard.  How returning NULL discourages
> people from using regulator_get_optional() when they shouldn't be using
> it in the first place is not clear to me.

I think this is the part I haven't understood until now.

There are many consumer drivers that will not have a regulator specified in
the DT - this may be because they are optional (possibly a rare thing) or
because they don't need to be specified (because they are always on and
require no software interaction)...

Where they are not specified, because there is really no reason for them to
be described in the DT - then these drivers should use regulator_get and
be happy with a dummy regulator. This gives a benefit as if another hardware
version uses the same driver but does have a regulator that needs software
control then we can be confident it will work.

Where regulators are really optional, then regulator_get_optional is used
and -ENODEV can be used by the caller to perform a different course of action
if required. (Does this use-case actually exist?)

I guess I interpreted _optional as 'it's OK if you can't provide a regulator',
whereas the meaning is really 'get me a regulator that may not exist'.

Is my understanding correct? If so I guess another course of action would
be to clean-up users of _optional and convert them to regulator_get where
appropriate?

Thanks,

Andrew Murray