iio: dac: Add regulator framework to LTC2632 device driver

PATCH In review Silvan Murer 2 participants 3 messages May 14, 2018 – May 15, 2018

Subsystems (2)

Device tree bindings IIO

lore

Share & tools

lore

Silvan Murer

author

2018-05-14 10:31 UTC #0 lore raw

PATCH 1/1 iio: dac: Add regulator framework to LTC2632 device driver

This patch adds support for external reference voltage through the regulator framework. The patch add also the remove function to the device driver.

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

---
.../devicetree/bindings/iio/dac/ltc2632.txt | 9 +++ drivers/iio/dac/ltc2632.c | 86 +++++++++++++++++-----

 2 files changed, 78 insertions(+), 17 deletions(-)

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

index eb911e5..d369a4b 100644
@@ -14,10 +14,19 @@ apply. In particular, "reg" and "spi-max-frequency" properties must be given.

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 ac5e05f..4a5c5bd 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,41 @@ 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 (IS_ERR(st->vref_reg)) {
+ /* 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 {
+ /* 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 +296,23 @@ 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 devm_iio_device_register(&spi->dev, 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);
+
+ devm_iio_device_unregister(&spi->dev, indio_dev);
+
+ if (st->vref_reg != NULL) {
+ regulator_disable(st->vref_reg);
+ devm_regulator_put(st->vref_reg);
}

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

static const struct spi_device_id ltc2632_id[] = {
@@ -276,15 +326,6 @@ static const struct spi_device_id ltc2632_id[] = { };
MODULE_DEVICE_TABLE(spi, ltc2632_id);

-static struct spi_driver ltc2632_driver = {
- .driver = {
- .name = "ltc2632",
- },
- .probe = ltc2632_probe,
- .id_table = ltc2632_id,
-};
-module_spi_driver(ltc2632_driver);
-
static const struct of_device_id ltc2632_of_match[] = {
{
.compatible = "lltc,ltc2632-l12",
@@ -309,6 +350,17 @@ static const struct of_device_id ltc2632_of_match[] = { };
MODULE_DEVICE_TABLE(of, ltc2632_of_match);

+static struct spi_driver ltc2632_driver = {
+ .driver = {
+ .name = "ltc2632",
+ .of_match_table = of_match_ptr(ltc2632_of_match),
+ },
+ .probe = ltc2632_probe,
+ .remove = ltc2632_remove,
+ .id_table = ltc2632_id,
+};
+module_spi_driver(ltc2632_driver);
+
MODULE_AUTHOR("Maxime Roussin-Belanger <[email protected]>"); MODULE_DESCRIPTION("LTC2632 DAC SPI driver");
MODULE_LICENSE("GPL v2");
--
2.7.4

Lars-Peter Clausen

2018-05-14 11:14 UTC #1 lore raw

Quoted 4 lines · Silvan Murer

This patch adds support for external reference voltage through the regulator framework. The patch add also the remove function to the device driver.
Signed-off-by Silvan Murer <[email protected]>

Hi,

Thanks for the patch. A few comments.

Quoted 26 lines

---
.../devicetree/bindings/iio/dac/ltc2632.txt | 9 +++ drivers/iio/dac/ltc2632.c | 86 +++++++++++++++++-----
 2 files changed, 78 insertions(+), 17 deletions(-)
Documentation/devicetree/bindings/iio/dac/ltc2632.txt +0 -0
index eb911e5..d369a4b 100644
@@ -14,10 +14,19 @@ apply. In particular, "reg" and "spi-max-frequency" properties must be given.
Example:
Documentation/devicetree/bindings/iio/dac/ltc2632.txt +8 -0
+	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>;

Documentation/devicetree/bindings/iio/dac/ltc2632.txt

Quoted 1 line

Documentation/devicetree/bindings/iio/dac/ltc2632.txt +1 -0
+			vref-supply = <&vref>; /* optional */

This should not only update the example, but also add a 'optional properties' section where the property is documented.

drivers/iio/dac/ltc2632.c

Quoted 6 lines

};
};
drivers/iio/dac/ltc2632.c +0 -0
index ac5e05f..4a5c5bd 100644

