[2/3] dt-bindings: iio: temperature: ltc2983: support more parts

  From: Cosmin Tanislav <cosmin.tanislav@analog.com>

Add support for the following parts:
 * LTC2984
 * LTC2986
 * LTM2985

The LTC2984 is a variant of the LTC2983 with EEPROM.
The LTC2986 is a variant of the LTC2983 with only 10 channels,
EEPROM and support for active analog temperature sensors.
The LTM2985 is software-compatible with the LTC2986.

Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
---
 .../bindings/iio/temperature/adi,ltc2983.yaml | 63 +++++++++++++++++--
 1 file changed, 59 insertions(+), 4 deletions(-)
  

On 14/10/2022 08:37, Cosmin Tanislav wrote:
> From: Cosmin Tanislav <cosmin.tanislav@analog.com>
> 
> Add support for the following parts:
>  * LTC2984
>  * LTC2986
>  * LTM2985
> 
> The LTC2984 is a variant of the LTC2983 with EEPROM.
> The LTC2986 is a variant of the LTC2983 with only 10 channels,
> EEPROM and support for active analog temperature sensors.
> The LTM2985 is software-compatible with the LTC2986.
> 
> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
> ---
>  .../bindings/iio/temperature/adi,ltc2983.yaml | 63 +++++++++++++++++--
>  1 file changed, 59 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> index 722781aa4697..c33ab524fb64 100644
> --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> @@ -4,19 +4,27 @@
>  $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
> +title: Analog Devices LTC2983, LTC2986, LTM2985 Multi-sensor Temperature system
>  
>  maintainers:
>    - Nuno Sá <nuno.sa@analog.com>
>  
>  description: |
> -  Analog Devices LTC2983 Multi-Sensor Digital Temperature Measurement System
> +  Analog Devices LTC2983, LTC2984, LTC2986, LTM2985 Multi-Sensor Digital
> +  Temperature Measurement Systems
> +
>    https://www.analog.com/media/en/technical-documentation/data-sheets/2983fc.pdf
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/2984fb.pdf
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/29861fa.pdf
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ltm2985.pdf
>  
>  properties:
>    compatible:
>      enum:
>        - adi,ltc2983
> +      - adi,ltc2984
> +      - adi,ltc2986
> +      - adi,ltm2985
>  
>    reg:
>      maxItems: 1
> @@ -26,7 +34,7 @@ properties:
>  
>    adi,mux-delay-config-us:
>      description:
> -      The LTC2983 performs 2 or 3 internal conversion cycles per temperature
> +      The device performs 2 or 3 internal conversion cycles per temperature
>        result. Each conversion cycle is performed with different excitation and
>        input multiplexer configurations. Prior to each conversion, these
>        excitation circuits and input switch configurations are changed and an
> @@ -145,7 +153,7 @@ patternProperties:
>        adi,three-conversion-cycles:
>          description:
>            Boolean property which set's three conversion cycles removing
> -          parasitic resistance effects between the LTC2983 and the diode.
> +          parasitic resistance effects between the device and the diode.
>          type: boolean
>  
>        adi,average-on:
> @@ -353,6 +361,41 @@ patternProperties:
>          description: Boolean property which set's the adc as single-ended.
>          type: boolean
>  
> +  "^temp@":

There is already a property for thermocouple. Isn't a thermocouple a
temperature sensor? IOW, why new property is needed?

> +    type: object
> +    description:
> +      Represents a channel which is being used as an active analog temperature
> +      sensor.
> +
> +    properties:
> +      adi,sensor-type:
> +        description:
> +          Identifies the sensor as an active analog temperature sensor.
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        const: 31
> +
> +      adi,single-ended:
> +        description: Boolean property which sets the sensor as single-ended.

Drop "Boolean property which sets" - it's obvious from the type.



> +        type: boolean
> +
> +      adi,custom-temp:
> +        description:
> +          This is a table, where each entry should be a pair of

"This is a table" - obvious from the type.

> +          voltage(mv)-temperature(K). The entries must be given in nv and uK

mv-K or nv-uK? Confusing...

> +          so that, the original values must be multiplied by 1000000. For
> +          more details look at table 71 and 72.

There is no table 71 in the bindings... It seems you pasted it from
somewhere.

> +          Note should be signed, but dtc doesn't currently maintain the
> +          sign.

What do you mean? "Maintain" as allow or keep when building FDT?  What's
the problem of using negative numbers here and why it should be part of
bindings?

> +        $ref: /schemas/types.yaml#/definitions/uint64-matrix
> +        minItems: 3
> +        maxItems: 64
> +        items:
> +          minItems: 2
> +          maxItems: 2

