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

[Guenter Roeck <linux@roeck-us.net>]

On 5/20/22 03:13, Krzysztof Kozlowski wrote:
> On 20/05/2022 11:32, Slawomir Stepien wrote:
>> From: Slawomir Stepien <slawomir.stepien@nokia.com>
>>
>> Add binding description for temperature channels. Currently, support for
>> label and offset is implemented.
>>
>> Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
>> ---
>>   .../bindings/hwmon/national,lm90.yaml         | 39 +++++++++++++++++++
>>   1 file changed, 39 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/national,lm90.yaml b/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
>> index 066c02541fcf..9a5aa78d4db1 100644
>> --- a/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
>> +++ b/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
>> @@ -62,6 +62,37 @@ required:
>>   
>>   additionalProperties: false
>>   
>> +patternProperties:
> 
> Which models use this?
> 
>> +  "^channel@([0-2])$":
>> +    type: object
>> +    description: |
> 
> No need for |
> 
>> +      Represents channels of the device and their specific configuration.
>> +
>> +    properties:
>> +      reg:
>> +        description: |
> 
> The same.
> 
>> +          The channel number. 0 is local channel, 1-2 are remote channels.
>> +        items:
>> +          minimum: 0
>> +          maximum: 2
>> +
>> +      label:
>> +        description: |
> 
> The same.
> 
>> +          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.
> 

Temperature offset is a standard property for temperature sensor
chips with external channels, implemented by a diode or transistor.
Making it non-standard will mean that we'll have lots of
"vendor,offset" properties, one each for each vendor selling
temperature sensor chips with external channels. This gets
more complicated here because the lm90 driver does support chips
from several different vendors. Almost all of them support
this functionality. Which vendor do you select in this case ?

I would suggest to use temperature-offset-milliseconds, though.

>> +          The value (millidegree Celsius) to be programmed in the channel specific offset register
>> +          (if supported by device).
> 
> You described programming model which should not be put in the bindings.
> Please describe the hardware.
> 

It is a configuration value, which is hardware dependent because
it depends on the temperature diode or transistor connected to the chip.

Guenter

>> +          For remote channels only.
>> +        $ref: /schemas/types.yaml#/definitions/int32
>> +        default: 0
>> +
>> +    required:
>> +      - reg
>> +
>> +    additionalProperties: false
>> +
>>   examples:
>>     - |
>>       #include <dt-bindings/interrupt-controller/irq.h>
>> @@ -76,5 +107,13 @@ examples:
>>               vcc-supply = <&palmas_ldo6_reg>;
>>               interrupts = <4 IRQ_TYPE_LEVEL_LOW>;
>>               #thermal-sensor-cells = <1>;
>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
> I assume you tested the bindings with dt_bindings_check?
> 
> I have some doubts, as this should fail.
> 
>> +
>> +            channel@0 {
>> +                reg = <0x0>;
>> +                label = "internal";
>> +                offset = <1000>;
>> +            };
>>           };
>>       };
> 
> 
> Best regards,
> Krzysztof