Re: [PATCH v2 1/2] dt-bindings: iio: dac: add docs for ad8460

[Krzysztof Kozlowski <krzk@kernel.org>]

On 30/07/2024 05:05, Mariel Tinaco wrote:
> This adds the bindings documentation for the 14-bit
> High Voltage, High Current, Waveform Generator
> Digital-to-Analog converter.
> 
> Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com>

> +
> +  refio-1p2v-supply:
> +    description: Drive voltage in the range of 1.2V maximum to as low as
> +      low as 0.12V through the REF_IO pin to adjust full scale output span
> +
> +  clocks:

maxItems: 1
and drop description (or use items: - description, but then do not
repeat redundant parts)

> +    description: The clock for the DAC. This is the sync clock
> +
> +  adi,rset-ohms:
> +    description: Specify value of external resistor connected to FS_ADJ pin
> +      to establish internal HVDAC's reference current I_REF
> +    default: 2000
> +    minimum: 2000
> +    maximum: 20000
> +
> +  adi,range-microvolt:
> +    description: |
> +      Voltage output range specified as <minimum, maximum>
> +    oneOf:

Not an oneOf.

> +      - items:
> +          - const: -40000000
> +          - const: 40000000

Why do you need this property if this cannot be anything else? Drop.

> +
> +  adi,range-microamp:
> +    description: |

Do not need '|' unless you need to preserve formatting.

> +      Current output range specified as <minimum, maximum>
> +    oneOf:
> +      - items:
> +          - const: 0
> +          - const: 50000
> +      - items:
> +          - const: -50000
> +          - const: 50000
> +
> +  adi,temp-max-millicelsius:
> +    description: Overtemperature threshold
> +    default: 50000
> +    minimum: 20000
> +    maximum: 150000
> +
> +  sdn-reset-gpios:

reset-gpios or shutdown-gpios or anything from gpio-consumer-common
which is not deprecated.

> +    description: GPIO spec for the SHUTDOWN RESET pin. As the line is active high,

Do not repeat the obvious or redundant parts. There is no point in
saying that "GPIO spec is a GPIO spec for ...". It cannot be anything
else than GPIO spec. Instead say something useful. It's confusing to
have two reset pins, so explain what is the purpose of this pin.

> +      it should be marked GPIO_ACTIVE_HIGH.

Drop last part "it should be marked", because it is clearly incorrect.
Different board designs can have it differently.


> +    maxItems: 1
> +
> +  reset-gpios:
> +    description: GPIO spec for the RESET pin. As the line is active low, it
> +      should be marked GPIO_ACTIVE_LOW.

Again, marking it always as active low is not correct. It is enough to
say that line is active low.

> +    maxItems: 1
> +
> +  sdn-io-gpios:
> +    description: GPIO spec for the SHUTDOWN INPUT/OUTPUT pin. As the line is
> +      active high, it should be marked GPIO_ACTIVE_HIGH.

What's the purpose?

> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        dac@0 {
> +            compatible = "adi,ad8460";
> +            reg = <0>;
> +            spi-max-frequency = <8000000>;
> +            adi,rset-ohms = <2000>;
> +            adi,range-microvolt = <(-40000000) 40000000>;
> +            adi,range-microamp = <0 50000>;
> +            adi,temp-max-millicelsius = <50000>;

Custom properties go to the end. See DTS coding style.

> +
> +            dmas = <&tx_dma 0>;
> +            dma-names = "tx";


Best regards,
Krzysztof