[PATCH 0/6] Improvements and Enhancements for AD5791 DAC Driver

[PATCH 0/6] Improvements and Enhancements for AD5791 DAC Driver

[ahaslam]

From: Axel Haslam <ahaslam@baylibre.com>

These patches aim to improve on the ad5791 driver:
 - make use of chip_info / match tables, and drop device enum id.
 - Add reset, clr and ldac gpios that have to be set to the correct level in case they
   are not not hardwired on the setup/PCB.
 - simplify probe by using the devm_* functions to automatically free resources.

Axel Haslam (6):
  dt-bindings: iio: dac: ad5791: Add optional reset, clr and ldac gpios
  dt-bindings: iio: dac: ad5791: Add required voltage supplies
  iio: dac: ad5791: Include chip_info in device match tables
  iio: dac: ad5791: Add reset, clr and ldac gpios
  iio: dac: ad5791: Use devm_regulator_get_enable_read_voltage
  iio: dac: ad5791: Use devm_iio_device_register

 .../bindings/iio/dac/adi,ad5791.yaml          |  39 ++++
 drivers/iio/dac/ad5791.c                      | 195 ++++++++----------
 2 files changed, 124 insertions(+), 110 deletions(-)

-- 
2.34.1

[PATCH 1/6] dt-bindings: iio: dac: ad5791: Add optional reset, clr and ldac gpios

[ahaslam]

From: Axel Haslam <ahaslam@baylibre.com>

Depending on board layout, the ad57xx may need control of reset, clear,
and ldac pins by the host driver. Add optional bindings for these gpios.

Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
---
 .../devicetree/bindings/iio/dac/adi,ad5791.yaml   | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad5791.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad5791.yaml
index c81285d84db7..fe664378c966 100644
--- a/Documentation/devicetree/bindings/iio/dac/adi,ad5791.yaml
+++ b/Documentation/devicetree/bindings/iio/dac/adi,ad5791.yaml
@@ -31,6 +31,17 @@ properties:
       gain of two configuration.
     type: boolean
 
+  reset-gpios:
+    maxItems: 1
+
+  clear-gpios:
+    maxItems: 1
+
+  ldac-gpios:
+    description:
+      LDAC pin to be used as a hardware trigger to update the DAC channels.
+    maxItems: 1
+
 required:
   - compatible
   - reg
@@ -44,6 +55,7 @@ unevaluatedProperties: false
 
 examples:
   - |
+    #include <dt-bindings/gpio/gpio.h>
     spi {
         #address-cells = <1>;
         #size-cells = <0>;
@@ -53,6 +65,9 @@ examples:
             reg = <0>;
             vss-supply = <&dac_vss>;
             vdd-supply = <&dac_vdd>;
+            reset-gpios = <&gpio_bd 16 GPIO_ACTIVE_LOW>;
+            clear-gpios = <&gpio_bd 17 GPIO_ACTIVE_LOW>;
+            ldac-gpios = <&gpio_bd 18 GPIO_ACTIVE_HIGH>;
         };
     };
 ...
-- 
2.34.1

[PATCH 2/6] dt-bindings: iio: dac: ad5791: Add required voltage supplies

[ahaslam]

From: Axel Haslam <ahaslam@baylibre.com>

Vcc, iovcc, vrefp, and vrefn are needed for the DAC to work.
Add them as required bindings for ad5791.

Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
---
 .../bindings/iio/dac/adi,ad5791.yaml          | 24 +++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad5791.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad5791.yaml
index fe664378c966..79cb4b78a88a 100644
--- a/Documentation/devicetree/bindings/iio/dac/adi,ad5791.yaml
+++ b/Documentation/devicetree/bindings/iio/dac/adi,ad5791.yaml
@@ -26,6 +26,22 @@ properties:
   vdd-supply: true
   vss-supply: true
 
+  vcc-supply:
+    description:
+      Supply that powers the chip.
+
+  iovcc-supply:
+    description:
+      Supply for the digital interface.
+
+  vrefp-supply:
+    description:
+      Positive referance input voltage range. From 5v to (vdd - 2.5)
+
+  vrefn-supply:
+    description:
+      Negative referance input voltage range. From (vss + 2.5) to 0.
+
   adi,rbuf-gain2-en:
     description: Specify to allow an external amplifier to be connected in a
       gain of two configuration.
@@ -47,6 +63,10 @@ required:
   - reg
   - vdd-supply
   - vss-supply
+  - vcc-supply
+  - iovcc-supply
+  - vrefp-supply
+  - vrefn-supply
 
 allOf:
   - $ref: /schemas/spi/spi-peripheral-props.yaml#
@@ -65,6 +85,10 @@ examples:
             reg = <0>;
             vss-supply = <&dac_vss>;
             vdd-supply = <&dac_vdd>;
+            vcc-supply = <&dac_vcc>;
+            iovcc-supply = <&dac_iovcc>;
+            vrefp-supply = <&dac_vrefp>;
+            vrefn-supply = <&dac_vrefn>;
             reset-gpios = <&gpio_bd 16 GPIO_ACTIVE_LOW>;
             clear-gpios = <&gpio_bd 17 GPIO_ACTIVE_LOW>;
             ldac-gpios = <&gpio_bd 18 GPIO_ACTIVE_HIGH>;
-- 
2.34.1

[PATCH 3/6] iio: dac: ad5791: Include chip_info in device match tables

[ahaslam]

From: Axel Haslam <ahaslam@baylibre.com>

Include a chip info struct in device SPI and device OF match tables to
provide channel definitions for each particular ADC model and drop
device enum.

Suggested-by: Nuno Sa <nuno.sa@analog.com>
Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
---
 drivers/iio/dac/ad5791.c | 107 +++++++++++++++++++--------------------
 1 file changed, 51 insertions(+), 56 deletions(-)