Instead describe the items with "description" (and maybe constraints)
like here:

https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.yaml#L278

> +
> +    required:
> +      - adi,custom-temp
> +
>    "^rsense@":
>      type: object
>      description:
> @@ -382,6 +425,18 @@ required:
>    - reg
>    - interrupts
>  
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - adi,ltc2983
> +              - adi,ltc2984
> +    then:
> +      patternProperties:
> +        "^temp@": false
> +
>  additionalProperties: false
>  
>  examples:

Best regards,
Krzysztof
  

On 10/17/22 04:59, Krzysztof Kozlowski wrote:
> On 14/10/2022 08:37, Cosmin Tanislav wrote:
>> From: Cosmin Tanislav <cosmin.tanislav@analog.com>
>>
>> Add support for the following parts:
>>   * LTC2984
>>   * LTC2986
>>   * LTM2985
>>
>> The LTC2984 is a variant of the LTC2983 with EEPROM.
>> The LTC2986 is a variant of the LTC2983 with only 10 channels,
>> EEPROM and support for active analog temperature sensors.
>> The LTM2985 is software-compatible with the LTC2986.
>>
>> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
>> ---
>>   .../bindings/iio/temperature/adi,ltc2983.yaml | 63 +++++++++++++++++--
>>   1 file changed, 59 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
>> index 722781aa4697..c33ab524fb64 100644
>> --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
>> +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
>> @@ -4,19 +4,27 @@
>>   $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
>> +title: Analog Devices LTC2983, LTC2986, LTM2985 Multi-sensor Temperature system
>>   
>>   maintainers:
>>     - Nuno Sá <nuno.sa@analog.com>
>>   
>>   description: |
>> -  Analog Devices LTC2983 Multi-Sensor Digital Temperature Measurement System
>> +  Analog Devices LTC2983, LTC2984, LTC2986, LTM2985 Multi-Sensor Digital
>> +  Temperature Measurement Systems
>> +
>>     https://www.analog.com/media/en/technical-documentation/data-sheets/2983fc.pdf
>> +  https://www.analog.com/media/en/technical-documentation/data-sheets/2984fb.pdf
>> +  https://www.analog.com/media/en/technical-documentation/data-sheets/29861fa.pdf
>> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ltm2985.pdf
>>   
>>   properties:
>>     compatible:
>>       enum:
>>         - adi,ltc2983
>> +      - adi,ltc2984
>> +      - adi,ltc2986
>> +      - adi,ltm2985
>>   
>>     reg:
>>       maxItems: 1
>> @@ -26,7 +34,7 @@ properties:
>>   
>>     adi,mux-delay-config-us:
>>       description:
>> -      The LTC2983 performs 2 or 3 internal conversion cycles per temperature
>> +      The device performs 2 or 3 internal conversion cycles per temperature
>>         result. Each conversion cycle is performed with different excitation and
>>         input multiplexer configurations. Prior to each conversion, these
>>         excitation circuits and input switch configurations are changed and an
>> @@ -145,7 +153,7 @@ patternProperties:
>>         adi,three-conversion-cycles:
>>           description:
>>             Boolean property which set's three conversion cycles removing
>> -          parasitic resistance effects between the LTC2983 and the diode.
>> +          parasitic resistance effects between the device and the diode.
>>           type: boolean
>>   
>>         adi,average-on:
>> @@ -353,6 +361,41 @@ patternProperties:
>>           description: Boolean property which set's the adc as single-ended.
>>           type: boolean
>>   
>> +  "^temp@":
> 
> There is already a property for thermocouple. Isn't a thermocouple a
> temperature sensor? IOW, why new property is needed?
> 
This node is needed for active analog temperature sensors.
It has fewer options than the thermocouple, as it only supports
a table to map from voltage to temperature and specifying whether
the measurement is differential or single-ended.

If you did as much as glimpsed at the datasheet you would have
understood.

>> +    type: object
>> +    description:
>> +      Represents a channel which is being used as an active analog temperature
>> +      sensor.
>> +
>> +    properties:
>> +      adi,sensor-type:
>> +        description:
>> +          Identifies the sensor as an active analog temperature sensor.
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +        const: 31
>> +
>> +      adi,single-ended:
>> +        description: Boolean property which sets the sensor as single-ended.
> 
> Drop "Boolean property which sets" - it's obvious from the type.
> 

That's how the rest of the file is written.

> 
> 
>> +        type: boolean
>> +
>> +      adi,custom-temp:
>> +        description:
>> +          This is a table, where each entry should be a pair of
> 
> "This is a table" - obvious from the type.
> 

That's how the rest of the file is written.

