[PATCH v5 0/3] Add driver for AD3530R and AD3531R DACs

[PATCH v5 0/3] Add driver for AD3530R and AD3531R DACs

[Kim Seer Paller]

The AD3530/AD3530R (8-channel) and AD3531/AD3531R (4-channel) are
low-power, 16-bit, buffered voltage output DACs with software-
programmable gain controls, providing full-scale output spans of 2.5V or
5V for reference voltages of 2.5V. These devices operate from a single
2.7V to 5.5V supply and are guaranteed monotonic by design. The "R"
variants include a 2.5V, 5ppm/°C internal reference, which is disabled
by default.

Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
---
Changes in v5:
ad3530r:
- Replace return value to -ENODEV if no external and internal reference
  found.
- Replace device_property_present() with device_property_read_bool() for
  flag handling.
- Simplify vref_mv calculation by using range_multiplier directly.
- Change ldac GPIO initial state as it is toggled high to low later.
- Drop linux/kernel.h include.
- Add reviewer's tag.

- Link to v4: https://lore.kernel.org/r/20250412-togreg-v4-0-cb9e5309b99d@analog.com

Changes in v4:
Bindings:
- Add ad3531/ad3531r datasheet link.
- Add reviewer's tag.

ad3530r:
- Add commit description for unimplemented MUXOUT ADC monitoring feature.
- Use a DMA-safe buffer for bulk register read/write.
- Inline the AD3530R_CHAN_EXT_INFO macro for the "powerdown" attribute
  since it is only used once.
- Use enum ad3530r_mode for powerdown_mode.
- Refactor ad3530r_set_dac_powerdown() use chan->address for bitmask
  calculations and add helper variables.
- Simplify single-bit configuration with regmap_set_bits().
- Add .max_register for regmap_config.
- Rework regulator handling and move ad3530r_setup() after enabling the
  regulators.

- Link to v3: https://lore.kernel.org/r/20250403-togreg-v3-0-d4b06a4af5a9@analog.com

Changes in v3:
- Drop ABI docs.

Bindings:
- Drop reviewer's tag.
- Update commit message.
- Add non-r variants to compatible list.
- Add io-channels property to enable ADC channel support for MUXOUT
  readings.
- Switch to unevaluatedProperties: false.

ad3530r:
- Update commit message.
- Drop spi field from ad3530r_state and use regmap to retrieve the device
  pointer.
- Update mutex lock comment and use devm_mutex_init().
- Fix LDAC gpio pulse logic.
- Replace usleep_range() with fsleep().
- Use sizeof(reg_val) instead of hardcoded value in regmap_bulk_read.
- Drop reporting of zero offset.
- Add internal_ref_support chip_info parameter and modify reference
  handling.

- Link to v2: https://lore.kernel.org/r/20250324-togreg-v2-0-f211d781923e@analog.com

Changes in v2:
Bindings:
- Updated commit message.
- Changed adi,double-output-range to adi,range-double property.

ad3530r:
- Changed data type to __be16 to resolve sparse warnings related to
  type mismatches.

- Link to v1: https://lore.kernel.org/r/20250319-togreg-v1-0-d8244a502f2c@analog.com

---
Kim Seer Paller (3):
      iio: ABI: add new DAC powerdown mode
      dt-bindings: iio: dac: Add adi,ad3530r.yaml
      iio: dac: ad3530r: Add driver for AD3530R and AD3531R

 Documentation/ABI/testing/sysfs-bus-iio            |   2 +
 .../devicetree/bindings/iio/dac/adi,ad3530r.yaml   | 100 ++++
 MAINTAINERS                                        |   8 +
 drivers/iio/dac/Kconfig                            |  11 +
 drivers/iio/dac/Makefile                           |   1 +
 drivers/iio/dac/ad3530r.c                          | 503 +++++++++++++++++++++
 6 files changed, 625 insertions(+)
---
base-commit: 3159d40a2ca0ae14e69e1cae8b12f04c933d0445
change-id: 20250319-togreg-fc6a0af961ed

Best regards,
-- 
Kim Seer Paller <kimseer.paller@analog.com>

[PATCH v5 1/3] iio: ABI: add new DAC powerdown mode

[Kim Seer Paller]

Add a new powerdown mode for DACs with 7.7kohm and 32kohm resistor
to GND.

Reviewed-by: David Lechner <dlechner@baylibre.com>
Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
---
 Documentation/ABI/testing/sysfs-bus-iio | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index 33c09c4ac60a4feec82308461643134f5ba84b66..190bfcc1e836b69622692d7c056c0092e00f1a9b 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -741,7 +741,9 @@ Description:
 		1kohm_to_gnd: connected to ground via an 1kOhm resistor,
 		2.5kohm_to_gnd: connected to ground via a 2.5kOhm resistor,
 		6kohm_to_gnd: connected to ground via a 6kOhm resistor,
+		7.7kohm_to_gnd: connected to ground via a 7.7kOhm resistor,
 		20kohm_to_gnd: connected to ground via a 20kOhm resistor,
+		32kohm_to_gnd: connected to ground via a 32kOhm resistor,
 		42kohm_to_gnd: connected to ground via a 42kOhm resistor,
 		90kohm_to_gnd: connected to ground via a 90kOhm resistor,
 		100kohm_to_gnd: connected to ground via an 100kOhm resistor,

-- 
2.34.1

[PATCH v5 2/3] dt-bindings: iio: dac: Add adi,ad3530r.yaml

[Kim Seer Paller]

Document the AD3530/AD3530R (8-channel) and AD3531/AD3531R (4-channel)
low-power, 16-bit, buffered voltage output DACs with software-
programmable gain controls. They provide full-scale output spans of 2.5V
or 5V for reference voltages of 2.5V. These devices operate on a single
2.7V to 5.5V supply and are guaranteed to be monotonic by design.
The "R" variants include a 2.5V, 5ppm/°C internal reference, which is
disabled by default.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: David Lechner <dlechner@baylibre.com>
Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
---
 .../devicetree/bindings/iio/dac/adi,ad3530r.yaml   | 100 +++++++++++++++++++++
 MAINTAINERS                                        |   7 ++
 2 files changed, 107 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad3530r.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad3530r.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..a355d52a9d641e488fe291b97bc95ed115e96afd
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/dac/adi,ad3530r.yaml
@@ -0,0 +1,100 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/dac/adi,ad3530r.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD3530R and Similar DACs
+
+maintainers:
+  - Kim Seer Paller <kimseer.paller@analog.com>
+
+description: |
+  The AD3530/AD3530R (8-channel) and AD3531/AD3531R (4-channel) are low-power,
+  16-bit, buffered voltage output digital-to-analog converters (DACs) with
+  software-programmable gain controls, providing full-scale output spans of 2.5V
+  or 5V for reference voltages of 2.5V. These devices operate from a single 2.7V
+  to 5.5V supply and are guaranteed monotonic by design. The "R" variants
+  include a 2.5V, 5ppm/°C internal reference, which is disabled by default.
+  Datasheet can be found here:
+  https://www.analog.com/media/en/technical-documentation/data-sheets/ad3530_ad530r.pdf
+  https://www.analog.com/media/en/technical-documentation/data-sheets/ad3531-ad3531r.pdf
+
+properties:
+  compatible:
+    enum:
+      - adi,ad3530
+      - adi,ad3530r
+      - adi,ad3531
+      - adi,ad3531r
+
+  reg:
+    maxItems: 1
+
+  spi-max-frequency:
+    maximum: 50000000
+
+  vdd-supply:
+    description: Power Supply Input.
+
+  iovdd-supply:
+    description: Digital Power Supply Input.
+
+  io-channels:
+    description:
+      ADC channel used to monitor internal die temperature, output voltages, and
+      current of a selected channel via the MUXOUT pin.
+    maxItems: 1
+
+  ref-supply:
+    description:
+      Reference Input/Output. The voltage at the REF pin sets the full-scale
+      range of all channels. If not provided the internal reference is used and
+      also provided on the VREF pin.
+
+  reset-gpios:
+    description:
+      Active low signal that is falling edge sensitive. When it is deasserted,
+      the digital core initialization is performed and all DAC registers except
+      the Interface Configuration A register are reset to their default values.
+    maxItems: 1
+
+  ldac-gpios:
+    description:
+      LDAC pin to be used as a hardware trigger to update the DAC channels. If
+      not present, the DAC channels are updated by Software LDAC.
+    maxItems: 1
+
+  adi,range-double:
+    description:
+      Configure the output range for all channels. If the property is present,
+      the output will range from 0V to 2Vref. If the property is not present,
+      the output will range from 0V to Vref.
+    type: boolean
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+  - iovdd-supply
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        dac@0 {
+            compatible = "adi,ad3530r";
+            reg = <0>;
+            spi-max-frequency = <1000000>;
+
+            vdd-supply = <&vdd>;
+            iovdd-supply = <&iovdd>;
+        };
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 01079a189c93697c1db6b0ca4e54212d25589974..4ca59fc1bf25aa49fecf78a90b2e1b73f25a2c05 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1300,6 +1300,13 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git
 F:	drivers/net/amt.c
 
+ANALOG DEVICES INC AD3530R DRIVER
+M:	Kim Seer Paller <kimseer.paller@analog.com>
+L:	linux-iio@vger.kernel.org
+S:	Supported
+W:	https://ez.analog.com/linux-software-drivers
+F:	Documentation/devicetree/bindings/iio/dac/adi,ad3530r.yaml
+
 ANALOG DEVICES INC AD3552R DRIVER
 M:	Nuno Sá <nuno.sa@analog.com>
 L:	linux-iio@vger.kernel.org

-- 
2.34.1

[PATCH v5 3/3] iio: dac: ad3530r: Add driver for AD3530R and AD3531R

[Kim Seer Paller]

The AD3530/AD3530R (8-channel) and AD3531/AD3531R (4-channel) are
low-power, 16-bit, buffered voltage output DACs with software-
programmable gain controls, providing full-scale output spans of 2.5V or
5V for reference voltages of 2.5V. These devices operate from a single
2.7V to 5.5V supply and are guaranteed monotonic by design. The "R"
variants include a 2.5V, 5ppm/°C internal reference, which is disabled
by default.