diff --git a/drivers/iio/dac/ad5791.c b/drivers/iio/dac/ad5791.c
index 553431bf0232..a11e81211669 100644
--- a/drivers/iio/dac/ad5791.c
+++ b/drivers/iio/dac/ad5791.c
@@ -65,7 +65,9 @@
  */
 
 struct ad5791_chip_info {
-	int (*get_lin_comp)	(unsigned int span);
+	const char *name;
+	const struct iio_chan_spec channel;
+	int (*get_lin_comp)(unsigned int span);
 };
 
 /**
@@ -98,13 +100,6 @@ struct ad5791_state {
 	} data[3] __aligned(IIO_DMA_MINALIGN);
 };
 
-enum ad5791_supported_device_ids {
-	ID_AD5760,
-	ID_AD5780,
-	ID_AD5781,
-	ID_AD5791,
-};
-
 static int ad5791_spi_write(struct ad5791_state *st, u8 addr, u32 val)
 {
 	st->data[0].d32 = cpu_to_be32(AD5791_CMD_WRITE |
@@ -228,20 +223,6 @@ static int ad5780_get_lin_comp(unsigned int span)
 	else
 		return AD5780_LINCOMP_10_20;
 }
-static const struct ad5791_chip_info ad5791_chip_info_tbl[] = {
-	[ID_AD5760] = {
-		.get_lin_comp = ad5780_get_lin_comp,
-	},
-	[ID_AD5780] = {
-		.get_lin_comp = ad5780_get_lin_comp,
-	},
-	[ID_AD5781] = {
-		.get_lin_comp = ad5791_get_lin_comp,
-	},
-	[ID_AD5791] = {
-		.get_lin_comp = ad5791_get_lin_comp,
-	},
-};
 
 static int ad5791_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan,
@@ -289,30 +270,34 @@ static const struct iio_chan_spec_ext_info ad5791_ext_info[] = {
 	{ },
 };
 
-#define AD5791_CHAN(bits, _shift) {			\
-	.type = IIO_VOLTAGE,				\
-	.output = 1,					\
-	.indexed = 1,					\
-	.address = AD5791_ADDR_DAC0,			\
-	.channel = 0,					\
-	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
-	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |	\
-		BIT(IIO_CHAN_INFO_OFFSET),		\
-	.scan_type = {					\
-		.sign = 'u',				\
-		.realbits = (bits),			\
-		.storagebits = 24,			\
-		.shift = (_shift),			\
-	},						\
-	.ext_info = ad5791_ext_info,			\
+#define AD5791_DEFINE_CHIP_INFO(_name, bits, _shift, _lin_comp)		\
+static const struct ad5791_chip_info _name##_chip_info = {		\
+	.name = #_name,							\
+	.get_lin_comp = &(_lin_comp),					\
+	.channel = {							\
+			.type = IIO_VOLTAGE,				\
+			.output = 1,					\
+			.indexed = 1,					\
+			.address = AD5791_ADDR_DAC0,			\
+			.channel = 0,					\
+			.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
+			.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |	\
+				BIT(IIO_CHAN_INFO_OFFSET),		\
+			.scan_type = {					\
+				.sign = 'u',				\
+				.realbits = (bits),			\
+				.storagebits = 24,			\
+				.shift = (_shift),			\
+			},						\
+			.ext_info = ad5791_ext_info,			\
+	},								\
 }
 
-static const struct iio_chan_spec ad5791_channels[] = {
-	[ID_AD5760] = AD5791_CHAN(16, 4),
-	[ID_AD5780] = AD5791_CHAN(18, 2),
-	[ID_AD5781] = AD5791_CHAN(18, 2),
-	[ID_AD5791] = AD5791_CHAN(20, 0)
-};
+AD5791_DEFINE_CHIP_INFO(ad5760, 16, 4, ad5780_get_lin_comp);
+AD5791_DEFINE_CHIP_INFO(ad5780, 18, 2, ad5780_get_lin_comp);
+AD5791_DEFINE_CHIP_INFO(ad5781, 18, 2, ad5791_get_lin_comp);
+AD5791_DEFINE_CHIP_INFO(ad5790, 20, 0, ad5791_get_lin_comp);
+AD5791_DEFINE_CHIP_INFO(ad5791, 20, 0, ad5791_get_lin_comp);
 
 static int ad5791_write_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan,
@@ -400,9 +385,9 @@ static int ad5791_probe(struct spi_device *spi)
 	if (ret)
 		goto error_disable_reg_neg;
 
-	st->chip_info =	&ad5791_chip_info_tbl[spi_get_device_id(spi)
-					      ->driver_data];
-
+	st->chip_info = spi_get_device_match_data(spi);
+	if (!st->chip_info)
+		return dev_err_probe(&spi->dev, -EINVAL, "no chip info\n");
 
 	st->ctrl = AD5761_CTRL_LINCOMP(st->chip_info->get_lin_comp(st->vref_mv))
 		  | (use_rbuf_gain2 ? 0 : AD5791_CTRL_RBUF) |
@@ -416,10 +401,9 @@ static int ad5791_probe(struct spi_device *spi)
 	spi_set_drvdata(spi, indio_dev);
 	indio_dev->info = &ad5791_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
-	indio_dev->channels
-		= &ad5791_channels[spi_get_device_id(spi)->driver_data];
+	indio_dev->channels = &st->chip_info->channel;
 	indio_dev->num_channels = 1;
-	indio_dev->name = spi_get_device_id(st->spi)->name;
+	indio_dev->name = st->chip_info->name;
 	ret = iio_device_register(indio_dev);
 	if (ret)
 		goto error_disable_reg_neg;
@@ -448,19 +432,30 @@ static void ad5791_remove(struct spi_device *spi)
 		regulator_disable(st->reg_vss);
 }
 
+static const struct of_device_id ad5791_of_match[] = {
+	{ .compatible = "adi,ad5760", .data = &ad5760_chip_info },
+	{ .compatible = "adi,ad5780", .data = &ad5780_chip_info },
+	{ .compatible = "adi,ad5781", .data = &ad5781_chip_info },
+	{ .compatible = "adi,ad5790", .data = &ad5790_chip_info },
+	{ .compatible = "adi,ad5791", .data = &ad5791_chip_info },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ad5791_of_match);
+
 static const struct spi_device_id ad5791_id[] = {
-	{"ad5760", ID_AD5760},
-	{"ad5780", ID_AD5780},
-	{"ad5781", ID_AD5781},
-	{"ad5790", ID_AD5791},
-	{"ad5791", ID_AD5791},
-	{}
+	{ "ad5760", (kernel_ulong_t)&ad5760_chip_info },
+	{ "ad5780", (kernel_ulong_t)&ad5780_chip_info },
+	{ "ad5781", (kernel_ulong_t)&ad5781_chip_info },
+	{ "ad5790", (kernel_ulong_t)&ad5790_chip_info },
+	{ "ad5791", (kernel_ulong_t)&ad5791_chip_info },
+	{ }
 };
 MODULE_DEVICE_TABLE(spi, ad5791_id);
 
 static struct spi_driver ad5791_driver = {
 	.driver = {
 		   .name = "ad5791",
+		   .of_match_table = ad5791_of_match,
 		   },
 	.probe = ad5791_probe,
 	.remove = ad5791_remove,
-- 
2.34.1

[PATCH 4/6] iio: dac: ad5791: Add reset, clr and ldac gpios

[ahaslam]

From: Axel Haslam <ahaslam@baylibre.com>

The ad7591 has reset, clr and ldac gpios. For the DAC to output data
continuously written to the data register the state of these gpios needs
to be set by the driver.

Add these gpios to the driver making them optional in case they are fixed
on the pcb.

Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
---
 drivers/iio/dac/ad5791.c | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/dac/ad5791.c b/drivers/iio/dac/ad5791.c
index a11e81211669..a7cf19346cf0 100644
--- a/drivers/iio/dac/ad5791.c
+++ b/drivers/iio/dac/ad5791.c
@@ -9,6 +9,7 @@
 #include <linux/interrupt.h>
 #include <linux/fs.h>
 #include <linux/device.h>
+#include <linux/delay.h>
 #include <linux/kernel.h>
 #include <linux/spi/spi.h>
 #include <linux/slab.h>
@@ -87,6 +88,9 @@ struct ad5791_state {
 	struct spi_device		*spi;
 	struct regulator		*reg_vdd;
 	struct regulator		*reg_vss;
+	struct gpio_desc		*gpio_reset;
+	struct gpio_desc		*gpio_clear;
+	struct gpio_desc		*gpio_ldac;
 	const struct ad5791_chip_info	*chip_info;
 	unsigned short			vref_mv;
 	unsigned int			vref_neg_mv;
@@ -336,6 +340,22 @@ static int ad5791_probe(struct spi_device *spi)
 	if (!indio_dev)
 		return -ENOMEM;
 	st = iio_priv(indio_dev);
+
+	st->gpio_reset = devm_gpiod_get_optional(&spi->dev, "reset",
+						 GPIOD_OUT_HIGH);
+	if (IS_ERR(st->gpio_reset))
+		return PTR_ERR(st->gpio_reset);
+
+	st->gpio_clear = devm_gpiod_get_optional(&spi->dev, "clear",
+						 GPIOD_OUT_LOW);
+	if (IS_ERR(st->gpio_clear))
+		return PTR_ERR(st->gpio_clear);
+
+	st->gpio_ldac = devm_gpiod_get_optional(&spi->dev, "ldac",
+						GPIOD_OUT_HIGH);
+	if (IS_ERR(st->gpio_ldac))
+		return PTR_ERR(st->gpio_ldac);
+
 	st->reg_vdd = devm_regulator_get(&spi->dev, "vdd");
 	if (!IS_ERR(st->reg_vdd)) {
 		ret = regulator_enable(st->reg_vdd);
@@ -381,9 +401,14 @@ static int ad5791_probe(struct spi_device *spi)
 		dev_warn(&spi->dev, "reference voltage unspecified\n");
 	}
 
-	ret = ad5791_spi_write(st, AD5791_ADDR_SW_CTRL, AD5791_SWCTRL_RESET);
-	if (ret)
-		goto error_disable_reg_neg;
+	if (st->gpio_reset) {
+		fsleep(20);
+		gpiod_set_value_cansleep(st->gpio_reset, 0);
+	} else {
+		ret = ad5791_spi_write(st, AD5791_ADDR_SW_CTRL, AD5791_SWCTRL_RESET);
+		if (ret)
+			goto error_disable_reg_neg;
+	}
 
 	st->chip_info = spi_get_device_match_data(spi);
 	if (!st->chip_info)
-- 
2.34.1

[PATCH 5/6] iio: dac: ad5791: Use devm_regulator_get_enable_read_voltage

[ahaslam]

From: Axel Haslam <ahaslam@baylibre.com>

Simplify probe by using of the devm_regulator_get_enable_read_voltage.

Suggested-by: David Lechner <dlechner@baylibre.com>
Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
---
 drivers/iio/dac/ad5791.c | 56 +++++++++-------------------------------
 1 file changed, 12 insertions(+), 44 deletions(-)

diff --git a/drivers/iio/dac/ad5791.c b/drivers/iio/dac/ad5791.c
index a7cf19346cf0..cf3d41a10c20 100644
--- a/drivers/iio/dac/ad5791.c
+++ b/drivers/iio/dac/ad5791.c
@@ -356,32 +356,6 @@ static int ad5791_probe(struct spi_device *spi)
 	if (IS_ERR(st->gpio_ldac))
 		return PTR_ERR(st->gpio_ldac);
 
-	st->reg_vdd = devm_regulator_get(&spi->dev, "vdd");
-	if (!IS_ERR(st->reg_vdd)) {
-		ret = regulator_enable(st->reg_vdd);
-		if (ret)
-			return ret;
-
-		ret = regulator_get_voltage(st->reg_vdd);
-		if (ret < 0)
-			goto error_disable_reg_pos;
-
-		pos_voltage_uv = ret;
-	}
-
-	st->reg_vss = devm_regulator_get(&spi->dev, "vss");
-	if (!IS_ERR(st->reg_vss)) {
-		ret = regulator_enable(st->reg_vss);
-		if (ret)
-			goto error_disable_reg_pos;
-
-		ret = regulator_get_voltage(st->reg_vss);
-		if (ret < 0)
-			goto error_disable_reg_neg;
-
-		neg_voltage_uv = ret;
-	}
-
 	st->pwr_down = true;
 	st->spi = spi;
 
@@ -391,7 +365,15 @@ static int ad5791_probe(struct spi_device *spi)
 		use_rbuf_gain2 = device_property_read_bool(&spi->dev,
 							   "adi,rbuf-gain2-en");
 
-	if (!IS_ERR(st->reg_vss) && !IS_ERR(st->reg_vdd)) {
+	pos_voltage_uv = devm_regulator_get_enable_read_voltage(&spi->dev, "vdd");
+	if (pos_voltage_uv < 0 && pos_voltage_uv != -ENODEV)
+		return dev_err_probe(&spi->dev, ret, "failed to get vdd voltage\n");
+
+	neg_voltage_uv = devm_regulator_get_enable_read_voltage(&spi->dev, "vss");
+	if (neg_voltage_uv < 0 && neg_voltage_uv != -ENODEV)
+		return dev_err_probe(&spi->dev, ret, "failed to get vss voltage\n");
+
+	if (neg_voltage_uv >= 0 && pos_voltage_uv >= 0) {
 		st->vref_mv = (pos_voltage_uv + neg_voltage_uv) / 1000;
 		st->vref_neg_mv = neg_voltage_uv / 1000;
 	} else if (pdata) {
@@ -407,7 +389,7 @@ static int ad5791_probe(struct spi_device *spi)
 	} else {
 		ret = ad5791_spi_write(st, AD5791_ADDR_SW_CTRL, AD5791_SWCTRL_RESET);
 		if (ret)
-			goto error_disable_reg_neg;
+			return dev_err_probe(&spi->dev, ret, "fail to reset\n");
 	}
 
 	st->chip_info = spi_get_device_match_data(spi);
@@ -421,7 +403,7 @@ static int ad5791_probe(struct spi_device *spi)
 	ret = ad5791_spi_write(st, AD5791_ADDR_CTRL, st->ctrl |
 		AD5791_CTRL_OPGND | AD5791_CTRL_DACTRI);
 	if (ret)
-		goto error_disable_reg_neg;
+		return dev_err_probe(&spi->dev, ret, "fail to write ctrl register\n");
 
 	spi_set_drvdata(spi, indio_dev);
 	indio_dev->info = &ad5791_info;
@@ -431,30 +413,16 @@ static int ad5791_probe(struct spi_device *spi)
 	indio_dev->name = st->chip_info->name;
 	ret = iio_device_register(indio_dev);
 	if (ret)
-		goto error_disable_reg_neg;
+		return dev_err_probe(&spi->dev, ret, "unable to register iio device\n");
 
 	return 0;
-
-error_disable_reg_neg:
-	if (!IS_ERR(st->reg_vss))
-		regulator_disable(st->reg_vss);
-error_disable_reg_pos:
-	if (!IS_ERR(st->reg_vdd))
-		regulator_disable(st->reg_vdd);
-	return ret;
 }
 
 static void ad5791_remove(struct spi_device *spi)
 {
 	struct iio_dev *indio_dev = spi_get_drvdata(spi);
-	struct ad5791_state *st = iio_priv(indio_dev);
 
 	iio_device_unregister(indio_dev);
-	if (!IS_ERR(st->reg_vdd))
-		regulator_disable(st->reg_vdd);
-
-	if (!IS_ERR(st->reg_vss))
-		regulator_disable(st->reg_vss);
 }
 
 static const struct of_device_id ad5791_of_match[] = {
-- 
2.34.1

[PATCH 6/6] iio: dac: ad5791: Use devm_iio_device_register

[ahaslam]

From: Axel Haslam <ahaslam@baylibre.com>

Use devm_iio_device_register to automatically free the iio device.
since this is the last remaining resource that was not automatically
freed, we can drop the ".remove" callback.

Suggested-by: David Lechner <dlechner@baylibre.com>
Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
---
 drivers/iio/dac/ad5791.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/iio/dac/ad5791.c b/drivers/iio/dac/ad5791.c
index cf3d41a10c20..21332c9aca5d 100644
--- a/drivers/iio/dac/ad5791.c
+++ b/drivers/iio/dac/ad5791.c
@@ -405,24 +405,12 @@ static int ad5791_probe(struct spi_device *spi)
 	if (ret)
 		return dev_err_probe(&spi->dev, ret, "fail to write ctrl register\n");
 
-	spi_set_drvdata(spi, indio_dev);
 	indio_dev->info = &ad5791_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->channels = &st->chip_info->channel;
 	indio_dev->num_channels = 1;
 	indio_dev->name = st->chip_info->name;
-	ret = iio_device_register(indio_dev);
-	if (ret)
-		return dev_err_probe(&spi->dev, ret, "unable to register iio device\n");
-
-	return 0;
-}
-
-static void ad5791_remove(struct spi_device *spi)
-{
-	struct iio_dev *indio_dev = spi_get_drvdata(spi);
-
-	iio_device_unregister(indio_dev);
+	return devm_iio_device_register(&spi->dev, indio_dev);
 }
 
 static const struct of_device_id ad5791_of_match[] = {
@@ -451,7 +439,6 @@ static struct spi_driver ad5791_driver = {
 		   .of_match_table = ad5791_of_match,
 		   },
 	.probe = ad5791_probe,
-	.remove = ad5791_remove,
 	.id_table = ad5791_id,
 };
 module_spi_driver(ad5791_driver);
-- 
2.34.1

Re: [PATCH 1/6] dt-bindings: iio: dac: ad5791: Add optional reset, clr and ldac gpios

[Krzysztof Kozlowski]

On Mon, Oct 28, 2024 at 08:11:13AM +0100, ahaslam@baylibre.com wrote:
> From: Axel Haslam <ahaslam@baylibre.com>
> 
> Depending on board layout, the ad57xx may need control of reset, clear,
> and ldac pins by the host driver. Add optional bindings for these gpios.
> 
> Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
> ---
>  .../devicetree/bindings/iio/dac/adi,ad5791.yaml   | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 2/6] dt-bindings: iio: dac: ad5791: Add required voltage supplies

[Krzysztof Kozlowski]

On Mon, Oct 28, 2024 at 08:11:14AM +0100, ahaslam@baylibre.com wrote:
> From: Axel Haslam <ahaslam@baylibre.com>
> 
> Vcc, iovcc, vrefp, and vrefn are needed for the DAC to work.
> Add them as required bindings for ad5791.
> 
> Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
> ---
>  .../bindings/iio/dac/adi,ad5791.yaml          | 24 +++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad5791.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad5791.yaml
> index fe664378c966..79cb4b78a88a 100644
> --- a/Documentation/devicetree/bindings/iio/dac/adi,ad5791.yaml
> +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad5791.yaml
> @@ -26,6 +26,22 @@ properties:
>    vdd-supply: true
>    vss-supply: true
>  
> +  vcc-supply:
> +    description:
> +      Supply that powers the chip.
> +
> +  iovcc-supply:
> +    description:
> +      Supply for the digital interface.
> +
> +  vrefp-supply:
> +    description:
> +      Positive referance input voltage range. From 5v to (vdd - 2.5)
> +
> +  vrefn-supply:
> +    description:
> +      Negative referance input voltage range. From (vss + 2.5) to 0.
> +
>    adi,rbuf-gain2-en:
>      description: Specify to allow an external amplifier to be connected in a
>        gain of two configuration.
> @@ -47,6 +63,10 @@ required:
>    - reg
>    - vdd-supply
>    - vss-supply
> +  - vcc-supply
> +  - iovcc-supply
> +  - vrefp-supply
> +  - vrefn-supply

So you have six required supplies?

Datasheet says "A voltage range of 2.7 V to 5.5 V *can* be connected",
so doesn't it mean this is optional? Although similar wording is for
other supplies, so maybe it's just imprecise language?

Best regards,
Krzysztof

Re: [PATCH 3/6] iio: dac: ad5791: Include chip_info in device match tables

[kernel test robot]

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on linus/master v6.12-rc5 next-20241028]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/ahaslam-baylibre-com/dt-bindings-iio-dac-ad5791-Add-optional-reset-clr-and-ldac-gpios/20241028-151319
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20241028071118.699951-4-ahaslam%40baylibre.com
patch subject: [PATCH 3/6] iio: dac: ad5791: Include chip_info in device match tables
config: parisc-randconfig-r071-20241028 (https://download.01.org/0day-ci/archive/20241028/202410282349.YFq0jd85-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241028/202410282349.YFq0jd85-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410282349.YFq0jd85-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/iio/dac/ad5791.c:71: warning: Function parameter or struct member 'name' not described in 'ad5791_chip_info'
>> drivers/iio/dac/ad5791.c:71: warning: Function parameter or struct member 'channel' not described in 'ad5791_chip_info'


vim +71 drivers/iio/dac/ad5791.c

20374d1a36df3e drivers/staging/iio/dac/ad5791.c Lars-Peter Clausen 2012-06-04  61  
20374d1a36df3e drivers/staging/iio/dac/ad5791.c Lars-Peter Clausen 2012-06-04  62  /**
20374d1a36df3e drivers/staging/iio/dac/ad5791.c Lars-Peter Clausen 2012-06-04  63   * struct ad5791_chip_info - chip specific information
20374d1a36df3e drivers/staging/iio/dac/ad5791.c Lars-Peter Clausen 2012-06-04  64   * @get_lin_comp:	function pointer to the device specific function
20374d1a36df3e drivers/staging/iio/dac/ad5791.c Lars-Peter Clausen 2012-06-04  65   */
20374d1a36df3e drivers/staging/iio/dac/ad5791.c Lars-Peter Clausen 2012-06-04  66  
20374d1a36df3e drivers/staging/iio/dac/ad5791.c Lars-Peter Clausen 2012-06-04  67  struct ad5791_chip_info {
18e83b303d6e05 drivers/iio/dac/ad5791.c         Axel Haslam        2024-10-28  68  	const char *name;
18e83b303d6e05 drivers/iio/dac/ad5791.c         Axel Haslam        2024-10-28  69  	const struct iio_chan_spec channel;
20374d1a36df3e drivers/staging/iio/dac/ad5791.c Lars-Peter Clausen 2012-06-04  70  	int (*get_lin_comp)(unsigned int span);
20374d1a36df3e drivers/staging/iio/dac/ad5791.c Lars-Peter Clausen 2012-06-04 @71  };
20374d1a36df3e drivers/staging/iio/dac/ad5791.c Lars-Peter Clausen 2012-06-04  72  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 5/6] iio: dac: ad5791: Use devm_regulator_get_enable_read_voltage

[kernel test robot]

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on linus/master v6.12-rc5 next-20241028]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/ahaslam-baylibre-com/dt-bindings-iio-dac-ad5791-Add-optional-reset-clr-and-ldac-gpios/20241028-151319
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20241028071118.699951-6-ahaslam%40baylibre.com
patch subject: [PATCH 5/6] iio: dac: ad5791: Use devm_regulator_get_enable_read_voltage
config: x86_64-buildonly-randconfig-004-20241028 (https://download.01.org/0day-ci/archive/20241029/202410290245.0RC0cDV4-lkp@intel.com/config)
compiler: clang version 19.1.2 (https://github.com/llvm/llvm-project 7ba7d8e2f7b6445b60679da826210cdde29eaf8b)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241029/202410290245.0RC0cDV4-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410290245.0RC0cDV4-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/iio/dac/ad5791.c:14:
   In file included from include/linux/spi/spi.h:17:
   In file included from include/linux/scatterlist.h:8:
   In file included from include/linux/mm.h:2213:
   include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     518 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
>> drivers/iio/dac/ad5791.c:370:35: warning: variable 'ret' is uninitialized when used here [-Wuninitialized]
     370 |                 return dev_err_probe(&spi->dev, ret, "failed to get vdd voltage\n");
         |                                                 ^~~
   drivers/iio/dac/ad5791.c:336:9: note: initialize the variable 'ret' to silence this warning
     336 |         int ret, pos_voltage_uv = 0, neg_voltage_uv = 0;
         |                ^
         |                 = 0
   2 warnings generated.


vim +/ret +370 drivers/iio/dac/ad5791.c

   330	
   331	static int ad5791_probe(struct spi_device *spi)
   332	{
   333		const struct ad5791_platform_data *pdata = dev_get_platdata(&spi->dev);
   334		struct iio_dev *indio_dev;
   335		struct ad5791_state *st;
   336		int ret, pos_voltage_uv = 0, neg_voltage_uv = 0;
   337		bool use_rbuf_gain2;
   338	
   339		indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
   340		if (!indio_dev)
   341			return -ENOMEM;
   342		st = iio_priv(indio_dev);
   343	
   344		st->gpio_reset = devm_gpiod_get_optional(&spi->dev, "reset",
   345							 GPIOD_OUT_HIGH);
   346		if (IS_ERR(st->gpio_reset))
   347			return PTR_ERR(st->gpio_reset);
   348	
   349		st->gpio_clear = devm_gpiod_get_optional(&spi->dev, "clear",
   350							 GPIOD_OUT_LOW);
   351		if (IS_ERR(st->gpio_clear))
   352			return PTR_ERR(st->gpio_clear);
   353	
   354		st->gpio_ldac = devm_gpiod_get_optional(&spi->dev, "ldac",
   355							GPIOD_OUT_HIGH);
   356		if (IS_ERR(st->gpio_ldac))
   357			return PTR_ERR(st->gpio_ldac);
   358	
   359		st->pwr_down = true;
   360		st->spi = spi;
   361	
   362		if (pdata)
   363			use_rbuf_gain2 = pdata->use_rbuf_gain2;
   364		else
   365			use_rbuf_gain2 = device_property_read_bool(&spi->dev,
   366								   "adi,rbuf-gain2-en");
   367	
   368		pos_voltage_uv = devm_regulator_get_enable_read_voltage(&spi->dev, "vdd");
   369		if (pos_voltage_uv < 0 && pos_voltage_uv != -ENODEV)
 > 370			return dev_err_probe(&spi->dev, ret, "failed to get vdd voltage\n");
   371	
   372		neg_voltage_uv = devm_regulator_get_enable_read_voltage(&spi->dev, "vss");
   373		if (neg_voltage_uv < 0 && neg_voltage_uv != -ENODEV)
   374			return dev_err_probe(&spi->dev, ret, "failed to get vss voltage\n");
   375	
   376		if (neg_voltage_uv >= 0 && pos_voltage_uv >= 0) {
   377			st->vref_mv = (pos_voltage_uv + neg_voltage_uv) / 1000;
   378			st->vref_neg_mv = neg_voltage_uv / 1000;
   379		} else if (pdata) {
   380			st->vref_mv = pdata->vref_pos_mv + pdata->vref_neg_mv;
   381			st->vref_neg_mv = pdata->vref_neg_mv;
   382		} else {
   383			dev_warn(&spi->dev, "reference voltage unspecified\n");
   384		}
   385	
   386		if (st->gpio_reset) {
   387			fsleep(20);
   388			gpiod_set_value_cansleep(st->gpio_reset, 0);
   389		} else {
   390			ret = ad5791_spi_write(st, AD5791_ADDR_SW_CTRL, AD5791_SWCTRL_RESET);
   391			if (ret)
   392				return dev_err_probe(&spi->dev, ret, "fail to reset\n");
   393		}
   394	
   395		st->chip_info = spi_get_device_match_data(spi);
   396		if (!st->chip_info)
   397			return dev_err_probe(&spi->dev, -EINVAL, "no chip info\n");
   398	
   399		st->ctrl = AD5761_CTRL_LINCOMP(st->chip_info->get_lin_comp(st->vref_mv))
   400			  | (use_rbuf_gain2 ? 0 : AD5791_CTRL_RBUF) |
   401			  AD5791_CTRL_BIN2SC;
   402	
   403		ret = ad5791_spi_write(st, AD5791_ADDR_CTRL, st->ctrl |
   404			AD5791_CTRL_OPGND | AD5791_CTRL_DACTRI);
   405		if (ret)
   406			return dev_err_probe(&spi->dev, ret, "fail to write ctrl register\n");
   407	
   408		spi_set_drvdata(spi, indio_dev);
   409		indio_dev->info = &ad5791_info;
   410		indio_dev->modes = INDIO_DIRECT_MODE;
   411		indio_dev->channels = &st->chip_info->channel;
   412		indio_dev->num_channels = 1;
   413		indio_dev->name = st->chip_info->name;
   414		ret = iio_device_register(indio_dev);
   415		if (ret)
   416			return dev_err_probe(&spi->dev, ret, "unable to register iio device\n");
   417	
   418		return 0;
   419	}
   420	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 4/6] iio: dac: ad5791: Add reset, clr and ldac gpios

[kernel test robot]

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on linus/master v6.12-rc5 next-20241028]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/ahaslam-baylibre-com/dt-bindings-iio-dac-ad5791-Add-optional-reset-clr-and-ldac-gpios/20241028-151319
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20241028071118.699951-5-ahaslam%40baylibre.com
patch subject: [PATCH 4/6] iio: dac: ad5791: Add reset, clr and ldac gpios
config: parisc-randconfig-r071-20241028 (https://download.01.org/0day-ci/archive/20241029/202410290242.TjrDXcKG-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241029/202410290242.TjrDXcKG-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410290242.TjrDXcKG-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/iio/dac/ad5791.c:72: warning: Function parameter or struct member 'name' not described in 'ad5791_chip_info'
   drivers/iio/dac/ad5791.c:72: warning: Function parameter or struct member 'channel' not described in 'ad5791_chip_info'
>> drivers/iio/dac/ad5791.c:105: warning: Function parameter or struct member 'gpio_reset' not described in 'ad5791_state'
>> drivers/iio/dac/ad5791.c:105: warning: Function parameter or struct member 'gpio_clear' not described in 'ad5791_state'
>> drivers/iio/dac/ad5791.c:105: warning: Function parameter or struct member 'gpio_ldac' not described in 'ad5791_state'


vim +105 drivers/iio/dac/ad5791.c

20374d1a36df3e drivers/staging/iio/dac/ad5791.c Lars-Peter Clausen 2012-06-04   73  
20374d1a36df3e drivers/staging/iio/dac/ad5791.c Lars-Peter Clausen 2012-06-04   74  /**
20374d1a36df3e drivers/staging/iio/dac/ad5791.c Lars-Peter Clausen 2012-06-04   75   * struct ad5791_state - driver instance specific data
ff96bf519acdb3 drivers/iio/dac/ad5791.c         Peter Meerwald     2014-12-06   76   * @spi:			spi_device
20374d1a36df3e drivers/staging/iio/dac/ad5791.c Lars-Peter Clausen 2012-06-04   77   * @reg_vdd:		positive supply regulator
20374d1a36df3e drivers/staging/iio/dac/ad5791.c Lars-Peter Clausen 2012-06-04   78   * @reg_vss:		negative supply regulator
20374d1a36df3e drivers/staging/iio/dac/ad5791.c Lars-Peter Clausen 2012-06-04   79   * @chip_info:		chip model specific constants
20374d1a36df3e drivers/staging/iio/dac/ad5791.c Lars-Peter Clausen 2012-06-04   80   * @vref_mv:		actual reference voltage used
20374d1a36df3e drivers/staging/iio/dac/ad5791.c Lars-Peter Clausen 2012-06-04   81   * @vref_neg_mv:	voltage of the negative supply
0071aa300271a4 drivers/iio/dac/ad5791.c         zuoqilin           2021-01-28   82   * @ctrl:		control register cache
3b1c0b129590d7 drivers/iio/dac/ad5791.c         Lee Jones          2020-07-16   83   * @pwr_down_mode:	current power down mode
3b1c0b129590d7 drivers/iio/dac/ad5791.c         Lee Jones          2020-07-16   84   * @pwr_down:		true if device is powered down
3b1c0b129590d7 drivers/iio/dac/ad5791.c         Lee Jones          2020-07-16   85   * @data:		spi transfer buffers
20374d1a36df3e drivers/staging/iio/dac/ad5791.c Lars-Peter Clausen 2012-06-04   86   */
20374d1a36df3e drivers/staging/iio/dac/ad5791.c Lars-Peter Clausen 2012-06-04   87  struct ad5791_state {
20374d1a36df3e drivers/staging/iio/dac/ad5791.c Lars-Peter Clausen 2012-06-04   88  	struct spi_device		*spi;
20374d1a36df3e drivers/staging/iio/dac/ad5791.c Lars-Peter Clausen 2012-06-04   89  	struct regulator		*reg_vdd;
20374d1a36df3e drivers/staging/iio/dac/ad5791.c Lars-Peter Clausen 2012-06-04   90  	struct regulator		*reg_vss;
fcc5d80fd09f4c drivers/iio/dac/ad5791.c         Axel Haslam        2024-10-28   91  	struct gpio_desc		*gpio_reset;
fcc5d80fd09f4c drivers/iio/dac/ad5791.c         Axel Haslam        2024-10-28   92  	struct gpio_desc		*gpio_clear;
fcc5d80fd09f4c drivers/iio/dac/ad5791.c         Axel Haslam        2024-10-28   93  	struct gpio_desc		*gpio_ldac;
20374d1a36df3e drivers/staging/iio/dac/ad5791.c Lars-Peter Clausen 2012-06-04   94  	const struct ad5791_chip_info	*chip_info;
20374d1a36df3e drivers/staging/iio/dac/ad5791.c Lars-Peter Clausen 2012-06-04   95  	unsigned short			vref_mv;
20374d1a36df3e drivers/staging/iio/dac/ad5791.c Lars-Peter Clausen 2012-06-04   96  	unsigned int			vref_neg_mv;
20374d1a36df3e drivers/staging/iio/dac/ad5791.c Lars-Peter Clausen 2012-06-04   97  	unsigned			ctrl;
20374d1a36df3e drivers/staging/iio/dac/ad5791.c Lars-Peter Clausen 2012-06-04   98  	unsigned			pwr_down_mode;
20374d1a36df3e drivers/staging/iio/dac/ad5791.c Lars-Peter Clausen 2012-06-04   99  	bool				pwr_down;
791bb52a0cd2cc drivers/iio/dac/ad5791.c         Lars-Peter Clausen 2013-11-25  100  
791bb52a0cd2cc drivers/iio/dac/ad5791.c         Lars-Peter Clausen 2013-11-25  101  	union {
791bb52a0cd2cc drivers/iio/dac/ad5791.c         Lars-Peter Clausen 2013-11-25  102  		__be32 d32;
791bb52a0cd2cc drivers/iio/dac/ad5791.c         Lars-Peter Clausen 2013-11-25  103  		u8 d8[4];
b2d5e9de77c877 drivers/iio/dac/ad5791.c         Jonathan Cameron   2022-05-08  104  	} data[3] __aligned(IIO_DMA_MINALIGN);
20374d1a36df3e drivers/staging/iio/dac/ad5791.c Lars-Peter Clausen 2012-06-04 @105  };
20374d1a36df3e drivers/staging/iio/dac/ad5791.c Lars-Peter Clausen 2012-06-04  106  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 3/6] iio: dac: ad5791: Include chip_info in device match tables