>> +          voltage(mv)-temperature(K). The entries must be given in nv and uK
> 
> mv-K or nv-uK? Confusing...

That's how the rest of the file is written.

The chip uses mv-K, but the binding specifies nv-uK, the driver
translates it into the appropriate unit.

> 
>> +          so that, the original values must be multiplied by 1000000. For
>> +          more details look at table 71 and 72.
> 
> There is no table 71 in the bindings... It seems you pasted it from
> somewhere.
> 

It's pretty obvious that "Table" in a binding refers to the datasheet.
But if you meant datasheet, not binding, you're looking at the wrong
datasheet then.
Also, that's how the rest of the file is written.

>> +          Note should be signed, but dtc doesn't currently maintain the
>> +          sign.
> 
> What do you mean? "Maintain" as allow or keep when building FDT?  What's
> the problem of using negative numbers here and why it should be part of
> bindings?
> 

You're really clueless, I'll let you figure it out on your own.
Also, that's how the rest of the file is written.

>> +        $ref: /schemas/types.yaml#/definitions/uint64-matrix
>> +        minItems: 3
>> +        maxItems: 64
>> +        items:
>> +          minItems: 2
>> +          maxItems: 2
> 
> Instead describe the items with "description" (and maybe constraints)
> like here:
> 
> https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.yaml#L278
> 

That's how the rest of the file is written.
If you really want to use something different, you can submit a
patch later and fix the whole binding however you want.

>> +
>> +    required:
>> +      - adi,custom-temp
>> +
>>     "^rsense@":
>>       type: object
>>       description:
>> @@ -382,6 +425,18 @@ required:
>>     - reg
>>     - interrupts
>>   
>> +allOf:
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - adi,ltc2983
>> +              - adi,ltc2984
>> +    then:
>> +      patternProperties:
>> +        "^temp@": false
>> +
>>   additionalProperties: false
>>   
>>   examples:
> 
> Best regards,
> Krzysztof
>
  

On 10/14/22 18:37, Jonathan Cameron wrote:
> On Fri, 14 Oct 2022 15:37:23 +0300
> Cosmin Tanislav <demonsingur@gmail.com> wrote:
> 
>> From: Cosmin Tanislav <cosmin.tanislav@analog.com>
>>
>> Add support for the following parts:
>>   * LTC2984
>>   * LTC2986
>>   * LTM2985
>>
>> The LTC2984 is a variant of the LTC2983 with EEPROM.
>> The LTC2986 is a variant of the LTC2983 with only 10 channels,
>> EEPROM and support for active analog temperature sensors.
> 
> If they 'work' but with fewer features, perhaps a fallback
> compatible?
> 

10 channels vs 20 channels. Using ltc2983 compatible as fallback
would allow you to have 10 non-functional channels specified in the
dts.

>> The LTM2985 is software-compatible with the LTC2986.
> 
> Fallback compatible?
> 
>>
>> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
>> ---
>>   .../bindings/iio/temperature/adi,ltc2983.yaml | 63 +++++++++++++++++--
>>   1 file changed, 59 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
>> index 722781aa4697..c33ab524fb64 100644
>> --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
>> +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
>> @@ -4,19 +4,27 @@
>>   $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
>> +title: Analog Devices LTC2983, LTC2986, LTM2985 Multi-sensor Temperature system
>>   
>>   maintainers:
>>     - Nuno Sá <nuno.sa@analog.com>
>>   
>>   description: |
>> -  Analog Devices LTC2983 Multi-Sensor Digital Temperature Measurement System
>> +  Analog Devices LTC2983, LTC2984, LTC2986, LTM2985 Multi-Sensor Digital
>> +  Temperature Measurement Systems
>> +
>>     https://www.analog.com/media/en/technical-documentation/data-sheets/2983fc.pdf
>> +  https://www.analog.com/media/en/technical-documentation/data-sheets/2984fb.pdf
>> +  https://www.analog.com/media/en/technical-documentation/data-sheets/29861fa.pdf
>> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ltm2985.pdf
>>   
>>   properties:
>>     compatible:
>>       enum:
>>         - adi,ltc2983
>> +      - adi,ltc2984
>> +      - adi,ltc2986
>> +      - adi,ltm2985
>>   
>>     reg:
>>       maxItems: 1
>> @@ -26,7 +34,7 @@ properties:
>>   
>>     adi,mux-delay-config-us:
>>       description:
>> -      The LTC2983 performs 2 or 3 internal conversion cycles per temperature
>> +      The device performs 2 or 3 internal conversion cycles per temperature
> 
> Definitely a lesson here in avoiding device names in the descriptions!
> 
>>         result. Each conversion cycle is performed with different excitation and
>>         input multiplexer configurations. Prior to each conversion, these
>>         excitation circuits and input switch configurations are changed and an
>> @@ -145,7 +153,7 @@ patternProperties:
>>         adi,three-conversion-cycles:
>>           description:
>>             Boolean property which set's three conversion cycles removing
>> -          parasitic resistance effects between the LTC2983 and the diode.
>> +          parasitic resistance effects between the device and the diode.
>>           type: boolean
>>   
>>         adi,average-on:
>> @@ -353,6 +361,41 @@ patternProperties:
>>           description: Boolean property which set's the adc as single-ended.
>>           type: boolean
>>   
>> +  "^temp@":
>> +    type: object
>> +    description:
>> +      Represents a channel which is being used as an active analog temperature
>> +      sensor.
>> +
>> +    properties:
>> +      adi,sensor-type:
>> +        description:
>> +          Identifies the sensor as an active analog temperature sensor.
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +        const: 31
> 
> Ah. This is a bit odd as it's fixed for the channel type.  However there
> is precedence in this binding so fair enough.
> 

