Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 13 Apr 2022 08:34:44 -0500
From:   Rob Herring <robh@kernel.org>
To:     Camel Guo <Camel.Guo@axis.com>
Cc:     Guenter Roeck <linux@roeck-us.net>,
        Jean Delvare <jdelvare@suse.com>,
        Krzysztof Kozlowski <krzk+dt@kernel.org>,
        "linux-hwmon@vger.kernel.org" <linux-hwmon@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        kernel <kernel@axis.com>
Subject: Re: [PATCH v2 1/2] dt-bindings: hwmon: Add TMP401, TMP411 and TMP43x
Message-ID: <YlbRdCXnPPurC2wC@robh.at.kernel.org>
References: <20220412135232.1943677-1-camel.guo@axis.com>
 <20220412135232.1943677-2-camel.guo@axis.com>
 <YlXwyKkkC1VoPpjU@robh.at.kernel.org>
 <77167ffd-5674-9f6f-df51-3233e67fe9a7@axis.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <77167ffd-5674-9f6f-df51-3233e67fe9a7@axis.com>
Precedence: bulk

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