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

From: Rob Herring
Date: Wed Apr 13 2022 - 09:34:51 EST


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@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.

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