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

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 13 Aug 2017 17:25:50 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Wed, 2017-08-09 at 14:24 +0100, Jonathan Cameron wrote:
> > 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.  
> 
> ...and what is the conclusion to the patch itself? I didn't see any
> other way is to check ACPI_HANDLE() for ACPI case and leave the rest as
> is.

Good point.  I'd gotten so caught up in the thread I'd failed
to notice that the patch actually did what the end conclusion
was (as much as there was one).

Applied to the togreg branch of iio.git and pushed out as testing.

Thanks,

Jonathan

>