Add support for external reference voltage through the regulator framework

PATCH Superseded Silvan Murer 2 participants 5 messages May 15, 2018 – May 20, 2018

Subsystems (2)

Device tree bindings IIO

lore

Share & tools

lore

Silvan Murer

author

2018-05-15 20:14 UTC #0 lore raw

PATCH v2 1/1 Add support for external reference voltage through the regulator framework

Add support for external reference voltage through the regulator framework.

Signed-off-by Silvan Murer <[email protected]>

---
Changes in v2:
- Add 'optional properties' documentation
- Return an error when a regulator is specified
- Use internal reference when no regulator is specified
- Use iio_device_register instead of devm_iio_device_register

.../devicetree/bindings/iio/dac/ltc2632.txt | 14 +++++ drivers/iio/dac/ltc2632.c | 70 +++++++++++++++++++---

 2 files changed, 75 insertions(+), 9 deletions(-)

Documentation/devicetree/bindings/iio/dac/ltc2632.txt +0 -0

index eb911e5..e0d5fea 100644
@@ -12,12 +12,26 @@ Required properties:

Property rules described in Documentation/devicetree/bindings/spi/spi-bus.txt apply. In particular, "reg" and "spi-max-frequency" properties must be given.
+Optional properties:
+ - vref-supply: Phandle to the external reference voltage supply. This should
+ only be set if there is an external reference voltage connected to the VREF
+ pin. If the property is not set the internal reference is used.
+
Example:

+ vref: regulator-vref {
+ compatible = "regulator-fixed";
+ regulator-name = "vref-ltc2632";
+ regulator-min-microvolt = <1250000>;
+ regulator-max-microvolt = <1250000>;
+ regulator-always-on;
+ };
+
spi_master {
dac: ltc2632@0 {
compatible = "lltc,ltc2632-l12";
reg = <0>; /* CS0 */
spi-max-frequency = <1000000>;
+ vref-supply = <&vref>; /* optional */
};
};

drivers/iio/dac/ltc2632.c +0 -0

index d322b78..7127cf6 100644
@@ -2,6 +2,7 @@

* LTC2632 Digital to analog convertors spi driver
*
* Copyright 2017 Maxime Roussin-Bélanger
+ * expanded by Silvan Murer <[email protected]>
*
* Licensed under the GPL-2.
*/
@@ -10,6 +11,7 @@
#include <linux/spi/spi.h>
#include <linux/module.h>
#include <linux/iio/iio.h>
+#include <linux/regulator/consumer.h>

#define LTC2632_DAC_CHANNELS 2

