Re: [PATCH v5 2/3] dt-bindings: iio: dac: Add AD5529R

[sashiko-bot@kernel.org]

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