iio: ti-ads7950: Make it working on ACPI platforms

PATCH In review Andy Shevchenko 3 participants 17 messages Jul 28, 2017 – Aug 20, 2017 IIO

Share & tools

Andy Shevchenko

author

2017-07-28 22:20 UTC #0 lore raw

v2 cover iio: ti-ads7950: Make it working on ACPI platforms

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(-)

Signature · 1 line

2.13.2

Andy Shevchenko

author

2017-07-28 22:20 UTC #1 lore raw

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

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 <[email protected]>

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

drivers/iio/adc/ti-ads7950.c +20 -4

index 16a06633332c..ab669af291b7 100644
@@ -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");

Signature · 1 line

2.13.2

David Lechner

2017-07-30 01:27 UTC #2 lore raw

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

Quoted 22 lines · 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 <[email protected]>
---
  drivers/iio/adc/ti-ads7950.c | 24 ++++++++++++++++++++----
  1 file changed, 20 insertions(+), 4 deletions(-)
drivers/iio/adc/ti-ads7950.c +1 -0
index 16a06633332c..ab669af291b7 100644
@@ -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>

drivers/iio/adc/ti-ads7950.c line 45

Quoted 8 lines

drivers/iio/adc/ti-ads7950.c +5 -0

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

Quoted 33 lines

drivers/iio/adc/ti-ads7950.c +10 -4

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

drivers/iio/adc/ti-ads7950.c static int ti_ads7950_probe(struct spi_device *spi) · line 428

Quoted 6 lines

drivers/iio/adc/ti-ads7950.c +3 -0

@@ -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".

Quoted 5 lines

drivers/iio/adc/ti-ads7950.c +1 -0

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

Jonathan Cameron

maintainer

2017-07-30 13:32 UTC #3 lore raw

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

Quoted 72 lines · On Sat, 29 Jul 2017 20:27:13 -0500 David Lechner <[email protected]> wrote:

Quoted 22 lines · 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 <[email protected]>
---
  drivers/iio/adc/ti-ads7950.c | 24 ++++++++++++++++++++----
  1 file changed, 20 insertions(+), 4 deletions(-)
drivers/iio/adc/ti-ads7950.c +1 -0
index 16a06633332c..ab669af291b7 100644
@@ -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>

drivers/iio/adc/ti-ads7950.c line 45

Quoted 8 lines

drivers/iio/adc/ti-ads7950.c +5 -0

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

Quoted 36 lines

drivers/iio/adc/ti-ads7950.c +10 -4

+
  #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);

Quoted 7 lines

drivers/iio/adc/ti-ads7950.c static int ti_ads7950_probe(struct spi_device *spi) · line 428
Quoted 3 lines
drivers/iio/adc/ti-ads7950.c +3 -0
+	/* 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

Quoted 6 lines

Quoted 5 lines

drivers/iio/adc/ti-ads7950.c +1 -0

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

Andy Shevchenko

author

2017-08-01 15:45 UTC #4 lore raw

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

Quoted 3 lines · Jonathan Cameron

On Sat, 29 Jul 2017 20:27:13 -0500
David Lechner <[email protected]> wrote:

Thanks for review! My answers below.

Quoted 7 lines

Quoted 6 lines · Andy Shevchenko
Quoted 5 lines
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.

Quoted 12 lines

Quoted 4 lines
Quoted 4 lines
+/*
+ * 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.
Quoted 8 lines
Quoted 2 lines
+ */
+#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.

Quoted 9 lines

Quoted 7 lines
drivers/iio/adc/ti-ads7950.c static int ti_ads7950_probe(struct spi_device *spi) · line 428
Quoted 4 lines
(quoted diff) +1 -0
+	/* Use hard coded value for reference voltage in ACPI
case */
drivers/iio/adc/ti-ads7950.c +2 -0
+	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".

Quoted 3 lines

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?

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

David Lechner

2017-08-01 16:21 UTC #5 lore raw

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

Quoted 14 lines · Andy Shevchenko