Support for monitoring internal die temperature, output voltages, and
current of a selected channel via the MUXOUT pin using an external ADC
is currently not implemented.

Reviewed-by: David Lechner <dlechner@baylibre.com>
Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
---
 MAINTAINERS               |   1 +
 drivers/iio/dac/Kconfig   |  11 +
 drivers/iio/dac/Makefile  |   1 +
 drivers/iio/dac/ad3530r.c | 503 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 516 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 4ca59fc1bf25aa49fecf78a90b2e1b73f25a2c05..25356d5a1a09e5ff6e7ed8eaa98a8ea990e79d8e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1306,6 +1306,7 @@ L:	linux-iio@vger.kernel.org
 S:	Supported
 W:	https://ez.analog.com/linux-software-drivers
 F:	Documentation/devicetree/bindings/iio/dac/adi,ad3530r.yaml
+F:	drivers/iio/dac/ad3530r.c
 
 ANALOG DEVICES INC AD3552R DRIVER
 M:	Nuno Sá <nuno.sa@analog.com>
diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
index 4811ea973125a0dea1f8a9cdee1e0c045bc21981..e0996dc014a3d538ab6b4e0d50ff54ede50f1527 100644
--- a/drivers/iio/dac/Kconfig
+++ b/drivers/iio/dac/Kconfig
@@ -6,6 +6,17 @@
 
 menu "Digital to analog converters"
 
+config AD3530R
+	tristate "Analog Devices AD3530R and Similar DACs driver"
+	depends on SPI
+	select REGMAP_SPI
+	help
+	  Say yes here to build support for Analog Devices AD3530R, AD3531R
+	  Digital to Analog Converter.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ad3530r.
+
 config AD3552R_HS
 	tristate "Analog Devices AD3552R DAC High Speed driver"
 	select AD3552R_LIB
diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
index 8dd6cce81ed1152be4cf0af9ef877b5482ceb347..3684cd52b7fa9bc0ad9f855323dcbb2e4965c404 100644
--- a/drivers/iio/dac/Makefile
+++ b/drivers/iio/dac/Makefile
@@ -4,6 +4,7 @@
 #
 
 # When adding new entries keep the list in alphabetical order
+obj-$(CONFIG_AD3530R) += ad3530r.o
 obj-$(CONFIG_AD3552R_HS) += ad3552r-hs.o
 obj-$(CONFIG_AD3552R_LIB) += ad3552r-common.o
 obj-$(CONFIG_AD3552R) += ad3552r.o