@@ -28,7 +30,7 @@
/**
* struct ltc2632_chip_info - chip specific information
* @channels: channel spec for the DAC
- * @vref_mv: reference voltage
+ * @vref_mv: internal reference voltage
*/
struct ltc2632_chip_info {
const struct iio_chan_spec *channels;
@@ -39,10 +41,14 @@ struct ltc2632_chip_info {
* struct ltc2632_state - driver instance specific data
* @spi_dev: pointer to the spi_device struct
* @powerdown_cache_mask used to show current channel powerdown state
+ * @vref_mv used reference voltage (internal or external)
+ * @vref_reg regulator for the reference voltage
*/
struct ltc2632_state {
struct spi_device *spi_dev;
unsigned int powerdown_cache_mask;
+ int vref_mv;
+ struct regulator *vref_reg;
};

enum ltc2632_supported_device_ids {
@@ -90,7 +96,7 @@ static int ltc2632_read_raw(struct iio_dev *indio_dev,
switch (m) {
case IIO_CHAN_INFO_SCALE:
- *val = chip_info->vref_mv;
+ *val = st->vref_mv;
*val2 = chan->scan_type.realbits;
return IIO_VAL_FRACTIONAL_LOG2;
}
@@ -247,6 +253,45 @@ static int ltc2632_probe(struct spi_device *spi) chip_info = (struct ltc2632_chip_info *) spi_get_device_id(spi)->driver_data;

+ st->vref_reg = devm_regulator_get_optional(&spi->dev, "vref");
+ if (PTR_ERR(st->vref_reg) == -ENODEV) {
+ /* use internal reference voltage */
+ st->vref_reg = NULL;
+ st->vref_mv = chip_info->vref_mv;
+
+ ret = ltc2632_spi_write(spi, LTC2632_CMD_INTERNAL_REFER,
+ 0, 0, 0);
+ if (ret) {
+ dev_err(&spi->dev,
+ "Set internal reference command failed, %d\n",
+ ret);
+ return ret;
+ }
+ } else if (IS_ERR(st->vref_reg)) {
+ dev_err(&spi->dev,
+ "Error getting voltage reference regulator\n");
+ return PTR_ERR(st->vref_reg);
+ } else {
+ /* use external reference voltage */
+ ret = regulator_enable(st->vref_reg);
+ if (ret) {
+ dev_err(&spi->dev,
+ "enable reference regulator failed, %d\n",
+ ret);
+ return ret;
+ }
+ st->vref_mv = regulator_get_voltage(st->vref_reg) / 1000;
+
+ ret = ltc2632_spi_write(spi, LTC2632_CMD_EXTERNAL_REFER,
+ 0, 0, 0);
+ if (ret) {
+ dev_err(&spi->dev,
+ "Set external reference command failed, %d\n",
+ ret);
+ return ret;
+ }
+ }
+
indio_dev->dev.parent = &spi->dev;
indio_dev->name = dev_of_node(&spi->dev) ? dev_of_node(&spi->dev)->name : spi_get_device_id(spi)->name; @@ -255,14 +300,20 @@ static int ltc2632_probe(struct spi_device *spi) indio_dev->channels = chip_info->channels; indio_dev->num_channels = LTC2632_DAC_CHANNELS;

- ret = ltc2632_spi_write(spi, LTC2632_CMD_INTERNAL_REFER, 0, 0, 0);
- if (ret) {
- dev_err(&spi->dev,
- "Set internal reference command failed, %d\n", ret);
- return ret;
- }
+ return iio_device_register(indio_dev);
+}
+
+static int ltc2632_remove(struct spi_device *spi)
+{
+ struct iio_dev *indio_dev = spi_get_drvdata(spi);
+ struct ltc2632_state *st = iio_priv(indio_dev);
+
+ iio_device_unregister(indio_dev);
+
+ if (st->vref_reg)
+ regulator_disable(st->vref_reg);

- return devm_iio_device_register(&spi->dev, indio_dev);
+ return 0;
}

static const struct spi_device_id ltc2632_id[] = {
@@ -306,6 +357,7 @@ static struct spi_driver ltc2632_driver = { .of_match_table = of_match_ptr(ltc2632_of_match), },
.probe = ltc2632_probe,
+ .remove = ltc2632_remove,
.id_table = ltc2632_id,
};
module_spi_driver(ltc2632_driver);
--
2.7.4

Lars-Peter Clausen

Reviewed-by on v2 1/1

2018-05-17 11:01 UTC #1 lore raw

Quoted 3 lines · Silvan Murer

Add support for external reference voltage through the regulator framework.
Signed-off-by Silvan Murer <[email protected]>

Looks good, thanks.

Reviewed-by Lars-Peter Clausen <[email protected]>

Just two tiny whitespace issues.

[...]

Review

Quoted 7 lines

enum ltc2632_supported_device_ids {
(quoted diff) +0 -0
@@ -90,7 +96,7 @@ static int ltc2632_read_raw(struct iio_dev *indio_dev,
switch (m) {
case IIO_CHAN_INFO_SCALE:
(quoted diff) +1 -1
-		*val = chip_info->vref_mv;
+		*val =  st->vref_mv;

Extra space after the '='.

Quoted 15 lines

*val2 = chan->scan_type.realbits;
return IIO_VAL_FRACTIONAL_LOG2;
}

(quoted diff) +0 -0

@@ -247,6 +253,45 @@ static int ltc2632_probe(struct spi_device *spi)
  	chip_info = (struct ltc2632_chip_info *)
  			spi_get_device_id(spi)->driver_data;

(quoted diff) +8 -0

+	st->vref_reg = devm_regulator_get_optional(&spi->dev, "vref");
+	if (PTR_ERR(st->vref_reg) == -ENODEV) {
+		/* use internal reference voltage */
+		st->vref_reg = NULL;
+		st->vref_mv = chip_info->vref_mv;
+
+		ret = ltc2632_spi_write(spi, LTC2632_CMD_INTERNAL_REFER,
+				0, 0, 0);

Review

Quoted 10 lines

(quoted diff) +10 -0

+		if (ret) {
+			dev_err(&spi->dev,
+				"Set internal reference command failed, %d\n",
+				ret);
+			return ret;
+		}
+	} else if (IS_ERR(st->vref_reg)) {
+		dev_err(&spi->dev,
+				"Error getting voltage reference regulator\n");
+			return PTR_ERR(st->vref_reg);

Extra tab before the 'return'.

Quoted 21 lines

(quoted diff) +21 -0

+	} else {
+		/* use external reference voltage */
+		ret = regulator_enable(st->vref_reg);
+		if (ret) {
+			dev_err(&spi->dev,
+				"enable reference regulator failed, %d\n",
+				ret);
+			return ret;
+		}
+		st->vref_mv = regulator_get_voltage(st->vref_reg) / 1000;
+
+		ret = ltc2632_spi_write(spi, LTC2632_CMD_EXTERNAL_REFER,
+				0, 0, 0);
+		if (ret) {
+			dev_err(&spi->dev,
+				"Set external reference command failed, %d\n",
+				ret);
+			return ret;
+		}
+	}
+

[...]

Silvan Murer

author

2018-05-18 09:13 UTC #2 lore raw

Hi Lars,
Thanks for your review.
Should I create a new version of this patch whithout the whitespace issues?
Generally question: What is the next step? Do you include the reviewed patch into the iio.git repository? And do you submit the patches to the mainline repository?
Sorry for the basic questions, currently I try to understand the whole flow of the patch flow :)

Quoted 84 lines · Lars-Peter Clausen

Quoted 5 lines · Silvan Murer
Add support for external reference voltage through the regulator framework.
Signed-off-by Silvan Murer <[email protected]>
Looks good, thanks.
Reviewed-by Lars-Peter Clausen <[email protected]>
Just two tiny whitespace issues.
[...]
Review
Quoted 9 lines
enum ltc2632_supported_device_ids {
(quoted diff) +0 -0
@@ -90,7 +96,7 @@ static int ltc2632_read_raw(struct iio_dev
 *indio_dev,
switch (m) {
case IIO_CHAN_INFO_SCALE:
(quoted diff) +1 -1
-		*val = chip_info->vref_mv;
+		*val =  st->vref_mv;
Extra space after the '='.
Quoted 30 lines
*val2 = chan->scan_type.realbits;
return IIO_VAL_FRACTIONAL_LOG2;
}
(quoted diff) +0 -0
@@ -247,6 +253,45 @@ static int ltc2632_probe(struct spi_device
 *spi)
  	chip_info = (struct ltc2632_chip_info *)
  			spi_get_device_id(spi)->driver_data;
(quoted diff) +1 -0
+	st->vref_reg = devm_regulator_get_optional(&spi->dev,
"vref");
(quoted diff) +6 -0
+	if (PTR_ERR(st->vref_reg) == -ENODEV) {
+		/* use internal reference voltage */
+		st->vref_reg = NULL;
+		st->vref_mv = chip_info->vref_mv;
+
+		ret = ltc2632_spi_write(spi,
LTC2632_CMD_INTERNAL_REFER,
(quoted diff) +4 -0
+				0, 0, 0);
+		if (ret) {
+			dev_err(&spi->dev,
+				"Set internal reference command
failed, %d\n",
(quoted diff) +6 -0
+				ret);
+			return ret;
+		}
+	} else if (IS_ERR(st->vref_reg)) {
+		dev_err(&spi->dev,
+				"Error getting voltage reference
regulator\n");
Review
Quoted 1 line
(quoted diff) +1 -0
+			return PTR_ERR(st->vref_reg);
Extra tab before the 'return'.
Quoted 26 lines
(quoted diff) +6 -0
+	} else {
+		/* use external reference voltage */
+		ret = regulator_enable(st->vref_reg);
+		if (ret) {
+			dev_err(&spi->dev,
+				"enable reference regulator
failed, %d\n",
(quoted diff) +4 -0
+				ret);
+			return ret;
+		}
+		st->vref_mv = regulator_get_voltage(st->vref_reg)
/ 1000;
(quoted diff) +2 -0
+
+		ret = ltc2632_spi_write(spi,
LTC2632_CMD_EXTERNAL_REFER,
(quoted diff) +4 -0
+				0, 0, 0);
+		if (ret) {
+			dev_err(&spi->dev,
+				"Set external reference command
failed, %d\n",
(quoted diff) +5 -0
+				ret);
+			return ret;
+		}
+	}
+
[...]

Lars-Peter Clausen

2018-05-18 09:59 UTC #3 lore raw

Quoted 9 lines · Silvan Murer

Hi Lars,
Thanks for your review.
Should I create a new version of this patch whithout the whitespace issues?
Generally question: What is the next step? Do you include the reviewed patch into the iio.git repository? And do you submit the patches to the mainline repository?
Sorry for the basic questions, currently I try to understand the whole flow of the patch flow :)

Hi,

Jonathan will pick the patch up and add it to the iio.git repository, when he finds the time and thinks the patch is good. And then the patches will find their way to mainline.

You could ask Jonathan to fix the whitespace errors when he picks up the patch. But it would be less work for him if you just send a version that has the whitespace fixed (Keep by Reviewed-by tag).

- Lars

Quoted 87 lines · Lars-Peter Clausen

Quoted 84 lines · Silvan Murer
Quoted 5 lines
Add support for external reference voltage through the regulator framework.
Signed-off-by Silvan Murer <[email protected]>
Looks good, thanks.
Reviewed-by Lars-Peter Clausen <[email protected]>
Just two tiny whitespace issues.
[...]
Review
Quoted 9 lines
enum ltc2632_supported_device_ids {
(quoted diff) +0 -0
@@ -90,7 +96,7 @@ static int ltc2632_read_raw(struct iio_dev
 *indio_dev,
switch (m) {
case IIO_CHAN_INFO_SCALE:
(quoted diff) +1 -1
-		*val = chip_info->vref_mv;
+		*val =  st->vref_mv;
Extra space after the '='.
Quoted 30 lines
*val2 = chan->scan_type.realbits;
return IIO_VAL_FRACTIONAL_LOG2;
}
(quoted diff) +0 -0
@@ -247,6 +253,45 @@ static int ltc2632_probe(struct spi_device
 *spi)
  	chip_info = (struct ltc2632_chip_info *)
  			spi_get_device_id(spi)->driver_data;
(quoted diff) +1 -0
+	st->vref_reg = devm_regulator_get_optional(&spi->dev,
"vref");
(quoted diff) +6 -0
+	if (PTR_ERR(st->vref_reg) == -ENODEV) {
+		/* use internal reference voltage */
+		st->vref_reg = NULL;
+		st->vref_mv = chip_info->vref_mv;
+
+		ret = ltc2632_spi_write(spi,
LTC2632_CMD_INTERNAL_REFER,
(quoted diff) +4 -0
+				0, 0, 0);
+		if (ret) {
+			dev_err(&spi->dev,
+				"Set internal reference command
failed, %d\n",
(quoted diff) +6 -0
+				ret);
+			return ret;
+		}
+	} else if (IS_ERR(st->vref_reg)) {
+		dev_err(&spi->dev,
+				"Error getting voltage reference
regulator\n");
Review
Quoted 1 line
(quoted diff) +1 -0
+			return PTR_ERR(st->vref_reg);
Extra tab before the 'return'.
Quoted 26 lines
(quoted diff) +6 -0
+	} else {
+		/* use external reference voltage */
+		ret = regulator_enable(st->vref_reg);
+		if (ret) {
+			dev_err(&spi->dev,
+				"enable reference regulator
failed, %d\n",
(quoted diff) +4 -0
+				ret);
+			return ret;
+		}
+		st->vref_mv = regulator_get_voltage(st->vref_reg)
/ 1000;
(quoted diff) +2 -0
+
+		ret = ltc2632_spi_write(spi,
LTC2632_CMD_EXTERNAL_REFER,
(quoted diff) +4 -0
+				0, 0, 0);
+		if (ret) {
+			dev_err(&spi->dev,
+				"Set external reference command
failed, %d\n",
(quoted diff) +5 -0
+				ret);
+			return ret;
+		}
+	}
+
[...]

Silvan Murer

author

2018-05-20 00:26 UTC #4 lore raw

Thank you for the additional informations.
I just submitted the fixed patch to the mailinglist.

Quoted 129 lines · Lars-Peter Clausen

Quoted 13 lines · Silvan Murer
Hi Lars,
Thanks for your review.
Should I create a new version of this patch whithout the whitespace issues?
Generally question: What is the next step? Do you include the reviewed
patch into the iio.git repository? And do you submit the patches to the
mainline repository?
Sorry for the basic questions, currently I try to understand the whole
flow of the patch flow :)
Hi,
Jonathan will pick the patch up and add it to the iio.git repository, when
he finds the time and thinks the patch is good. And then the patches will
find their way to mainline.
You could ask Jonathan to fix the whitespace errors when he picks up the
patch. But it would be less work for him if you just send a version that has
the whitespace fixed (Keep by Reviewed-by tag).
- Lars
Quoted 98 lines · Lars-Peter Clausen
Quoted 94 lines · Silvan Murer
Quoted 7 lines
Add support for external reference voltage through the regulator
framework.
Signed-off-by Silvan Murer <[email protected]>
Looks good, thanks.
Reviewed-by Lars-Peter Clausen <[email protected]>
Just two tiny whitespace issues.
[...]
Review
Quoted 10 lines
enum ltc2632_supported_device_ids {
(quoted diff) +0 -0
@@ -90,7 +96,7 @@ static int ltc2632_read_raw(struct iio_dev
 *indio_dev,
switch (m) {
case IIO_CHAN_INFO_SCALE:
(quoted diff) +1 -1
-		*val = chip_info->vref_mv;
+		*val =  st->vref_mv;
Extra space after the '='.
Quoted 33 lines
*val2 = chan->scan_type.realbits;
return IIO_VAL_FRACTIONAL_LOG2;
}
(quoted diff) +0 -0
@@ -247,6 +253,45 @@ static int ltc2632_probe(struct spi_device
 *spi)
  	chip_info = (struct ltc2632_chip_info *)
  			spi_get_device_id(spi)->driver_data;
(quoted diff) +1 -0
+	st->vref_reg = devm_regulator_get_optional(&spi->dev,
"vref");
(quoted diff) +6 -0
+	if (PTR_ERR(st->vref_reg) == -ENODEV) {
+		/* use internal reference voltage */
+		st->vref_reg = NULL;
+		st->vref_mv = chip_info->vref_mv;
+
+		ret = ltc2632_spi_write(spi,
LTC2632_CMD_INTERNAL_REFER,
(quoted diff) +4 -0
+				0, 0, 0);
+		if (ret) {
+			dev_err(&spi->dev,
+				"Set internal reference
command
failed, %d\n",
(quoted diff) +6 -0
+				ret);
+			return ret;
+		}
+	} else if (IS_ERR(st->vref_reg)) {
+		dev_err(&spi->dev,
+				"Error getting voltage
reference
regulator\n");
Review
Quoted 1 line
(quoted diff) +1 -0
+			return PTR_ERR(st->vref_reg);
Extra tab before the 'return'.
Quoted 29 lines
(quoted diff) +6 -0
+	} else {
+		/* use external reference voltage */
+		ret = regulator_enable(st->vref_reg);
+		if (ret) {
+			dev_err(&spi->dev,
+				"enable reference regulator
failed, %d\n",
(quoted diff) +4 -0
+				ret);
+			return ret;
+		}
+		st->vref_mv = regulator_get_voltage(st-
Quoted 1 line
vref_reg)
/ 1000;
(quoted diff) +2 -0
+
+		ret = ltc2632_spi_write(spi,
LTC2632_CMD_EXTERNAL_REFER,
(quoted diff) +4 -0
+				0, 0, 0);
+		if (ret) {
+			dev_err(&spi->dev,
+				"Set external reference
command
failed, %d\n",
(quoted diff) +5 -0
+				ret);
+			return ret;
+		}
+	}
+
[...]