Re: [PATCH v2 1/1] dt-bindings: iio: dds: Add AD9832/AD9835 binding

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The AD9832 and AD9835 require SPI Mode 2 (CPOL=1, CPHA=0), but the binding does not enforce `spi-cpol` and the example omits it.
- [High] The driver will unconditionally fail to probe when instantiated via Device Tree because it strictly requires `platform_data`.
- [Low] The example uses a specific part number for the node name (`ad9832@0`) rather than a generic functional name.
--

commit 0760129b4a1ce54473ebad28ebda22535e9eedd6
Author: Hungyu Lin <dennylin0707@gmail.com>

dt-bindings: iio: dds: Add AD9832/AD9835 binding

This commit adds device tree binding documentation for the Analog Devices
AD9832 and AD9835 SPI-controlled direct digital synthesizers.

Since the binding introduces device tree support, does the driver need
updates to actually probe successfully when instantiated this way?

Looking at ad9832_probe(), it unconditionally returns -ENODEV if
dev_get_platdata() is NULL:

drivers/iio/dds/ad9832.c:ad9832_probe() {
    struct ad9832_platform_data *pdata = dev_get_platdata(&spi->dev);
    
    if (!pdata) {
        dev_dbg(&spi->dev, "no platform data?\n");
        return -ENODEV;
    }
    ...
}

When instantiated via device tree, platform data is typically NULL. Will
the driver fail to probe without additional patches to handle missing
platform data?

> diff --git a/Documentation/devicetree/bindings/iio/dds/adi,ad9832.yaml b/Documentation/devicetree/bindings/iio/dds/adi,ad9832.yaml
> new file mode 100644
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/dds/adi,ad9832.yaml
[ ... ]
> properties:
>   compatible:
>     enum:
>       - adi,ad9832
>       - adi,ad9835
> 
>   reg:
>     maxItems: 1
> 
>   spi-max-frequency:
>     maximum: 20000000

Does this binding need to specify spi-cpol? 

The AD9832 hardware requires SPI Mode 2 (CPOL=1, CPHA=0) where the SPI
clock idles high. Since the driver doesn't set spi->mode in code and
relies on the device tree to configure polarity, omitting it might cause
the controller to default to Mode 0, which would cause communication to
fail.

Should spi-cpol: true be added to the properties and included in the
example?

[ ... ]
>     spi {
>       #address-cells = <1>;
>       #size-cells = <0>;
> 
>       ad9832@0 {

This isn't a bug, but does this violate the device tree convention for
generic node names? Usually, node names describe the hardware's function
rather than the specific part number, such as generator@0 or frequency@0.

>         compatible = "adi,ad9832";
>         reg = <0>;
>         spi-max-frequency = <20000000>;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260509002810.78372-2-dennylin0707@gmail.com?part=1