diff --git a/drivers/iio/dac/ad3530r.c b/drivers/iio/dac/ad3530r.c
new file mode 100644
index 0000000000000000000000000000000000000000..05bd191e5225bd267f42ba36bbd42a18e6f22291
--- /dev/null
+++ b/drivers/iio/dac/ad3530r.c
@@ -0,0 +1,503 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AD3530R/AD3530 8-channel, 16-bit Voltage Output DAC Driver
+ * AD3531R/AD3531 4-channel, 16-bit Voltage Output DAC Driver
+ *
+ * Copyright 2025 Analog Devices Inc.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/cleanup.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/mutex.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+
+#define AD3530R_INTERFACE_CONFIG_A		0x00
+#define AD3530R_OUTPUT_OPERATING_MODE_0		0x20
+#define AD3530R_OUTPUT_OPERATING_MODE_1		0x21
+#define AD3530R_OUTPUT_CONTROL_0		0x2A
+#define AD3530R_REFERENCE_CONTROL_0		0x3C
+#define AD3530R_SW_LDAC_TRIG_A			0xE5
+#define AD3530R_INPUT_CH			0xEB
+#define AD3530R_MAX_REG_ADDR			0xF9
+
+#define AD3531R_SW_LDAC_TRIG_A			0xDD
+#define AD3531R_INPUT_CH			0xE3
+
+#define AD3530R_SLD_TRIG_A			BIT(7)
+#define AD3530R_OUTPUT_CONTROL_RANGE		BIT(2)
+#define AD3530R_REFERENCE_CONTROL_SEL		BIT(0)
+#define AD3530R_REG_VAL_MASK			GENMASK(15, 0)
+
+#define AD3530R_SW_RESET			(BIT(7) | BIT(0))
+#define AD3530R_MAX_CHANNELS			8
+#define AD3531R_MAX_CHANNELS			4
+#define AD3530R_INTERNAL_VREF_MV		2500
+#define AD3530R_LDAC_PULSE_US			100
+
+enum ad3530r_mode {
+	AD3530R_NORMAL_OPERATION,
+	AD3530R_POWERDOWN_1K,
+	AD3530R_POWERDOWN_7K7,
+	AD3530R_POWERDOWN_32K,
+};
+
+struct ad3530r_chan {
+	enum ad3530r_mode powerdown_mode;
+	bool powerdown;
+};
+
+struct ad3530r_chip_info {
+	const char *name;
+	const struct iio_chan_spec *channels;
+	int (*input_ch_reg)(unsigned int c);
+	unsigned int num_channels;
+	unsigned int sw_ldac_trig_reg;
+	bool internal_ref_support;
+};
+
+struct ad3530r_state {
+	struct regmap *regmap;
+	/* lock to protect against multiple access to the device and shared data */
+	struct mutex lock;
+	struct ad3530r_chan chan[AD3530R_MAX_CHANNELS];
+	const struct ad3530r_chip_info *chip_info;
+	struct gpio_desc *ldac_gpio;
+	int vref_mv;
+	/*
+	 * DMA (thus cache coherency maintenance) may require the transfer
+	 * buffers to live in their own cache lines.
+	 */
+	__be16 buf __aligned(IIO_DMA_MINALIGN);
+};
+
+static int ad3530r_input_ch_reg(unsigned int c)
+{
+	return 2 * c + AD3530R_INPUT_CH;
+}
+
+static int ad3531r_input_ch_reg(unsigned int c)
+{
+	return 2 * c + AD3531R_INPUT_CH;
+}
+
+static const char * const ad3530r_powerdown_modes[] = {
+	"1kohm_to_gnd",
+	"7.7kohm_to_gnd",
+	"32kohm_to_gnd",
+};
+
+static int ad3530r_get_powerdown_mode(struct iio_dev *indio_dev,
+				      const struct iio_chan_spec *chan)
+{
+	struct ad3530r_state *st = iio_priv(indio_dev);
+
+	guard(mutex)(&st->lock);
+	return st->chan[chan->channel].powerdown_mode - 1;
+}
+
+static int ad3530r_set_powerdown_mode(struct iio_dev *indio_dev,
+				      const struct iio_chan_spec *chan,
+				      unsigned int mode)
+{
+	struct ad3530r_state *st = iio_priv(indio_dev);
+
+	guard(mutex)(&st->lock);
+	st->chan[chan->channel].powerdown_mode = mode + 1;
+
+	return 0;
+}
+
+static const struct iio_enum ad3530r_powerdown_mode_enum = {
+	.items = ad3530r_powerdown_modes,
+	.num_items = ARRAY_SIZE(ad3530r_powerdown_modes),
+	.get = ad3530r_get_powerdown_mode,
+	.set = ad3530r_set_powerdown_mode,
+};
+
+static ssize_t ad3530r_get_dac_powerdown(struct iio_dev *indio_dev,
+					 uintptr_t private,
+					 const struct iio_chan_spec *chan,
+					 char *buf)
+{
+	struct ad3530r_state *st = iio_priv(indio_dev);
+
+	guard(mutex)(&st->lock);
+	return sysfs_emit(buf, "%d\n", st->chan[chan->channel].powerdown);
+}
+
+static ssize_t ad3530r_set_dac_powerdown(struct iio_dev *indio_dev,
+					 uintptr_t private,
+					 const struct iio_chan_spec *chan,
+					 const char *buf, size_t len)
+{
+	struct ad3530r_state *st = iio_priv(indio_dev);
+	int ret;
+	unsigned int mask, val, reg;
+	bool powerdown;
+
+	ret = kstrtobool(buf, &powerdown);
+	if (ret)
+		return ret;
+
+	guard(mutex)(&st->lock);
+	mask = GENMASK(chan->address + 1, chan->address);
+	reg = chan->channel < AD3531R_MAX_CHANNELS ?
+	      AD3530R_OUTPUT_OPERATING_MODE_0 :
+	      AD3530R_OUTPUT_OPERATING_MODE_1;
+	val = (powerdown ? st->chan[chan->channel].powerdown_mode : 0)
+	       << chan->address;
+
+	ret = regmap_update_bits(st->regmap, reg, mask, val);
+	if (ret)
+		return ret;
+
+	st->chan[chan->channel].powerdown = powerdown;
+
+	return len;
+}
+
+static int ad3530r_trigger_hw_ldac(struct gpio_desc *ldac_gpio)
+{
+	gpiod_set_value_cansleep(ldac_gpio, 1);
+	fsleep(AD3530R_LDAC_PULSE_US);
+	gpiod_set_value_cansleep(ldac_gpio, 0);
+
+	return 0;
+}
+
+static int ad3530r_dac_write(struct ad3530r_state *st, unsigned int chan,
+			     unsigned int val)
+{
+	int ret;
+
+	guard(mutex)(&st->lock);
+	st->buf = cpu_to_be16(val);
+
+	ret = regmap_bulk_write(st->regmap, st->chip_info->input_ch_reg(chan),
+				&st->buf, sizeof(st->buf));
+	if (ret)
+		return ret;
+
+	if (st->ldac_gpio)
+		return ad3530r_trigger_hw_ldac(st->ldac_gpio);
+
+	return regmap_set_bits(st->regmap, st->chip_info->sw_ldac_trig_reg,
+			       AD3530R_SLD_TRIG_A);
+}
+
+static int ad3530r_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long info)
+{
+	struct ad3530r_state *st = iio_priv(indio_dev);
+	int ret;
+
+	guard(mutex)(&st->lock);
+	switch (info) {
+	case IIO_CHAN_INFO_RAW:
+		ret = regmap_bulk_read(st->regmap,
+				       st->chip_info->input_ch_reg(chan->channel),
+				       &st->buf, sizeof(st->buf));
+		if (ret)
+			return ret;
+
+		*val = FIELD_GET(AD3530R_REG_VAL_MASK, be16_to_cpu(st->buf));
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		*val = st->vref_mv;
+		*val2 = 16;
+
+		return IIO_VAL_FRACTIONAL_LOG2;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad3530r_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int val, int val2, long info)
+{
+	struct ad3530r_state *st = iio_priv(indio_dev);
+
+	switch (info) {
+	case IIO_CHAN_INFO_RAW:
+		if (val < 0 || val > U16_MAX)
+			return -EINVAL;
+
+		return ad3530r_dac_write(st, chan->channel, val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad3530r_reg_access(struct iio_dev *indio_dev, unsigned int reg,
+			      unsigned int writeval, unsigned int *readval)
+{
+	struct ad3530r_state *st = iio_priv(indio_dev);
+
+	if (readval)
+		return regmap_read(st->regmap, reg, readval);
+
+	return regmap_write(st->regmap, reg, writeval);
+}
+
+static const struct iio_chan_spec_ext_info ad3530r_ext_info[] = {
+	{
+		.name = "powerdown",
+		.shared = IIO_SEPARATE,
+		.read = ad3530r_get_dac_powerdown,
+		.write = ad3530r_set_dac_powerdown,
+	},
+	IIO_ENUM("powerdown_mode", IIO_SEPARATE, &ad3530r_powerdown_mode_enum),
+	IIO_ENUM_AVAILABLE("powerdown_mode", IIO_SHARED_BY_TYPE,
+			   &ad3530r_powerdown_mode_enum),
+	{ }
+};
+
+#define AD3530R_CHAN(_chan, _pos) {					\
+	.type = IIO_VOLTAGE,						\
+	.indexed = 1,							\
+	.channel = _chan,						\
+	.address = _pos,						\
+	.output = 1,							\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
+			      BIT(IIO_CHAN_INFO_SCALE),			\
+	.ext_info = ad3530r_ext_info,					\
+}
+
+static const struct iio_chan_spec ad3530r_channels[] = {
+	AD3530R_CHAN(0, 0),
+	AD3530R_CHAN(1, 2),
+	AD3530R_CHAN(2, 4),
+	AD3530R_CHAN(3, 6),
+	AD3530R_CHAN(4, 0),
+	AD3530R_CHAN(5, 2),
+	AD3530R_CHAN(6, 4),
+	AD3530R_CHAN(7, 6),
+};
+
+static const struct iio_chan_spec ad3531r_channels[] = {
+	AD3530R_CHAN(0, 0),
+	AD3530R_CHAN(1, 2),
+	AD3530R_CHAN(2, 4),
+	AD3530R_CHAN(3, 6),
+};
+
+static const struct ad3530r_chip_info ad3530_chip = {
+	.name = "ad3530",
+	.channels = ad3530r_channels,
+	.num_channels = ARRAY_SIZE(ad3530r_channels),
+	.sw_ldac_trig_reg = AD3530R_SW_LDAC_TRIG_A,
+	.input_ch_reg = ad3530r_input_ch_reg,
+	.internal_ref_support = false,
+};
+
+static const struct ad3530r_chip_info ad3530r_chip = {
+	.name = "ad3530r",
+	.channels = ad3530r_channels,
+	.num_channels = ARRAY_SIZE(ad3530r_channels),
+	.sw_ldac_trig_reg = AD3530R_SW_LDAC_TRIG_A,
+	.input_ch_reg = ad3530r_input_ch_reg,
+	.internal_ref_support = true,
+};
+
+static const struct ad3530r_chip_info ad3531_chip = {
+	.name = "ad3531",
+	.channels = ad3531r_channels,
+	.num_channels = ARRAY_SIZE(ad3531r_channels),
+	.sw_ldac_trig_reg = AD3531R_SW_LDAC_TRIG_A,
+	.input_ch_reg = ad3531r_input_ch_reg,
+	.internal_ref_support = false,
+};
+
+static const struct ad3530r_chip_info ad3531r_chip = {
+	.name = "ad3531r",
+	.channels = ad3531r_channels,
+	.num_channels = ARRAY_SIZE(ad3531r_channels),
+	.sw_ldac_trig_reg = AD3531R_SW_LDAC_TRIG_A,
+	.input_ch_reg = ad3531r_input_ch_reg,
+	.internal_ref_support = true,
+};
+
+static int ad3530r_setup(struct ad3530r_state *st, int vref,
+			 bool has_external_vref)
+{
+	struct device *dev = regmap_get_device(st->regmap);
+	struct gpio_desc *reset_gpio;
+	int i, ret;
+	u8 range_multiplier;
+
+	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(reset_gpio))
+		return dev_err_probe(dev, PTR_ERR(reset_gpio),
+				     "Failed to get reset GPIO\n");
+
+	if (reset_gpio) {
+		/* Perform hardware reset */
+		fsleep(1000);
+		gpiod_set_value_cansleep(reset_gpio, 0);
+	} else {
+		/* Perform software reset */
+		ret = regmap_update_bits(st->regmap, AD3530R_INTERFACE_CONFIG_A,
+					 AD3530R_SW_RESET, AD3530R_SW_RESET);
+		if (ret)
+			return ret;
+	}
+
+	fsleep(10000);
+
+	range_multiplier = 1;
+	if (device_property_read_bool(dev, "adi,range-double")) {
+		ret = regmap_set_bits(st->regmap, AD3530R_OUTPUT_CONTROL_0,
+				      AD3530R_OUTPUT_CONTROL_RANGE);
+		if (ret)
+			return ret;
+
+		range_multiplier = 2;
+	}
+
+	if (!has_external_vref && st->chip_info->internal_ref_support) {
+		ret = regmap_set_bits(st->regmap, AD3530R_REFERENCE_CONTROL_0,
+				      AD3530R_REFERENCE_CONTROL_SEL);
+		if (ret)
+			return ret;
+
+		st->vref_mv = range_multiplier * AD3530R_INTERNAL_VREF_MV;
+	}
+
+	if (has_external_vref)
+		st->vref_mv = range_multiplier * vref / 1000;
+
+	/* Set operating mode to normal operation. */
+	ret = regmap_write(st->regmap, AD3530R_OUTPUT_OPERATING_MODE_0,
+			   AD3530R_NORMAL_OPERATION);
+	if (ret)
+		return ret;
+
+	if (st->chip_info->num_channels > AD3531R_MAX_CHANNELS) {
+		ret = regmap_write(st->regmap, AD3530R_OUTPUT_OPERATING_MODE_1,
+				   AD3530R_NORMAL_OPERATION);
+		if (ret)
+			return ret;
+	}
+
+	for (i = 0; i < st->chip_info->num_channels; i++)
+		st->chan[i].powerdown_mode = AD3530R_POWERDOWN_32K;
+
+	st->ldac_gpio = devm_gpiod_get_optional(dev, "ldac", GPIOD_OUT_LOW);
+	if (IS_ERR(st->ldac_gpio))
+		return dev_err_probe(dev, PTR_ERR(st->ldac_gpio),
+				     "Failed to get ldac GPIO\n");
+
+	return 0;
+}
+
+static const struct regmap_config ad3530r_regmap_config = {
+	.reg_bits = 16,
+	.val_bits = 8,
+	.max_register = AD3530R_MAX_REG_ADDR,
+};
+
+static const struct iio_info ad3530r_info = {
+	.read_raw = ad3530r_read_raw,
+	.write_raw = ad3530r_write_raw,
+	.debugfs_reg_access = ad3530r_reg_access,
+};
+
+static int ad3530r_probe(struct spi_device *spi)
+{
+	static const char * const regulators[] = { "vdd", "iovdd" };
+	struct device *dev = &spi->dev;
+	struct iio_dev *indio_dev;
+	struct ad3530r_state *st;
+	bool has_external_vref;
+	int ret, vref;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+
+	st->regmap = devm_regmap_init_spi(spi, &ad3530r_regmap_config);
+	if (IS_ERR(st->regmap))
+		return dev_err_probe(dev, PTR_ERR(st->regmap),
+				     "Failed to init regmap");
+
+	ret = devm_mutex_init(dev, &st->lock);
+	if (ret)
+		return ret;
+
+	st->chip_info = spi_get_device_match_data(spi);
+	if (!st->chip_info)
+		return -ENODEV;
+
+	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulators),
+					     regulators);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable regulators\n");
+
+	vref = devm_regulator_get_enable_read_voltage(dev, "ref");
+	if (vref < 0 && vref != -ENODEV)
+		return vref;
+
+	has_external_vref = vref != -ENODEV;
+
+	if (!st->chip_info->internal_ref_support && !has_external_vref)
+		return -ENODEV;
+
+	ret = ad3530r_setup(st, vref, has_external_vref);
+	if (ret)
+		return ret;
+
+	indio_dev->name = st->chip_info->name;
+	indio_dev->info = &ad3530r_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = st->chip_info->channels;
+	indio_dev->num_channels = st->chip_info->num_channels;
+
+	return devm_iio_device_register(&spi->dev, indio_dev);
+}
+
+static const struct spi_device_id ad3530r_id[] = {
+	{ "ad3530", (kernel_ulong_t)&ad3530_chip },
+	{ "ad3530r", (kernel_ulong_t)&ad3530r_chip },
+	{ "ad3531", (kernel_ulong_t)&ad3531_chip },
+	{ "ad3531r", (kernel_ulong_t)&ad3531r_chip },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, ad3530r_id);
+
+static const struct of_device_id ad3530r_of_match[] = {
+	{ .compatible = "adi,ad3530", .data = &ad3530_chip },
+	{ .compatible = "adi,ad3530r", .data = &ad3530r_chip },
+	{ .compatible = "adi,ad3531", .data = &ad3531_chip },
+	{ .compatible = "adi,ad3531r", .data = &ad3531r_chip },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ad3530r_of_match);
+
+static struct spi_driver ad3530r_driver = {
+	.driver = {
+		.name = "ad3530r",
+		.of_match_table = ad3530r_of_match,
+	},
+	.probe = ad3530r_probe,
+	.id_table = ad3530r_id,
+};
+module_spi_driver(ad3530r_driver);
+
+MODULE_AUTHOR("Kim Seer Paller <kimseer.paller@analog.com>");
+MODULE_DESCRIPTION("Analog Devices AD3530R and Similar DACs Driver");
+MODULE_LICENSE("GPL");

