Devicetree
 help / color / mirror / Atom feed
* [PATCH v5 0/3] iio: dac: Add support for AD5529R DAC
@ 2026-07-01  6:40 Janani Sunil
  2026-07-01  6:40 ` [PATCH v5 1/3] dt-bindings: spi: Add spi,device-addr peripheral property Janani Sunil
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Janani Sunil @ 2026-07-01  6:40 UTC (permalink / raw)
  To: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Philipp Zabel, Jonathan Corbet,
	Shuah Khan, Mark Brown
  Cc: linux-iio, devicetree, linux-kernel, linux-doc, Janani Sunil,
	linux-spi, Janani Sunil

This patch series adds support for Analog Devices AD5529R, a 16 channel
16 and 12 bit voltage Digital-to-Analog Converter (DAC) with integrated
precision reference. The AD5529R operates from both unipolar and
bipolar supplies. The device communicates via SPI interface.

**Device Overview:**
The AD5529R features 16 independent DAC channels, with 16 or 12 bit
resolution, allowing independently programmable output ranges. The
internal 4.096V precision reference sets the accuracy of the output
voltage.

**Features Implemented:**
- Support for AD5529R 12-bit and 16-bit variants via device match data.
- Reset support via GPIO.
- Dual regmap configuration to handle 8 and 16 bit registers.
- Per-channel output range configuration from devicetree.
- Optional external reference and bipolar supply handling.

**Patch Summary:**
1. **dt-bindings**: Binding documentation with channel configuration.
2. **driver**: Implement IIO DAC Driver with regmap support.

**Testing:**
The driver was compiled and tested on the EVAL-AD5529R-ARDZ using a
coraZ7 with a mainline v7.0 kernel.

**Driver Rationale:**
AD5529R introduces:
1. A unique register layout
2. Mixed 8-bit and 16-bit register accesses
3. Hardware specific features like function generators, multi-die
hotpath registers etc.

The device warrants its own drivers due to these fundamental
architectural differences, that would require substantial changes to
existing drivers without providing reusable benefits. The standalone
driver also allows future extensions for related devices in the same
family.

**Not Implemented in this Series:**
The binding includes generic, per-channel device addressing needed for
multi-device support using a shared CS, but the driver presently
supports only a single device.

Signed-off-by: Janani Sunil <janani.sunil@analog.com>
---
Changes in v5:
- Move register bitfield definitions next to their parent register
  addresses.
- Remove spurious extra indent.
- Rename ad5529r_output_ranges_mv[] to ad5529r_output_ranges_mV[].
- Remove extra parentheses in regmap_reg_range() for the readback range.
- Use reset_control_reset() instead of reset_control_deassert().
- Use 10 * USEC_PER_MSEC instead of a bare 10000 in fsleep().
- Use fwnode_property_present() to explicitly guard the optional property.
- Rewrite external_vref detection using explicit if/else.
- Follow reverse christmas tree variable declaration order.
- Improve invalid channel error message to include the maximum.
- Add a new spi property to include the SPI device address.
- Update ad5529r devicetree binding to allow more than 16 channels to include multiple DACs.
- Update cover letter to add a mention about the multi device support.
- Update driver commit message to describe the device further.
- Link to v4: https://lore.kernel.org/r/20260609-ad5529r-driver-v4-0-2e4c02234a1a@analog.com

Changes in v4:
- Fix DT child-node regex for hexadecimal channel addresses.
- Wrap long DT binding description lines.
- Simplify optional `vref-supply` and `hvss-supply` handling.
- Update REF_SEL programming for optional external reference use.
- Clean up range parsing and error messages.
- Simplify debugfs register access by calling regmap helpers directly.
- Add clarifying comments for reset settling time and RAW reads from `DAC_INPUT_A`.
- Remove an unused vref regulator pointer and an include.
- Rename the REF_SEL bit define and clean up small driver details.
- Toggle pins defined as PWM pins, instead of GPIOs
- Update cover letter to sync up latest changes.
- Link to v3: https://lore.kernel.org/r/20260519-ad5529r-driver-v3-0-267c0731aa68@analog.com

Changes in v3:
- Split into adi,ad5529r-16 and adi,ad5529r-12 device tree compatibles
- Add DT-based output range configuration via adi,output-range-microvolt
- Expand DT binding: vref-supply, clear/tg GPIOs, interrupts, muxout
- Correct power supply voltage specifications as per datasheet
- Reduce SPI frequency limit to 25MHz as per datasheet specs
- Switch to autoincrement addressing mode, remove +1 register offsets
- Use DT match data instead of device ID detection for fallback support
- Implement dynamic scale/offset calculation per configured channel range
- Added explicit val_format_endian and reg_stride for 16-bit regmap bus
- Code cleanup: alphabetical includes, ARRAY_SIZE(), unused defines
- Minor: .sign→.format field, simplify read/write order, optional hvss-supply
- Remove redundant driver documentation ad5529r.rst
- Link to v2: https://lore.kernel.org/r/20260508-ad5529r-driver-v2-0-e315441685d7@analog.com

Changes in v2:
- Fix IIO scale to use millivolts per ABI requirement
- Fix documentation voltage calculations (2.5V not 2.048V)
- Fix bipolar ranges in documentation (±5V, ±10V, ±15V, ±20V)
- Fix alphabetical ordering in documentation index
- Add missing newline to documentation file
- Fix scale units description (millivolts not microvolts)
- Include a section for driver rationale in the cover letter
- Reword contents in cover letter 12/16 bit generic->variant
- Add dependency array for spi-cpha and spi-cpol properties
- Link to v1: https://lore.kernel.org/r/20260507-ad5529r-driver-v1-0-b4460f3cb44f@analog.com

---
Janani Sunil (3):
      dt-bindings: spi: Add spi,device-addr peripheral property
      dt-bindings: iio: dac: Add AD5529R
      iio: dac: Add AD5529R DAC driver support

 .../devicetree/bindings/iio/dac/adi,ad5529r.yaml   | 216 +++++++++
 .../bindings/spi/spi-peripheral-props.yaml         |   5 +
 MAINTAINERS                                        |   8 +
 drivers/iio/dac/Kconfig                            |  17 +
 drivers/iio/dac/Makefile                           |   1 +
 drivers/iio/dac/ad5529r.c                          | 531 +++++++++++++++++++++
 6 files changed, 778 insertions(+)
---
base-commit: 93df88612859e8e19dec93c69d563b4b73e9bd4b
change-id: 20260507-ad5529r-driver-866bbdd864de

Best regards,
-- 
Janani Sunil <janani.sunil@analog.com>


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v5 1/3] dt-bindings: spi: Add spi,device-addr peripheral property
  2026-07-01  6:40 [PATCH v5 0/3] iio: dac: Add support for AD5529R DAC Janani Sunil
@ 2026-07-01  6:40 ` Janani Sunil
  2026-07-01  6:52   ` sashiko-bot
  2026-07-01 11:04   ` Conor Dooley
  2026-07-01  6:40 ` [PATCH v5 2/3] dt-bindings: iio: dac: Add AD5529R Janani Sunil
  2026-07-01  6:40 ` [PATCH v5 3/3] iio: dac: Add AD5529R DAC driver support Janani Sunil
  2 siblings, 2 replies; 15+ messages in thread
From: Janani Sunil @ 2026-07-01  6:40 UTC (permalink / raw)
  To: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Philipp Zabel, Jonathan Corbet,
	Shuah Khan, Mark Brown
  Cc: linux-iio, devicetree, linux-kernel, linux-doc, Janani Sunil,
	linux-spi, Janani Sunil

Some SPI devices support sharing a single chip select across multiple
physical chips by encoding a device address in the SPI frame itself.
Add a generic spi,device-addr property to document this per-peripheral
address. This property belongs in channel or sub-device nodes of
peripherals that use this addressing scheme.

Signed-off-by: Janani Sunil <janani.sunil@analog.com>
---
 Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
index 880a9f624566..3774e8018355 100644
--- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
@@ -142,6 +142,11 @@ properties:
     minItems: 2
     maxItems: 4
 
+  spi,device-addr:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Device address used when multiple peripherals share a single chip select.
+
   st,spi-midi-ns:
     deprecated: true
     description: |

