Re: [PATCH v2 1/2] iio: adc: ti-ads7950: Allow to use on ACPI platforms

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 01 Aug 2017 20:24:31 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Tue, 2017-08-01 at 12:15 -0500, David Lechner wrote:
> > On 08/01/2017 11:41 AM, Andy Shevchenko wrote:  
> > > On Tue, 2017-08-01 at 11:21 -0500, David Lechner wrote:  
> > > > On 08/01/2017 10:45 AM, Andy Shevchenko wrote:  
> > > > >       
> > > > > > > > +	/* Use hard coded value for reference voltage in
> > > > > > > > ACPI
> > > > > > > > case */
> > > > > > > > +	if (ACPI_COMPANION(&spi->dev))
> > > > > > > > +		st->vref_mv =
> > > > > > > > TI_ADS7950_VA_MV_ACPI_DEFAULT;  
> > > > > > > 
> > > > > > > Instead of checking or ACPI, you could just say "if we have
> > > > > > > a
> > > > > > > dummy
> > > > > > > regulator, then use the default value".  
> > > > > 
> > > > >   
> > > > > > Agreed. Sounds sensible to me.  Hopefully in DT people will
> > > > > > provide the right regulator, but chances are this won't
> > > > > > always happen.  
> > > > > 
> > > > > There is no call like
> > > > > regulator_is_dummy()
> > > > > (and, looking into the code of regulator framework, can't be)
> > > > > 
> > > > > Can you elaborate a bit, maybe I'm missing something obvious?
> > > > >   
> > > > 
> > > > I haven't tested this, but shouldn't regulator_get_voltage()
> > > > return
> > > > an
> > > > error for a dummy regulator? You could use this as your test.  
> > > 
> > > While it would work it's very fragile.
Hmm. The optional get is what we have always used when a regulator
has been added to a drivers bindings after the initial merge.
In that case there is little choice (particularly as the one
added is often the power supply regulator rather than anything
related to scale.

If you want to do the scale thing, then you want to not expose
the attribute at all if the scale isn't available.  So do
it by swapping the iio_chan_spec array for one without the
scale bit set for the relevant channels.

So if we want to support as described (using the default)
then the optional regulator get is the only way to go that
I can think of...

To a degree, as it was originally in the bindings for this one
tough luck if it's not specified.  Someone didn't implement
the dt properly...   So I wouldn't do the fall back at all.

Jonathan
> > > 
> > > _regulator_get_voltage() will return error code even for normal
> > > regulators if something happened there. And user gets an impression
> > > that
> > > everything is okay while it's not.
> > > 
> > > So, I wouldn't go this way.  
> >   
> 
> > In drivers/iio/adc/max11100.c:91. The solution was to only support
> > raw 
> > value and not scaled if there is a dummy regulator.  
> 
> The comment there is just partially correct. While get_voltage() indeed
> returns -EINVAL for dummy it might return same or other error code for
> non-dummy regulator.
> 
> So, by the fact the code there should be done like in the rest (the
> below is mangled, can't be used directly as a patch):
> 
> --- a/drivers/iio/adc/max11100.c
> +++ b/drivers/iio/adc/max11100.c
> @@ -88,8 +88,7 @@ static int max11100_read_raw(struct iio_dev
> *indio_dev,
>         case IIO_CHAN_INFO_SCALE:
>                 vref_uv = regulator_get_voltage(state->vref_reg);
>                 if (vref_uv < 0)
> -                       /* dummy regulator "get_voltage" returns -EINVAL
> */
> -                       return -EINVAL;
> +                       return vref_uv;
> 
>                 *val =  vref_uv / 1000;
>                 *val2 = MAX11100_LSB_DIV;
> 
>