-- 
2.34.1

Re: [PATCH v5 2/3] dt-bindings: iio: dac: Add adi,ad3530r.yaml

[Rob Herring (Arm)]

On Mon, 21 Apr 2025 12:24:53 +0800, Kim Seer Paller wrote:
> Document the AD3530/AD3530R (8-channel) and AD3531/AD3531R (4-channel)
> low-power, 16-bit, buffered voltage output DACs with software-
> programmable gain controls. They provide full-scale output spans of 2.5V
> or 5V for reference voltages of 2.5V. These devices operate on a single
> 2.7V to 5.5V supply and are guaranteed to be monotonic by design.
> The "R" variants include a 2.5V, 5ppm/°C internal reference, which is
> disabled by default.
> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Reviewed-by: David Lechner <dlechner@baylibre.com>
> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
> ---
>  .../devicetree/bindings/iio/dac/adi,ad3530r.yaml   | 100 +++++++++++++++++++++
>  MAINTAINERS                                        |   7 ++
>  2 files changed, 107 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/soc/fsl/fsl,ls1028a-reset.yaml: maintainers:0: 'Frank Li' does not match '@'
	from schema $id: http://devicetree.org/meta-schemas/base.yaml#

doc reference errors (make refcheckdocs):
Documentation/userspace-api/netlink/netlink-raw.rst: :doc:`rt_link<../../networking/netlink_spec/rt_link>`
Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/leds/backlight/ti,lp8864.yaml
Documentation/userspace-api/netlink/netlink-raw.rst: :doc:`rt_link<../../networking/netlink_spec/rt_link>`
MAINTAINERS: Documentation/devicetree/bindings/leds/backlight/ti,lp8864.yaml

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250421-togreg-v5-2-94341574240f@analog.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v5 3/3] iio: dac: ad3530r: Add driver for AD3530R and AD3531R

[Jonathan Cameron]

On Mon, 21 Apr 2025 12:24:54 +0800
Kim Seer Paller <kimseer.paller@analog.com> wrote:

> The AD3530/AD3530R (8-channel) and AD3531/AD3531R (4-channel) are
> low-power, 16-bit, buffered voltage output DACs with software-
> programmable gain controls, providing full-scale output spans of 2.5V or
> 5V for reference voltages of 2.5V. These devices operate from a single
> 2.7V to 5.5V supply and are guaranteed monotonic by design. The "R"
> variants include a 2.5V, 5ppm/°C internal reference, which is disabled
> by default.
> 
> Support for monitoring internal die temperature, output voltages, and
> current of a selected channel via the MUXOUT pin using an external ADC
> is currently not implemented.
> 
> Reviewed-by: David Lechner <dlechner@baylibre.com>
> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
Hi.

Just one thing from a final pre merge look through.

The initialization of powerdown mode works but only because the NORMAL
mode == 0.  That should be setting it explicitly for each set of 4 channels
as needed.

I don't really mind how you solve that.  There are lots of options
to build up the 4 fields in each of those registers.

Jonathan

> diff --git a/drivers/iio/dac/ad3530r.c b/drivers/iio/dac/ad3530r.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..05bd191e5225bd267f42ba36bbd42a18e6f22291
> --- /dev/null
> +++ b/drivers/iio/dac/ad3530r.c
> @@ -0,0 +1,503 @@

> +
> +static ssize_t ad3530r_set_dac_powerdown(struct iio_dev *indio_dev,
> +					 uintptr_t private,
> +					 const struct iio_chan_spec *chan,
> +					 const char *buf, size_t len)
> +{
> +	struct ad3530r_state *st = iio_priv(indio_dev);
> +	int ret;
> +	unsigned int mask, val, reg;
> +	bool powerdown;
> +
> +	ret = kstrtobool(buf, &powerdown);
> +	if (ret)
> +		return ret;
> +
> +	guard(mutex)(&st->lock);
> +	mask = GENMASK(chan->address + 1, chan->address);

I think maybe we need a macro to get the mask from the channel number?
Using address for this seems overkill given how simple that maths is.
Ideally that macro could perhaps be used in the code below to avoid
all the defines I suggested.


> +	reg = chan->channel < AD3531R_MAX_CHANNELS ?
> +	      AD3530R_OUTPUT_OPERATING_MODE_0 :
> +	      AD3530R_OUTPUT_OPERATING_MODE_1;
> +	val = (powerdown ? st->chan[chan->channel].powerdown_mode : 0)
> +	       << chan->address;
> +


> +static int ad3530r_setup(struct ad3530r_state *st, int vref,
> +			 bool has_external_vref)
> +{

> +
> +	if (has_external_vref)
> +		st->vref_mv = range_multiplier * vref / 1000;
> +
> +	/* Set operating mode to normal operation. */
> +	ret = regmap_write(st->regmap, AD3530R_OUTPUT_OPERATING_MODE_0,
> +			   AD3530R_NORMAL_OPERATION);
Is this actually doing that?  I think the register is lots of 2 bit
fields and this is only setting it for the first channel?

This works because that value is 0.  Logically however we should set it
to
	(AD3530R_NORMAL_OPERATION << 6) |
	(AD3530R_NORMAL_OPERATION << 4) |
	(AD3530R_NORMAL_OPERATION << 2) |
	(AD3530R_NORMAL_OPERATION << 0)

Or possibly better as

	FIELD_PREP(AD3530R_OP_MODE_0_CHAN0_MSK, AD3530R_NORMAL_OPERATION) |
	FIELD_PREP(AD3530R_OP_MODE_0_CHAN1_MSK, AD3530R_NORMAL_OPERATION) |

etc

Names are a bit long, so maybe consider shortening some of the defines.

> +	if (ret)
> +		return ret;
> +
> +	if (st->chip_info->num_channels > AD3531R_MAX_CHANNELS) {

If we have it explicit that we have multiple fields this will become more obvious.
However I'd use the number 4 here rather than the number of channels the AD3531R happens
to have.


> +		ret = regmap_write(st->regmap, AD3530R_OUTPUT_OPERATING_MODE_1,
> +				   AD3530R_NORMAL_OPERATION);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	for (i = 0; i < st->chip_info->num_channels; i++)
> +		st->chan[i].powerdown_mode = AD3530R_POWERDOWN_32K;
> +
> +	st->ldac_gpio = devm_gpiod_get_optional(dev, "ldac", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->ldac_gpio))
> +		return dev_err_probe(dev, PTR_ERR(st->ldac_gpio),
> +				     "Failed to get ldac GPIO\n");
> +
> +	return 0;
> +}
> +
> +static const struct regmap_config ad3530r_regmap_config = {
> +	.reg_bits = 16,
> +	.val_bits = 8,
> +	.max_register = AD3530R_MAX_REG_ADDR,
> +};

Re: [PATCH v5 3/3] iio: dac: ad3530r: Add driver for AD3530R and AD3531R

[Andy Shevchenko]

On Mon, Apr 21, 2025 at 12:24:54PM +0800, Kim Seer Paller wrote:
> The AD3530/AD3530R (8-channel) and AD3531/AD3531R (4-channel) are
> low-power, 16-bit, buffered voltage output DACs with software-
> programmable gain controls, providing full-scale output spans of 2.5V or
> 5V for reference voltages of 2.5V. These devices operate from a single
> 2.7V to 5.5V supply and are guaranteed monotonic by design. The "R"
> variants include a 2.5V, 5ppm/°C internal reference, which is disabled
> by default.
> 
> Support for monitoring internal die temperature, output voltages, and
> current of a selected channel via the MUXOUT pin using an external ADC
> is currently not implemented.

...

> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/cleanup.h>
> +#include <linux/delay.h>

> +#include <linux/device.h>

I don't see how you use this. But

+ dev_printk.h

> +#include <linux/gpio/consumer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/mutex.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>

...

> +#define AD3530R_SW_RESET			(BIT(7) | BIT(0))

> +#define AD3530R_MAX_CHANNELS			8
> +#define AD3531R_MAX_CHANNELS			4

Sounds to me that these two shouldn't be grouped with the rest here. Perhaps
move them out to after the LDAC_PULSE?

> +#define AD3530R_INTERNAL_VREF_MV		2500

_mV (yes, with Volts and Amperes we use proper spelling).

> +#define AD3530R_LDAC_PULSE_US			100

...

> +	int (*input_ch_reg)(unsigned int c);

c? channel?

...

> +	int vref_mv;

_mV

...

> +static int ad3530r_input_ch_reg(unsigned int c)
> +{
> +	return 2 * c + AD3530R_INPUT_CH;
> +}
> +
> +static int ad3531r_input_ch_reg(unsigned int c)
> +{
> +	return 2 * c + AD3531R_INPUT_CH;
> +}

c --> channel

...

> +static const char * const ad3530r_powerdown_modes[] = {
> +	"1kohm_to_gnd",

kOhm

> +	"7.7kohm_to_gnd",

Ditto.

> +	"32kohm_to_gnd",

Ditto.

> +};

...

> +static const struct iio_enum ad3530r_powerdown_mode_enum = {
> +	.items = ad3530r_powerdown_modes,
> +	.num_items = ARRAY_SIZE(ad3530r_powerdown_modes),

+ array_size.h

> +	.get = ad3530r_get_powerdown_mode,
> +	.set = ad3530r_set_powerdown_mode,
> +};

...

> +static ssize_t ad3530r_get_dac_powerdown(struct iio_dev *indio_dev,
> +					 uintptr_t private,
> +					 const struct iio_chan_spec *chan,
> +					 char *buf)
> +{
> +	struct ad3530r_state *st = iio_priv(indio_dev);
> +
> +	guard(mutex)(&st->lock);
> +	return sysfs_emit(buf, "%d\n", st->chan[chan->channel].powerdown);

+ sysfs.h

> +}

...

> +static ssize_t ad3530r_set_dac_powerdown(struct iio_dev *indio_dev,
> +					 uintptr_t private,
> +					 const struct iio_chan_spec *chan,
> +					 const char *buf, size_t len)
> +{
> +	struct ad3530r_state *st = iio_priv(indio_dev);
> +	int ret;
> +	unsigned int mask, val, reg;
> +	bool powerdown;
> +
> +	ret = kstrtobool(buf, &powerdown);

+ kstrtox.h

> +	if (ret)
> +		return ret;
> +
> +	guard(mutex)(&st->lock);
> +	mask = GENMASK(chan->address + 1, chan->address);
> +	reg = chan->channel < AD3531R_MAX_CHANNELS ?
> +	      AD3530R_OUTPUT_OPERATING_MODE_0 :
> +	      AD3530R_OUTPUT_OPERATING_MODE_1;
> +	val = (powerdown ? st->chan[chan->channel].powerdown_mode : 0)
> +	       << chan->address;

Please, move the operator to the previous line, Or even

	... pdmode;

	pdmode = powerdown ? st->chan[chan->channel].powerdown_mode : 0;
	val = pdmode << ...;

> +	ret = regmap_update_bits(st->regmap, reg, mask, val);
> +	if (ret)
> +		return ret;
> +
> +	st->chan[chan->channel].powerdown = powerdown;
> +
> +	return len;
> +}

...

> +	struct ad3530r_state *st = iio_priv(indio_dev);
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (val < 0 || val > U16_MAX)

U16_MAX is an abstract type with this limit, do you have any predefined HW
limit instead? Probably better to use that one as defined via BIT() / GENMASK().

> +			return -EINVAL;
> +
> +		return ad3530r_dac_write(st, chan->channel, val);
> +	default:
> +		return -EINVAL;
> +	}