-- 
2.43.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v5 2/3] dt-bindings: iio: dac: Add AD5529R
  2026-07-01  6:40 [PATCH v5 0/3] iio: dac: Add support for AD5529R DAC Janani Sunil
  2026-07-01  6:40 ` [PATCH v5 1/3] dt-bindings: spi: Add spi,device-addr peripheral property Janani Sunil
@ 2026-07-01  6:40 ` Janani Sunil
  2026-07-01  6:52   ` sashiko-bot
                     ` (2 more replies)
  2026-07-01  6:40 ` [PATCH v5 3/3] iio: dac: Add AD5529R DAC driver support Janani Sunil
  2 siblings, 3 replies; 15+ messages in thread
From: Janani Sunil @ 2026-07-01  6:40 UTC (permalink / raw)
  To: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Philipp Zabel, Jonathan Corbet,
	Shuah Khan, Mark Brown
  Cc: linux-iio, devicetree, linux-kernel, linux-doc, Janani Sunil,
	linux-spi, Janani Sunil

Devicetree bindings for AD5529R 16 channel 12/16 bit high voltage,
buffered voltage output digital-to-analog converter (DAC) with an
integrated precision reference.

Signed-off-by: Janani Sunil <janani.sunil@analog.com>
---
 .../devicetree/bindings/iio/dac/adi,ad5529r.yaml   | 216 +++++++++++++++++++++
 MAINTAINERS                                        |   7 +
 2 files changed, 223 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad5529r.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad5529r.yaml
new file mode 100644
index 000000000000..97075b1c919d
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/dac/adi,ad5529r.yaml
@@ -0,0 +1,216 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/dac/adi,ad5529r.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD5529R 16-Channel 12/16-bit High Voltage DAC
+
+maintainers:
+  - Janani Sunil <janani.sunil@analog.com>
+
+description: |
+  The AD5529R is a 16-channel, 12-bit or 16-bit, high voltage, buffered voltage
+  output digital-to-analog converter (DAC) with an integrated precision reference.
+  The device operates from unipolar and bipolar supplies. It is guaranteed
+  monotonic and has built-in rail-to-rail output buffers that can source or
+  sink up to 25mA.
+
+  Specifications:
+  * 16 independent 12-bit or 16-bit DAC channels
+  * Independently programmable output ranges: 0V to 5V, 0V to 10V, 0V to 20V,
+    0V to 40V, ±5V, ±10V, ±15V, and ±20V
+  * The device supports SPI communication with Mode 0 and Mode 3.
+  * 4.096V precision reference, 12ppm/°C maximum
+  * Built-in function generation: Toggle, Sinusoidal Dither, and Ramp waveforms
+  * Multiplexer for output voltage, load current sense and die temperature
+
+  Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad5529r.pdf
+
+properties:
+  compatible:
+    enum:
+      - adi,ad5529r-16   # 16-bit variant
+      - adi,ad5529r-12   # 12-bit variant
+
+  reg:
+    maxItems: 1
+
+  spi-max-frequency:
+    maximum: 25000000
+    description:
+      Maximum SPI frequency. The device supports SPI Mode 0 and Mode 3.
+      Read operations are limited to 25MHz maximum.
+
+  reset-gpios:
+    maxItems: 1
+    description:
+      GPIO connected to the RESET pin. Active low. When asserted low,
+      performs a power-on reset and initializes the device to its default state.
+
+  clear-gpios:
+    maxItems: 1
+    description:
+      GPIO connected to the CLEAR pin. Active low. When asserted low,
+      clears all DAC data registers without affecting configuration settings.
+
+  interrupts:
+    maxItems: 1
+    description:
+      Interrupt connected to the ALARM pin. Active low interrupt output
+      for overtemperature conditions, SPI CRC errors, and function completion.
+
+  pwms:
+    minItems: 1
+    maxItems: 4
+    description:
+      PWM signals connected to the TG0-TG3 toggle pins. Pulsing these pins
+      based on trigger edge settings allows selected DACs to be updated
+      synchronously for digital function generation.
+
+  pwm-names:
+    minItems: 1
+    maxItems: 4
+    items:
+      enum: [ tg0, tg1, tg2, tg3 ]
+
+  io-channels:
+    maxItems: 1
+    description:
+      ADC channel connected to the MUXOUT pin for monitoring output voltage,
+      load current sense, and die temperature.
+
+  io-channel-names:
+    const: muxout
+
+  vdd-supply:
+    description: Digital power supply (1.08V to 1.98V)
+
+  avdd-supply:
+    description: Analog power supply (4.75V to 5.25V)
+
+  hvdd-supply:
+    description:
+      High voltage positive supply (7V to 45V). Supply voltage should be chosen
+      based on configured output ranges (see datasheet Table 9).
+
+  hvss-supply:
+    description:
+      High voltage negative supply (-22.5V to 0V). Required only when using
+      bipolar output ranges (±5V, ±10V, ±15V, ±20V). Supply voltage should be
+      chosen based on configured output ranges (see datasheet Table 9).
+
+  vref-supply:
+    description:
+      External voltage reference supply (4.056V to 4.136V, typically 4.096V).
+      When specified, the device uses external reference mode and the VREF pin
+      becomes an input. The device uses the internal 4.096V precision reference
+      otherwise.
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+patternProperties:
+  "^channel@([0-9a-f]{1,2})$":
+    type: object
+    description: Child nodes for individual channel configuration
+
+    properties:
+      reg:
+        description: Channel number.
+        minimum: 0
+        maximum: 63
+
+      spi,device-addr:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        enum: [0, 1, 2, 3]
+        description:
+          Device address selected by the ID0 and ID1 pins. Up to four AD5529R
+          devices can share a single SPI chip select; each device responds only
+          to transfers whose address bits [13:12] match its configured address.
+
+      adi,output-range-microvolt:
+        description: |
+          Output voltage range for this channel as [min, max] in microvolts.
+          If not specified, defaults to 0V to 5V range.
+        oneOf:
+          - items:
+              - const: 0
+              - enum: [5000000, 10000000, 20000000, 40000000]
+          - items:
+              - const: -5000000
+              - const: 5000000
+          - items:
+              - const: -10000000
+              - const: 10000000
+          - items:
+              - const: -15000000
+              - const: 15000000
+          - items:
+              - const: -20000000
+              - const: 20000000
+
+    required:
+      - reg
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+  - avdd-supply
+  - hvdd-supply
+
+dependencies:
+  spi-cpha: [ spi-cpol ]
+  spi-cpol: [ spi-cpha ]
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        dac@0 {
+            compatible = "adi,ad5529r-16";
+            reg = <0>;
+            spi-max-frequency = <25000000>;
+
+            vdd-supply = <&vdd_regulator>;
+            avdd-supply = <&avdd_regulator>;
+            hvdd-supply = <&hvdd_regulator>;
+            hvss-supply = <&hvss_regulator>;
+
+            reset-gpios = <&gpio0 87 GPIO_ACTIVE_LOW>;
+
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            channel@0 {
+                reg = <0>;
+                adi,output-range-microvolt = <0 5000000>;
+            };
+
+            channel@1 {
+                reg = <1>;
+                adi,output-range-microvolt = <(-10000000) 10000000>;
+            };
+
+            channel@2 {
+                reg = <2>;
+                adi,output-range-microvolt = <0 40000000>;
+            };
+        };
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index d6c3c7d22403..320e84765ce6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1507,6 +1507,13 @@ W:	https://ez.analog.com/linux-software-drivers
 F:	Documentation/devicetree/bindings/iio/adc/adi,ad4851.yaml
 F:	drivers/iio/adc/ad4851.c
 
+ANALOG DEVICES INC AD5529R DRIVER
+M:	Janani Sunil <janani.sunil@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,ad5529r.yaml
+
 ANALOG DEVICES INC AD5706R DRIVER
 M:	Alexis Czezar Torreno <alexisczezar.torreno@analog.com>
 L:	linux-iio@vger.kernel.org

-- 
2.43.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v5 3/3] iio: dac: Add AD5529R DAC driver support
  2026-07-01  6:40 [PATCH v5 0/3] iio: dac: Add support for AD5529R DAC Janani Sunil
  2026-07-01  6:40 ` [PATCH v5 1/3] dt-bindings: spi: Add spi,device-addr peripheral property Janani Sunil
  2026-07-01  6:40 ` [PATCH v5 2/3] dt-bindings: iio: dac: Add AD5529R Janani Sunil
@ 2026-07-01  6:40 ` Janani Sunil
  2026-07-01  6:52   ` sashiko-bot
                     ` (2 more replies)
  2 siblings, 3 replies; 15+ messages in thread
From: Janani Sunil @ 2026-07-01  6:40 UTC (permalink / raw)
  To: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Philipp Zabel, Jonathan Corbet,
	Shuah Khan, Mark Brown
  Cc: linux-iio, devicetree, linux-kernel, linux-doc, Janani Sunil,
	linux-spi, Janani Sunil

Add support for AD5529R 16-channel, 12/16 bit Digital to Analog Converter
from Analog Devices.

The device communicates over SPI and supports per-channel output range
configuration. An optional external 4.096V reference can be used in
place of the internal reference.

Signed-off-by: Janani Sunil <janani.sunil@analog.com>
---
 MAINTAINERS               |   1 +
 drivers/iio/dac/Kconfig   |  17 ++
 drivers/iio/dac/Makefile  |   1 +
 drivers/iio/dac/ad5529r.c | 531 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 550 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 320e84765ce6..143714e27d51 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1513,6 +1513,7 @@ L:	linux-iio@vger.kernel.org
 S:	Supported
 W:	https://ez.analog.com/linux-software-drivers
 F:	Documentation/devicetree/bindings/iio/dac/adi,ad5529r.yaml
+F:	drivers/iio/dac/ad5529r.c
 
 ANALOG DEVICES INC AD5706R DRIVER
 M:	Alexis Czezar Torreno <alexisczezar.torreno@analog.com>
diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
index 657c68e75542..bb1d59889a2a 100644
--- a/drivers/iio/dac/Kconfig
+++ b/drivers/iio/dac/Kconfig
@@ -134,6 +134,23 @@ config AD5449
 	  To compile this driver as a module, choose M here: the
 	  module will be called ad5449.
 
+config AD5529R
+	tristate "Analog Devices AD5529R High Voltage DAC driver"
+	depends on SPI_MASTER
+	select REGMAP_SPI
+	help
+	  Say yes here to build support for Analog Devices AD5529R
+	  16-Channel, 12-Bit/16-Bit, 40V High Voltage Precision Digital to Analog
+	  Converter.
+
+	  The device features multiple output voltage ranges from -20V to +20V,
+	  built-in 4.096V voltage reference, and digital functions including
+	  toggle, dither, and ramp modes. Supports both 12-bit and 16-bit
+	  resolution variants.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ad5529r.
+
 config AD5592R_BASE
 	tristate
 
diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
index 003431798498..f35e060b3643 100644
--- a/drivers/iio/dac/Makefile
+++ b/drivers/iio/dac/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_AD5446) += ad5446.o
 obj-$(CONFIG_AD5446_SPI) += ad5446-spi.o
 obj-$(CONFIG_AD5446_I2C) += ad5446-i2c.o
 obj-$(CONFIG_AD5449) += ad5449.o
+obj-$(CONFIG_AD5529R) += ad5529r.o
 obj-$(CONFIG_AD5592R_BASE) += ad5592r-base.o
 obj-$(CONFIG_AD5592R) += ad5592r.o
 obj-$(CONFIG_AD5593R) += ad5593r.o
