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

[Rob Herring <robh@kernel.org>]

On Wed, Apr 13, 2022 at 09:13:39AM +0000, Camel Guo wrote:
> 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@axis.com>
> >> ---
> >> 
> >> +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.

Okay, that makes sense to keep them separate.

> >> +    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

Yes, except your indentation is off. As-is, it's all 'description'. It 
should be like this:

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

Rob