[...

Quoted 14 lines

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

drivers/iio/dac/ltc2632.c

Quoted 2 lines

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

+	st->vref_reg = devm_regulator_get_optional(&spi->dev, "vref");
+	if (IS_ERR(st->vref_reg)) {

There are two error cases that should be handled. One is no regulator is specified and the other is a regulator is specified, but something went wrong. In the later case the error should be reported and not ignored. Have a look at e.g. ad5592r-base.c as an example.

Quoted 12 lines

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

+		/* 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;
+		}

drivers/iio/dac/ltc2632.c

Quoted 10 lines

drivers/iio/dac/ltc2632.c +10 -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;

Should be space around '/'.

Quoted 38 lines

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

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

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

@@ -255,14 +296,23 @@ static int ltc2632_probe(struct spi_device *spi)
  	indio_dev->channels = chip_info->channels;
  	indio_dev->num_channels = LTC2632_DAC_CHANNELS;

drivers/iio/dac/ltc2632.c +13 -5

-	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 devm_iio_device_register(&spi->dev, 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);
+
+	devm_iio_device_unregister(&spi->dev, indio_dev);
+
+	if (st->vref_reg != NULL) {
+		regulator_disable(st->vref_reg);
+		devm_regulator_put(st->vref_reg);

}

drivers/iio/dac/ltc2632.c

Quoted 2 lines

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

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

The idea behind the devm_* interface is that you do not explicitly call it in the remove() callback. It will automatically run after the remove function.

This means in this case you can remove the devm_regulator_put() and devm_iio_device_free().

The devm_iio_device_unregister() still needs to say since we have to unregister the device before we disable the regulator. But you can simplify this by using the non-managed API (iio_device_unregister()/iio_device_unregister()).

Quoted 24 lines

drivers/iio/dac/ltc2632.c +1 -0
+	return 0;
}

static const struct spi_device_id ltc2632_id[] = {
drivers/iio/dac/ltc2632.c +0 -0
@@ -276,15 +326,6 @@ static const struct spi_device_id ltc2632_id[] = {
  };
  MODULE_DEVICE_TABLE(spi, ltc2632_id);
drivers/iio/dac/ltc2632.c +0 -9
-static struct spi_driver ltc2632_driver = {
-	.driver		= {
-		.name	= "ltc2632",
-	},
-	.probe		= ltc2632_probe,
-	.id_table	= ltc2632_id,
-};
-module_spi_driver(ltc2632_driver);
-
static const struct of_device_id ltc2632_of_match[] = { {
.compatible = "lltc,ltc2632-l12",
drivers/iio/dac/ltc2632.c +0 -0
@@ -309,6 +350,17 @@ static const struct of_device_id ltc2632_of_match[] = {
  };
  MODULE_DEVICE_TABLE(of, ltc2632_of_match);

drivers/iio/dac/ltc2632.c

Quoted 4 lines

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

+static struct spi_driver ltc2632_driver = {
+	.driver		= {
+		.name	= "ltc2632",
+		.of_match_table = of_match_ptr(ltc2632_of_match),

It's a bit strange that of_match_table was not assigned in the first place. I think this should be a separate change and be declared as a fix.

drivers/iio/dac/ltc2632.c

Quoted 3 lines

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

+	},
+	.probe		= ltc2632_probe,
+	.remove     = ltc2632_remove,

This line uses tabs for alignment, while all the other lines use tabs.

Quoted 3 lines

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

+	.id_table	= ltc2632_id,
+};
+module_spi_driver(ltc2632_driver);

Silvan Murer

author

2018-05-15 14:43 UTC #2 lore raw

Thanks Lars for the feedback.
I created just now, a new patch which includes just the of_match_table fix. For the other stuffe i will create a new version of the patch (v2) soon.

Quoted 218 lines · Lars-Peter Clausen

Quoted 6 lines · Silvan Murer
This patch adds support for external reference voltage through the regulator framework.
The patch add also the remove function to the device driver.
Signed-off-by Silvan Murer <[email protected]>
Hi,
Thanks for the patch. A few comments.
Quoted 30 lines
---
.../devicetree/bindings/iio/dac/ltc2632.txt | 9 +++ drivers/iio/dac/ltc2632.c | 86
+++++++++++++++++-----
 2 files changed, 78 insertions(+), 17 deletions(-)
Documentation/devicetree/bindings/iio/dac/ltc2632.txt +0 -0
index eb911e5..d369a4b 100644
@@ -14,10 +14,19 @@ apply. In particular, "reg" and "spi-max-
frequency" properties must be given.

Example:
Documentation/devicetree/bindings/iio/dac/ltc2632.txt +8 -0
+	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>;
Documentation/devicetree/bindings/iio/dac/ltc2632.txt
Quoted 1 line
Documentation/devicetree/bindings/iio/dac/ltc2632.txt +1 -0
+			vref-supply = <&vref>; /* optional */
This should not only update the example, but also add a 'optional properties' section where the property is documented.
drivers/iio/dac/ltc2632.c
Quoted 7 lines
};
};
drivers/iio/dac/ltc2632.c +0 -0
index ac5e05f..4a5c5bd 100644
[...
Quoted 19 lines
enum ltc2632_supported_device_ids {
drivers/iio/dac/ltc2632.c +0 -0
@@ -90,7 +96,7 @@ static int ltc2632_read_raw(struct iio_dev
 *indio_dev,
switch (m) {
case IIO_CHAN_INFO_SCALE:
drivers/iio/dac/ltc2632.c +1 -1
-		*val = chip_info->vref_mv;
+		*val = st->vref_mv;
*val2 = chan->scan_type.realbits;
return IIO_VAL_FRACTIONAL_LOG2;
}
drivers/iio/dac/ltc2632.c +0 -0
@@ -247,6 +253,41 @@ static int ltc2632_probe(struct spi_device
 *spi)
  	chip_info = (struct ltc2632_chip_info *)
  			spi_get_device_id(spi)->driver_data;
drivers/iio/dac/ltc2632.c +1 -0
+	st->vref_reg = devm_regulator_get_optional(&spi->dev,
"vref");
drivers/iio/dac/ltc2632.c
Quoted 1 line
drivers/iio/dac/ltc2632.c +1 -0
+	if (IS_ERR(st->vref_reg)) {
There are two error cases that should be handled. One is no regulator is
specified and the other is a regulator is specified, but something went
wrong. In the later case the error should be reported and not ignored. Have
a look at e.g. ad5592r-base.c as an example.
Quoted 26 lines
drivers/iio/dac/ltc2632.c +5 -0
+		/* use internal reference voltage */
+		st->vref_reg = NULL;
+		st->vref_mv = chip_info->vref_mv;
+
+		ret = ltc2632_spi_write(spi,
LTC2632_CMD_INTERNAL_REFER,
drivers/iio/dac/ltc2632.c +4 -0
+				0, 0, 0);
+		if (ret) {
+			dev_err(&spi->dev,
+				"Set internal reference command
failed, %d\n",
drivers/iio/dac/ltc2632.c +9 -0
+				ret);
+			return ret;
+		}
+	} else {
+		/* use external reference voltage */
+		ret = regulator_enable(st->vref_reg);
+		if (ret) {
+			dev_err(&spi->dev,
+				"enable reference regulator
failed, %d\n",
drivers/iio/dac/ltc2632.c +4 -0
+				ret);
+			return ret;
+		}
+		st->vref_mv = regulator_get_voltage(st-
Quoted 1 line
Quoted 1 line
vref_reg)/1000;
Should be space around '/'.
Quoted 46 lines
drivers/iio/dac/ltc2632.c +2 -0
+
+		ret = ltc2632_spi_write(spi,
LTC2632_CMD_EXTERNAL_REFER,
drivers/iio/dac/ltc2632.c +4 -0
+				0, 0, 0);
+		if (ret) {
+			dev_err(&spi->dev,
+				"Set external reference command
failed, %d\n",
drivers/iio/dac/ltc2632.c +5 -0
+				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;
drivers/iio/dac/ltc2632.c +0 -0
@@ -255,14 +296,23 @@ static int ltc2632_probe(struct spi_device
 *spi)
  	indio_dev->channels = chip_info->channels;
  	indio_dev->num_channels = LTC2632_DAC_CHANNELS;
drivers/iio/dac/ltc2632.c +0 -1
-	ret = ltc2632_spi_write(spi, LTC2632_CMD_INTERNAL_REFER,
0, 0, 0);
drivers/iio/dac/ltc2632.c +0 -3
-	if (ret) {
-		dev_err(&spi->dev,
-			"Set internal reference command failed,
%d\n", ret);
drivers/iio/dac/ltc2632.c +13 -1
-		return ret;
+	return devm_iio_device_register(&spi->dev, 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);
+
+	devm_iio_device_unregister(&spi->dev, indio_dev);
+
+	if (st->vref_reg != NULL) {
+		regulator_disable(st->vref_reg);
+		devm_regulator_put(st->vref_reg);
}
drivers/iio/dac/ltc2632.c
Quoted 2 lines
drivers/iio/dac/ltc2632.c +1 -1
-	return devm_iio_device_register(&spi->dev, indio_dev);
+	devm_iio_device_free(&spi->dev, indio_dev);
The idea behind the devm_* interface is that you do not explicitly call it
in the remove() callback. It will automatically run after the remove function.
This means in this case you can remove the devm_regulator_put() and devm_iio_device_free().
The devm_iio_device_unregister() still needs to say since we have to unregister the device before we disable the regulator. But you can simplify
this by using the non-managed API (iio_device_unregister()/iio_device_unregister()).
Quoted 27 lines
drivers/iio/dac/ltc2632.c +1 -0
+	return 0;
}

static const struct spi_device_id ltc2632_id[] = {
drivers/iio/dac/ltc2632.c +0 -0
@@ -276,15 +326,6 @@ static const struct spi_device_id ltc2632_id[]
 = {
  };
  MODULE_DEVICE_TABLE(spi, ltc2632_id);
drivers/iio/dac/ltc2632.c +0 -9
-static struct spi_driver ltc2632_driver = {
-	.driver		= {
-		.name	= "ltc2632",
-	},
-	.probe		= ltc2632_probe,
-	.id_table	= ltc2632_id,
-};
-module_spi_driver(ltc2632_driver);
-
static const struct of_device_id ltc2632_of_match[] = { {
.compatible = "lltc,ltc2632-l12",
drivers/iio/dac/ltc2632.c +0 -0
@@ -309,6 +350,17 @@ static const struct of_device_id
 ltc2632_of_match[] = {
  };
  MODULE_DEVICE_TABLE(of, ltc2632_of_match);
drivers/iio/dac/ltc2632.c
Quoted 4 lines
drivers/iio/dac/ltc2632.c +4 -0
+static struct spi_driver ltc2632_driver = {
+	.driver		= {
+		.name	= "ltc2632",
+		.of_match_table = of_match_ptr(ltc2632_of_match),
It's a bit strange that of_match_table was not assigned in the first place.
I think this should be a separate change and be declared as a fix.
drivers/iio/dac/ltc2632.c
Quoted 4 lines
drivers/iio/dac/ltc2632.c +3 -0
+	},
+	.probe		= ltc2632_probe,
+	.remove     = ltc2632_remove,
This line uses tabs for alignment, while all the other lines use tabs.
Quoted 4 lines
drivers/iio/dac/ltc2632.c +3 -0
+	.id_table	= ltc2632_id,
+};
+module_spi_driver(ltc2632_driver);