diff --git a/drivers/iio/dac/ad5529r.c b/drivers/iio/dac/ad5529r.c
new file mode 100644
index 000000000000..4841bd608482
--- /dev/null
+++ b/drivers/iio/dac/ad5529r.c
@@ -0,0 +1,531 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * AD5529R Digital-to-Analog Converter Driver
+ * 16-Channel, 12/16-Bit, 40V High Voltage Precision DAC
+ *
+ * Copyright 2026 Analog Devices Inc.
+ * Author: Janani Sunil <janani.sunil@analog.com>
+ */
+
+#include <linux/array_size.h>
+#include <linux/bits.h>
+#include <linux/delay.h>
+#include <linux/dev_printk.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/iio/iio.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/reset.h>
+#include <linux/spi/spi.h>
+
+#define AD5529R_REG_INTERFACE_CONFIG_A		0x00
+#define   AD5529R_INTERFACE_CONFIG_A_SW_RESET	(BIT(7) | BIT(0))
+#define   AD5529R_INTERFACE_CONFIG_A_ADDR_ASCENSION	BIT(5)
+#define   AD5529R_INTERFACE_CONFIG_A_SDO_ENABLE	BIT(4)
+#define AD5529R_REG_DEVICE_CONFIG		0x02
+#define AD5529R_REG_CHIP_GRADE			0x06
+#define AD5529R_REG_SCRATCH_PAD			0x0A
+#define AD5529R_REG_SPI_REVISION		0x0B
+#define AD5529R_REG_VENDOR_H			0x0D
+#define AD5529R_REG_STREAM_MODE			0x0E
+#define AD5529R_REG_INTERFACE_STATUS_A		0x11
+#define AD5529R_REG_MULTI_DAC_CH_SEL		0x14
+#define AD5529R_REG_OUT_RANGE_BASE		0x3C
+#define AD5529R_REG_OUT_RANGE(ch)		(AD5529R_REG_OUT_RANGE_BASE + (ch) * 2)
+#define AD5529R_REG_DAC_INPUT_A_BASE		0x148
+#define AD5529R_REG_DAC_INPUT_A(ch)		(AD5529R_REG_DAC_INPUT_A_BASE + (ch) * 2)
+#define AD5529R_REG_DAC_DATA_READBACK_BASE	0x16A
+#define AD5529R_REG_TSENS_ALERT_FLAG		0x18C
+#define AD5529R_REG_TSENS_SHTD_FLAG		0x18E
+#define AD5529R_REG_FUNC_BUSY			0x1A0
+#define AD5529R_REG_REF_SEL			0x1A2
+#define   AD5529R_REF_SEL_INTERNAL_REF		BIT(0)
+#define AD5529R_REG_INIT_CRC_ERR_STAT		0x1A4
+#define AD5529R_REG_MULTI_DAC_HOTPATH_SW_LDAC	0x1A8
+
+#define AD5529R_MAX_REGISTER			0x232
+#define AD5529R_8BIT_REG_MAX			0x13
+#define AD5529R_SPI_READ_FLAG			0x80
+
+struct ad5529r_model_data {
+	const char *model_name;
+	unsigned int resolution;
+	const struct iio_chan_spec *channels;
+	unsigned int num_channels;
+};
+
+#define AD5529R_DAC_CHANNEL(chan, bits) {			\
+	.type = IIO_VOLTAGE,					\
+	.indexed = 1,						\
+	.output = 1,						\
+	.channel = (chan),					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
+			      BIT(IIO_CHAN_INFO_SCALE) |	\
+			      BIT(IIO_CHAN_INFO_OFFSET),	\
+	.scan_type = {						\
+		.format = 'u',					\
+		.realbits = (bits),				\
+		.storagebits = 16,				\
+	},							\
+}
+
+static const char * const ad5529r_supply_names[] = {
+	"vdd",
+	"avdd",
+	"hvdd",
+};
+
+static const struct iio_chan_spec ad5529r_channels_16bit[] = {
+	AD5529R_DAC_CHANNEL(0, 16),
+	AD5529R_DAC_CHANNEL(1, 16),
+	AD5529R_DAC_CHANNEL(2, 16),
+	AD5529R_DAC_CHANNEL(3, 16),
+	AD5529R_DAC_CHANNEL(4, 16),
+	AD5529R_DAC_CHANNEL(5, 16),
+	AD5529R_DAC_CHANNEL(6, 16),
+	AD5529R_DAC_CHANNEL(7, 16),
+	AD5529R_DAC_CHANNEL(8, 16),
+	AD5529R_DAC_CHANNEL(9, 16),
+	AD5529R_DAC_CHANNEL(10, 16),
+	AD5529R_DAC_CHANNEL(11, 16),
+	AD5529R_DAC_CHANNEL(12, 16),
+	AD5529R_DAC_CHANNEL(13, 16),
+	AD5529R_DAC_CHANNEL(14, 16),
+	AD5529R_DAC_CHANNEL(15, 16),
+};
+
+static const struct iio_chan_spec ad5529r_channels_12bit[] = {
+	AD5529R_DAC_CHANNEL(0, 12),
+	AD5529R_DAC_CHANNEL(1, 12),
+	AD5529R_DAC_CHANNEL(2, 12),
+	AD5529R_DAC_CHANNEL(3, 12),
+	AD5529R_DAC_CHANNEL(4, 12),
+	AD5529R_DAC_CHANNEL(5, 12),
+	AD5529R_DAC_CHANNEL(6, 12),
+	AD5529R_DAC_CHANNEL(7, 12),
+	AD5529R_DAC_CHANNEL(8, 12),
+	AD5529R_DAC_CHANNEL(9, 12),
+	AD5529R_DAC_CHANNEL(10, 12),
+	AD5529R_DAC_CHANNEL(11, 12),
+	AD5529R_DAC_CHANNEL(12, 12),
+	AD5529R_DAC_CHANNEL(13, 12),
+	AD5529R_DAC_CHANNEL(14, 12),
+	AD5529R_DAC_CHANNEL(15, 12),
+};
+
+static const struct ad5529r_model_data ad5529r_16bit_model_data = {
+	.model_name = "ad5529r-16",
+	.resolution = 16,
+	.channels = ad5529r_channels_16bit,
+	.num_channels = ARRAY_SIZE(ad5529r_channels_16bit),
+};
+
+static const struct ad5529r_model_data ad5529r_12bit_model_data = {
+	.model_name = "ad5529r-12",
+	.resolution = 12,
+	.channels = ad5529r_channels_12bit,
+	.num_channels = ARRAY_SIZE(ad5529r_channels_12bit),
+};
+
+enum ad5529r_output_range {
+	AD5529R_RANGE_0V_5V,
+	AD5529R_RANGE_0V_10V,
+	AD5529R_RANGE_0V_20V,
+	AD5529R_RANGE_0V_40V,
+	AD5529R_RANGE_NEG5V_5V,
+	AD5529R_RANGE_NEG10V_10V,
+	AD5529R_RANGE_NEG15V_15V,
+	AD5529R_RANGE_NEG20V_20V,
+};
+
+static const s32 ad5529r_output_ranges_mV[8][2] = {
+	[AD5529R_RANGE_0V_5V] = { 0, 5000 },
+	[AD5529R_RANGE_0V_10V] = { 0, 10000 },
+	[AD5529R_RANGE_0V_20V] = { 0, 20000 },
+	[AD5529R_RANGE_0V_40V] = { 0, 40000 },
+	[AD5529R_RANGE_NEG5V_5V] = { -5000, 5000 },
+	[AD5529R_RANGE_NEG10V_10V] = { -10000, 10000 },
+	[AD5529R_RANGE_NEG15V_15V] = { -15000, 15000 },
+	[AD5529R_RANGE_NEG20V_20V] = { -20000, 20000 },
+};
+
+struct ad5529r_state {
+	struct spi_device *spi;
+	const struct ad5529r_model_data *model_data;
+	struct regmap *regmap_8bit;
+	struct regmap *regmap_16bit;
+	enum ad5529r_output_range output_range_idx[16];
+};
+
+static const struct regmap_range ad5529r_8bit_readable_ranges[] = {
+	regmap_reg_range(AD5529R_REG_INTERFACE_CONFIG_A, AD5529R_REG_CHIP_GRADE),
+	regmap_reg_range(AD5529R_REG_SCRATCH_PAD, AD5529R_REG_VENDOR_H),
+	regmap_reg_range(AD5529R_REG_STREAM_MODE, AD5529R_REG_INTERFACE_STATUS_A),
+};
+
+static const struct regmap_range ad5529r_16bit_readable_ranges[] = {
+	regmap_reg_range(AD5529R_REG_MULTI_DAC_CH_SEL, AD5529R_REG_INIT_CRC_ERR_STAT),
+	regmap_reg_range(AD5529R_REG_MULTI_DAC_HOTPATH_SW_LDAC, AD5529R_MAX_REGISTER),
+};
+
+static const struct regmap_access_table ad5529r_8bit_readable_table = {
+	.yes_ranges = ad5529r_8bit_readable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(ad5529r_8bit_readable_ranges),
+};
+
+static const struct regmap_access_table ad5529r_16bit_readable_table = {
+	.yes_ranges = ad5529r_16bit_readable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(ad5529r_16bit_readable_ranges),
+};
+
+static const struct regmap_range ad5529r_8bit_read_only_ranges[] = {
+	regmap_reg_range(AD5529R_REG_DEVICE_CONFIG, AD5529R_REG_CHIP_GRADE),
+	regmap_reg_range(AD5529R_REG_SPI_REVISION, AD5529R_REG_VENDOR_H),
+};
+
+static const struct regmap_range ad5529r_16bit_read_only_ranges[] = {
+	regmap_reg_range(AD5529R_REG_DAC_DATA_READBACK_BASE,
+			 AD5529R_REG_DAC_DATA_READBACK_BASE + 15 * 2),
+	regmap_reg_range(AD5529R_REG_TSENS_ALERT_FLAG, AD5529R_REG_TSENS_SHTD_FLAG),
+	regmap_reg_range(AD5529R_REG_FUNC_BUSY, AD5529R_REG_FUNC_BUSY),
+	regmap_reg_range(AD5529R_REG_INIT_CRC_ERR_STAT, AD5529R_REG_INIT_CRC_ERR_STAT),
+};
+
+static const struct regmap_access_table ad5529r_8bit_writeable_table = {
+	.no_ranges = ad5529r_8bit_read_only_ranges,
+	.n_no_ranges = ARRAY_SIZE(ad5529r_8bit_read_only_ranges),
+};
+
+static const struct regmap_access_table ad5529r_16bit_writeable_table = {
+	.no_ranges = ad5529r_16bit_read_only_ranges,
+	.n_no_ranges = ARRAY_SIZE(ad5529r_16bit_read_only_ranges),
+};
+
+static const struct regmap_config ad5529r_regmap_8bit_config = {
+	.name = "ad5529r-8bit",
+	.reg_bits = 16,
+	.val_bits = 8,
+	.max_register = AD5529R_8BIT_REG_MAX,
+	.read_flag_mask = AD5529R_SPI_READ_FLAG,
+	.rd_table = &ad5529r_8bit_readable_table,
+	.wr_table = &ad5529r_8bit_writeable_table,
+};
+
+static const struct regmap_config ad5529r_regmap_16bit_config = {
+	.name = "ad5529r-16bit",
+	.reg_bits = 16,
+	.val_bits = 16,
+	.max_register = AD5529R_MAX_REGISTER,
+	.read_flag_mask = AD5529R_SPI_READ_FLAG,
+	.val_format_endian = REGMAP_ENDIAN_LITTLE,
+	.rd_table = &ad5529r_16bit_readable_table,
+	.wr_table = &ad5529r_16bit_writeable_table,
+	.reg_stride = 2,
+};
+
+static struct regmap *ad5529r_get_regmap(struct ad5529r_state *st,
+					 unsigned int reg)
+{
+	if (reg <= AD5529R_8BIT_REG_MAX)
+		return st->regmap_8bit;
+
+	return st->regmap_16bit;
+}
+
+static int ad5529r_reset(struct ad5529r_state *st)
+{
+	struct reset_control *rst;
+	int ret;
+
+	rst = devm_reset_control_get_optional_exclusive(&st->spi->dev, NULL);
+	if (IS_ERR(rst))
+		return PTR_ERR(rst);
+
+	if (rst) {
+		ret = reset_control_reset(rst);
+		if (ret)
+			return ret;
+	} else {
+		ret = regmap_write(st->regmap_8bit, AD5529R_REG_INTERFACE_CONFIG_A,
+				   AD5529R_INTERFACE_CONFIG_A_SW_RESET);
+		if (ret)
+			return ret;
+	}
+
+	/*
+	 * Wait 10 ms for digital initialization to complete.
+	 * Per datasheet, Interface Status A register NOT_READY_ERR bit is
+	 * set if SPI transactions are attempted before digital initialization
+	 * completes.
+	 */
+	fsleep(10 * USEC_PER_MSEC);
+
+	return regmap_write(st->regmap_8bit, AD5529R_REG_INTERFACE_CONFIG_A,
+			    AD5529R_INTERFACE_CONFIG_A_SDO_ENABLE |
+			    AD5529R_INTERFACE_CONFIG_A_ADDR_ASCENSION);
+}
+
+static int ad5529r_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)
+{
+	struct ad5529r_state *st = iio_priv(indio_dev);
+	unsigned int reg_addr, reg_val_h;
+	int ret, range_idx, span_mv;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		/*
+		 * Read from DAC_INPUT_A register rather than DAC_DATA_READBACK.
+		 * The DAC operates in transparent mode and directly reflects
+		 * whatever value is written to the INPUT_A register.
+		 */
+		reg_addr = AD5529R_REG_DAC_INPUT_A(chan->channel);
+		ret = regmap_read(st->regmap_16bit, reg_addr, &reg_val_h);
+		if (ret)
+			return ret;
+
+		*val = reg_val_h;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		range_idx = st->output_range_idx[chan->channel];
+
+		span_mv = ad5529r_output_ranges_mV[range_idx][1] -
+			  ad5529r_output_ranges_mV[range_idx][0];
+		*val = span_mv;
+		*val2 = st->model_data->resolution;
+
+		return IIO_VAL_FRACTIONAL_LOG2;
+	case IIO_CHAN_INFO_OFFSET:
+		range_idx = st->output_range_idx[chan->channel];
+
+		if (ad5529r_output_ranges_mV[range_idx][0] < 0)
+			*val = -(1 << (st->model_data->resolution - 1));
+		else
+			*val = 0;
+
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad5529r_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int val, int val2, long mask)
+{
+	struct ad5529r_state *st = iio_priv(indio_dev);
+	unsigned int reg_addr;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		if (val < 0 || val > GENMASK(st->model_data->resolution - 1, 0))
+			return -EINVAL;
+
+		reg_addr = AD5529R_REG_DAC_INPUT_A(chan->channel);
+
+		return regmap_write(st->regmap_16bit, reg_addr, val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad5529r_find_output_range(const s32 *vals)
+{
+	for (unsigned int i = 0; i < ARRAY_SIZE(ad5529r_output_ranges_mV); i++) {
+		if (vals[0] == ad5529r_output_ranges_mV[i][0] * 1000 &&
+		    vals[1] == ad5529r_output_ranges_mV[i][1] * 1000)
+			return i;
+	}
+
+	return -EINVAL;
+}
+
+static int ad5529r_parse_channel_ranges(struct device *dev,
+					struct ad5529r_state *st)
+{
+	s32 vals[2];
+	int ret, range_idx;
+	u32 ch;
+
+	device_for_each_child_node_scoped(dev, child) {
+		ret = fwnode_property_read_u32(child, "reg", &ch);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Missing reg property in channel node\n");
+
+		if (ch >= 16)
+			return dev_err_probe(dev, -EINVAL,
+					     "Channel %u exceeds maximum supported channel 15\n",
+					     ch);
+
+		if (fwnode_property_present(child, "adi,output-range-microvolt")) {
+			/*
+			 * DT stores cells as raw 32-bit values; signed endpoints are
+			 * encoded by dtc in two's-complement and then interpreted
+			 * here as s32.
+			 */
+			ret = fwnode_property_read_u32_array(child,
+							     "adi,output-range-microvolt",
+							     (u32 *)vals, ARRAY_SIZE(vals));
+			if (ret < 0)
+				return dev_err_probe(dev, ret,
+						     "Failed to read range for ch %u\n",
+						     ch);
+
+			range_idx = ad5529r_find_output_range(vals);
+			if (range_idx < 0)
+				return dev_err_probe(dev, range_idx,
+						     "Invalid range [%d %d] for ch %u\n",
+						     vals[0], vals[1], ch);
+		} else {
+			range_idx = AD5529R_RANGE_0V_5V;
+		}
+
+		st->output_range_idx[ch] = range_idx;
+		ret = regmap_write(st->regmap_16bit,
+				   AD5529R_REG_OUT_RANGE(ch), range_idx);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Failed to configure range for ch %u\n",
+					     ch);
+	}
+
+	return 0;
+}
+
+static int ad5529r_reg_access(struct iio_dev *indio_dev,
+			      unsigned int reg,
+			      unsigned int writeval,
+			      unsigned int *readval)
+{
+	struct ad5529r_state *st = iio_priv(indio_dev);
+
+	if (readval)
+		return regmap_read(ad5529r_get_regmap(st, reg), reg, readval);
+
+	return regmap_write(ad5529r_get_regmap(st, reg), reg, writeval);
+}
+
+static const struct iio_info ad5529r_info = {
+	.read_raw = ad5529r_read_raw,
+	.write_raw = ad5529r_write_raw,
+	.debugfs_reg_access = ad5529r_reg_access,
+};
+
+static int ad5529r_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct iio_dev *indio_dev;
+	struct ad5529r_state *st;
+	bool external_vref;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+
+	st->spi = spi;
+
+	st->model_data = spi_get_device_match_data(spi);
+	if (!st->model_data)
+		return dev_err_probe(dev, -EINVAL, "Failed to identify device variant\n");
+
+	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(ad5529r_supply_names),
+					     ad5529r_supply_names);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to get and enable regulators\n");
+
+	ret = devm_regulator_get_enable_optional(dev, "hvss");
+	if (ret && ret != -ENODEV)
+		return dev_err_probe(dev, ret,
+				     "Failed to get and enable hvss regulator\n");
+
+	/*
+	 * The datasheet mentions a 4.096V external reference for correct
+	 * operation.
+	 */
+	ret = devm_regulator_get_enable_optional(dev, "vref");
+	if (ret == -ENODEV) {
+		external_vref = false;
+	} else if (ret) {
+		return dev_err_probe(dev, ret,
+				     "Failed to get and enable vref regulator\n");
+	} else {
+		external_vref = true;
+	}
+
+	st->regmap_8bit = devm_regmap_init_spi(spi, &ad5529r_regmap_8bit_config);
+	if (IS_ERR(st->regmap_8bit))
+		return dev_err_probe(dev, PTR_ERR(st->regmap_8bit),
+				     "Failed to initialize 8-bit regmap\n");
+
+	st->regmap_16bit = devm_regmap_init_spi(spi, &ad5529r_regmap_16bit_config);
+	if (IS_ERR(st->regmap_16bit))
+		return dev_err_probe(dev, PTR_ERR(st->regmap_16bit),
+				     "Failed to initialize 16-bit regmap\n");
+
+	ret = ad5529r_reset(st);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to reset device\n");
+
+	ret = regmap_assign_bits(st->regmap_16bit, AD5529R_REG_REF_SEL,
+				 AD5529R_REF_SEL_INTERNAL_REF,
+				 external_vref ? 0 : AD5529R_REF_SEL_INTERNAL_REF);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to configure reference\n");
+
+	ret = ad5529r_parse_channel_ranges(dev, st);
+	if (ret)
+		return ret;
+
+	indio_dev->name = st->model_data->model_name;
+	indio_dev->info = &ad5529r_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = st->model_data->channels;
+	indio_dev->num_channels = st->model_data->num_channels;
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct of_device_id ad5529r_of_match[] = {
+	{ .compatible = "adi,ad5529r-16", .data = &ad5529r_16bit_model_data },
+	{ .compatible = "adi,ad5529r-12", .data = &ad5529r_12bit_model_data },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ad5529r_of_match);
+
+static const struct spi_device_id ad5529r_id[] = {
+	{
+		.name = "ad5529r-16",
+		.driver_data = (kernel_ulong_t)&ad5529r_16bit_model_data,
+	},
+	{
+		.name = "ad5529r-12",
+		.driver_data = (kernel_ulong_t)&ad5529r_12bit_model_data,
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, ad5529r_id);
+
+static struct spi_driver ad5529r_driver = {
+	.driver = {
+		.name = "ad5529r",
+		.of_match_table = ad5529r_of_match,
+	},
+	.probe = ad5529r_probe,
+	.id_table = ad5529r_id,
+};
+module_spi_driver(ad5529r_driver);
+
+MODULE_AUTHOR("Janani Sunil <janani.sunil@analog.com>");
+MODULE_DESCRIPTION("Analog Devices AD5529R 12/16-bit DAC driver");
+MODULE_LICENSE("GPL");

-- 
2.43.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 2/3] dt-bindings: iio: dac: Add AD5529R
  2026-07-01  6:40 ` [PATCH v5 2/3] dt-bindings: iio: dac: Add AD5529R Janani Sunil
@ 2026-07-01  6:52   ` sashiko-bot
  2026-07-01 11:07   ` Conor Dooley
  2026-07-01 18:41   ` Jonathan Cameron
  2 siblings, 0 replies; 15+ messages in thread
