Re: [PATCH 3/8] dt-bindings: hwmon: Allow specifying channels for lm90

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

On 20/05/2022 15:23, Slawomir Stepien wrote:
>>>>>  
>>>>> +patternProperties:
>>>>
>>>> Which models use this?
>>>
>>> This is used in tmp421 model.
>>
>> Then please add allOf:if:then disallowing the property for other models.
> 
> A misunderstanding. The channel node can be used by every device supported by lm90. At least each
> channel of each device can have label.

OK

> 
>>>>> +  "^channel@([0-2])$":
>>>>> +    type: object
>>>>> +    description: |
>>>>
>>>> No need for |
>>>
>>> Will fix in v2.
>>>
>>>>> +      Represents channels of the device and their specific configuration.
>>>>> +
>>>>> +    properties:
>>>>> +      reg:
>>>>> +        description: |
>>>>
>>>> The same.
>>>
>>> Will fix in v2.
>>>
>>>>> +          The channel number. 0 is local channel, 1-2 are remote channels.
>>>>> +        items:
>>>>> +          minimum: 0
>>>>> +          maximum: 2
>>>>> +
>>>>> +      label:
>>>>> +        description: |
>>>>
>>>> The same.
>>>
>>> Will fix in v2.
>>>
>>>>> +          A descriptive name for this channel, like "ambient" or "psu".
>>>>> +
>>>>> +      offset:
>>>>> +        description: |
>>>>
>>>> This does not look like standard property, so you need vendor and unit
>>>> suffix.
>>>
>>> Currently in lm90 we have support for devices that have different width (including sign) for offset
>>> register. We have 10 bits, 11 bits and 12 bits. Do I understand correctly that I can use the same
>>> vendor prefix if the width is the same? Just like "ti" was used for adi and ti in
>>> "ti,extended-range-enable"?
>>>
>>> For example:
>>>
>>> adi,10-bit-offset-millicelsius # (only for adt7481)
>>> adi,11-bit-offset-millicelsius # (for adt7461 but also for lm90 and others)
>>> ti,12-bit-offset-millicelsius  # (ti - since only tmp451 and tmp461 supports 12 bit)
>>
>> Wait, these are then strictly per-compatible, so there is no sense in DT
>> property at all.
> 
> But isn't the value of offset a hardware-design-time calculation? So if I have a piece of
> hardware that describes itself using device-tree, then offset information should be stored on the
> device-tree rather then be "calculated" by the software running on that piece of hardware?
> 
> And what if such piece of hardware has been changed (e.g. new PCB version) and now the offset are
> different? Then if device-tree is on hardware (e.g. on EEPROM) with new offsets, then software would
> not require a change to support this new hardware version.

OK, I misunderstood.

This should be only one property then, choose some reasonable vendor,
and in allOf:if:then you can put constraints about minimum/maximum
values per model.

Best regards,
Krzysztof