linux-kernel - Re: [PATCH 2/3] dt-bindings: iio: temperature: ltc2983: support more parts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Tue, 18 Oct 2022 08:01:58 +0200
From:   Nuno Sá <noname.nuno@...il.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Cosmin Tanislav <demonsingur@...il.com>
Cc:     Nuno Sá <nuno.sa@...log.com>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Michael Hennerich <Michael.Hennerich@...log.com>,
        Jonathan Cameron <jic23@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Cosmin Tanislav <cosmin.tanislav@...log.com>
Subject: Re: [PATCH 2/3] dt-bindings: iio: temperature: ltc2983: support
 more parts

On Mon, 2022-10-17 at 19:32 -0400, Krzysztof Kozlowski wrote:
> On 17/10/2022 05:38, Nuno Sá wrote:
> > Hi Krzysztof,
> > 
> 
> (...)
> 
> > > > @@ -353,6 +361,41 @@ patternProperties:
> > > >          description: Boolean property which set's the adc as
> > > > single-ended.
> > > >          type: boolean
> > > >  
> > > > +  "^temp@":
> > > 
> > > There is already a property for thermocouple. Isn't a
> > > thermocouple a
> > > temperature sensor? IOW, why new property is needed?
> > > 
> > 
> > Well, most of the patternProps in this bindings are temperature
> > sensors... It's just that the device(s) support different types of
> > them. 'adi,sensor-type' is used to identify each sensor (as this
> > translates in different configurations being written in the device
> > channels).
> 
> Sure.
> 
> > 
> > > > +    type: object
> > > > +    description:
> > > > +      Represents a channel which is being used as an active
> > > > analog
> > > > temperature
> > > > +      sensor.
> > > > +
> > > > +    properties:
> > > > +      adi,sensor-type:
> > > > +        description:
> > > > +          Identifies the sensor as an active analog
> > > > temperature
> > > > sensor.
> > > > +        $ref: /schemas/types.yaml#/definitions/uint32
> > > > +        const: 31
> > > > +
> > > > +      adi,single-ended:
> > > > +        description: Boolean property which sets the sensor as
> > > > single-ended.
> > > 
> > > Drop "Boolean property which sets" - it's obvious from the type.
> > > 
> > > 
> > > 
> > > > +        type: boolean
> > > > +
> > > > +      adi,custom-temp:
> > > > +        description:
> > > > +          This is a table, where each entry should be a pair
> > > > of
> > > 
> > > "This is a table" - obvious from the type.
> > > 
> > > > +          voltage(mv)-temperature(K). The entries must be
> > > > given in
> > > > nv and uK
> > > 
> > > mv-K or nv-uK? Confusing...
> > 
> > Yeah, a bit. In Cosmin defense, I think he's just keeping the same
> > "style" as the rest of the properties...
> 
> That's not the best approach for two reasons:
> 1. The unit used by hardware matters less here, because bindings are
> used to write DTS. In many, many other cases there will be some
> translation (just take random voltage regulator bindings).
> 
> 2. What the driver is doing matters even less.
> 
> So just describe here what is expected in DTS.
> 

Alright, I see. So we just refer to nv-uK as that is what I wanted for
dts to expect (reason being to have more resolution).

> > 
> > > 
> > > > +          so that, the original values must be multiplied by
> > > > 1000000. For
> > > > +          more details look at table 71 and 72.
> > > 
> > > There is no table 71 in the bindings... It seems you pasted it
> > > from
> > > somewhere.
> > 
> > I'm fairly sure this refers to the datasheet. I see now that this
> > can
> > be confusing (again this kind of references are being (ab)used in
> > the
> > rest of the file).
> 
> Yep, but there are now multiple datasheets, aren't there?
> 

Hmm yeah that's true. By the time I wrote this binding I was not even
thinking on the possibility of new parts being added to it... I guess
the lesson in here is to avoid this kind os specific descriptions.

> > 
> > > 
> > > > +          Note should be signed, but dtc doesn't currently
> > > > maintain the
> > > > +          sign.
> > > 
> > > What do you mean? "Maintain" as allow or keep when building FDT? 
> > > What's
> > > the problem of using negative numbers here and why it should be
> > > part
> > > of
> > > bindings?
> > > 
> > > > +        $ref: /schemas/types.yaml#/definitions/uint64-matrix
> > > > +        minItems: 3
> > > > +        maxItems: 64
> > > > +        items:
> > > > +          minItems: 2
> > > > +          maxItems: 2
> > > 
> > > Instead describe the items with "description" (and maybe
> > > constraints)
> > > like here:
> > > 
> > > https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.yaml#L278
> > > 
> > 
> > Neat... My only comment (which probably applies to my previous
> > ones) is
> > that the rest of the properties are already in this "style". So
> > maybe,
> > follow up patches with small clean-ups would be more appropriate?
> 
> Of course. It would be great if the file was improved before or after
> this one.
> 

Ok, IMO it would make sense to have it in this series but if Cosmin
does not feel like fixing my mess :), I'll send a separate patch with
your inputs...

- Nuno Sá