From: sashiko-bot @ 2026-07-01  6:52 UTC (permalink / raw)
  To: Janani Sunil; +Cc: conor+dt, devicetree, robh

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The Device Tree binding incorrectly aggregates up to four separate physical AD5529R chips into a single logical DT node, misplacing the `spi,device-addr` property into individual IIO channel subnodes and extending the channel `reg` limit to 63.
--

commit 8170de6b140882817db48df2f479428c10fc9dbc
Author: Janani Sunil <janani.sunil@analog.com>

dt-bindings: iio: dac: Add AD5529R

This commit adds Devicetree bindings for the Analog Devices AD5529R, a
16-channel 12/16-bit high voltage DAC. It includes properties for
power supplies, PWM, GPIOs, and channel configurations.

> diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad5529r.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad5529r.yaml
> new file mode 100644
> index 0000000000000..97075b1c919d1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad5529r.yaml
[ ... ]
> +patternProperties:
> +  "^channel@([0-9a-f]{1,2})$":
> +    type: object
> +    description: Child nodes for individual channel configuration
> +
> +    properties:
> +      reg:
> +        description: Channel number.
> +        minimum: 0
> +        maximum: 63

[Severity: High]
Does this topology correctly model the hardware? The AD5529R is a 16-channel
DAC, but setting the maximum to 63 implies flattening up to four separate
physical chips into a single logical Device Tree node.

