Re: [PATCH RFC v4 01/10] dt-bindings: iio: frequency: add ad9910

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The binding defines `#clock-cells` indicating clock provider capabilities but fails to include it in the `required` list and the `examples` block.
- [Medium] The `lock-detect-gpios` dependency is written with an unquoted comma (`[ adi,pll-enable ]`), causing it to evaluate to two non-existent properties.
--

commit 02fb206b49b457eba593a7f4a35e17f11545fbf5
Author: Rodrigo Alencar <rodrigo.alencar@analog.com>

dt-bindings: iio: frequency: add ad9910

DT-bindings for AD9910, a 1 GSPS DDS with 14-bit DAC. It includes
configurations for clocks, DAC current, reset and basic GPIO control.

> diff --git a/Documentation/devicetree/bindings/iio/frequency/adi,ad9910.yaml b/Documentation/devicetree/bindings/iio/frequency/adi,ad9910.yaml
> new file mode 100644
> index 0000000000000..3b76871630c90
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/frequency/adi,ad9910.yaml

[ ... ]

> +dependencies:
> +  adi,charge-pump-current-microamp: [ 'adi,pll-enable' ]
> +  adi,refclk-out-drive-strength: [ 'adi,pll-enable' ]
> +  lock-detect-gpios: [ adi,pll-enable ]

Does this dependency evaluate to two non-existent properties instead of
"adi,pll-enable"? In YAML flow collections, an unquoted comma acts as an
element separator, so this gets parsed as a list containing "adi" and
"pll-enable". 

Since neither property exists, this could cause device trees using
lock-detect-gpios to fail schema validation.

Could we enclose the property name in quotes (e.g., [ 'adi,pll-enable' ])
like the other dependencies?

> +  interrupts: [ interrupt-names ]
> +  clocks: [ clock-names ]
> +  '#clock-cells': [ clock-output-names ]
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - dvdd-io33-supply
> +  - avdd33-supply
> +  - dvdd18-supply
> +  - avdd18-supply

Since the binding defines #clock-cells, indicating clock provider
capabilities, shouldn't it be included in this required list?

According to the dt-bindings guidelines, when hardware has provider
capabilities, the corresponding standard properties must be explicitly
required to ensure schema validation catches incomplete device tree nodes
before runtime.

[ ... ]

> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        dds@0 {
> +            compatible = "adi,ad9910";
> +            reg = <0>;
> +            spi-max-frequency = <1000000>;
> +            clocks = <&ad9910_refclk>;
> +            clock-names = "ref_clk";

[ ... ]

> +            adi,pll-enable;
> +            adi,charge-pump-current-microamp = <387>;
> +            adi,refclk-out-drive-strength = "disabled";

Should #clock-cells also be added to this example block?

The dt-bindings guidelines state that all required properties must be
included in the examples section to pass schema validation.

> +        };
> +    };
> +...

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508-ad9910-iio-driver-v4-0-d26bfd20ee3d@analog.com?part=1