[Jonathan Cameron]

On Mon, 28 Oct 2024 08:11:15 +0100
ahaslam@baylibre.com wrote:

> From: Axel Haslam <ahaslam@baylibre.com>
> 
> Include a chip info struct in device SPI and device OF match tables to
> provide channel definitions for each particular ADC model and drop
> device enum.
> 
> Suggested-by: Nuno Sa <nuno.sa@analog.com>
> Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
> ---
>  drivers/iio/dac/ad5791.c | 107 +++++++++++++++++++--------------------
>  1 file changed, 51 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/iio/dac/ad5791.c b/drivers/iio/dac/ad5791.c
> index 553431bf0232..a11e81211669 100644
> --- a/drivers/iio/dac/ad5791.c
> +++ b/drivers/iio/dac/ad5791.c
> @@ -65,7 +65,9 @@
>   */
Whilst adding the docs for the missing entries, please delete this
blank line!

Otherwise patch looks good to me.

Jonathan
	
>  
>  struct ad5791_chip_info {
> -	int (*get_lin_comp)	(unsigned int span);
> +	const char *name;
> +	const struct iio_chan_spec channel;
> +	int (*get_lin_comp)(unsigned int span);
>  };
>  
>  /**
> @@ -98,13 +100,6 @@ struct ad5791_state {
>  	} data[3] __aligned(IIO_DMA_MINALIGN);
>  };
>  
> -enum ad5791_supported_device_ids {
> -	ID_AD5760,
> -	ID_AD5780,
> -	ID_AD5781,
> -	ID_AD5791,
> -};
> -
>  static int ad5791_spi_write(struct ad5791_state *st, u8 addr, u32 val)
>  {
>  	st->data[0].d32 = cpu_to_be32(AD5791_CMD_WRITE |
> @@ -228,20 +223,6 @@ static int ad5780_get_lin_comp(unsigned int span)
>  	else
>  		return AD5780_LINCOMP_10_20;
>  }
> -static const struct ad5791_chip_info ad5791_chip_info_tbl[] = {
> -	[ID_AD5760] = {
> -		.get_lin_comp = ad5780_get_lin_comp,
> -	},
> -	[ID_AD5780] = {
> -		.get_lin_comp = ad5780_get_lin_comp,
> -	},
> -	[ID_AD5781] = {
> -		.get_lin_comp = ad5791_get_lin_comp,
> -	},
> -	[ID_AD5791] = {
> -		.get_lin_comp = ad5791_get_lin_comp,
> -	},
> -};
>  
>  static int ad5791_read_raw(struct iio_dev *indio_dev,
>  			   struct iio_chan_spec const *chan,
> @@ -289,30 +270,34 @@ static const struct iio_chan_spec_ext_info ad5791_ext_info[] = {
>  	{ },
>  };
>  
> -#define AD5791_CHAN(bits, _shift) {			\
> -	.type = IIO_VOLTAGE,				\
> -	.output = 1,					\
> -	.indexed = 1,					\
> -	.address = AD5791_ADDR_DAC0,			\
> -	.channel = 0,					\
> -	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
> -	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |	\
> -		BIT(IIO_CHAN_INFO_OFFSET),		\
> -	.scan_type = {					\
> -		.sign = 'u',				\
> -		.realbits = (bits),			\
> -		.storagebits = 24,			\
> -		.shift = (_shift),			\
> -	},						\
> -	.ext_info = ad5791_ext_info,			\
> +#define AD5791_DEFINE_CHIP_INFO(_name, bits, _shift, _lin_comp)		\
> +static const struct ad5791_chip_info _name##_chip_info = {		\
> +	.name = #_name,							\
> +	.get_lin_comp = &(_lin_comp),					\
> +	.channel = {							\
> +			.type = IIO_VOLTAGE,				\
> +			.output = 1,					\
> +			.indexed = 1,					\
> +			.address = AD5791_ADDR_DAC0,			\
> +			.channel = 0,					\
> +			.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
> +			.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |	\
> +				BIT(IIO_CHAN_INFO_OFFSET),		\
> +			.scan_type = {					\
> +				.sign = 'u',				\
> +				.realbits = (bits),			\
> +				.storagebits = 24,			\
> +				.shift = (_shift),			\
> +			},						\
> +			.ext_info = ad5791_ext_info,			\
> +	},								\
>  }
>  
> -static const struct iio_chan_spec ad5791_channels[] = {
> -	[ID_AD5760] = AD5791_CHAN(16, 4),
> -	[ID_AD5780] = AD5791_CHAN(18, 2),
> -	[ID_AD5781] = AD5791_CHAN(18, 2),
> -	[ID_AD5791] = AD5791_CHAN(20, 0)
> -};
> +AD5791_DEFINE_CHIP_INFO(ad5760, 16, 4, ad5780_get_lin_comp);
> +AD5791_DEFINE_CHIP_INFO(ad5780, 18, 2, ad5780_get_lin_comp);
> +AD5791_DEFINE_CHIP_INFO(ad5781, 18, 2, ad5791_get_lin_comp);
> +AD5791_DEFINE_CHIP_INFO(ad5790, 20, 0, ad5791_get_lin_comp);
> +AD5791_DEFINE_CHIP_INFO(ad5791, 20, 0, ad5791_get_lin_comp);

Re: [PATCH 6/6] iio: dac: ad5791: Use devm_iio_device_register

[Jonathan Cameron]

On Mon, 28 Oct 2024 08:11:18 +0100
ahaslam@baylibre.com wrote:

> From: Axel Haslam <ahaslam@baylibre.com>
> 
> Use devm_iio_device_register to automatically free the iio device.
> since this is the last remaining resource that was not automatically
> freed, we can drop the ".remove" callback.
> 
> Suggested-by: David Lechner <dlechner@baylibre.com>
> Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
Hi Axel,

The bot did a much better review job than me this time.
Other than the obvious solutions to the things it pointed out,
this series looks fine to me.

Jonathan

> ---
>  drivers/iio/dac/ad5791.c | 15 +--------------
>  1 file changed, 1 insertion(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/dac/ad5791.c b/drivers/iio/dac/ad5791.c
> index cf3d41a10c20..21332c9aca5d 100644
> --- a/drivers/iio/dac/ad5791.c
> +++ b/drivers/iio/dac/ad5791.c
> @@ -405,24 +405,12 @@ static int ad5791_probe(struct spi_device *spi)
>  	if (ret)
>  		return dev_err_probe(&spi->dev, ret, "fail to write ctrl register\n");
>  
> -	spi_set_drvdata(spi, indio_dev);
>  	indio_dev->info = &ad5791_info;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->channels = &st->chip_info->channel;
>  	indio_dev->num_channels = 1;
>  	indio_dev->name = st->chip_info->name;
> -	ret = iio_device_register(indio_dev);
> -	if (ret)
> -		return dev_err_probe(&spi->dev, ret, "unable to register iio device\n");
> -
> -	return 0;
> -}
> -
> -static void ad5791_remove(struct spi_device *spi)
> -{
> -	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> -
> -	iio_device_unregister(indio_dev);
> +	return devm_iio_device_register(&spi->dev, indio_dev);
>  }
>  
>  static const struct of_device_id ad5791_of_match[] = {
> @@ -451,7 +439,6 @@ static struct spi_driver ad5791_driver = {
>  		   .of_match_table = ad5791_of_match,
>  		   },
>  	.probe = ad5791_probe,
> -	.remove = ad5791_remove,
>  	.id_table = ad5791_id,
>  };
>  module_spi_driver(ad5791_driver);

Re: [PATCH 2/6] dt-bindings: iio: dac: ad5791: Add required voltage supplies

[Axel Haslam]

On Mon, 28 Oct 2024 at 09:06, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Mon, Oct 28, 2024 at 08:11:14AM +0100, ahaslam@baylibre.com wrote:
> > From: Axel Haslam <ahaslam@baylibre.com>
> >
> > Vcc, iovcc, vrefp, and vrefn are needed for the DAC to work.
> > Add them as required bindings for ad5791.
> >
> > Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
> > ---
> >  .../bindings/iio/dac/adi,ad5791.yaml          | 24 +++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad5791.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad5791.yaml
> > index fe664378c966..79cb4b78a88a 100644
> > --- a/Documentation/devicetree/bindings/iio/dac/adi,ad5791.yaml
> > +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad5791.yaml
> > @@ -26,6 +26,22 @@ properties:
> >    vdd-supply: true
> >    vss-supply: true
> >
> > +  vcc-supply:
> > +    description:
> > +      Supply that powers the chip.
> > +
> > +  iovcc-supply:
> > +    description:
> > +      Supply for the digital interface.
> > +
> > +  vrefp-supply:
> > +    description:
> > +      Positive referance input voltage range. From 5v to (vdd - 2.5)
> > +
> > +  vrefn-supply:
> > +    description:
> > +      Negative referance input voltage range. From (vss + 2.5) to 0.
> > +
> >    adi,rbuf-gain2-en:
> >      description: Specify to allow an external amplifier to be connected in a
> >        gain of two configuration.
> > @@ -47,6 +63,10 @@ required:
> >    - reg
> >    - vdd-supply
> >    - vss-supply
> > +  - vcc-supply
> > +  - iovcc-supply
> > +  - vrefp-supply
> > +  - vrefn-supply
>
> So you have six required supplies?
>
> Datasheet says "A voltage range of 2.7 V to 5.5 V *can* be connected",
> so doesn't it mean this is optional? Although similar wording is for
> other supplies, so maybe it's just imprecise language?

looks like unfortunate wording. Like you said, Vdd, Vss are already required
and have the same *can* word in their description like all other supplies
which i think its meant for the voltage level options of the power supply.

Vcc:  is mentioned as need to "power on" in the startup sequence
section of the datasheet,
iovcc: we can't interface the chip without this supply.
vrefp: minimum input of 5v.
vrefn: from vss up to 0 volts max.

so vcc, iovcc, and vrefp to me, look required for the hw to work.
but i have a small doubt about vrefn since it could potentially be 0V.
Does this mean it should be an optional binding where we assume its 0
if not present?
or is it ok to leave it as required?

Regards
Axel.


>
> Best regards,
> Krzysztof
>

Re: [PATCH 2/6] dt-bindings: iio: dac: ad5791: Add required voltage supplies

[Krzysztof Kozlowski]

On 28/10/2024 08:11, ahaslam@baylibre.com wrote:
> From: Axel Haslam <ahaslam@baylibre.com>
> 
> Vcc, iovcc, vrefp, and vrefn are needed for the DAC to work.
> Add them as required bindings for ad5791.
> 
> Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
> ---
>  .../bindings/iio/dac/adi,ad5791.yaml          | 24 +++++++++++++++++++
>  1 file changed, 24 insertions(+)

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH 5/6] iio: dac: ad5791: Use devm_regulator_get_enable_read_voltage

[ahaslam]

From: Axel Haslam <ahaslam@baylibre.com>

Simplify probe by using of the devm_regulator_get_enable_read_voltage.

Suggested-by: David Lechner <dlechner@baylibre.com>
Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
---
 drivers/iio/dac/ad5791.c | 58 ++++++++++------------------------------
 1 file changed, 14 insertions(+), 44 deletions(-)

diff --git a/drivers/iio/dac/ad5791.c b/drivers/iio/dac/ad5791.c
index c5d4d755d57a..92d47e766fd3 100644
--- a/drivers/iio/dac/ad5791.c
+++ b/drivers/iio/dac/ad5791.c
@@ -360,32 +360,6 @@ static int ad5791_probe(struct spi_device *spi)
 	if (IS_ERR(st->gpio_ldac))
 		return PTR_ERR(st->gpio_ldac);
 
-	st->reg_vdd = devm_regulator_get(&spi->dev, "vdd");
-	if (!IS_ERR(st->reg_vdd)) {
-		ret = regulator_enable(st->reg_vdd);
-		if (ret)
-			return ret;
-
-		ret = regulator_get_voltage(st->reg_vdd);
-		if (ret < 0)
-			goto error_disable_reg_pos;
-
-		pos_voltage_uv = ret;
-	}
-
-	st->reg_vss = devm_regulator_get(&spi->dev, "vss");
-	if (!IS_ERR(st->reg_vss)) {
-		ret = regulator_enable(st->reg_vss);
-		if (ret)
-			goto error_disable_reg_pos;
-
-		ret = regulator_get_voltage(st->reg_vss);
-		if (ret < 0)
-			goto error_disable_reg_neg;
-
-		neg_voltage_uv = ret;
-	}
-
 	st->pwr_down = true;
 	st->spi = spi;
 
@@ -395,7 +369,17 @@ static int ad5791_probe(struct spi_device *spi)
 		use_rbuf_gain2 = device_property_read_bool(&spi->dev,
 							   "adi,rbuf-gain2-en");
 
-	if (!IS_ERR(st->reg_vss) && !IS_ERR(st->reg_vdd)) {
+	pos_voltage_uv = devm_regulator_get_enable_read_voltage(&spi->dev, "vdd");
+	if (pos_voltage_uv < 0 && pos_voltage_uv != -ENODEV)
+		return dev_err_probe(&spi->dev, pos_voltage_uv,
+				     "failed to get vdd voltage\n");
+
+	neg_voltage_uv = devm_regulator_get_enable_read_voltage(&spi->dev, "vss");
+	if (neg_voltage_uv < 0 && neg_voltage_uv != -ENODEV)
+		return dev_err_probe(&spi->dev, neg_voltage_uv,
+				     "failed to get vss voltage\n");
+
+	if (neg_voltage_uv >= 0 && pos_voltage_uv >= 0) {
 		st->vref_mv = (pos_voltage_uv + neg_voltage_uv) / 1000;
 		st->vref_neg_mv = neg_voltage_uv / 1000;
 	} else if (pdata) {
@@ -411,7 +395,7 @@ static int ad5791_probe(struct spi_device *spi)
 	} else {
 		ret = ad5791_spi_write(st, AD5791_ADDR_SW_CTRL, AD5791_SWCTRL_RESET);
 		if (ret)
-			goto error_disable_reg_neg;
+			return dev_err_probe(&spi->dev, ret, "fail to reset\n");
 	}
 
 	st->chip_info = spi_get_device_match_data(spi);
@@ -425,7 +409,7 @@ static int ad5791_probe(struct spi_device *spi)
 	ret = ad5791_spi_write(st, AD5791_ADDR_CTRL, st->ctrl |
 		AD5791_CTRL_OPGND | AD5791_CTRL_DACTRI);
 	if (ret)
-		goto error_disable_reg_neg;
+		return dev_err_probe(&spi->dev, ret, "fail to write ctrl register\n");
 
 	spi_set_drvdata(spi, indio_dev);
 	indio_dev->info = &ad5791_info;
@@ -435,30 +419,16 @@ static int ad5791_probe(struct spi_device *spi)
 	indio_dev->name = st->chip_info->name;
 	ret = iio_device_register(indio_dev);
 	if (ret)
-		goto error_disable_reg_neg;
+		return dev_err_probe(&spi->dev, ret, "unable to register iio device\n");
 
 	return 0;
-
-error_disable_reg_neg:
-	if (!IS_ERR(st->reg_vss))
-		regulator_disable(st->reg_vss);
-error_disable_reg_pos:
-	if (!IS_ERR(st->reg_vdd))
-		regulator_disable(st->reg_vdd);
-	return ret;
 }
 
 static void ad5791_remove(struct spi_device *spi)
 {
 	struct iio_dev *indio_dev = spi_get_drvdata(spi);
-	struct ad5791_state *st = iio_priv(indio_dev);
 
 	iio_device_unregister(indio_dev);
-	if (!IS_ERR(st->reg_vdd))
-		regulator_disable(st->reg_vdd);
-
-	if (!IS_ERR(st->reg_vss))
-		regulator_disable(st->reg_vss);
 }
 
 static const struct of_device_id ad5791_of_match[] = {
-- 
2.34.1