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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 20/05/2022 14:38, Slawomir Stepien wrote:
> On maj 20, 2022 12:13, Krzysztof Kozlowski wrote:
>> On 20/05/2022 11:32, Slawomir Stepien wrote:
>>> From: Slawomir Stepien <slawomir.stepien@xxxxxxxxx>
>>>
>>> Add binding description for temperature channels. Currently, support for
>>> label and offset is implemented.
>>>
>>> Signed-off-by: Slawomir Stepien <slawomir.stepien@xxxxxxxxx>
>>> ---
>>>  .../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?
> 
> This is used in tmp421 model.

Then please add allOf:if:then disallowing the property for other models.

> 
>>> +  "^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.

> 
>>> +          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.
> 
> I am also not sure about the "-millicelsius" in example above. From device point-of-view, this
> offset is just two's complement, so is it more desirable to have the values here as just bytes
> rather than millicelsius?

No, the programming model of a device can change and should be
abstracted to hardware property.

> 
>>> +          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.
> 
> I did. All was fine. What should fail here?

This:

linux/out/Documentation/devicetree/bindings/hwmon/national,lm90.example.dtb:
sensor@4c: '#address-cells', '#size-cells' do not match any of the
regexes: '^channel@([0-2])$', 'pinctrl-[0-9]+'

	From schema:
linux/linux/Documentation/devicetree/bindings/hwmon/national,lm90.yaml


So no, you did not test it. Asking reviewer to perform a test which you
can do by yourself is a huge waste of reviewers time.

Best regards,
Krzysztof



[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux