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

["Tinaco, Mariel" <Mariel.Tinaco@xxxxxxxxxx>]

> -----Original Message-----
> From: Jonathan Cameron <jic23@xxxxxxxxxx>
> Sent: Sunday, September 8, 2024 1:02 AM
> To: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
> Cc: Tinaco, Mariel <Mariel.Tinaco@xxxxxxxxxx>; linux-iio@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Lars-Peter Clausen
> <lars@xxxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>; Krzysztof Kozlowski
> <krzk+dt@xxxxxxxxxx>; Hennerich, Michael <Michael.Hennerich@xxxxxxxxxx>;
> Conor Dooley <conor+dt@xxxxxxxxxx>; Marcelo Schmitt
> <marcelo.schmitt1@xxxxxxxxx>; Dimitri Fedrau <dima.fedrau@xxxxxxxxx>;
> David Lechner <dlechner@xxxxxxxxxxxx>; Nuno Sá
> <noname.nuno@xxxxxxxxx>
> Subject: Re: [PATCH v3 1/2] dt-bindings: iio: dac: add docs for ad8460
> 
> [External]
> 
> On Wed, 4 Sep 2024 08:20:53 +0200
> Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
> 
> > On Wed, Sep 04, 2024 at 10:30:39AM +0800, Mariel Tinaco wrote:
> > > This adds the bindings documentation for the 14-bit
> >
> > Please do not use "This commit/patch/change", but imperative mood. See
> > longer explanation here:
> > https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v5.17.1/s
> > ource/Documentation/process/submitting-
> patches.rst*L95__;Iw!!A3Ni8CS0y
> > 2Y!7lj0hq-U2ClkGNYfHqjR3-k-
> ea6TFUFsgEYQokkU95K6TXPHIPU33VxQcl_iH_etJ4k
> > pbPEV39dP1oAd$
> >
> > > High Voltage, High Current, Waveform Generator Digital-to-Analog
> > > converter.
> > >
> > > Signed-off-by: Mariel Tinaco <Mariel.Tinaco@xxxxxxxxxx>
> > > ---
> > >  .../bindings/iio/dac/adi,ad8460.yaml          | 154 ++++++++++++++++++
> > >  MAINTAINERS                                   |   7 +
> > >  2 files changed, 161 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml
> >
> > > +  adi,range-microvolt:
> > > +    description: Voltage output range specified as <minimum, maximum>
> > > +    oneOf:
> >
> > This oneOf does not make sense. There is only one condition. Drop.
> >
> > > +      - items:
> > > +          - enum: [0, -10000000, -20000000, -30000000, -40000000, -
> 55000000]
> > > +          - enum: [10000000, 20000000, 30000000, 40000000,
> > > + 55000000]
> >
> > What's the default? It's not a required property.
> >
> > > +
> > > +  adi,range-microamp:
> > > +    description: Current output range specified as <minimum, maximum>
> > > +    oneOf:
> > > +      - items:
> > > +          - enum: [-50000, -100000, -300000, -500000, -1000000]
> >
> > I don't understand why 0 is not listed here.
> 
> I'm not sure why it is a list at all. Seems like the hardware allows a continuous
> value so this should just specify max and min.
> 

That's right, the values can be flexible but only at a certain range. 
The first element of the array should only be in the negative range, while
The second element of the array should only be in the positive range.

Is there a way to do this with the max and min attribute?

Items:
	Item 1
		min: -10000
		max: 0
	item 2
		min: 0
		max: 10000 

> >
> > > +          - enum: [50000, 100000, 300000, 500000, 1000000]
> > > +      - items:
> > > +          - const: 0
> > > +          - enum: [50000, 100000, 300000, 500000, 1000000]
> > > +
> >
> > What's the default? It's not a required property.
> >
> > > +  adi,max-millicelsius:
> > > +    description: Overtemperature threshold
> > > +    default: 50000
> > > +    minimum: 20000
> > > +    maximum: 150000
> > > +
> > > +  shutdown-reset-gpios:
> > > +    description: Corresponds to SDN_RESET pin. To exit shutdown
> > > +      or sleep mode, pulse SDN_RESET HIGH, then leave LOW.
> > > +    maxItems: 1
> > > +
> > > +  reset-gpios:
> > > +    description: Manual Power On Reset (POR). Pull this GPIO pin
> > > +      LOW and then HIGH to reset all digital registers to default
> > > +    maxItems: 1
> > > +
> > > +  shutdown-gpios:
> > > +    description: Corresponds to SDN_IO pin. Shutdown may be
> > > +      initiated by the user, by pulsing SDN_IO high. To exit shutdown,
> > > +      pulse SDN_IO low, then float.
> > > +    maxItems: 1
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - clocks
> >
> > Some supplies are for sure required. Devices rarely can operate
> > without power provided.
> >
> > > +
> > > +allOf:
> > > +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > > +
> > > +additionalProperties: false
> >
> > unevaluatedProperties instead.
> >
> > Best regards,
> > Krzysztof
> >