Re: [PATCH] PM / OPP: Initialize regulator pointer to an error value

[Arnd Bergmann]

On Tuesday 16 February 2016 01:56:16 Mark Brown wrote:
> On Tue, Feb 16, 2016 at 06:30:59AM +0530, Viresh Kumar wrote:
> 
> > - And so I left the regulator pointer to NULL in OPP core.
> > - But then I realized that its not safe to call many regulator core
> >   APIs with NULL regulator, as those caused the crashes reported by
> >   multiple people now.
> > - clk APIs guarantee that they return early when NULL clk is passed to
> >   them.
> > - Do we need to do the same for regulator core as well ?
> 
> No, NULL is explicitly not something you can substitute in,
> essentially all the users are just not bothering to implement error
> checking and we don't want to encourage that.  The set of use cases
> where we legitimately have optional supplies is very small, much smaller
> than clocks, because it makes the electrical engineering a lot harder.
> 

I must have misinterpreted the idea behind that API as well then.

>From this function definition:

/*
 * Make sure client drivers will still build on systems with no software
 * controllable voltage or current regulators.
 */             
static inline struct regulator *__must_check regulator_get(struct device *dev,
        const char *id)
{       
        /* Nothing except the stubbed out regulator API should be
         * looking at the value except to check if it is an error
         * value. Drivers are free to handle NULL specifically by
         * skipping all regulator API calls, but they don't have to.
         * Drivers which don't, should make sure they properly handle
         * corner cases of the API, such as regulator_get_voltage()
         * returning 0.
         */             
        return NULL;
}

my reading was that the expected behavior in any driver was:

* call regulator_get()
* if IS_ERR(), fail device probe function, never use invalid
  pointer other than PTR_ERR()
* if NULL, and regulator is required, fail probe so we never
  use the regulator
* if NULL, and regulators are optional, continue with the NULL
  value.
* drivers never look into the regulator pointer, and only
  pass it into regulator APIs which can cope with the NULL
  value when CONFIG_REGULATOR is disabled.

That would be similar to what we have for clocks. Which part of
my interpretation is wrong?

	Arnd