I think it

>> +
>> +      adi,single-ended:
>> +        description: Boolean property which sets the sensor as single-ended.
>> +        type: boolean
>> +
>> +      adi,custom-temp:
>> +        description:
>> +          This is a table, where each entry should be a pair of
>> +          voltage(mv)-temperature(K). The entries must be given in nv and uK
>> +          so that, the original values must be multiplied by 1000000. For
>> +          more details look at table 71 and 72.
>> +          Note should be signed, but dtc doesn't currently maintain the
>> +          sign.
>> +        $ref: /schemas/types.yaml#/definitions/uint64-matrix
>> +        minItems: 3
>> +        maxItems: 64
>> +        items:
>> +          minItems: 2
>> +          maxItems: 2
>> +
>> +    required:
>> +      - adi,custom-temp
>> +
>>     "^rsense@":
>>       type: object
>>       description:
>> @@ -382,6 +425,18 @@ required:
>>     - reg
>>     - interrupts
>>   
>> +allOf:
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - adi,ltc2983
>> +              - adi,ltc2984
>> +    then:
>> +      patternProperties:
>> +        "^temp@": false
>> +
>>   additionalProperties: false
>>   
>>   examples:
>
  

Hi Krzysztof,

As I wrote the original bindings, let me reply to some of your
points...

On Sun, 2022-10-16 at 21:59 -0400, Krzysztof Kozlowski wrote:
> On 14/10/2022 08:37, Cosmin Tanislav wrote:
> > From: Cosmin Tanislav <cosmin.tanislav@analog.com>
> > 
> > Add support for the following parts:
> >  * LTC2984
> >  * LTC2986
> >  * LTM2985
> > 
> > The LTC2984 is a variant of the LTC2983 with EEPROM.
> > The LTC2986 is a variant of the LTC2983 with only 10 channels,
> > EEPROM and support for active analog temperature sensors.
> > The LTM2985 is software-compatible with the LTC2986.
> > 
> > Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
> > ---
> >  .../bindings/iio/temperature/adi,ltc2983.yaml | 63
> > +++++++++++++++++--
> >  1 file changed, 59 insertions(+), 4 deletions(-)
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yam
> > l
> > b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yam
> > l
> > index 722781aa4697..c33ab524fb64 100644
> > ---
> > a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yam
> > l
> > +++
> > b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yam
> > l
> > @@ -4,19 +4,27 @@
> >  $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
> > +title: Analog Devices LTC2983, LTC2986, LTM2985 Multi-sensor
> > Temperature system
> >  
> >  maintainers:
> >    - Nuno Sá <nuno.sa@analog.com>
> >  
> >  description: |
> > -  Analog Devices LTC2983 Multi-Sensor Digital Temperature
> > Measurement System
> > +  Analog Devices LTC2983, LTC2984, LTC2986, LTM2985 Multi-Sensor
> > Digital
> > +  Temperature Measurement Systems
> > +
> >   
> > https://www.analog.com/media/en/technical-documentation/data-sheets/2983fc.pdf
> > + 
> > https://www.analog.com/media/en/technical-documentation/data-sheets/2984fb.pdf
> > + 
> > https://www.analog.com/media/en/technical-documentation/data-sheets/29861fa.pdf
> > + 
> > https://www.analog.com/media/en/technical-documentation/data-sheets/ltm2985.pdf
> >  
> >  properties:
> >    compatible:
> >      enum:
> >        - adi,ltc2983
> > +      - adi,ltc2984
> > +      - adi,ltc2986
> > +      - adi,ltm2985
> >  
> >    reg:
> >      maxItems: 1
> > @@ -26,7 +34,7 @@ properties:
> >  
> >    adi,mux-delay-config-us:
> >      description:
> > -      The LTC2983 performs 2 or 3 internal conversion cycles per
> > temperature
> > +      The device performs 2 or 3 internal conversion cycles per
> > temperature
> >        result. Each conversion cycle is performed with different
> > excitation and
> >        input multiplexer configurations. Prior to each conversion,
> > these
> >        excitation circuits and input switch configurations are
> > changed and an
> > @@ -145,7 +153,7 @@ patternProperties:
> >        adi,three-conversion-cycles:
> >          description:
> >            Boolean property which set's three conversion cycles
> > removing
> > -          parasitic resistance effects between the LTC2983 and the
> > diode.
> > +          parasitic resistance effects between the device and the
> > diode.
> >          type: boolean
> >  
> >        adi,average-on:
> > @@ -353,6 +361,41 @@ patternProperties:
> >          description: Boolean property which set's the adc as
> > single-ended.
> >          type: boolean
> >  
> > +  "^temp@":
> 
> There is already a property for thermocouple. Isn't a thermocouple a
> temperature sensor? IOW, why new property is needed?
> 

