[PATCH v2 0/2] iio: ti-ads7950: Make it working on ACPI platforms

[PATCH v2 0/2] iio: ti-ads7950: Make it working on ACPI platforms

[Andy Shevchenko]

ACPI has no regulators, thus we need to provide a reference voltage
value for ACPI platforms for now. I leave regulator request in place
because for ACPI we will get dummy regulator. This is done in patch 1.

Since we have no dedicated ACPI ID we may use compatible string, so,
patch 2 adds them.

In v2:
- correct subject in patch 1
- add tag for patch 2
- change patch 1 to leave a regulator request in place
- do not add tag for patch 1 because of above

Andy Shevchenko (2):
  iio: adc: ti-ads7950: Allow to use on ACPI platforms
  iio: adc: ti-ads7950: Add OF device ID table

 drivers/iio/adc/ti-ads7950.c | 42 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 38 insertions(+), 4 deletions(-)

-- 
2.13.2

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

[Andy Shevchenko]

ACPI enabled platforms do not have a mean of regulators. Instead we use
hard coded voltage value for reference pin. When value is 0 (default) we
fall back to request a regulator.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/adc/ti-ads7950.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
index 16a06633332c..ab669af291b7 100644
--- a/drivers/iio/adc/ti-ads7950.c
+++ b/drivers/iio/adc/ti-ads7950.c
@@ -21,6 +21,7 @@
  * GNU General Public License for more details.
  */
 
+#include <linux/acpi.h>
 #include <linux/bitops.h>
 #include <linux/device.h>
 #include <linux/err.h>
@@ -37,6 +38,12 @@
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iio/triggered_buffer.h>
 
+/*
+ * In case of ACPI, we use the 5000 mV as default for the reference pin.
+ * Device tree users encode that via the vref-supply regulator.
+ */
+#define TI_ADS7950_VA_MV_ACPI_DEFAULT	5000
+
 #define TI_ADS7950_CR_MANUAL	BIT(12)
 #define TI_ADS7950_CR_WRITE	BIT(11)
 #define TI_ADS7950_CR_CHAN(ch)	((ch) << 7)
@@ -58,6 +65,7 @@ struct ti_ads7950_state {
 	struct spi_message	scan_single_msg;
 
 	struct regulator	*reg;
+	unsigned int		vref_mv;
 
 	unsigned int		settings;
 
@@ -305,11 +313,15 @@ static int ti_ads7950_get_range(struct ti_ads7950_state *st)
 {
 	int vref;
 
-	vref = regulator_get_voltage(st->reg);
-	if (vref < 0)
-		return vref;
+	if (st->vref_mv) {
+		vref = st->vref_mv;
+	} else {
+		vref = regulator_get_voltage(st->reg);
+		if (vref < 0)
+			return vref;
 
-	vref /= 1000;
+		vref /= 1000;
+	}
 
 	if (st->settings & TI_ADS7950_CR_RANGE_5V)
 		vref *= 2;
@@ -411,6 +423,10 @@ static int ti_ads7950_probe(struct spi_device *spi)
 	spi_message_init_with_transfers(&st->scan_single_msg,
 					st->scan_single_xfer, 3);
 
+	/* Use hard coded value for reference voltage in ACPI case */
+	if (ACPI_COMPANION(&spi->dev))
+		st->vref_mv = TI_ADS7950_VA_MV_ACPI_DEFAULT;
+
 	st->reg = devm_regulator_get(&spi->dev, "vref");
 	if (IS_ERR(st->reg)) {
 		dev_err(&spi->dev, "Failed get get regulator \"vref\"\n");
-- 
2.13.2

[PATCH v2 2/2] iio: adc: ti-ads7950: Add OF device ID table

[Andy Shevchenko]

The driver doesn't have a struct of_device_id table but supported devices
are registered via Device Trees. This is working on the assumption that a
SPI device registered via OF will always match a legacy SPI device ID and
that the MODALIAS reported will always be of the form spi:<device>.

There is an ACPI method to enumerate such devices via specific ACPI ID
and use of compatible strings. It will not work for the drivers which
have no OF match ID table present.

Besides this could change in the future so the correct approach is to
have an OF device ID table if the devices are registered via OF.

Tested-by: David Lechner <david@lechnology.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/adc/ti-ads7950.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
index ab669af291b7..a376190914ad 100644
--- a/drivers/iio/adc/ti-ads7950.c
+++ b/drivers/iio/adc/ti-ads7950.c
@@ -491,9 +491,27 @@ static const struct spi_device_id ti_ads7950_id[] = {
 };
 MODULE_DEVICE_TABLE(spi, ti_ads7950_id);
 
+static const struct of_device_id ads7950_of_table[] = {
+	{ .compatible = "ti,ads7950", .data = &ti_ads7950_chip_info[TI_ADS7950] },
+	{ .compatible = "ti,ads7951", .data = &ti_ads7950_chip_info[TI_ADS7951] },
+	{ .compatible = "ti,ads7952", .data = &ti_ads7950_chip_info[TI_ADS7952] },
+	{ .compatible = "ti,ads7953", .data = &ti_ads7950_chip_info[TI_ADS7953] },
+	{ .compatible = "ti,ads7954", .data = &ti_ads7950_chip_info[TI_ADS7954] },
+	{ .compatible = "ti,ads7955", .data = &ti_ads7950_chip_info[TI_ADS7955] },
+	{ .compatible = "ti,ads7956", .data = &ti_ads7950_chip_info[TI_ADS7956] },
+	{ .compatible = "ti,ads7957", .data = &ti_ads7950_chip_info[TI_ADS7957] },
+	{ .compatible = "ti,ads7958", .data = &ti_ads7950_chip_info[TI_ADS7958] },
+	{ .compatible = "ti,ads7959", .data = &ti_ads7950_chip_info[TI_ADS7959] },
+	{ .compatible = "ti,ads7960", .data = &ti_ads7950_chip_info[TI_ADS7960] },
+	{ .compatible = "ti,ads7961", .data = &ti_ads7950_chip_info[TI_ADS7961] },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ads7950_of_table);
+
 static struct spi_driver ti_ads7950_driver = {
 	.driver = {
 		.name	= "ads7950",
+		.of_match_table = ads7950_of_table,
 	},
 	.probe		= ti_ads7950_probe,
 	.remove		= ti_ads7950_remove,
-- 
2.13.2

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

[David Lechner]

On 07/28/2017 05:20 PM, Andy Shevchenko wrote:
> ACPI enabled platforms do not have a mean of regulators. Instead we use
> hard coded voltage value for reference pin. When value is 0 (default) we
> fall back to request a regulator.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   drivers/iio/adc/ti-ads7950.c | 24 ++++++++++++++++++++----
>   1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
> index 16a06633332c..ab669af291b7 100644
> --- a/drivers/iio/adc/ti-ads7950.c
> +++ b/drivers/iio/adc/ti-ads7950.c
> @@ -21,6 +21,7 @@
>    * GNU General Public License for more details.
>    */
>   
> +#include <linux/acpi.h>
>   #include <linux/bitops.h>
>   #include <linux/device.h>
>   #include <linux/err.h>
> @@ -37,6 +38,12 @@
>   #include <linux/iio/trigger_consumer.h>
>   #include <linux/iio/triggered_buffer.h>
>   
> +/*
> + * In case of ACPI, we use the 5000 mV as default for the reference pin.
> + * Device tree users encode that via the vref-supply regulator.
> + */
> +#define TI_ADS7950_VA_MV_ACPI_DEFAULT	5000

Now that I've thought about it a bit, you don't need to call this the 
ACPI default, just call it the default. Then it will be obvious that 
this is used when you don't have a regulator.

> +
>   #define TI_ADS7950_CR_MANUAL	BIT(12)
>   #define TI_ADS7950_CR_WRITE	BIT(11)
>   #define TI_ADS7950_CR_CHAN(ch)	((ch) << 7)
> @@ -58,6 +65,7 @@ struct ti_ads7950_state {
>   	struct spi_message	scan_single_msg;
>   
>   	struct regulator	*reg;
> +	unsigned int		vref_mv;
>   
>   	unsigned int		settings;
>   
> @@ -305,11 +313,15 @@ static int ti_ads7950_get_range(struct ti_ads7950_state *st)
>   {
>   	int vref;
>   
> -	vref = regulator_get_voltage(st->reg);
> -	if (vref < 0)
> -		return vref;
> +	if (st->vref_mv) {
> +		vref = st->vref_mv;
> +	} else {
> +		vref = regulator_get_voltage(st->reg);
> +		if (vref < 0)
> +			return vref;
>   
> -	vref /= 1000;
> +		vref /= 1000;
> +	}
>   
>   	if (st->settings & TI_ADS7950_CR_RANGE_5V)
>   		vref *= 2;
> @@ -411,6 +423,10 @@ static int ti_ads7950_probe(struct spi_device *spi)
>   	spi_message_init_with_transfers(&st->scan_single_msg,
>   					st->scan_single_xfer, 3);
>   
> +	/* 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".

> +
>   	st->reg = devm_regulator_get(&spi->dev, "vref");
>   	if (IS_ERR(st->reg)) {
>   		dev_err(&spi->dev, "Failed get get regulator \"vref\"\n");
> 

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

[Jonathan Cameron]

On Sat, 29 Jul 2017 20:27:13 -0500
David Lechner <david@lechnology.com> wrote:

> On 07/28/2017 05:20 PM, Andy Shevchenko wrote:
> > ACPI enabled platforms do not have a mean of regulators. Instead we use
> > hard coded voltage value for reference pin. When value is 0 (default) we
> > fall back to request a regulator.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >   drivers/iio/adc/ti-ads7950.c | 24 ++++++++++++++++++++----
> >   1 file changed, 20 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
> > index 16a06633332c..ab669af291b7 100644
> > --- a/drivers/iio/adc/ti-ads7950.c
> > +++ b/drivers/iio/adc/ti-ads7950.c
> > @@ -21,6 +21,7 @@
> >    * GNU General Public License for more details.
> >    */
> >   
> > +#include <linux/acpi.h>
> >   #include <linux/bitops.h>
> >   #include <linux/device.h>
> >   #include <linux/err.h>
> > @@ -37,6 +38,12 @@
> >   #include <linux/iio/trigger_consumer.h>
> >   #include <linux/iio/triggered_buffer.h>
> >   
> > +/*
> > + * In case of ACPI, we use the 5000 mV as default for the reference pin.
> > + * Device tree users encode that via the vref-supply regulator.
> > + */
> > +#define TI_ADS7950_VA_MV_ACPI_DEFAULT	5000  
> 
> Now that I've thought about it a bit, you don't need to call this the 
> ACPI default, just call it the default. Then it will be obvious that 
> this is used when you don't have a regulator.
> 
> > +
> >   #define TI_ADS7950_CR_MANUAL	BIT(12)
> >   #define TI_ADS7950_CR_WRITE	BIT(11)
> >   #define TI_ADS7950_CR_CHAN(ch)	((ch) << 7)
> > @@ -58,6 +65,7 @@ struct ti_ads7950_state {
> >   	struct spi_message	scan_single_msg;
> >   
> >   	struct regulator	*reg;
> > +	unsigned int		vref_mv;
> >   
> >   	unsigned int		settings;
> >   
> > @@ -305,11 +313,15 @@ static int ti_ads7950_get_range(struct ti_ads7950_state *st)
> >   {
> >   	int vref;
> >   
> > -	vref = regulator_get_voltage(st->reg);
> > -	if (vref < 0)
> > -		return vref;
> > +	if (st->vref_mv) {
> > +		vref = st->vref_mv;
> > +	} else {
> > +		vref = regulator_get_voltage(st->reg);
> > +		if (vref < 0)
> > +			return vref;
> >   
> > -	vref /= 1000;
> > +		vref /= 1000;
> > +	}
> >   
> >   	if (st->settings & TI_ADS7950_CR_RANGE_5V)
> >   		vref *= 2;
> > @@ -411,6 +423,10 @@ static int ti_ads7950_probe(struct spi_device *spi)
> >   	spi_message_init_with_transfers(&st->scan_single_msg,
> >   					st->scan_single_xfer, 3);
> >   
> > +	/* 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.

Jonathan
> > +
> >   	st->reg = devm_regulator_get(&spi->dev, "vref");
> >   	if (IS_ERR(st->reg)) {
> >   		dev_err(&spi->dev, "Failed get get regulator \"vref\"\n");
> >   
> 

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

[Andy Shevchenko]

On Sun, 2017-07-30 at 14:31 +0100, Jonathan Cameron wrote:
> On Sat, 29 Jul 2017 20:27:13 -0500
> David Lechner <david@lechnology.com> wrote:
> 

Thanks for review! My answers below.

> > On 07/28/2017 05:20 PM, Andy Shevchenko wrote:
> > > ACPI enabled platforms do not have a mean of regulators. Instead
> > > we use
> > > hard coded voltage value for reference pin. When value is 0
> > > (default) we
> > > fall back to request a regulator.

> > > +/*
> > > + * In case of ACPI, we use the 5000 mV as default for the
> > > reference pin.
> > > + * Device tree users encode that via the vref-supply regulator.
> > > + */
> > > +#define TI_ADS7950_VA_MV_ACPI_DEFAULT	5000  
> > 
> > Now that I've thought about it a bit, you don't need to call this
> > the 
> > ACPI default, just call it the default. Then it will be obvious
> > that 
> > this is used when you don't have a regulator.

OK.
  
> > > +	/* 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?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 2/2] iio: adc: ti-ads7950: Add OF device ID table

[Andy Shevchenko]

On Sat, 2017-07-29 at 01:20 +0300, Andy Shevchenko wrote:
> The driver doesn't have a struct of_device_id table but supported
> devices
> are registered via Device Trees. This is working on the assumption
> that a
> SPI device registered via OF will always match a legacy SPI device ID
> and
> that the MODALIAS reported will always be of the form spi:<device>.
> 
> There is an ACPI method to enumerate such devices via specific ACPI ID
> and use of compatible strings. It will not work for the drivers which
> have no OF match ID table present.
> 
> Besides this could change in the future so the correct approach is to
> have an OF device ID table if the devices are registered via OF.
> 

Btw, this patch can be applied independently.

For ACPI case both of them are needed, and order doesn't matter.

> Tested-by: David Lechner <david@lechnology.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/iio/adc/ti-ads7950.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-
> ads7950.c
> index ab669af291b7..a376190914ad 100644
> --- a/drivers/iio/adc/ti-ads7950.c
> +++ b/drivers/iio/adc/ti-ads7950.c
> @@ -491,9 +491,27 @@ static const struct spi_device_id ti_ads7950_id[]
> = {
>  };
>  MODULE_DEVICE_TABLE(spi, ti_ads7950_id);
>  
> +static const struct of_device_id ads7950_of_table[] = {
> +	{ .compatible = "ti,ads7950", .data =
> &ti_ads7950_chip_info[TI_ADS7950] },
> +	{ .compatible = "ti,ads7951", .data =
> &ti_ads7950_chip_info[TI_ADS7951] },
> +	{ .compatible = "ti,ads7952", .data =
> &ti_ads7950_chip_info[TI_ADS7952] },
> +	{ .compatible = "ti,ads7953", .data =
> &ti_ads7950_chip_info[TI_ADS7953] },
> +	{ .compatible = "ti,ads7954", .data =
> &ti_ads7950_chip_info[TI_ADS7954] },
> +	{ .compatible = "ti,ads7955", .data =
> &ti_ads7950_chip_info[TI_ADS7955] },
> +	{ .compatible = "ti,ads7956", .data =
> &ti_ads7950_chip_info[TI_ADS7956] },
> +	{ .compatible = "ti,ads7957", .data =
> &ti_ads7950_chip_info[TI_ADS7957] },
> +	{ .compatible = "ti,ads7958", .data =
> &ti_ads7950_chip_info[TI_ADS7958] },
> +	{ .compatible = "ti,ads7959", .data =
> &ti_ads7950_chip_info[TI_ADS7959] },
> +	{ .compatible = "ti,ads7960", .data =
> &ti_ads7950_chip_info[TI_ADS7960] },
> +	{ .compatible = "ti,ads7961", .data =
> &ti_ads7950_chip_info[TI_ADS7961] },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, ads7950_of_table);
> +
>  static struct spi_driver ti_ads7950_driver = {
>  	.driver = {
>  		.name	= "ads7950",
> +		.of_match_table = ads7950_of_table,
>  	},
>  	.probe		= ti_ads7950_probe,
>  	.remove		= ti_ads7950_remove,

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[David Lechner]

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.

static int ti_ads7950_get_range(struct ti_ads7950_state *st)
{
	int vref;

	vref = regulator_get_voltage(st->reg);
	if (vref < 0)
-		return vref;
+		vref = 2500000;

	vref /= 1000;

	if (st->settings & TI_ADS7950_CR_RANGE_5V)
		vref *= 2;

	return vref;
}

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

[Andy Shevchenko]

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.

_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.

> 
> static int ti_ads7950_get_range(struct ti_ads7950_state *st)
> {
> 	int vref;
> 
> 	vref = regulator_get_voltage(st->reg);
> 	if (vref < 0)
> -		return vref;
> +		vref = 2500000;
> 
> 	vref /= 1000;
> 
> 	if (st->settings & TI_ADS7950_CR_RANGE_5V)
> 		vref *= 2;
> 
> 	return vref;
> }
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[David Lechner]

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.
> 
> _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.
> 
>>
>> static int ti_ads7950_get_range(struct ti_ads7950_state *st)
>> {
>> 	int vref;
>>
>> 	vref = regulator_get_voltage(st->reg);
>> 	if (vref < 0)
>> -		return vref;
>> +		vref = 2500000;
>>
>> 	vref /= 1000;
>>
>> 	if (st->settings & TI_ADS7950_CR_RANGE_5V)
>> 		vref *= 2;
>>
>> 	return vref;
>> }
>>
> 

OK. There is also devm_regulator_get_optional(). This will return 
ERR_PTR(-ENODEV) instead of a dummy regulator. Then you could add a 
check for this error and set st->reg = NULL if we get -ENODEV. Make the 
enable and disable conditional. And use the default voltage if we don't 
have a regulator.


But this is probably even messier than the original ACPI patch. :-/

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

[David Lechner]

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.
> 
> _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.

> 
>>
>> static int ti_ads7950_get_range(struct ti_ads7950_state *st)
>> {
>> 	int vref;
>>
>> 	vref = regulator_get_voltage(st->reg);
>> 	if (vref < 0)
>> -		return vref;
>> +		vref = 2500000;
>>
>> 	vref /= 1000;
>>
>> 	if (st->settings & TI_ADS7950_CR_RANGE_5V)
>> 		vref *= 2;
>>
>> 	return vref;
>> }
>>
> 

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

[Andy Shevchenko]

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.
> > 
> > _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;


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[Andy Shevchenko]

On Tue, 2017-08-01 at 12:09 -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.
> > 
> > _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.

> OK. There is also devm_regulator_get_optional(). This will return 
> ERR_PTR(-ENODEV) instead of a dummy regulator. Then you could add a 
> check for this error and set st->reg = NULL if we get -ENODEV. Make
> the 
> enable and disable conditional. And use the default voltage if we
> don't 
> have a regulator.

Luckily it is consistent for now with error code, so -ENODEV it is, but
see comment below...

reg = devm_regulator_get_optional();
if (IS_ERR()) {
  if (PTR_ERR() != -ENODEV)
    return PTR_ERR();

  /*
OK     >> dummy, or absent regulator, or ... 
Not OK >> supply can't be found, or ->ops->enable() returns -ENODEV
   */

Less fragile, but still fragile...

}

> But this is probably even messier than the original ACPI patch. :-/

It's still no-go from my point of view.

So... ?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[Jonathan Cameron]

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;
> 
> 

Re: [PATCH v2 2/2] iio: adc: ti-ads7950: Add OF device ID table

[Jonathan Cameron]

On Tue, 01 Aug 2017 18:48:58 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Sat, 2017-07-29 at 01:20 +0300, Andy Shevchenko wrote:
> > The driver doesn't have a struct of_device_id table but supported
> > devices
> > are registered via Device Trees. This is working on the assumption
> > that a
> > SPI device registered via OF will always match a legacy SPI device ID
> > and
> > that the MODALIAS reported will always be of the form spi:<device>.
> > 
> > There is an ACPI method to enumerate such devices via specific ACPI ID
> > and use of compatible strings. It will not work for the drivers which
> > have no OF match ID table present.
> > 
> > Besides this could change in the future so the correct approach is to
> > have an OF device ID table if the devices are registered via OF.
> >   
> 
> Btw, this patch can be applied independently.
> 
> For ACPI case both of them are needed, and order doesn't matter.
Good point.  Applied. 

Thanks,

Jonathan
> 
> > Tested-by: David Lechner <david@lechnology.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/iio/adc/ti-ads7950.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-
> > ads7950.c
> > index ab669af291b7..a376190914ad 100644
> > --- a/drivers/iio/adc/ti-ads7950.c
> > +++ b/drivers/iio/adc/ti-ads7950.c
> > @@ -491,9 +491,27 @@ static const struct spi_device_id ti_ads7950_id[]
> > = {
> >  };
> >  MODULE_DEVICE_TABLE(spi, ti_ads7950_id);
> >  
> > +static const struct of_device_id ads7950_of_table[] = {
> > +	{ .compatible = "ti,ads7950", .data =
> > &ti_ads7950_chip_info[TI_ADS7950] },
> > +	{ .compatible = "ti,ads7951", .data =
> > &ti_ads7950_chip_info[TI_ADS7951] },
> > +	{ .compatible = "ti,ads7952", .data =
> > &ti_ads7950_chip_info[TI_ADS7952] },
> > +	{ .compatible = "ti,ads7953", .data =
> > &ti_ads7950_chip_info[TI_ADS7953] },
> > +	{ .compatible = "ti,ads7954", .data =
> > &ti_ads7950_chip_info[TI_ADS7954] },
> > +	{ .compatible = "ti,ads7955", .data =
> > &ti_ads7950_chip_info[TI_ADS7955] },
> > +	{ .compatible = "ti,ads7956", .data =
> > &ti_ads7950_chip_info[TI_ADS7956] },
> > +	{ .compatible = "ti,ads7957", .data =
> > &ti_ads7950_chip_info[TI_ADS7957] },
> > +	{ .compatible = "ti,ads7958", .data =
> > &ti_ads7950_chip_info[TI_ADS7958] },
> > +	{ .compatible = "ti,ads7959", .data =
> > &ti_ads7950_chip_info[TI_ADS7959] },
> > +	{ .compatible = "ti,ads7960", .data =
> > &ti_ads7950_chip_info[TI_ADS7960] },
> > +	{ .compatible = "ti,ads7961", .data =
> > &ti_ads7950_chip_info[TI_ADS7961] },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, ads7950_of_table);
> > +
> >  static struct spi_driver ti_ads7950_driver = {
> >  	.driver = {
> >  		.name	= "ads7950",
> > +		.of_match_table = ads7950_of_table,
> >  	},
> >  	.probe		= ti_ads7950_probe,
> >  	.remove		= ti_ads7950_remove,  
> 

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

[Andy Shevchenko]

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.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[Jonathan Cameron]

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

>