Re: [PATCH v2 1/2] dt-bindings: hwmon: Add TMP401, TMP411 and TMP43x

[Camel Guo]

On 4/12/22 23:36, Rob Herring wrote:
> On Tue, Apr 12, 2022 at 03:52:31PM +0200, Camel Guo wrote:
>> Document the TMP401, TMP411 and TMP43x device devicetree bindings
>> 
>> Signed-off-by: Camel Guo <camel.guo@xxxxxxxx>
>> ---
>> 
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - ti,tmp401
>> +      - ti,tmp411
>> +      - ti,tmp431
>> +      - ti,tmp432
>> +      - ti,tmp435
>> +
>> +  reg:
>> +    maxItems: 1
>> +
> 
>> +  '#address-cells':
>> +    const: 1
>> +
>> +  '#size-cells':
>> +    const: 0
> 
> You don't have any child nodes and these are for child nodes with 'reg'.

Ack! I will fix it in v3.
> 
>> +
>> +  ti,extended-range-enable:
>> +    description:
>> +      When set, this sensor measures over extended temperature range.
>> +    type: boolean
>> +
>> +  ti,n-factor:
> 
> Funny, I just ran across this property today for tmp421...
> 
> Can the schema be shared?

Yes, this property is in ti,tmp421.yaml and ti,tmp464.yaml as well. But 
I guess maybe it is better to separate tmp401 from them.

That is because the chips supported in ti,tmp421,yaml has three channels 
and the chips supported in ti,tmp464.yaml has eight channels and this 
property n-factor is for each channel/child node. But the chips 
supported in ti,tmp401.yaml only has one channel. n-factor is for this 
chip.

> 
>> +    description:
>> +      value to be used for converting remote channel measurements to
>> +      temperature.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    items:
>> +      minimum: 0
>> +      maximum: 255
> 
> Isn't this property signed and should be -128 to -127? The code treats
> the existing cases as signed. One schema is correct and one is like you
> have it.

Ack! will fix it in v3

> 
>> +
>> +  ti,beta-compensation:
>> +    description:
>> +      value to select beta correction range.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    items:
>> +      minimum: 0
>> +      maximum: 15
> 
> Drop 'items'. It is not an array.

Not sure if I understand correctly. Do you means it should be like this? 
If so, I guess ti,n-factor should also be changed like this. Am I right?

   ti,beta-compensation:
    description:
      value to select beta correction range.
      $ref: /schemas/types.yaml#/definitions/uint32
      minimum: 0
      maximum: 15

> 
>> +
>> +allOf:
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - ti,tmp401
>> +    then:
>> +      properties:
>> +        ti,n-factor: false
>> +
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - ti,tmp401
>> +              - ti,tmp411
>> +    then:
>> +      properties:
>> +        ti,beta-compensation: false
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    i2c {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
>> +      sensor@4c {
>> +        compatible = "ti,tmp401";
>> +        reg = <0x4c>;
>> +      };
>> +    };
>> +  - |
>> +    i2c {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
>> +      sensor@4c {
>> +        compatible = "ti,tmp431";
>> +        reg = <0x4c>;
>> +        ti,extended-range-enable;
>> +        ti,n-factor = <0x3b>;
>> +        ti,beta-compensation = <0x7>;
>> +      };
>> +    };
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 61d9f114c37f..6b0d8f5cc68e 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -19838,6 +19838,7 @@ TMP401 HARDWARE MONITOR DRIVER
>>  M:   Guenter Roeck <linux@xxxxxxxxxxxx>
>>  L:   linux-hwmon@xxxxxxxxxxxxxxx
>>  S:   Maintained
>> +F:   Documentation/devicetree/bindings/hwmon/ti,tmp401.yaml
>>  F:   Documentation/hwmon/tmp401.rst
>>  F:   drivers/hwmon/tmp401.c
>>  
>> -- 
>> 2.30.2
>> 
>> 

Thanks for your review. I will fix most of these comments in v3.