By aggregating multiple chips into a single node, they are forced to share a
single reset-gpios, clear-gpios, and interrupts definition. If a board wires
these physical pins to different SoC GPIOs or interrupts for each chip, it
would be structurally impossible to describe that hardware using this binding.

> +
> +      spi,device-addr:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [0, 1, 2, 3]
> +        description:
> +          Device address selected by the ID0 and ID1 pins. Up to four AD5529R
> +          devices can share a single SPI chip select; each device responds only
> +          to transfers whose address bits [13:12] match its configured address.

[Severity: High]
Should spi,device-addr be defined inside the channel subnode scope?

Since this property represents the physical ID0 and ID1 pins on the chip itself,
placing it in every single channel subnode seems semantically incorrect and
redundant. Should the child nodes represent the physical chips (allowing
per-chip physical properties like spi,device-addr and reset-gpios) rather than
flattening all channels across chips into one level?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701-ad5529r-driver-v5-0-ed087900e642@analog.com?part=2

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 3/3] iio: dac: Add AD5529R DAC driver support
  2026-07-01  6:40 ` [PATCH v5 3/3] iio: dac: Add AD5529R DAC driver support Janani Sunil
@ 2026-07-01  6:52   ` sashiko-bot
  2026-07-01  9:17   ` Andy Shevchenko
  2026-07-01 18:55   ` Jonathan Cameron
  2 siblings, 0 replies; 15+ messages in thread
From: sashiko-bot @ 2026-07-01  6:52 UTC (permalink / raw)
  To: Janani Sunil; +Cc: robh, conor+dt, devicetree

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Channels not specified in the device tree are exposed to userspace but their hardware range is not explicitly configured.
--

commit eb86f7024ceca6fa634016d239f7ccfbf35f6a11
Author: Janani Sunil <janani.sunil@analog.com>
Date:   Wed Jul 1 06:40:41 2026 +0000

iio: dac: Add AD5529R DAC driver support

This commit adds support for the Analog Devices AD5529R 16-channel, 12/16-bit
Digital to Analog Converter. The device communicates over SPI and supports
per-channel output range configuration.

> diff --git a/drivers/iio/dac/ad5529r.c b/drivers/iio/dac/ad5529r.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/iio/dac/ad5529r.c
[ ... ]
> +static int ad5529r_parse_channel_ranges(struct device *dev,
> +					struct ad5529r_state *st)
> +{
> +	s32 vals[2];
> +	int ret, range_idx;
> +	u32 ch;
> +
> +	device_for_each_child_node_scoped(dev, child) {
> +		ret = fwnode_property_read_u32(child, "reg", &ch);
> +		if (ret)
[ ... ]
> +		st->output_range_idx[ch] = range_idx;
> +		ret = regmap_write(st->regmap_16bit,
> +				   AD5529R_REG_OUT_RANGE(ch), range_idx);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to configure range for ch %u\n",
> +					     ch);
> +	}
> +
> +	return 0;
> +}
[ ... ]
> +static int ad5529r_probe(struct spi_device *spi)
> +{
[ ... ]
> +	ret = ad5529r_parse_channel_ranges(dev, st);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->name = st->model_data->model_name;
> +	indio_dev->info = &ad5529r_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = st->model_data->channels;
> +	indio_dev->num_channels = st->model_data->num_channels;

[Severity: High]
If a channel is omitted from the device tree during
ad5529r_parse_channel_ranges(), will it remain uninitialized in hardware
while still being unconditionally exposed to userspace here?

Since ad5529r_parse_channel_ranges() only configures the output range
registers for channels explicitly defined in the device tree child nodes,
unlisted channels will retain whatever state the hardware bootloader or reset
left them in.

The software state for these unlisted channels in st->output_range_idx
defaults to 0 (AD5529R_RANGE_0V_5V). Could this discrepancy cause unexpected
high-voltage outputs if userspace writes to these unconfigured hardware
channels?

> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701-ad5529r-driver-v5-0-ed087900e642@analog.com?part=3

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 1/3] dt-bindings: spi: Add spi,device-addr peripheral property
  2026-07-01  6:40 ` [PATCH v5 1/3] dt-bindings: spi: Add spi,device-addr peripheral property Janani Sunil
@ 2026-07-01  6:52   ` sashiko-bot
  2026-07-01 11:04   ` Conor Dooley
  1 sibling, 0 replies; 15+ messages in thread
From: sashiko-bot @ 2026-07-01  6:52 UTC (permalink / raw)
  To: Janani Sunil; +Cc: robh, conor+dt, devicetree

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Architectural misplacement of the `spi,device-addr` property and incorrect hardware modeling for shared chip selects.
- [Low] Invalid property name formatting (`spi,device-addr`) causing schema validation failure.
--

commit a13113dfcdf0e6c76170858ecffc2c11d4c536cf
Author: Janani Sunil <janani.sunil@analog.com>

dt-bindings: spi: Add spi,device-addr peripheral property

Some SPI devices support sharing a single chip select across multiple
physical chips by encoding a device address in the SPI frame itself.
Add a generic spi,device-addr property to document this per-peripheral
address. This property belongs in channel or sub-device nodes of
peripherals that use this addressing scheme.

> diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> index 880a9f6245667..3774e80183555 100644
> --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> @@ -142,6 +142,11 @@ properties:
>      minItems: 2
>      maxItems: 4
>  
> +  spi,device-addr:

[Severity: Low]
Does this property name pass make dt_binding_check?

Since spi is not a registered vendor in vendor-prefixes.yaml, the comma
syntax typically causes a validation failure. Should this regression be fixed
by using a hyphen instead for a subsystem generic property, such as
spi-device-addr?

[Severity: High]
Does this architectural approach correctly model the hardware?

By placing this property in spi-peripheral-props.yaml, it applies to direct
children of the SPI controller, yet the commit message notes it belongs in
channel or sub-device nodes.

Standard Device Tree architecture generally models a shared chip select bus as a
parent node (with #address-cells and #size-cells) and the individual
chips as child nodes using the standard reg property.

Does using a custom addressing property on sub-channels bypass the standard
DT bus addressing model?

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Device address used when multiple peripherals share a single chip select.
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701-ad5529r-driver-v5-0-ed087900e642@analog.com?part=1

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 3/3] iio: dac: Add AD5529R DAC driver support
  2026-07-01  6:40 ` [PATCH v5 3/3] iio: dac: Add AD5529R DAC driver support Janani Sunil
  2026-07-01  6:52   ` sashiko-bot
@ 2026-07-01  9:17   ` Andy Shevchenko
  2026-07-01 18:55   ` Jonathan Cameron
  2 siblings, 0 replies; 15+ messages in thread
From: Andy Shevchenko @ 2026-07-01  9:17 UTC (permalink / raw)
  To: Janani Sunil
  Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Philipp Zabel, Jonathan Corbet,
	Shuah Khan, Mark Brown, linux-iio, devicetree, linux-kernel,
	linux-doc, Janani Sunil, linux-spi

On Wed, Jul 01, 2026 at 08:40:41AM +0200, Janani Sunil wrote:
> Add support for AD5529R 16-channel, 12/16 bit Digital to Analog Converter
> from Analog Devices.
> 
> The device communicates over SPI and supports per-channel output range
> configuration. An optional external 4.096V reference can be used in
> place of the internal reference.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
A couple of nit-picks below.

...

> +static int ad5529r_find_output_range(const s32 *vals)
> +{
> +	for (unsigned int i = 0; i < ARRAY_SIZE(ad5529r_output_ranges_mV); i++) {
> +		if (vals[0] == ad5529r_output_ranges_mV[i][0] * 1000 &&
> +		    vals[1] == ad5529r_output_ranges_mV[i][1] * 1000)

So, the correct way is to have either defined constant in units.h or while now
(MICRO / MILLI). So, with a temporary pointer this can be achieved without
uglifying the code

		... *range = &ad5529r_output_ranges_mV[i];

		if (vals[0] == range[0] * (MICRO / MILLI) &&
		    vals[1] == range[1] * (MICRO / MILLI))

> +			return i;
> +	}
> +
> +	return -EINVAL;
> +}

...

> +	/*
> +	 * The datasheet mentions a 4.096V external reference for correct
> +	 * operation.
> +	 */
> +	ret = devm_regulator_get_enable_optional(dev, "vref");
> +	if (ret == -ENODEV) {
> +		external_vref = false;
> +	} else if (ret) {
> +		return dev_err_probe(dev, ret,
> +				     "Failed to get and enable vref regulator\n");
> +	} else {
> +		external_vref = true;
> +	}

All branches are single statement. No {} are needed.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 1/3] dt-bindings: spi: Add spi,device-addr peripheral property
  2026-07-01  6:40 ` [PATCH v5 1/3] dt-bindings: spi: Add spi,device-addr peripheral property Janani Sunil
  2026-07-01  6:52   ` sashiko-bot
@ 2026-07-01 11:04   ` Conor Dooley
  2026-07-01 18:29     ` Jonathan Cameron
  1 sibling, 1 reply; 15+ messages in thread
From: Conor Dooley @ 2026-07-01 11:04 UTC (permalink / raw)
  To: Janani Sunil
  Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Philipp Zabel, Jonathan Corbet,
	Shuah Khan, Mark Brown, linux-iio, devicetree, linux-kernel,
	linux-doc, Janani Sunil, linux-spi

[-- Attachment #1: Type: text/plain, Size: 1764 bytes --]

On Wed, Jul 01, 2026 at 08:40:39AM +0200, Janani Sunil wrote:
> Some SPI devices support sharing a single chip select across multiple
> physical chips by encoding a device address in the SPI frame itself.
> Add a generic spi,device-addr property to document this per-peripheral
> address. This property belongs in channel or sub-device nodes of
> peripherals that use this addressing scheme.
> 
> Signed-off-by: Janani Sunil <janani.sunil@analog.com>
> ---
>  Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> index 880a9f624566..3774e8018355 100644
> --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> @@ -142,6 +142,11 @@ properties:
>      minItems: 2
>      maxItems: 4
>  
> +  spi,device-addr:

To match other generic spi properties, s/,/-/.

However, you don't actually use this as a spi peripheral's property in
your device binding, so you've got your wires crossed here somewhere.

If it's a generic dac channel property (as you use it) it should be in
dac.yaml (or adc.yaml for the other device that I asked you to add it
for as proof of being generic), or it is a spi peripheral property and
needs to go into the dac node itself.

pw-bot: changes-requested

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Device address used when multiple peripherals share a single chip select.
> +
>    st,spi-midi-ns:
>      deprecated: true
>      description: |
> 
> -- 
> 2.43.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 2/3] dt-bindings: iio: dac: Add AD5529R
  2026-07-01  6:40 ` [PATCH v5 2/3] dt-bindings: iio: dac: Add AD5529R Janani Sunil
  2026-07-01  6:52   ` sashiko-bot
@ 2026-07-01 11:07   ` Conor Dooley
  2026-07-01 18:41   ` Jonathan Cameron
  2 siblings, 0 replies; 15+ messages in thread
From: Conor Dooley @ 2026-07-01 11:07 UTC (permalink / raw)
  To: Janani Sunil
  Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Philipp Zabel, Jonathan Corbet,
	Shuah Khan, Mark Brown, linux-iio, devicetree, linux-kernel,
	linux-doc, Janani Sunil, linux-spi

[-- Attachment #1: Type: text/plain, Size: 1756 bytes --]

On Wed, Jul 01, 2026 at 08:40:40AM +0200, Janani Sunil wrote:

> +patternProperties:
> +  "^channel@([0-9a-f]{1,2})$":
> +    type: object
> +    description: Child nodes for individual channel configuration
> +
> +    properties:
> +      reg:
> +        description: Channel number.
> +        minimum: 0
> +        maximum: 63
> +
> +      spi,device-addr:
> +        $ref: /schemas/types.yaml#/definitions/uint32

Same comments apply here about whether this is a channel or a spi
peripheral level property.

> +        enum: [0, 1, 2, 3]
> +        description:
> +          Device address selected by the ID0 and ID1 pins. Up to four AD5529R
> +          devices can share a single SPI chip select; each device responds only
> +          to transfers whose address bits [13:12] match its configured address.
> +
> +      adi,output-range-microvolt:

I didn't notice this on the previous versions, but doesn't this
duplicate the common output-range-microvolt in dac.yaml, which you
should be including here because these channels are dacs?

pw-bot: changes-requested

Cheers,
Conor.

> +        description: |
> +          Output voltage range for this channel as [min, max] in microvolts.
> +          If not specified, defaults to 0V to 5V range.
> +        oneOf:
> +          - items:
> +              - const: 0
> +              - enum: [5000000, 10000000, 20000000, 40000000]
> +          - items:
> +              - const: -5000000
> +              - const: 5000000
> +          - items:
> +              - const: -10000000
> +              - const: 10000000
> +          - items:
> +              - const: -15000000
> +              - const: 15000000
> +          - items:
> +              - const: -20000000
> +              - const: 20000000

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 1/3] dt-bindings: spi: Add spi,device-addr peripheral property
  2026-07-01 11:04   ` Conor Dooley
@ 2026-07-01 18:29     ` Jonathan Cameron
  2026-07-01 18:48       ` David Lechner
  0 siblings, 1 reply; 15+ messages in thread
From: Jonathan Cameron @ 2026-07-01 18:29 UTC (permalink / raw)
  To: Conor Dooley
  Cc: Janani Sunil, Lars-Peter Clausen, Michael Hennerich,
	David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Philipp Zabel, Jonathan Corbet,
	Shuah Khan, Mark Brown, linux-iio, devicetree, linux-kernel,
	linux-doc, Janani Sunil, linux-spi

On Wed, 1 Jul 2026 12:04:37 +0100
Conor Dooley <conor@kernel.org> wrote:

> On Wed, Jul 01, 2026 at 08:40:39AM +0200, Janani Sunil wrote:
> > Some SPI devices support sharing a single chip select across multiple
> > physical chips by encoding a device address in the SPI frame itself.
> > Add a generic spi,device-addr property to document this per-peripheral
> > address. This property belongs in channel or sub-device nodes of
> > peripherals that use this addressing scheme.
> > 
> > Signed-off-by: Janani Sunil <janani.sunil@analog.com>
> > ---
> >  Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > index 880a9f624566..3774e8018355 100644
> > --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > @@ -142,6 +142,11 @@ properties:
> >      minItems: 2
> >      maxItems: 4
> >  
> > +  spi,device-addr:  
> 
> To match other generic spi properties, s/,/-/.
> 
> However, you don't actually use this as a spi peripheral's property in
> your device binding, so you've got your wires crossed here somewhere.

If we are going to make this generic (which I'm not against) I think
it should also work for the case of multiple independent devices.
So it can also be a top level device node spi property.

That kind of makes me wonder if we are better off having it always
in the top level node, but allowing multiple values to represent
sub devices under this.  That would leave figuring out mappings of which
channels are on which device to the driver. The driver must know the
mapping afterall.  For the example something like


#include <dt-bindings/gpio/gpio.h>
spi {
    #address-cells = <1>;
    #size-cells = <0>;
    dac@0 {
        compatible = "adi,ad5529r-16";
        reg = <0>;
        spi-max-frequency = <25000000>;

        spi-device-addreses = <0 3>
...

        #address-cells = <1>;
        #size-cells = <0>;

        channel@0 {
            reg = <0>;
            adi,output-range-microvolt = <0 5000000>;
        };

        channel@16 { #on second device using dev addr 3
            reg = <16>;
            adi,output-range-microvolt = <(-10000000) 10000000>;
        };
        channel@18 { #3rd channel on device using dev addr 3
            reg = <18>;
            adi,output-range-microvolt = <0 40000000>;
        };
    };
};

Where devices are truely independent then you would have separate device
nodes each with one entry in spi-device-addresses

I'm a bit dubious about putting this in the spi namespace though given
it is not part of any standard specification.  Do we have any precedence
for that sort of thing?

Jonathan

> 
> If it's a generic dac channel property (as you use it) it should be in
> dac.yaml (or adc.yaml for the other device that I asked you to add it
> for as proof of being generic), or it is a spi peripheral property and
> needs to go into the dac node itself.
> 
> pw-bot: changes-requested
> 
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      Device address used when multiple peripherals share a single chip select.
> > +
> >    st,spi-midi-ns:
> >      deprecated: true
> >      description: |
> > 
> > -- 
> > 2.43.0
> >   


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 2/3] dt-bindings: iio: dac: Add AD5529R
  2026-07-01  6:40 ` [PATCH v5 2/3] dt-bindings: iio: dac: Add AD5529R Janani Sunil
  2026-07-01  6:52   ` sashiko-bot
  2026-07-01 11:07   ` Conor Dooley
@ 2026-07-01 18:41   ` Jonathan Cameron
  2 siblings, 0 replies; 15+ messages in thread