...

> +static const struct iio_chan_spec_ext_info ad3530r_ext_info[] = {
> +	{
> +		.name = "powerdown",
> +		.shared = IIO_SEPARATE,
> +		.read = ad3530r_get_dac_powerdown,
> +		.write = ad3530r_set_dac_powerdown,
> +	},
> +	IIO_ENUM("powerdown_mode", IIO_SEPARATE, &ad3530r_powerdown_mode_enum),
> +	IIO_ENUM_AVAILABLE("powerdown_mode", IIO_SHARED_BY_TYPE,
> +			   &ad3530r_powerdown_mode_enum),
> +	{ }
> +};
> +
> +#define AD3530R_CHAN(_chan, _pos) {					\

Slightly better to have the curly braces on the same column as it's easier to
read.

#define AD3530R_CHAN(_chan, _pos)				\
{								\

(and make it one TAB less for the backslash).

> +	.type = IIO_VOLTAGE,						\
> +	.indexed = 1,							\
> +	.channel = _chan,						\
> +	.address = _pos,						\
> +	.output = 1,							\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
> +			      BIT(IIO_CHAN_INFO_SCALE),			\
> +	.ext_info = ad3530r_ext_info,					\
> +}

...

> +static int ad3530r_setup(struct ad3530r_state *st, int vref,
> +			 bool has_external_vref)
> +{
> +	struct device *dev = regmap_get_device(st->regmap);
> +	struct gpio_desc *reset_gpio;
> +	int i, ret;
> +	u8 range_multiplier;

+ types.h (and especially for boolean type along with true/false definitions.

> +
> +	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(reset_gpio),
> +				     "Failed to get reset GPIO\n");

+ err.h

> +	if (reset_gpio) {
> +		/* Perform hardware reset */
> +		fsleep(1000);

(1 * USEC_PER_MSEC) ?

> +		gpiod_set_value_cansleep(reset_gpio, 0);
> +	} else {
> +		/* Perform software reset */
> +		ret = regmap_update_bits(st->regmap, AD3530R_INTERFACE_CONFIG_A,
> +					 AD3530R_SW_RESET, AD3530R_SW_RESET);
> +		if (ret)
> +			return ret;
> +	}

> +	fsleep(10000);

10 * USEC_PER_MSEC

With these constants it's less error prone (when 3 or more zeroes) and easier
to get the units without looking into fsleep() implementation / documentation.

> +	range_multiplier = 1;
> +	if (device_property_read_bool(dev, "adi,range-double")) {
> +		ret = regmap_set_bits(st->regmap, AD3530R_OUTPUT_CONTROL_0,
> +				      AD3530R_OUTPUT_CONTROL_RANGE);
> +		if (ret)
> +			return ret;
> +
> +		range_multiplier = 2;
> +	}
> +
> +	if (!has_external_vref && st->chip_info->internal_ref_support) {
> +		ret = regmap_set_bits(st->regmap, AD3530R_REFERENCE_CONTROL_0,
> +				      AD3530R_REFERENCE_CONTROL_SEL);
> +		if (ret)
> +			return ret;
> +
> +		st->vref_mv = range_multiplier * AD3530R_INTERNAL_VREF_MV;
> +	}
> +
> +	if (has_external_vref)
> +		st->vref_mv = range_multiplier * vref / 1000;

MILLI?


> +	/* Set operating mode to normal operation. */
> +	ret = regmap_write(st->regmap, AD3530R_OUTPUT_OPERATING_MODE_0,
> +			   AD3530R_NORMAL_OPERATION);
> +	if (ret)
> +		return ret;
> +
> +	if (st->chip_info->num_channels > AD3531R_MAX_CHANNELS) {
> +		ret = regmap_write(st->regmap, AD3530R_OUTPUT_OPERATING_MODE_1,
> +				   AD3530R_NORMAL_OPERATION);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	for (i = 0; i < st->chip_info->num_channels; i++)
> +		st->chan[i].powerdown_mode = AD3530R_POWERDOWN_32K;
> +
> +	st->ldac_gpio = devm_gpiod_get_optional(dev, "ldac", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->ldac_gpio))
> +		return dev_err_probe(dev, PTR_ERR(st->ldac_gpio),
> +				     "Failed to get ldac GPIO\n");
> +
> +	return 0;
> +}

...

> +	vref = devm_regulator_get_enable_read_voltage(dev, "ref");
> +	if (vref < 0 && vref != -ENODEV)
> +		return vref;
> +
> +	has_external_vref = vref != -ENODEV;

Wouldn't be better just make this 0 when it's == -ENODEV and check just the
value without having this additional boolean variable (note, I haven't checked
the meaning of Vref == 0 in case it's possible in real life and hardware
behaves adequately)?

> +	if (!st->chip_info->internal_ref_support && !has_external_vref)
> +		return -ENODEV;

> +	ret = ad3530r_setup(st, vref, has_external_vref);
> +	if (ret)
> +		return ret;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 3/3] iio: dac: ad3530r: Add driver for AD3530R and AD3531R

[David Lechner]

On 4/22/25 10:11 AM, Andy Shevchenko wrote:
> On Mon, Apr 21, 2025 at 12:24:54PM +0800, Kim Seer Paller wrote:

...

> 
>> +#define AD3530R_INTERNAL_VREF_MV		2500
> 
> _mV (yes, with Volts and Amperes we use proper spelling).

When did we start doing that? No one asked me to do this in any of the new
drivers I did in the last year, so I didn't know this was a thing we should
be doing.


> 
>> +#define AD3530R_LDAC_PULSE_US			100
> 
> ...
> 
>> +	int (*input_ch_reg)(unsigned int c);
> 
> c? channel?
> 
> ...
> 
>> +	int vref_mv;
> 
> _mV
> 
> ...
> 
>> +static int ad3530r_input_ch_reg(unsigned int c)
>> +{
>> +	return 2 * c + AD3530R_INPUT_CH;
>> +}
>> +
>> +static int ad3531r_input_ch_reg(unsigned int c)
>> +{
>> +	return 2 * c + AD3531R_INPUT_CH;
>> +}
> 
> c --> channel
> 
> ...
> 
>> +static const char * const ad3530r_powerdown_modes[] = {
>> +	"1kohm_to_gnd",
> 
> kOhm
> 
>> +	"7.7kohm_to_gnd",
> 
> Ditto.
> 
>> +	"32kohm_to_gnd",
> 
> Ditto.

These are defined by sysfs ABI, so can't be changed otherwise it would break
userspace.

Comes from...
What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_powerdown_mode

> 
>> +};
> 

Re: [PATCH v5 3/3] iio: dac: ad3530r: Add driver for AD3530R and AD3531R

[Andy Shevchenko]

On Tue, Apr 22, 2025 at 11:37:06AM -0500, David Lechner wrote:
> On 4/22/25 10:11 AM, Andy Shevchenko wrote:
> > On Mon, Apr 21, 2025 at 12:24:54PM +0800, Kim Seer Paller wrote:

...

> >> +#define AD3530R_INTERNAL_VREF_MV		2500
> > 
> > _mV (yes, with Volts and Amperes we use proper spelling).
> 
> When did we start doing that? No one asked me to do this in any of the new
> drivers I did in the last year, so I didn't know this was a thing we should
> be doing.

I remember a discussion for one driver a year or so ago. But I can't find
quickly a reference. The rationale is to be as closer as possible to real
world (physics). And, for instance, regulator framework does that already.
It's a pity not many people aware...

...

> >> +static const char * const ad3530r_powerdown_modes[] = {
> >> +	"1kohm_to_gnd",
> > 
> > kOhm
> > 
> >> +	"7.7kohm_to_gnd",
> > 
> > Ditto.
> > 
> >> +	"32kohm_to_gnd",
> > 
> > Ditto.
> 
> These are defined by sysfs ABI, so can't be changed otherwise it would break
> userspace.

Ah, okay then.

> Comes from...
> What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_powerdown_mode

> >> +};

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v5 2/3] dt-bindings: iio: dac: Add adi,ad3530r.yaml

[Paller, Kim Seer]