Quoted 10 lines
Quoted 8 lines
drivers/iio/adc/ti-ads7950.c static int ti_ads7950_probe(struct spi_device *spi) · line 428
Quoted 5 lines
(quoted diff) +1 -0
+	/* Use hard coded value for reference voltage in ACPI
case */
drivers/iio/adc/ti-ads7950.c +2 -0
+	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".
Quoted 3 lines
Agreed. Sounds sensible to me. Hopefully in DT people will provide the right regulator, but chances are this won't always happen.

Quoted 6 lines

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

Andy Shevchenko

author

2017-08-01 16:41 UTC #6 lore raw

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

Quoted 20 lines · David Lechner

Quoted 19 lines · Andy Shevchenko
Quoted 10 lines
Quoted 8 lines
drivers/iio/adc/ti-ads7950.c static int ti_ads7950_probe(struct spi_device *spi) · line 428
Quoted 4 lines
(quoted diff) +1 -0
+	/* Use hard coded value for reference voltage in ACPI
case */
drivers/iio/adc/ti-ads7950.c +2 -0
+	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".
Quoted 3 lines
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)

Quoted 6 lines

Quoted 2 lines
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.

Quoted 18 lines

static int ti_ads7950_get_range(struct ti_ads7950_state *st)

{
	int vref;

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

(quoted diff) +1 -1

-		return vref;
+		vref = 2500000;

	vref /= 1000;

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

	return vref;
}

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

David Lechner

2017-08-01 17:09 UTC #7 lore raw

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

Quoted 46 lines · Andy Shevchenko

Quoted 20 lines · David Lechner
Quoted 19 lines · Andy Shevchenko
Quoted 11 lines
Quoted 9 lines
drivers/iio/adc/ti-ads7950.c static int ti_ads7950_probe(struct spi_device *spi) · line 428
Quoted 5 lines
(quoted diff) +1 -0
+	/* Use hard coded value for reference voltage in ACPI
case */
drivers/iio/adc/ti-ads7950.c +2 -0
+	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".
Quoted 3 lines
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)
Quoted 6 lines
Quoted 2 lines
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.
Quoted 10 lines
static int ti_ads7950_get_range(struct ti_ads7950_state *st)
{
	int vref;

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

Quoted 9 lines

Quoted 8 lines

	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. :-/

Andy Shevchenko

author

2017-08-01 17:44 UTC #8 lore raw

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

Quoted 41 lines · David Lechner

Quoted 31 lines · Andy Shevchenko
Quoted 22 lines · David Lechner
Quoted 21 lines · Andy Shevchenko
Quoted 12 lines
Quoted 11 lines
Quoted 6 lines
(quoted diff) +1 -0
+	/* Use hard coded value for reference voltage in
ACPI
case */
(quoted diff) +2 -0
+	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".
Quoted 3 lines
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)
Quoted 7 lines
Quoted 2 lines
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.
Quoted 8 lines
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.

Quoted 7 lines

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

}

Quoted 1 line

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

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

So... ?

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

David Lechner

2017-08-01 17:15 UTC #9 lore raw

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

In reply to Andy Shevchenko

Quoted 28 lines · Andy Shevchenko

Quoted 20 lines · David Lechner
Quoted 19 lines · Andy Shevchenko
Quoted 11 lines
Quoted 9 lines
drivers/iio/adc/ti-ads7950.c static int ti_ads7950_probe(struct spi_device *spi) · line 428
Quoted 5 lines
(quoted diff) +1 -0
+	/* Use hard coded value for reference voltage in ACPI
case */
drivers/iio/adc/ti-ads7950.c +2 -0
+	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".
Quoted 3 lines
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)
Quoted 6 lines
Quoted 2 lines
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.

Quoted 7 lines

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.

Quoted 20 lines

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

	vref = regulator_get_voltage(st->reg);
	if (vref < 0)
(quoted diff) +1 -1
-		return vref;
+		vref = 2500000;
	vref /= 1000;

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

	return vref;
}

Andy Shevchenko

author

2017-08-01 17:24 UTC #10 lore raw

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

Quoted 40 lines · David Lechner

