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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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