> -----Original Message-----
> From: Rob Herring (Arm) <robh@kernel.org>
> Sent: Monday, April 21, 2025 1:29 PM
> To: Paller, Kim Seer <KimSeer.Paller@analog.com>
> Cc: Conor Dooley <conor+dt@kernel.org>; linux-kernel@vger.kernel.org; Andy
> Shevchenko <andy@kernel.org>; Lars-Peter Clausen <lars@metafoo.de>; Nuno
> Sá <noname.nuno@gmail.com>; Sa, Nuno <Nuno.Sa@analog.com>;
> devicetree@vger.kernel.org; Krzysztof Kozlowski <krzk+dt@kernel.org>;
> Hennerich, Michael <Michael.Hennerich@analog.com>; linux-
> iio@vger.kernel.org; Jonathan Cameron <jic23@kernel.org>; Krzysztof
> Kozlowski <krzysztof.kozlowski@linaro.org>; David Lechner
> <dlechner@baylibre.com>
> Subject: Re: [PATCH v5 2/3] dt-bindings: iio: dac: Add adi,ad3530r.yaml
> 
> [External]
> 
> 
> On Mon, 21 Apr 2025 12:24:53 +0800, Kim Seer Paller wrote:
> > Document the AD3530/AD3530R (8-channel) and AD3531/AD3531R (4-
> channel)
> > low-power, 16-bit, buffered voltage output DACs with software-
> > programmable gain controls. They provide full-scale output spans of
> > 2.5V or 5V for reference voltages of 2.5V. These devices operate on a
> > single 2.7V to 5.5V supply and are guaranteed to be monotonic by design.
> > The "R" variants include a 2.5V, 5ppm/°C internal reference, which is
> > disabled by default.
> >
> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > Reviewed-by: David Lechner <dlechner@baylibre.com>
> > Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
> > ---
> >  .../devicetree/bindings/iio/dac/adi,ad3530r.yaml   | 100
> +++++++++++++++++++++
> >  MAINTAINERS                                        |   7 ++
> >  2 files changed, 107 insertions(+)
> >
> 
> My bot found errors running 'make dt_binding_check' on your patch:
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-
> ci/linux/Documentation/devicetree/bindings/soc/fsl/fsl,ls1028a-reset.yaml:
> maintainers:0: 'Frank Li' does not match '@'
> 	from schema $id:
> https://urldefense.com/v3/__http://devicetree.org/meta-
> schemas/base.yaml*__;Iw!!A3Ni8CS0y2Y!5_8LSBMQDGzDudUoqQnNMjX7xJUq
> nraYNchQ_A3pjbCPArrjLG-DhM4Ycsv0lSZN4dOkjYmd6_lIvBib$

This error is not related to the ad3530r DT bindings.

> 
> doc reference errors (make refcheckdocs):
> Documentation/userspace-api/netlink/netlink-raw.rst:
> :doc:`rt_link<../../networking/netlink_spec/rt_link>`
> Warning: MAINTAINERS references a file that doesn't exist:
> Documentation/devicetree/bindings/leds/backlight/ti,lp8864.yaml
> Documentation/userspace-api/netlink/netlink-raw.rst:
> :doc:`rt_link<../../networking/netlink_spec/rt_link>`
> MAINTAINERS:
> Documentation/devicetree/bindings/leds/backlight/ti,lp8864.yaml
> 
> See
> https://urldefense.com/v3/__https://patchwork.ozlabs.org/project/devicetree-
> bindings/patch/20250421-togreg-v5-2-
> 94341574240f@analog.com__;!!A3Ni8CS0y2Y!5_8LSBMQDGzDudUoqQnNMjX
> 7xJUqnraYNchQ_A3pjbCPArrjLG-DhM4Ycsv0lSZN4dOkjYmd60Ff7zKu$
> 
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
> 
> If you already ran 'make dt_binding_check' and didn't see the above error(s),
> then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.

RE: [PATCH v5 3/3] iio: dac: ad3530r: Add driver for AD3530R and AD3531R