From: Jonathan Cameron @ 2026-07-01 18:41 UTC (permalink / raw)
  To: Janani Sunil
  Cc: Lars-Peter Clausen, Michael Hennerich, David Lechner,
	Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Philipp Zabel, Jonathan Corbet, Shuah Khan,
	Mark Brown, linux-iio, devicetree, linux-kernel, linux-doc,
	Janani Sunil, linux-spi

On Wed, 1 Jul 2026 08:40:40 +0200
Janani Sunil <janani.sunil@analog.com> wrote:

> Devicetree bindings for AD5529R 16 channel 12/16 bit high voltage,
> buffered voltage output digital-to-analog converter (DAC) with an
> integrated precision reference.
> 
> Signed-off-by: Janani Sunil <janani.sunil@analog.com>
Putting aside our device address discussions as being handled elsewhere
one minor thing inline

> diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad5529r.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad5529r.yaml
> new file mode 100644
> index 000000000000..97075b1c919d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad5529r.yaml

> +properties:
> +patternProperties:
> +  "^channel@([0-9a-f]{1,2})$":
> +    type: object
> +    description: Child nodes for individual channel configuration
> +
> +    properties:

> +      adi,output-range-microvolt:
> +        description: |
> +          Output voltage range for this channel as [min, max] in microvolts.
> +          If not specified, defaults to 0V to 5V range.

No way to specify the default as part of the binding rather than a comment?
I haven't checked but does
       default: [0 5000000]
not work?
> +        oneOf:
> +          - items:
> +              - const: 0
> +              - enum: [5000000, 10000000, 20000000, 40000000]
> +          - items:
> +              - const: -5000000
> +              - const: 5000000
> +          - items:
> +              - const: -10000000
> +              - const: 10000000
> +          - items:
> +              - const: -15000000
> +              - const: 15000000
> +          - items:
> +              - const: -20000000
> +              - const: 20000000
> +
> +    required:
> +      - reg
> +
> +    additionalProperties: false




^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 1/3] dt-bindings: spi: Add spi,device-addr peripheral property
  2026-07-01 18:29     ` Jonathan Cameron
@ 2026-07-01 18:48       ` David Lechner
  2026-07-01 20:31         ` Conor Dooley
  0 siblings, 1 reply; 15+ messages in thread
From: David Lechner @ 2026-07-01 18:48 UTC (permalink / raw)
  To: Jonathan Cameron, Conor Dooley
  Cc: Janani Sunil, Lars-Peter Clausen, Michael Hennerich, Nuno Sá,
	Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Philipp Zabel, Jonathan Corbet, Shuah Khan, Mark Brown, linux-iio,
	devicetree, linux-kernel, linux-doc, Janani Sunil, linux-spi

Note that a few subsystems, including spi want the subject
to be `spi: dt-bindings:` rather than the other way around.

See https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html


On 7/1/26 1:29 PM, Jonathan Cameron wrote:
> On Wed, 1 Jul 2026 12:04:37 +0100
> Conor Dooley <conor@kernel.org> wrote:
> 
>> On Wed, Jul 01, 2026 at 08:40:39AM +0200, Janani Sunil wrote:
>>> Some SPI devices support sharing a single chip select across multiple
>>> physical chips by encoding a device address in the SPI frame itself.
>>> Add a generic spi,device-addr property to document this per-peripheral
>>> address. This property belongs in channel or sub-device nodes of
>>> peripherals that use this addressing scheme.
>>>
>>> Signed-off-by: Janani Sunil <janani.sunil@analog.com>
>>> ---
>>>  Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
>>> index 880a9f624566..3774e8018355 100644
>>> --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
>>> +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
>>> @@ -142,6 +142,11 @@ properties:
>>>      minItems: 2
>>>      maxItems: 4
>>>  
>>> +  spi,device-addr:  
>>
>> To match other generic spi properties, s/,/-/.
>>
>> However, you don't actually use this as a spi peripheral's property in
>> your device binding, so you've got your wires crossed here somewhere.
> 
> If we are going to make this generic (which I'm not against) I think
> it should also work for the case of multiple independent devices.
> So it can also be a top level device node spi property.
> 
> That kind of makes me wonder if we are better off having it always
> in the top level node, but allowing multiple values to represent
> sub devices under this.  That would leave figuring out mappings of which
> channels are on which device to the driver. The driver must know the
> mapping afterall.  For the example something like
> 
> 
> #include <dt-bindings/gpio/gpio.h>
> spi {
>     #address-cells = <1>;
>     #size-cells = <0>;
>     dac@0 {
>         compatible = "adi,ad5529r-16";
>         reg = <0>;
>         spi-max-frequency = <25000000>;
> 
>         spi-device-addreses = <0 3>
> ...
> 
>         #address-cells = <1>;
>         #size-cells = <0>;
> 
>         channel@0 {
>             reg = <0>;
>             adi,output-range-microvolt = <0 5000000>;
>         };
> 
>         channel@16 { #on second device using dev addr 3
>             reg = <16>;
>             adi,output-range-microvolt = <(-10000000) 10000000>;
>         };
>         channel@18 { #3rd channel on device using dev addr 3
>             reg = <18>;
>             adi,output-range-microvolt = <0 40000000>;
>         };
>     };
> };
> 
> Where devices are truely independent then you would have separate device
> nodes each with one entry in spi-device-addresses
> 
> I'm a bit dubious about putting this in the spi namespace though given
> it is not part of any standard specification.  Do we have any precedence
> for that sort of thing?

It seems like most SPI controllers/devices don't really follow any
standards, so I think there is plenty of precedence for a property
like this. It would be nice to see one or two more examples of SPI
peripherals with this feature though other than the one chip in
this series. Otherwise, I wouldn't try to make it a standard property.

I'm also in favor of making it an array and letting the device-specific
bindings decide what multiple devices with different addresses on the
same CS line means.

But... if we are leaving it up to devices to deal with the property rather
than the core SPI code, maybe it shouldn't be a standard SPI property.
Although, I suppose the core SPI code could parse the property and just
pass that information in the struct spi_device to let the device driver
do what it wants with it.

> 
> Jonathan
> 
>>
>> If it's a generic dac channel property (as you use it) it should be in
>> dac.yaml (or adc.yaml for the other device that I asked you to add it
>> for as proof of being generic), or it is a spi peripheral property and
>> needs to go into the dac node itself.
>>
>> pw-bot: changes-requested
>>
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    description:
>>> +      Device address used when multiple peripherals share a single chip select.
>>> +
>>>    st,spi-midi-ns:
>>>      deprecated: true
>>>      description: |
>>>
>>> -- 
>>> 2.43.0
>>>   
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 3/3] iio: dac: Add AD5529R DAC driver support
  2026-07-01  6:40 ` [PATCH v5 3/3] iio: dac: Add AD5529R DAC driver support Janani Sunil
  2026-07-01  6:52   ` sashiko-bot
  2026-07-01  9:17   ` Andy Shevchenko