Quoted 39 lines · Andy Shevchenko
Quoted 23 lines · David Lechner
Quoted 22 lines · Andy Shevchenko
Quoted 13 lines
Quoted 11 lines
Quoted 6 lines
(quoted diff) +1 -0
+	/* Use hard coded value for reference voltage in
ACPI
case */
(quoted diff) +2 -0
+	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".
Quoted 3 lines
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)
Quoted 7 lines
Quoted 2 lines
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.

Quoted 6 lines

Quoted 1 line
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):

drivers/iio/adc/max11100.c +0 -0

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

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Jonathan Cameron

maintainer

2017-08-09 13:25 UTC #11 lore raw

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

Quoted 29 lines · On Tue, 01 Aug 2017 20:24:31 +0300 Andy Shevchenko…

Quoted 28 lines · David Lechner
Quoted 27 lines · Andy Shevchenko
Quoted 26 lines · David Lechner
Quoted 24 lines · Andy Shevchenko
Quoted 13 lines
Quoted 11 lines
Quoted 6 lines
(quoted diff) +1 -0
+	/* Use hard coded value for reference voltage in
ACPI
case */
(quoted diff) +2 -0
+	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".
Quoted 3 lines
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?

Quoted 6 lines

Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
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

Quoted 36 lines

Quoted 6 lines
Quoted 6 lines
_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.
Quoted 6 lines
Quoted 1 line
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):
drivers/iio/adc/max11100.c +0 -0
@@ -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

author

2017-08-13 14:25 UTC #12 lore raw

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

Quoted 56 lines · Jonathan Cameron

Quoted 34 lines · On Tue, 01 Aug 2017 20:24:31 +0300 Andy Shevchenko…
Quoted 33 lines · David Lechner
Quoted 32 lines · Andy Shevchenko
Quoted 31 lines · David Lechner
Quoted 29 lines · Andy Shevchenko
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
(quoted diff) +1 -0
+	/* Use hard coded value for reference voltage
in
ACPI
case */
(quoted diff) +1 -0
+	if (ACPI_COMPANION(&spi->dev))
Quoted 8 lines
Quoted 8 lines
Quoted 2 lines
(quoted diff) +1 -0
+		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".
Quoted 4 lines
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?
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
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.

Quoted 7 lines

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.

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Jonathan Cameron

maintainer

2017-08-20 11:05 UTC #13 lore raw

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

Quoted 61 lines · On Sun, 13 Aug 2017 17:25:50 +0300 Andy Shevchenko…

Quoted 60 lines · Jonathan Cameron
Quoted 34 lines · On Tue, 01 Aug 2017 20:24:31 +0300 Andy Shevchenko…
Quoted 33 lines · David Lechner
Quoted 32 lines · Andy Shevchenko
Quoted 31 lines · David Lechner
Quoted 29 lines · Andy Shevchenko
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
(quoted diff) +1 -0
+	/* Use hard coded value for reference voltage
in
ACPI
case */
(quoted diff) +1 -0
+	if (ACPI_COMPANION(&spi->dev))
Quoted 8 lines
Quoted 8 lines
Quoted 2 lines
(quoted diff) +1 -0
+		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".
Quoted 4 lines
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?
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
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...

Quoted 7 lines

Quoted 3 lines
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

Quoted 1 line

Andy Shevchenko

author

2017-07-28 22:20 UTC #14 lore raw

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

In reply to 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 <[email protected]>

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/iio/adc/ti-ads7950.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

drivers/iio/adc/ti-ads7950.c +18 -0

index ab669af291b7..a376190914ad 100644
@@ -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,

Signature · 1 line

2.13.2

Andy Shevchenko

author

2017-08-01 15:48 UTC #15 lore raw

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

Quoted 8 lines · 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>.

Quoted 7 lines

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.

Quoted 52 lines

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

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Jonathan Cameron

maintainer

2017-08-09 13:27 UTC #16 lore raw

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

Quoted 13 lines · On Tue, 01 Aug 2017 18:48:58 +0300 Andy Shevchenko…

Quoted 12 lines · 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.

Quoted 7 lines

Quoted 3 lines
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

Quoted 54 lines

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