[Paller, Kim Seer]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Monday, April 21, 2025 9:48 PM
> To: Paller, Kim Seer <KimSeer.Paller@analog.com>
> Cc: Lars-Peter Clausen <lars@metafoo.de>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Rob Herring <robh@kernel.org>; Krzysztof
> Kozlowski <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; David
> Lechner <dlechner@baylibre.com>; Nuno Sá <noname.nuno@gmail.com>; Sa,
> Nuno <Nuno.Sa@analog.com>; Andy Shevchenko <andy@kernel.org>; linux-
> iio@vger.kernel.org; linux-kernel@vger.kernel.org; devicetree@vger.kernel.org
> Subject: Re: [PATCH v5 3/3] iio: dac: ad3530r: Add driver for AD3530R and
> AD3531R
> 
> [External]
> 
> On Mon, 21 Apr 2025 12:24:54 +0800
> Kim Seer Paller <kimseer.paller@analog.com> wrote:
> 
> > The AD3530/AD3530R (8-channel) and AD3531/AD3531R (4-channel) are
> > low-power, 16-bit, buffered voltage output DACs with software-
> > programmable gain controls, providing full-scale output spans of 2.5V or
> > 5V for reference voltages of 2.5V. These devices operate from a single
> > 2.7V to 5.5V supply and are guaranteed monotonic by design. The "R"
> > variants include a 2.5V, 5ppm/°C internal reference, which is disabled
> > by default.
> >
> > Support for monitoring internal die temperature, output voltages, and
> > current of a selected channel via the MUXOUT pin using an external ADC
> > is currently not implemented.
> >
> > Reviewed-by: David Lechner <dlechner@baylibre.com>
> > Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
> Hi.
> 
> Just one thing from a final pre merge look through.
> 
> The initialization of powerdown mode works but only because the NORMAL
> mode == 0.  That should be setting it explicitly for each set of 4 channels
> as needed.
> 
> I don't really mind how you solve that.  There are lots of options
> to build up the 4 fields in each of those registers.
> 
> Jonathan
> 
> > diff --git a/drivers/iio/dac/ad3530r.c b/drivers/iio/dac/ad3530r.c
> > new file mode 100644
> > index
> 0000000000000000000000000000000000000000..05bd191e5225bd267f42ba
> 36bbd42a18e6f22291
> > --- /dev/null
> > +++ b/drivers/iio/dac/ad3530r.c
> > @@ -0,0 +1,503 @@
> 
> > +
> > +static ssize_t ad3530r_set_dac_powerdown(struct iio_dev *indio_dev,
> > +					 uintptr_t private,
> > +					 const struct iio_chan_spec *chan,
> > +					 const char *buf, size_t len)
> > +{
> > +	struct ad3530r_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +	unsigned int mask, val, reg;
> > +	bool powerdown;
> > +
> > +	ret = kstrtobool(buf, &powerdown);
> > +	if (ret)
> > +		return ret;
> > +
> > +	guard(mutex)(&st->lock);
> > +	mask = GENMASK(chan->address + 1, chan->address);
> 
> I think maybe we need a macro to get the mask from the channel number?
> Using address for this seems overkill given how simple that maths is.
> Ideally that macro could perhaps be used in the code below to avoid
> all the defines I suggested.

The motivation for using the chan->address field was to hide the calculation a bit.
However, would using a macro like 
#define AD3530R_OP_MODE_CHAN_MSK(chan)	GENMASK(2 * chan + 1, 2 * chan) 
be a good approach in this case? This drops the need for the address field and
can also be used to explicitly set the operating mode for the 4 fields of the register.
What do you think?

> 
> 
> > +	reg = chan->channel < AD3531R_MAX_CHANNELS ?
> > +	      AD3530R_OUTPUT_OPERATING_MODE_0 :
> > +	      AD3530R_OUTPUT_OPERATING_MODE_1;
> > +	val = (powerdown ? st->chan[chan->channel].powerdown_mode : 0)
> > +	       << chan->address;
> > +
> 
> 
> > +static int ad3530r_setup(struct ad3530r_state *st, int vref,
> > +			 bool has_external_vref)
> > +{
> 
> > +
> > +	if (has_external_vref)
> > +		st->vref_mv = range_multiplier * vref / 1000;
> > +
> > +	/* Set operating mode to normal operation. */
> > +	ret = regmap_write(st->regmap,
> AD3530R_OUTPUT_OPERATING_MODE_0,
> > +			   AD3530R_NORMAL_OPERATION);
> Is this actually doing that?  I think the register is lots of 2 bit
> fields and this is only setting it for the first channel?
> 
> This works because that value is 0.  Logically however we should set it
> to
> 	(AD3530R_NORMAL_OPERATION << 6) |
> 	(AD3530R_NORMAL_OPERATION << 4) |
> 	(AD3530R_NORMAL_OPERATION << 2) |
> 	(AD3530R_NORMAL_OPERATION << 0)
> 
> Or possibly better as
> 
> 	FIELD_PREP(AD3530R_OP_MODE_0_CHAN0_MSK,
> AD3530R_NORMAL_OPERATION) |
> 	FIELD_PREP(AD3530R_OP_MODE_0_CHAN1_MSK,
> AD3530R_NORMAL_OPERATION) |
> 

Considering the above macro, would this also suffice?

	val = FIELD_PREP(AD3530R_OP_MODE_CHAN_MSK(0), AD3530R_NORMAL_OP) |
	      FIELD_PREP(AD3530R_OP_MODE_CHAN_MSK(1), AD3530R_NORMAL_OP) |
	      FIELD_PREP(AD3530R_OP_MODE_CHAN_MSK(2), AD3530R_NORMAL_OP) |
	      FIELD_PREP(AD3530R_OP_MODE_CHAN_MSK(3), AD3530R_NORMAL_OP);

	ret = regmap_write(st->regmap, AD3530R_OUTPUT_OPERATING_MODE_0, val);
	...

> etc
> 
> Names are a bit long, so maybe consider shortening some of the defines.
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (st->chip_info->num_channels > AD3531R_MAX_CHANNELS) {
> 
> If we have it explicit that we have multiple fields this will become more
> obvious.
> However I'd use the number 4 here rather than the number of channels the
> AD3531R happens
> to have.
> 
> 
> > +		ret = regmap_write(st->regmap,
> AD3530R_OUTPUT_OPERATING_MODE_1,
> > +				   AD3530R_NORMAL_OPERATION);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	for (i = 0; i < st->chip_info->num_channels; i++)
> > +		st->chan[i].powerdown_mode = AD3530R_POWERDOWN_32K;
> > +
> > +	st->ldac_gpio = devm_gpiod_get_optional(dev, "ldac",
> GPIOD_OUT_LOW);
> > +	if (IS_ERR(st->ldac_gpio))
> > +		return dev_err_probe(dev, PTR_ERR(st->ldac_gpio),
> > +				     "Failed to get ldac GPIO\n");
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct regmap_config ad3530r_regmap_config = {
> > +	.reg_bits = 16,
> > +	.val_bits = 8,
> > +	.max_register = AD3530R_MAX_REG_ADDR,
> > +};

RE: [PATCH v5 3/3] iio: dac: ad3530r: Add driver for AD3530R and AD3531R

[Paller, Kim Seer]

> -----Original Message-----
> From: Andy Shevchenko <andy@kernel.org>
> Sent: Tuesday, April 22, 2025 11:11 PM
> To: Paller, Kim Seer <KimSeer.Paller@analog.com>
> Cc: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen
> <lars@metafoo.de>; Hennerich, Michael <Michael.Hennerich@analog.com>;
> Rob Herring <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>;
> Conor Dooley <conor+dt@kernel.org>; David Lechner
> <dlechner@baylibre.com>; Nuno Sá <noname.nuno@gmail.com>; Sa, Nuno
> <Nuno.Sa@analog.com>; linux-iio@vger.kernel.org; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org
> Subject: Re: [PATCH v5 3/3] iio: dac: ad3530r: Add driver for AD3530R and
> AD3531R
> 
> [External]
> 
> On Mon, Apr 21, 2025 at 12:24:54PM +0800, Kim Seer Paller wrote:
> > The AD3530/AD3530R (8-channel) and AD3531/AD3531R (4-channel) are
> > low-power, 16-bit, buffered voltage output DACs with software-
> > programmable gain controls, providing full-scale output spans of 2.5V
> > or 5V for reference voltages of 2.5V. These devices operate from a
> > single 2.7V to 5.5V supply and are guaranteed monotonic by design. The "R"
> > variants include a 2.5V, 5ppm/°C internal reference, which is disabled
> > by default.
> >
> > Support for monitoring internal die temperature, output voltages, and
> > current of a selected channel via the MUXOUT pin using an external ADC
> > is currently not implemented.
> 
> ...
> 
> > +#include <linux/bitfield.h>
> > +#include <linux/bits.h>
> > +#include <linux/cleanup.h>
> > +#include <linux/delay.h>
> 
> > +#include <linux/device.h>
> 
> I don't see how you use this. But
> 
> + dev_printk.h
> 
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/module.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/mutex.h>
> > +#include <linux/property.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h> #include <linux/spi/spi.h>
> 
> ...
> 
> > +#define AD3530R_SW_RESET			(BIT(7) | BIT(0))
> 
> > +#define AD3530R_MAX_CHANNELS			8
> > +#define AD3531R_MAX_CHANNELS			4
> 
> Sounds to me that these two shouldn't be grouped with the rest here. Perhaps
> move them out to after the LDAC_PULSE?
> 
> > +#define AD3530R_INTERNAL_VREF_MV		2500
> 
> _mV (yes, with Volts and Amperes we use proper spelling).
> 
> > +#define AD3530R_LDAC_PULSE_US			100
> 
> ...
> 
> > +	int (*input_ch_reg)(unsigned int c);
> 
> c? channel?

Thank you for the feedback. I'll change this to 'channel' to make it clearer.

> 
> ...
> 
> > +	int vref_mv;
> 
> _mV
> 
> ...
> 
> > +static int ad3530r_input_ch_reg(unsigned int c) {
> > +	return 2 * c + AD3530R_INPUT_CH;
> > +}
> > +
> > +static int ad3531r_input_ch_reg(unsigned int c) {
> > +	return 2 * c + AD3531R_INPUT_CH;
> > +}
> 
> c --> channel
> 
> ...
> 
> > +static const char * const ad3530r_powerdown_modes[] = {
> > +	"1kohm_to_gnd",
> 
> kOhm
> 
> > +	"7.7kohm_to_gnd",
> 
> Ditto.
> 
> > +	"32kohm_to_gnd",
> 
> Ditto.
> 
> > +};
> 
> ...
> 
> > +static const struct iio_enum ad3530r_powerdown_mode_enum = {
> > +	.items = ad3530r_powerdown_modes,
> > +	.num_items = ARRAY_SIZE(ad3530r_powerdown_modes),
> 
> + array_size.h
> 
> > +	.get = ad3530r_get_powerdown_mode,
> > +	.set = ad3530r_set_powerdown_mode,
> > +};
> 
> ...
> 
> > +static ssize_t ad3530r_get_dac_powerdown(struct iio_dev *indio_dev,
> > +					 uintptr_t private,
> > +					 const struct iio_chan_spec *chan,
> > +					 char *buf)
> > +{
> > +	struct ad3530r_state *st = iio_priv(indio_dev);
> > +
> > +	guard(mutex)(&st->lock);
> > +	return sysfs_emit(buf, "%d\n", st->chan[chan->channel].powerdown);
> 
> + sysfs.h
> 
> > +}
> 
> ...
> 
> > +static ssize_t ad3530r_set_dac_powerdown(struct iio_dev *indio_dev,
> > +					 uintptr_t private,
> > +					 const struct iio_chan_spec *chan,
> > +					 const char *buf, size_t len)
> > +{
> > +	struct ad3530r_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +	unsigned int mask, val, reg;
> > +	bool powerdown;
> > +
> > +	ret = kstrtobool(buf, &powerdown);
> 
> + kstrtox.h
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	guard(mutex)(&st->lock);
> > +	mask = GENMASK(chan->address + 1, chan->address);
> > +	reg = chan->channel < AD3531R_MAX_CHANNELS ?
> > +	      AD3530R_OUTPUT_OPERATING_MODE_0 :
> > +	      AD3530R_OUTPUT_OPERATING_MODE_1;
> > +	val = (powerdown ? st->chan[chan->channel].powerdown_mode : 0)
> > +	       << chan->address;
> 
> Please, move the operator to the previous line, Or even
> 
> 	... pdmode;
> 
> 	pdmode = powerdown ? st->chan[chan->channel].powerdown_mode :
> 0;
> 	val = pdmode << ...;
> 
> > +	ret = regmap_update_bits(st->regmap, reg, mask, val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	st->chan[chan->channel].powerdown = powerdown;
> > +
> > +	return len;
> > +}
> 
> ...
> 
> > +	struct ad3530r_state *st = iio_priv(indio_dev);
> > +
> > +	switch (info) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		if (val < 0 || val > U16_MAX)
> 
> U16_MAX is an abstract type with this limit, do you have any predefined HW
> limit instead? Probably better to use that one as defined via BIT() / GENMASK().

Will add a macro for this hardware limit.

> 
> > +			return -EINVAL;
> > +
> > +		return ad3530r_dac_write(st, chan->channel, val);
> > +	default:
> > +		return -EINVAL;
> > +	}
> 
> ...
> 
> > +static const struct iio_chan_spec_ext_info ad3530r_ext_info[] = {
> > +	{
> > +		.name = "powerdown",
> > +		.shared = IIO_SEPARATE,
> > +		.read = ad3530r_get_dac_powerdown,
> > +		.write = ad3530r_set_dac_powerdown,
> > +	},
> > +	IIO_ENUM("powerdown_mode", IIO_SEPARATE,
> &ad3530r_powerdown_mode_enum),
> > +	IIO_ENUM_AVAILABLE("powerdown_mode", IIO_SHARED_BY_TYPE,
> > +			   &ad3530r_powerdown_mode_enum),
> > +	{ }
> > +};
> > +
> > +#define AD3530R_CHAN(_chan, _pos) {
> 	\
> 
> Slightly better to have the curly braces on the same column as it's easier to
> read.
> 
> #define AD3530R_CHAN(_chan, _pos)				\
> {								\
> 
> (and make it one TAB less for the backslash).
> 
> > +	.type = IIO_VOLTAGE,						\
> > +	.indexed = 1,							\
> > +	.channel = _chan,						\
> > +	.address = _pos,						\
> > +	.output = 1,							\
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> 	\
> > +			      BIT(IIO_CHAN_INFO_SCALE),			\
> > +	.ext_info = ad3530r_ext_info,					\
> > +}
> 
> ...
> 
> > +static int ad3530r_setup(struct ad3530r_state *st, int vref,
> > +			 bool has_external_vref)
> > +{
> > +	struct device *dev = regmap_get_device(st->regmap);
> > +	struct gpio_desc *reset_gpio;
> > +	int i, ret;
> > +	u8 range_multiplier;
> 
> + types.h (and especially for boolean type along with true/false definitions.
> 
> > +
> > +	reset_gpio = devm_gpiod_get_optional(dev, "reset",
> GPIOD_OUT_HIGH);
> > +	if (IS_ERR(reset_gpio))
> > +		return dev_err_probe(dev, PTR_ERR(reset_gpio),
> > +				     "Failed to get reset GPIO\n");
> 
> + err.h
> 
> > +	if (reset_gpio) {
> > +		/* Perform hardware reset */
> > +		fsleep(1000);
> 
> (1 * USEC_PER_MSEC) ?
> 
> > +		gpiod_set_value_cansleep(reset_gpio, 0);
> > +	} else {
> > +		/* Perform software reset */
> > +		ret = regmap_update_bits(st->regmap,
> AD3530R_INTERFACE_CONFIG_A,
> > +					 AD3530R_SW_RESET,
> AD3530R_SW_RESET);
> > +		if (ret)
> > +			return ret;
> > +	}
> 
> > +	fsleep(10000);
> 
> 10 * USEC_PER_MSEC
> 
> With these constants it's less error prone (when 3 or more zeroes) and easier to
> get the units without looking into fsleep() implementation / documentation.

I'll take note of this.

> 
> > +	range_multiplier = 1;
> > +	if (device_property_read_bool(dev, "adi,range-double")) {
> > +		ret = regmap_set_bits(st->regmap,
> AD3530R_OUTPUT_CONTROL_0,
> > +				      AD3530R_OUTPUT_CONTROL_RANGE);
> > +		if (ret)
> > +			return ret;
> > +
> > +		range_multiplier = 2;
> > +	}
> > +
> > +	if (!has_external_vref && st->chip_info->internal_ref_support) {
> > +		ret = regmap_set_bits(st->regmap,
> AD3530R_REFERENCE_CONTROL_0,
> > +				      AD3530R_REFERENCE_CONTROL_SEL);
> > +		if (ret)
> > +			return ret;
> > +
> > +		st->vref_mv = range_multiplier *
> AD3530R_INTERNAL_VREF_MV;
> > +	}
> > +
> > +	if (has_external_vref)
> > +		st->vref_mv = range_multiplier * vref / 1000;
> 
> MILLI?

Yes this is milli, will change this also to vref_mV

	st->vref_mV = range_multiplier * vref_mV / 1000;

> 
> 
> > +	/* Set operating mode to normal operation. */
> > +	ret = regmap_write(st->regmap,
> AD3530R_OUTPUT_OPERATING_MODE_0,
> > +			   AD3530R_NORMAL_OPERATION);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (st->chip_info->num_channels > AD3531R_MAX_CHANNELS) {
> > +		ret = regmap_write(st->regmap,
> AD3530R_OUTPUT_OPERATING_MODE_1,
> > +				   AD3530R_NORMAL_OPERATION);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	for (i = 0; i < st->chip_info->num_channels; i++)
> > +		st->chan[i].powerdown_mode = AD3530R_POWERDOWN_32K;
> > +
> > +	st->ldac_gpio = devm_gpiod_get_optional(dev, "ldac",
> GPIOD_OUT_LOW);
> > +	if (IS_ERR(st->ldac_gpio))
> > +		return dev_err_probe(dev, PTR_ERR(st->ldac_gpio),
> > +				     "Failed to get ldac GPIO\n");
> > +
> > +	return 0;
> > +}
> 
> ...
> 
> > +	vref = devm_regulator_get_enable_read_voltage(dev, "ref");
> > +	if (vref < 0 && vref != -ENODEV)
> > +		return vref;
> > +
> > +	has_external_vref = vref != -ENODEV;
> 
> Wouldn't be better just make this 0 when it's == -ENODEV and check just the
> value without having this additional boolean variable (note, I haven't checked
> the meaning of Vref == 0 in case it's possible in real life and hardware behaves
> adequately)?

I think it could be simpler to set vref to 0 when it's -ENODEV and check its value directly
without having additional boolean variable. I'll try this approach.

> 
> > +	if (!st->chip_info->internal_ref_support && !has_external_vref)
> > +		return -ENODEV;
> 
> > +	ret = ad3530r_setup(st, vref, has_external_vref);
> > +	if (ret)
> > +		return ret;
> 
> --
> With Best Regards,
> Andy Shevchenko
> 

Re: [PATCH v5 3/3] iio: dac: ad3530r: Add driver for AD3530R and AD3531R

[Andy Shevchenko]

On Wed, Apr 23, 2025 at 07:53:37AM +0000, Paller, Kim Seer wrote:
> > From: Andy Shevchenko <andy@kernel.org>
> > Sent: Tuesday, April 22, 2025 11:11 PM
> > On Mon, Apr 21, 2025 at 12:24:54PM +0800, Kim Seer Paller wrote:

First of all, there are a lot of comments left without replies while some of
them commented as "agree I will follow your advice". This is confusing. The
rule of thumb is to not reply with positive at all, just only for the things
that you want to clarify. And with that remove a lot of unneeded (you agree
with) context!

...

> > > +		st->vref_mv = range_multiplier * vref / 1000;
> > 
> > MILLI?
> 
> Yes this is milli, will change this also to vref_mV
> 
> 	st->vref_mV = range_multiplier * vref_mV / 1000;

Ah, I was not clear enough, MILLI in capital letters is defined constant which
you may use instead of 1000.

...

> > > +	vref = devm_regulator_get_enable_read_voltage(dev, "ref");
> > > +	if (vref < 0 && vref != -ENODEV)
> > > +		return vref;
> > > +
> > > +	has_external_vref = vref != -ENODEV;
> > 
> > Wouldn't be better just make this 0 when it's == -ENODEV and check just the
> > value without having this additional boolean variable (note, I haven't checked
> > the meaning of Vref == 0 in case it's possible in real life and hardware behaves
> > adequately)?
> 
> I think it could be simpler to set vref to 0 when it's -ENODEV and check its value directly
> without having additional boolean variable. I'll try this approach.

But double check that hardware doesn't support Vref == 0 in real life.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 3/3] iio: dac: ad3530r: Add driver for AD3530R and AD3531R

[Andy Shevchenko]

On Wed, Apr 23, 2025 at 07:50:51AM +0000, Paller, Kim Seer wrote:
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Monday, April 21, 2025 9:48 PM
> > To: Paller, Kim Seer <KimSeer.Paller@analog.com>
> > On Mon, 21 Apr 2025 12:24:54 +0800
> > Kim Seer Paller <kimseer.paller@analog.com> wrote:

...

> > > +	mask = GENMASK(chan->address + 1, chan->address);
> > 
> > I think maybe we need a macro to get the mask from the channel number?
> > Using address for this seems overkill given how simple that maths is.
> > Ideally that macro could perhaps be used in the code below to avoid
> > all the defines I suggested.
> 
> The motivation for using the chan->address field was to hide the calculation a bit.
> However, would using a macro like 
> #define AD3530R_OP_MODE_CHAN_MSK(chan)	GENMASK(2 * chan + 1, 2 * chan) 
> be a good approach in this case? This drops the need for the address field and
> can also be used to explicitly set the operating mode for the 4 fields of the register.
> What do you think?

Please, note that doing GENMASK(foo + X, foo) is highly discouraged as it may
give a very bad generated code (although I haven't checked recently if it's
still the case). The preferred way is GENMASK(X, 0) << foo. Where X is a
compile time constant.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v5 3/3] iio: dac: ad3530r: Add driver for AD3530R and AD3531R

[Paller, Kim Seer]

> -----Original Message-----
> From: Andy Shevchenko <andy@kernel.org>
> Sent: Thursday, April 24, 2025 12:50 AM
> To: Paller, Kim Seer <KimSeer.Paller@analog.com>
> Cc: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen
> <lars@metafoo.de>; Hennerich, Michael <Michael.Hennerich@analog.com>;
> Rob Herring <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>;
> Conor Dooley <conor+dt@kernel.org>; David Lechner
> <dlechner@baylibre.com>; Nuno Sá <noname.nuno@gmail.com>; Sa, Nuno
> <Nuno.Sa@analog.com>; linux-iio@vger.kernel.org; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org
> Subject: Re: [PATCH v5 3/3] iio: dac: ad3530r: Add driver for AD3530R and
> AD3531R
> 
> [External]
> 
> On Wed, Apr 23, 2025 at 07:53:37AM +0000, Paller, Kim Seer wrote:
> > > From: Andy Shevchenko <andy@kernel.org>
> > > Sent: Tuesday, April 22, 2025 11:11 PM
> > > On Mon, Apr 21, 2025 at 12:24:54PM +0800, Kim Seer Paller wrote:
> 
> First of all, there are a lot of comments left without replies while some of
> them commented as "agree I will follow your advice". This is confusing. The
> rule of thumb is to not reply with positive at all, just only for the things
> that you want to clarify. And with that remove a lot of unneeded (you agree
> with) context!
> 
> ...
> 
> > > > +		st->vref_mv = range_multiplier * vref / 1000;
> > >
> > > MILLI?
> >
> > Yes this is milli, will change this also to vref_mV
> >
> > 	st->vref_mV = range_multiplier * vref_mV / 1000;
> 
> Ah, I was not clear enough, MILLI in capital letters is defined constant which
> you may use instead of 1000.
> 
> ...
> 
> > > > +	vref = devm_regulator_get_enable_read_voltage(dev, "ref");
> > > > +	if (vref < 0 && vref != -ENODEV)
> > > > +		return vref;
> > > > +
> > > > +	has_external_vref = vref != -ENODEV;
> > >
> > > Wouldn't be better just make this 0 when it's == -ENODEV and check just
> the
> > > value without having this additional boolean variable (note, I haven't
> checked
> > > the meaning of Vref == 0 in case it's possible in real life and hardware
> behaves
> > > adequately)?
> >
> > I think it could be simpler to set vref to 0 when it's -ENODEV and check its
> value directly
> > without having additional boolean variable. I'll try this approach.
> 
> But double check that hardware doesn't support Vref == 0 in real life.

I confirm that the hardware doesn't support Vref == 0, as also stated in the datasheet.
 
> 
> --
> With Best Regards,
> Andy Shevchenko
> 

Re: [PATCH v5 3/3] iio: dac: ad3530r: Add driver for AD3530R and AD3531R

[Jonathan Cameron]

On Wed, 23 Apr 2025 19:52:13 +0300
Andy Shevchenko <andy@kernel.org> wrote:

> On Wed, Apr 23, 2025 at 07:50:51AM +0000, Paller, Kim Seer wrote:
> > > From: Jonathan Cameron <jic23@kernel.org>
> > > Sent: Monday, April 21, 2025 9:48 PM
> > > To: Paller, Kim Seer <KimSeer.Paller@analog.com>
> > > On Mon, 21 Apr 2025 12:24:54 +0800
> > > Kim Seer Paller <kimseer.paller@analog.com> wrote:  
> 
> ...
> 
> > > > +	mask = GENMASK(chan->address + 1, chan->address);  
> > > 
> > > I think maybe we need a macro to get the mask from the channel number?
> > > Using address for this seems overkill given how simple that maths is.
> > > Ideally that macro could perhaps be used in the code below to avoid
> > > all the defines I suggested.  
> > 
> > The motivation for using the chan->address field was to hide the calculation a bit.
> > However, would using a macro like 
> > #define AD3530R_OP_MODE_CHAN_MSK(chan)	GENMASK(2 * chan + 1, 2 * chan) 
> > be a good approach in this case? This drops the need for the address field and
> > can also be used to explicitly set the operating mode for the 4 fields of the register.
> > What do you think?  
> 
> Please, note that doing GENMASK(foo + X, foo) is highly discouraged as it may
> give a very bad generated code (although I haven't checked recently if it's
> still the case). The preferred way is GENMASK(X, 0) << foo. Where X is a
> compile time constant.
> 

With what Andy suggested as the implementation, this sort of macro
looks like a good solution to me.

Jonathan