Well, most of the patternProps in this bindings are temperature
sensors... It's just that the device(s) support different types of
them. 'adi,sensor-type' is used to identify each sensor (as this
translates in different configurations being written in the device
channels).

> > +    type: object
> > +    description:
> > +      Represents a channel which is being used as an active analog
> > temperature
> > +      sensor.
> > +
> > +    properties:
> > +      adi,sensor-type:
> > +        description:
> > +          Identifies the sensor as an active analog temperature
> > sensor.
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +        const: 31
> > +
> > +      adi,single-ended:
> > +        description: Boolean property which sets the sensor as
> > single-ended.
> 
> Drop "Boolean property which sets" - it's obvious from the type.
> 
> 
> 
> > +        type: boolean
> > +
> > +      adi,custom-temp:
> > +        description:
> > +          This is a table, where each entry should be a pair of
> 
> "This is a table" - obvious from the type.
> 
> > +          voltage(mv)-temperature(K). The entries must be given in
> > nv and uK
> 
> mv-K or nv-uK? Confusing...

Yeah, a bit. In Cosmin defense, I think he's just keeping the same
"style" as the rest of the properties...

> 
> > +          so that, the original values must be multiplied by
> > 1000000. For
> > +          more details look at table 71 and 72.
> 
> There is no table 71 in the bindings... It seems you pasted it from
> somewhere.

I'm fairly sure this refers to the datasheet. I see now that this can
be confusing (again this kind of references are being (ab)used in the
rest of the file).

> 
> > +          Note should be signed, but dtc doesn't currently
> > maintain the
> > +          sign.
> 
> What do you mean? "Maintain" as allow or keep when building FDT? 
> What's
> the problem of using negative numbers here and why it should be part
> of
> bindings?
> 
> > +        $ref: /schemas/types.yaml#/definitions/uint64-matrix
> > +        minItems: 3
> > +        maxItems: 64
> > +        items:
> > +          minItems: 2
> > +          maxItems: 2
> 
> Instead describe the items with "description" (and maybe constraints)
> like here:
> 
> https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.yaml#L278
> 

Neat... My only comment (which probably applies to my previous ones) is
that the rest of the properties are already in this "style". So maybe,
follow up patches with small clean-ups would be more appropriate?
> 


- Nuno Sá
  

On Mon, 2022-10-17 at 11:38 +0200, Nuno Sá wrote:
> Hi Krzysztof,
> 
> As I wrote the original bindings, let me reply to some of your
> points...
> 
> 

Just realized now there were already some replies (forgot to 'lei up'
before replying...)

- Nuno Sá
  