@ 2026-07-01 18:55   ` Jonathan Cameron
  2 siblings, 0 replies; 15+ messages in thread
From: Jonathan Cameron @ 2026-07-01 18:55 UTC (permalink / raw)
  To: Janani Sunil
  Cc: Lars-Peter Clausen, Michael Hennerich, David Lechner,
	Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Philipp Zabel, Jonathan Corbet, Shuah Khan,
	Mark Brown, linux-iio, devicetree, linux-kernel, linux-doc,
	Janani Sunil, linux-spi

On Wed, 1 Jul 2026 08:40:41 +0200
Janani Sunil <janani.sunil@analog.com> wrote:

> Add support for AD5529R 16-channel, 12/16 bit Digital to Analog Converter
> from Analog Devices.
> 
> The device communicates over SPI and supports per-channel output range
> configuration. An optional external 4.096V reference can be used in
> place of the internal reference.

This needs to respect the device address stuff in the dt binding.
It is fine to initially support just one device, but if it's address is 2 then
the driver should work.

> 
> Signed-off-by: Janani Sunil <janani.sunil@analog.com>
> ---
>  MAINTAINERS               |   1 +
>  drivers/iio/dac/Kconfig   |  17 ++
>  drivers/iio/dac/Makefile  |   1 +
>  drivers/iio/dac/ad5529r.c | 531 ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 550 insertions(+)
A few other things inline.

Thanks,

> diff --git a/drivers/iio/dac/ad5529r.c b/drivers/iio/dac/ad5529r.c
> new file mode 100644
> index 000000000000..4841bd608482
> --- /dev/null
> +++ b/drivers/iio/dac/ad5529r.c

> +#define AD5529R_DAC_CHANNEL(chan, bits) {			\
> +	.type = IIO_VOLTAGE,					\
> +	.indexed = 1,						\
> +	.output = 1,						\
> +	.channel = (chan),					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
> +			      BIT(IIO_CHAN_INFO_SCALE) |	\
> +			      BIT(IIO_CHAN_INFO_OFFSET),	\
> +	.scan_type = {						\
> +		.format = 'u',					\
> +		.realbits = (bits),				\
> +		.storagebits = 16,				\

None of .scan_type is yet used.  I'd suggest removing that for now.
It can be brought back when it is needed for buffered output support.

> +	},							\
> +}

> +static const struct ad5529r_model_data ad5529r_16bit_model_data = {
> +	.model_name = "ad5529r-16",
> +	.resolution = 16,
> +	.channels = ad5529r_channels_16bit,
> +	.num_channels = ARRAY_SIZE(ad5529r_channels_16bit),
Sashiko picked up on this:
https://sashiko.dev/#/patchset/20260701-ad5529r-driver-v5-0-ed087900e642%40analog.com

When we have channel specific nodes in DT we have always taken the absence
of a given node to mean that it is not wired up and so the driver should not provide it.

I don't mind if for now we insist on all channels being present but we should
then check they are all there in DT or that there are dt-binding specified defaults.

The decision on whether to present all channels or just those in DT is a driver
one but really hard to change in future so much better to just present the
channels that have dt subnodes rather than all of them.


> +};

> +
> +static int ad5529r_probe(struct spi_device *spi)
> +{
> +	ret = devm_regulator_get_enable_optional(dev, "hvss");
> +	if (ret && ret != -ENODEV)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to get and enable hvss regulator\n");
> +
> +	/*
> +	 * The datasheet mentions a 4.096V external reference for correct
> +	 * operation.

I don't see the comment as adding much value given the person who needs
to see that is the board designer and they don't tend to read drivers :)
So probably drop this comment as noise to us.

> +	 */
> +	ret = devm_regulator_get_enable_optional(dev, "vref");
> +	if (ret == -ENODEV) {
> +		external_vref = false;
> +	} else if (ret) {
> +		return dev_err_probe(dev, ret,
> +				     "Failed to get and enable vref regulator\n");
I'd just put that on one slightly long line.

> +	} else {
> +		external_vref = true;
> +	}

Those are all single statements so arguably don't need the {}  It is a bit
fuzzy when you have a line break like in the second one here though.

> +
> +	st->regmap_8bit = devm_regmap_init_spi(spi, &ad5529r_regmap_8bit_config);
> +	if (IS_ERR(st->regmap_8bit))
> +		return dev_err_probe(dev, PTR_ERR(st->regmap_8bit),
> +				     "Failed to initialize 8-bit regmap\n");
> +
> +	st->regmap_16bit = devm_regmap_init_spi(spi, &ad5529r_regmap_16bit_config);
> +	if (IS_ERR(st->regmap_16bit))
> +		return dev_err_probe(dev, PTR_ERR(st->regmap_16bit),
> +				     "Failed to initialize 16-bit regmap\n");
> +
> +	ret = ad5529r_reset(st);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to reset device\n");
> +
> +	ret = regmap_assign_bits(st->regmap_16bit, AD5529R_REG_REF_SEL,
> +				 AD5529R_REF_SEL_INTERNAL_REF,
> +				 external_vref ? 0 : AD5529R_REF_SEL_INTERNAL_REF);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to configure reference\n");
> +
> +	ret = ad5529r_parse_channel_ranges(dev, st);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->name = st->model_data->model_name;
> +	indio_dev->info = &ad5529r_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = st->model_data->channels;
> +	indio_dev->num_channels = st->model_data->num_channels;

See above.  Sadly I think you need to generate these dynamically as we
have per channel dt sub nodes.

> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 1/3] dt-bindings: spi: Add spi,device-addr peripheral property
  2026-07-01 18:48       ` David Lechner
@ 2026-07-01 20:31         ` Conor Dooley
  0 siblings, 0 replies; 15+ messages in thread
From: Conor Dooley @ 2026-07-01 20:31 UTC (permalink / raw)
  To: David Lechner
  Cc: Jonathan Cameron, Janani Sunil, Lars-Peter Clausen,
	Michael Hennerich, Nuno Sá, Andy Shevchenko, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Philipp Zabel, Jonathan Corbet,
	Shuah Khan, Mark Brown, linux-iio, devicetree, linux-kernel,
	linux-doc, Janani Sunil, linux-spi

[-- Attachment #1: Type: text/plain, Size: 5319 bytes --]

On Wed, Jul 01, 2026 at 01:48:24PM -0500, David Lechner wrote:
> Note that a few subsystems, including spi want the subject
> to be `spi: dt-bindings:` rather than the other way around.
> 
> See https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html
> 
> 
> On 7/1/26 1:29 PM, Jonathan Cameron wrote:
> > On Wed, 1 Jul 2026 12:04:37 +0100
> > Conor Dooley <conor@kernel.org> wrote:
> > 
> >> On Wed, Jul 01, 2026 at 08:40:39AM +0200, Janani Sunil wrote:
> >>> Some SPI devices support sharing a single chip select across multiple
> >>> physical chips by encoding a device address in the SPI frame itself.
> >>> Add a generic spi,device-addr property to document this per-peripheral
> >>> address. This property belongs in channel or sub-device nodes of
> >>> peripherals that use this addressing scheme.
> >>>
> >>> Signed-off-by: Janani Sunil <janani.sunil@analog.com>
> >>> ---
> >>>  Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml | 5 +++++
> >>>  1 file changed, 5 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> >>> index 880a9f624566..3774e8018355 100644
> >>> --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> >>> +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> >>> @@ -142,6 +142,11 @@ properties:
> >>>      minItems: 2
> >>>      maxItems: 4
> >>>  
> >>> +  spi,device-addr:  
> >>
> >> To match other generic spi properties, s/,/-/.
> >>
> >> However, you don't actually use this as a spi peripheral's property in
> >> your device binding, so you've got your wires crossed here somewhere.
> > 
> > If we are going to make this generic (which I'm not against) I think
> > it should also work for the case of multiple independent devices.
> > So it can also be a top level device node spi property.
> > 
> > That kind of makes me wonder if we are better off having it always
> > in the top level node, but allowing multiple values to represent
> > sub devices under this.  That would leave figuring out mappings of which
> > channels are on which device to the driver. The driver must know the
> > mapping afterall.  For the example something like
> > 
> > 
> > #include <dt-bindings/gpio/gpio.h>
> > spi {
> >     #address-cells = <1>;
> >     #size-cells = <0>;
> >     dac@0 {
> >         compatible = "adi,ad5529r-16";
> >         reg = <0>;
> >         spi-max-frequency = <25000000>;
> > 
> >         spi-device-addreses = <0 3>
> > ...
> > 
> >         #address-cells = <1>;
> >         #size-cells = <0>;
> > 
> >         channel@0 {
> >             reg = <0>;
> >             adi,output-range-microvolt = <0 5000000>;
> >         };
> > 
> >         channel@16 { #on second device using dev addr 3
> >             reg = <16>;
> >             adi,output-range-microvolt = <(-10000000) 10000000>;
> >         };
> >         channel@18 { #3rd channel on device using dev addr 3
> >             reg = <18>;
> >             adi,output-range-microvolt = <0 40000000>;
> >         };
> >     };
> > };
> > 
> > Where devices are truely independent then you would have separate device
> > nodes each with one entry in spi-device-addresses
> > 
> > I'm a bit dubious about putting this in the spi namespace though given
> > it is not part of any standard specification.  Do we have any precedence
> > for that sort of thing?
> 
> It seems like most SPI controllers/devices don't really follow any
> standards, so I think there is plenty of precedence for a property
> like this. It would be nice to see one or two more examples of SPI
> peripherals with this feature though other than the one chip in
> this series. Otherwise, I wouldn't try to make it a standard property.

There are two microchip devices with a property like "microchip,hw-addr"
that I pointed out on ?v3? (whichever thread had the long discussion) that
do the exact same thing as this device.
I did ask that the submitter convert those to the new property as
evidence that this is actually generic, but that request was not
implemented. If done at the device level, this should be trivial, as the
microchip devices don't actually have support for multiple devices on
the same cs in the drivers, they just parse the property to correctly
support a single device.

> I'm also in favor of making it an array and letting the device-specific
> bindings decide what multiple devices with different addresses on the
> same CS line means.

I kinda wanted it to be in the channels, but I think, on reflection,
that putting it at the device level is probably fine. Pretending
that two devices are one extended device is always going to have some
ickyness about presentation, and putting it in the channels means
defining it again and again and again for each type of device that wants
it.

> But... if we are leaving it up to devices to deal with the property rather
> than the core SPI code, maybe it shouldn't be a standard SPI property.
> Although, I suppose the core SPI code could parse the property and just
> pass that information in the struct spi_device to let the device driver
> do what it wants with it.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2026-07-01 20:31 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-01  6:40 [PATCH v5 0/3] iio: dac: Add support for AD5529R DAC Janani Sunil
2026-07-01  6:40 ` [PATCH v5 1/3] dt-bindings: spi: Add spi,device-addr peripheral property Janani Sunil
2026-07-01  6:52   ` sashiko-bot
2026-07-01 11:04   ` Conor Dooley
2026-07-01 18:29     ` Jonathan Cameron
2026-07-01 18:48       ` David Lechner
2026-07-01 20:31         ` Conor Dooley
2026-07-01  6:40 ` [PATCH v5 2/3] dt-bindings: iio: dac: Add AD5529R Janani Sunil
2026-07-01  6:52   ` sashiko-bot
2026-07-01 11:07   ` Conor Dooley
2026-07-01 18:41   ` Jonathan Cameron
2026-07-01  6:40 ` [PATCH v5 3/3] iio: dac: Add AD5529R DAC driver support Janani Sunil
2026-07-01  6:52   ` sashiko-bot
2026-07-01  9:17   ` Andy Shevchenko
2026-07-01 18:55   ` Jonathan Cameron

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox