Re: [PATCH 2/2] dt-bindings: iio: Add ltc2983 documentation

["Sa, Nuno" <Nuno.Sa@analog.com>]

On Mon, 2019-09-16 at 20:09 -0500, Rob Herring wrote:
> 
> On Mon, Sep 16, 2019 at 10:20 AM Sa, Nuno <Nuno.Sa@analog.com> wrote:
> > Hi Rob and Jonathan,
> > 
> > Some comments/questions inline.
> > 
> > Nuno Sá
> > 
> > On Sun, 2019-09-15 at 12:07 +0100, Jonathan Cameron wrote:
> > > On Fri, 13 Sep 2019 15:36:21 +0100
> > > Rob Herring <robh@kernel.org> wrote:
> > > 
> > > > On Mon, Sep 09, 2019 at 04:45:50PM +0200, Nuno Sá wrote:
> > > > > Document the LTC2983 temperature sensor devicetree bindings.
> > > > > 
> > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> > > > > ---
> > > > >  .../bindings/iio/temperature/adi,ltc2983.yaml | 442
> > > > > ++++++++++++++++++
> > > > >  MAINTAINERS                                   |   1 +
> > > > >  2 files changed, 443 insertions(+)
> > > > >  create mode 100644
> > > > > Documentation/devicetree/bindings/iio/temperature/adi,ltc2983
> > > > > .yam
> > > > > l
> > > > > 
> > > > > diff --git
> > > > > a/Documentation/devicetree/bindings/iio/temperature/adi,ltc29
> > > > > 83.y
> > > > > aml
> > > > > b/Documentation/devicetree/bindings/iio/temperature/adi,ltc29
> > > > > 83.y
> > > > > aml
> > > > > new file mode 100644
> > > > > index 000000000000..2b468b3ed177
> > > > > --- /dev/null
> > > > > +++
> > > > > b/Documentation/devicetree/bindings/iio/temperature/adi,ltc29
> > > > > 83.y
> > > > > aml
> > > > > @@ -0,0 +1,442 @@
> > > > > +# SPDX-License-Identifier: GPL-2.0
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id:
> > > > > http://devicetree.org/schemas/iio/temperature/adi,ltc2983.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: Analog Devices LTC2983 Multi-sensor Temperature
> > > > > system
> > > > > +
> > > > > +maintainers:
> > > > > +  - Nuno Sá <nuno.sa@analog.com>
> > > > > +
> > > > > +description: |
> > > > > +  Analog Devices LTC2983 Multi-Sensor Digital Temperature
> > > > > Measurement System
> > > > > +
> > > > > https://www.analog.com/media/en/technical-documentation/data-sheets/2983fc.pdf
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    enum:
> > > > > +      - adi,ltc2983
> > > > > +
> > > > > +  reg:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  interrupts:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  adi,temperature-celcius:
> > > > > +    description:
> > > > > +      If this property is present, the temperature is
> > > > > reported
> > > > > in Celsius.
> > > > > +    type: boolean
> > > > > +    maxItems: 1
> > > > 
> > > > It's a boolean, not an array so 'maxItems' doesn't make sense.
> > > > 
> > > > Running 'make dt_binding_check' should tell you this. You may
> > > > need
> > > > to
> > > > update dt-schema install though.
> > 
> > Rob, I'm having some issues with `make dt_binding_check`. I updated
> > dt-
> > schema and I get this when run it:
> > 
> > ...
> > "ruamel.yaml.constructor.DuplicateKeyError: while constructing a
> > mapping
> >   in "<unicode string>", line 4, column 1
> > found duplicate key "patternProperties" with value "{}" (original
> > value: "{}")
> >   in "<unicode string>", line 113, column 1"
> 
> Simply drop all but the first 'patternProperties'. You can have
> multiple patterns under one.
> 
Ok, got it.
> > If you want, I can paste the complete traceback in a following
> > email.
> > However I could use `dt-doc-validate
> > Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml`
> > directly by doing a manual change in `dt-doc-validate `. I changed
> > the
> > call `testtree = dtschema.load(filename, line_number=line_number,
> > duplicate_keys=False)` to `testtree = dtschema.load(filename,
> > line_number=line_number, duplicate_keys=True)`. Is this something
> > already known? I would not be surprised if it is some problem in my
> > environment. However, I even tried this in a clean docker container
> > based on ubuntu 18.04 and got the same behavior.
> 
> [...]
> 
> > > > > +patternProperties:
> > > > > +  "^rtd@([2-9]|1[0-9]|20)$":
> > > > > +    type: object
> > > > > +    description: Represents a rtd sensor which is connected
> > > > > to
> > > > > one of the device channels.
> > > > > +
> > > > > +    properties:
> > > > > +      reg:
> > > > > +        description: |
> > > > > +          The channel number. It can be connected to one of
> > > > > the
> > > > > 20 channels of the device.
> > > > > +        minimum: 2
> > > > > +        maximum: 20
> > > > > +        maxItems: 1
> > > > 
> > > > As this is pretty much the same for all child nodes, make a
> > > > pattern
> > > > that
> > > > matches all child nodes and put this there rather than
> > > > duplicating
> > > > it.
> > > > Then you only need 'minimum: 2' in the cases needing that.
> > 
> > I'm not sure I'm following your point here. So it's better to
> > clarify
> > it before sending a v2. Do you mean to add something like:
> > 
> > patternProperties:
> >   "^(thermocouple|diode|rtd|thermistor|adc|rsense)@([1-9]|1[0-
> > 9]|20)$"
> 
> Just ".*@([1-9]|1[0-9]|20)$" is fine.
> 
> >     type: object
> > 
> >     properties:
> >       reg:
> >        description: |
> >          The channel number. It can be connected to one of the 20
> > channels of the device.
> >        minimum: 1
> >        maximum: 20
> > 
> > And then, for instance, for a RTD I would have:
> > 
> > patternProperties:
> >   "^rtd@([2-9]|1[0-9]|20)$"
> 
> You've already defined the unit-address format above, so '^rtd@.*'
> would be sufficient here.

Ack.

> >     ...
> > 
> >     properties:
> >       reg:
> >        minimum: 2
> > 
> >     ...
> > 
> > Would this also make sense, or it's not really necessary?
> 
> Yes, makes sense.
> 
> > patternProperties:
> >   "^thermocouple@([1-9]|1[0-9]|20)$"
> >     type: object
> > 
> >     ...
> > 
> >     properties:
> >       reg:
> >        description: For differential thermocouples, the minimum is
> > 2.
> 
> Why do you have a constraint in free form text here?

There are sensors (like thermocouples) which can be configured as
differential or single-ended. Depending on that the 'reg' minimum value
is 1 or 2. The text was only giving a *note* on that. However I guess I
can just drop it.
 
> >     ...
> > 
> > Am I understanding it correctly?
> > > > > +thermistor
> > > > > +      adi,sensor-type:
> > > > > +        description: |
> > > > > +          Identifies the type of RTD connected to the
> > > > > device.
> > > > > +        allOf:
> > > > > +          - $ref: /schemas/types.yaml#/definitions/uint32
> > > > > +          - enum: [10 11 12 13 14 15 16 17]
> > > > > +        maxItems: 1
> > > > > +
> > > > > +      adi,rsense-handle:
> > > > > +        description: |
> > > > > +          Phandle pointing to a rsense object associated
> > > > > with
> > > > > this RTD.
> > > > > +        $ref: "/schemas/types.yaml#/definitions/phandle"
> > > > > +        maxItems: 1
> > > > > +
> > > > > +      adi,sensor-config:
> > > > > +        description: |
> > > > > +          Raw value which set's the sensor configuration.
> > > > > Look
> > > > > at table 28 of the
> > > > > +          datasheet for how to set this value for RTD's.
> > > > > +        allOf:
> > > > > +          - $ref: /schemas/types.yaml#/definitions/uint32
> > > > > +          - enum: [0 1 4 5 8 9 10 12 13 14]
> > > > > +        maxItems: 1
> > > > > +
> > > > > +      adi,excitation-current:
> > > > > +        description: |
> > > > > +          This property controls the magnitude of the
> > > > > excitation
> > > > > current applied
> > > > > +          to the RTD. Look at table 29 of the datasheet for
> > > > > more
> > > > > info.
> > > 
> > > Any way we can make this real units?  Can list valid value here.
> > 
> > For RTD's and diodes, it is possible to have it with real units.
> > However, for thermistors it's not really doable since, for
> > instance,
> > for them we have an "Auto Range" setting. So, I just wanted to be
> > consistent through all sensors having excitation-current
> > configuration.
> > Do you prefer to have it in real units where possible?
> 
> That's the preference if it makes sense. I have no idea what an RTD
> is
> to comment further.

Ack. I will come up with some proposal for this on v2.

> Rob