On 17/10/2022 02:53, Cosmin Tanislav wrote:
> 
> 
> On 10/17/22 04:59, Krzysztof Kozlowski wrote:
>> On 14/10/2022 08:37, Cosmin Tanislav wrote:
>>> From: Cosmin Tanislav <cosmin.tanislav@analog.com>
>>>
>>> Add support for the following parts:
>>>   * LTC2984
>>>   * LTC2986
>>>   * LTM2985
>>>
>>> The LTC2984 is a variant of the LTC2983 with EEPROM.
>>> The LTC2986 is a variant of the LTC2983 with only 10 channels,
>>> EEPROM and support for active analog temperature sensors.
>>> The LTM2985 is software-compatible with the LTC2986.
>>>
>>> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
>>> ---
>>>   .../bindings/iio/temperature/adi,ltc2983.yaml | 63 +++++++++++++++++--
>>>   1 file changed, 59 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
>>> index 722781aa4697..c33ab524fb64 100644
>>> --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
>>> +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
>>> @@ -4,19 +4,27 @@
>>>   $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
>>> +title: Analog Devices LTC2983, LTC2986, LTM2985 Multi-sensor Temperature system
>>>   
>>>   maintainers:
>>>     - Nuno Sá <nuno.sa@analog.com>
>>>   
>>>   description: |
>>> -  Analog Devices LTC2983 Multi-Sensor Digital Temperature Measurement System
>>> +  Analog Devices LTC2983, LTC2984, LTC2986, LTM2985 Multi-Sensor Digital
>>> +  Temperature Measurement Systems
>>> +
>>>     https://www.analog.com/media/en/technical-documentation/data-sheets/2983fc.pdf
>>> +  https://www.analog.com/media/en/technical-documentation/data-sheets/2984fb.pdf
>>> +  https://www.analog.com/media/en/technical-documentation/data-sheets/29861fa.pdf
>>> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ltm2985.pdf
>>>   
>>>   properties:
>>>     compatible:
>>>       enum:
>>>         - adi,ltc2983
>>> +      - adi,ltc2984
>>> +      - adi,ltc2986
>>> +      - adi,ltm2985
>>>   
>>>     reg:
>>>       maxItems: 1
>>> @@ -26,7 +34,7 @@ properties:
>>>   
>>>     adi,mux-delay-config-us:
>>>       description:
>>> -      The LTC2983 performs 2 or 3 internal conversion cycles per temperature
>>> +      The device performs 2 or 3 internal conversion cycles per temperature
>>>         result. Each conversion cycle is performed with different excitation and
>>>         input multiplexer configurations. Prior to each conversion, these
>>>         excitation circuits and input switch configurations are changed and an
>>> @@ -145,7 +153,7 @@ patternProperties:
>>>         adi,three-conversion-cycles:
>>>           description:
>>>             Boolean property which set's three conversion cycles removing
>>> -          parasitic resistance effects between the LTC2983 and the diode.
>>> +          parasitic resistance effects between the device and the diode.
>>>           type: boolean
>>>   
>>>         adi,average-on:
>>> @@ -353,6 +361,41 @@ patternProperties:
>>>           description: Boolean property which set's the adc as single-ended.
>>>           type: boolean
>>>   
>>> +  "^temp@":
>>
>> There is already a property for thermocouple. Isn't a thermocouple a
>> temperature sensor? IOW, why new property is needed?
>>
> This node is needed for active analog temperature sensors.
> It has fewer options than the thermocouple, as it only supports
> a table to map from voltage to temperature and specifying whether
> the measurement is differential or single-ended.
> 
> If you did as much as glimpsed at the datasheet you would have
> understood.

We receive a lot of bindings to review. If I glimpse through every
datasheet, when would I work?

Instead of expecting reviewer to dive into datasheets for this one
particular sensor, make it explicit in commit msg.

> 
>>> +    type: object
>>> +    description:
>>> +      Represents a channel which is being used as an active analog temperature
>>> +      sensor.
>>> +
>>> +    properties:
>>> +      adi,sensor-type:
>>> +        description:
>>> +          Identifies the sensor as an active analog temperature sensor.
>>> +        $ref: /schemas/types.yaml#/definitions/uint32
>>> +        const: 31
>>> +
>>> +      adi,single-ended:
>>> +        description: Boolean property which sets the sensor as single-ended.
>>
>> Drop "Boolean property which sets" - it's obvious from the type.
>>
> 
> That's how the rest of the file is written.

Not really an argument... You can correct the other pieces in separate
patch.

> 
>>
>>
>>> +        type: boolean
>>> +
>>> +      adi,custom-temp:
>>> +        description:
>>> +          This is a table, where each entry should be a pair of
>>
>> "This is a table" - obvious from the type.
>>
> 
> That's how the rest of the file is written.
> 
>>> +          voltage(mv)-temperature(K). The entries must be given in nv and uK
>>
>> mv-K or nv-uK? Confusing...
> 
> That's how the rest of the file is written.

The same.

> 
> The chip uses mv-K, but the binding specifies nv-uK, the driver
> translates it into the appropriate unit.

It does not matter here, what the driver is doing. Use only one unit
here, matching the DTS.

> 
>>
>>> +          so that, the original values must be multiplied by 1000000. For
>>> +          more details look at table 71 and 72.
>>
>> There is no table 71 in the bindings... It seems you pasted it from
>> somewhere.
>>
> 
> It's pretty obvious that "Table" in a binding refers to the datasheet.

There are multiple datasheets and how would I know to which one this refers?

> But if you meant datasheet, not binding, you're looking at the wrong
> datasheet then.
> Also, that's how the rest of the file is written.

Not really an argument... Poor examples like to spread, it's an effort
to drop them.

> 
>>> +          Note should be signed, but dtc doesn't currently maintain the
>>> +          sign.
>>
>> What do you mean? "Maintain" as allow or keep when building FDT?  What's
>> the problem of using negative numbers here and why it should be part of
>> bindings?
>>
> 
> You're really clueless, I'll let you figure it out on your own.

Yes, I am clueless and that's not the way how the review conversation
should look like.

NAK.

> Also, that's how the rest of the file is written.
> 
>>> +        $ref: /schemas/types.yaml#/definitions/uint64-matrix
>>> +        minItems: 3
>>> +        maxItems: 64
>>> +        items:
>>> +          minItems: 2
>>> +          maxItems: 2
>>
>> Instead describe the items with "description" (and maybe constraints)
>> like here:
>>
>> https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.yaml#L278
>>
> 
> That's how the rest of the file is written.
> If you really want to use something different, you can submit a
> patch later and fix the whole binding however you want.

Nope. I expect the new pieces to be correct, not incorrect because
"there is already poor pattern, so I will do the same".

If inconsistency bothers you, anyone can fix it in following up patch.
Also you.

Best regards,
Krzysztof
  

On 17/10/2022 05:38, Nuno Sá wrote:
> Hi Krzysztof,
> 

(...)

>>> @@ -353,6 +361,41 @@ patternProperties:
>>>          description: Boolean property which set's the adc as
>>> single-ended.
>>>          type: boolean
>>>  
>>> +  "^temp@":
>>
>> There is already a property for thermocouple. Isn't a thermocouple a
>> temperature sensor? IOW, why new property is needed?
>>
> 
> Well, most of the patternProps in this bindings are temperature
> sensors... It's just that the device(s) support different types of
> them. 'adi,sensor-type' is used to identify each sensor (as this
> translates in different configurations being written in the device
> channels).

Sure.

> 
>>> +    type: object
>>> +    description:
>>> +      Represents a channel which is being used as an active analog
>>> temperature
>>> +      sensor.
>>> +
>>> +    properties:
>>> +      adi,sensor-type:
>>> +        description:
>>> +          Identifies the sensor as an active analog temperature
>>> sensor.
>>> +        $ref: /schemas/types.yaml#/definitions/uint32
>>> +        const: 31
>>> +
>>> +      adi,single-ended:
>>> +        description: Boolean property which sets the sensor as
>>> single-ended.
>>
>> Drop "Boolean property which sets" - it's obvious from the type.
>>
>>
>>
>>> +        type: boolean
>>> +
>>> +      adi,custom-temp:
>>> +        description:
>>> +          This is a table, where each entry should be a pair of
>>
>> "This is a table" - obvious from the type.
>>
>>> +          voltage(mv)-temperature(K). The entries must be given in
>>> nv and uK
>>
>> mv-K or nv-uK? Confusing...
> 
> Yeah, a bit. In Cosmin defense, I think he's just keeping the same
> "style" as the rest of the properties...

That's not the best approach for two reasons:
1. The unit used by hardware matters less here, because bindings are
used to write DTS. In many, many other cases there will be some
translation (just take random voltage regulator bindings).

2. What the driver is doing matters even less.

So just describe here what is expected in DTS.

> 
>>
>>> +          so that, the original values must be multiplied by
>>> 1000000. For
>>> +          more details look at table 71 and 72.
>>
>> There is no table 71 in the bindings... It seems you pasted it from
>> somewhere.
> 
> I'm fairly sure this refers to the datasheet. I see now that this can
> be confusing (again this kind of references are being (ab)used in the
> rest of the file).

Yep, but there are now multiple datasheets, aren't there?

> 
>>
>>> +          Note should be signed, but dtc doesn't currently
>>> maintain the
>>> +          sign.
>>
>> What do you mean? "Maintain" as allow or keep when building FDT? 
>> What's
>> the problem of using negative numbers here and why it should be part
>> of
>> bindings?
>>
>>> +        $ref: /schemas/types.yaml#/definitions/uint64-matrix
>>> +        minItems: 3
>>> +        maxItems: 64
>>> +        items:
>>> +          minItems: 2
>>> +          maxItems: 2
>>
>> Instead describe the items with "description" (and maybe constraints)
>> like here:
>>
>> https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.yaml#L278
>>
> 
> Neat... My only comment (which probably applies to my previous ones) is
> that the rest of the properties are already in this "style". So maybe,
> follow up patches with small clean-ups would be more appropriate?

Of course. It would be great if the file was improved before or after
this one.


Best regards,
Krzysztof
  

On Mon, 2022-10-17 at 19:32 -0400, Krzysztof Kozlowski wrote:
> On 17/10/2022 05:38, Nuno Sá wrote:
> > Hi Krzysztof,
> > 
> 
> (...)
> 
> > > > @@ -353,6 +361,41 @@ patternProperties:
> > > >          description: Boolean property which set's the adc as
> > > > single-ended.
> > > >          type: boolean
> > > >  
> > > > +  "^temp@":
> > > 
> > > There is already a property for thermocouple. Isn't a
> > > thermocouple a
> > > temperature sensor? IOW, why new property is needed?
> > > 
> > 
> > Well, most of the patternProps in this bindings are temperature
> > sensors... It's just that the device(s) support different types of
> > them. 'adi,sensor-type' is used to identify each sensor (as this
> > translates in different configurations being written in the device
> > channels).
> 
> Sure.
> 
> > 
> > > > +    type: object
> > > > +    description:
> > > > +      Represents a channel which is being used as an active
> > > > analog
> > > > temperature
> > > > +      sensor.
> > > > +
> > > > +    properties:
> > > > +      adi,sensor-type:
> > > > +        description:
> > > > +          Identifies the sensor as an active analog
> > > > temperature
> > > > sensor.
> > > > +        $ref: /schemas/types.yaml#/definitions/uint32
> > > > +        const: 31
> > > > +
> > > > +      adi,single-ended:
> > > > +        description: Boolean property which sets the sensor as
> > > > single-ended.
> > > 
> > > Drop "Boolean property which sets" - it's obvious from the type.
> > > 
> > > 
> > > 
> > > > +        type: boolean
> > > > +
> > > > +      adi,custom-temp:
> > > > +        description:
> > > > +          This is a table, where each entry should be a pair
> > > > of
> > > 
> > > "This is a table" - obvious from the type.
> > > 
> > > > +          voltage(mv)-temperature(K). The entries must be
> > > > given in
> > > > nv and uK
> > > 
> > > mv-K or nv-uK? Confusing...
> > 
> > Yeah, a bit. In Cosmin defense, I think he's just keeping the same
> > "style" as the rest of the properties...
> 
> That's not the best approach for two reasons:
> 1. The unit used by hardware matters less here, because bindings are
> used to write DTS. In many, many other cases there will be some
> translation (just take random voltage regulator bindings).
> 
> 2. What the driver is doing matters even less.
> 
> So just describe here what is expected in DTS.
> 

Alright, I see. So we just refer to nv-uK as that is what I wanted for
dts to expect (reason being to have more resolution).

> > 
> > > 
> > > > +          so that, the original values must be multiplied by
> > > > 1000000. For
> > > > +          more details look at table 71 and 72.
> > > 
> > > There is no table 71 in the bindings... It seems you pasted it
> > > from
> > > somewhere.
> > 
> > I'm fairly sure this refers to the datasheet. I see now that this
> > can
> > be confusing (again this kind of references are being (ab)used in
> > the
> > rest of the file).
> 
> Yep, but there are now multiple datasheets, aren't there?
> 

Hmm yeah that's true. By the time I wrote this binding I was not even
thinking on the possibility of new parts being added to it... I guess
the lesson in here is to avoid this kind os specific descriptions.

> > 
> > > 
> > > > +          Note should be signed, but dtc doesn't currently
> > > > maintain the
> > > > +          sign.
> > > 
> > > What do you mean? "Maintain" as allow or keep when building FDT? 
> > > What's
> > > the problem of using negative numbers here and why it should be
> > > part
> > > of
> > > bindings?
> > > 
> > > > +        $ref: /schemas/types.yaml#/definitions/uint64-matrix
> > > > +        minItems: 3
> > > > +        maxItems: 64
> > > > +        items:
> > > > +          minItems: 2
> > > > +          maxItems: 2
> > > 
> > > Instead describe the items with "description" (and maybe
> > > constraints)
> > > like here:
> > > 
> > > https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.yaml#L278
> > > 
> > 
> > Neat... My only comment (which probably applies to my previous
> > ones) is
> > that the rest of the properties are already in this "style". So
> > maybe,
> > follow up patches with small clean-ups would be more appropriate?
> 
> Of course. It would be great if the file was improved before or after
> this one.
> 

Ok, IMO it would make sense to have it in this series but if Cosmin
does not feel like fixing my mess :), I'll send a separate patch with
your inputs...

- Nuno Sá