Re: [PATCH 8/8] dt-bindings: hwmon: allow specifying channels for tmp421

[Rob Herring <robh@kernel.org>]

On Tue, Sep 21, 2021 at 3:52 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Tue, Sep 21, 2021 at 02:06:18PM -0500, Rob Herring wrote:
> > On Tue, Sep 21, 2021 at 7:58 AM Guenter Roeck <linux@roeck-us.net> wrote:
> > >
> > > On Mon, Sep 20, 2021 at 05:24:09PM -0500, Rob Herring wrote:
> > > > On Tue, Sep 07, 2021 at 03:46:14PM +0200, Krzysztof Adamski wrote:
> > > > > Add binding description for the per temperature channel configuration
> > > > > like labels and n-factor.
> > > > >
> > > > > Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> > > > > ---
> > > > >  .../devicetree/bindings/hwmon/tmp421.yaml     | 66 +++++++++++++++++++
> > > > >  1 file changed, 66 insertions(+)
> > > >
> > > > I'd keep this separate...
> > > >
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/hwmon/tmp421.yaml b/Documentation/devicetree/bindings/hwmon/tmp421.yaml
> > > > > index 53940e146ee6..56085fdf1b57 100644
> > > > > --- a/Documentation/devicetree/bindings/hwmon/tmp421.yaml
> > > > > +++ b/Documentation/devicetree/bindings/hwmon/tmp421.yaml
> > > > > @@ -24,12 +24,49 @@ properties:
> > > > >    reg:
> > > > >      maxItems: 1
> > > > >
> > > > > +  '#address-cells':
> > > > > +    const: 1
> > > > > +
> > > > > +  '#size-cells':
> > > > > +    const: 0
> > > > > +
> > > > >  required:
> > > > >    - compatible
> > > > >    - reg
> > > > >
> > > > >  additionalProperties: false
> > > > >
> > > > > +patternProperties:
> > > > > +  "^input@([0-4])$":
> > > > > +    type: object
> > > > > +    description: |
> > > > > +      Represents channels of the device and their specific configuration.
> > > > > +
> > > > > +    properties:
> > > > > +      reg:
> > > > > +        description: |
> > > > > +          The channel number. 0 is local channel, 1-4 are remote channels
> > > > > +        items:
> > > > > +          minimum: 0
> > > > > +          maximum: 4
> > > > > +
> > > > > +      label:
> > > > > +        description: |
> > > > > +          A descriptive name for this channel, like "ambient" or "psu".
> > > > > +
> > > > > +      n-factor:
> > > >
> > > > ti,n-factor
> > >
> > > n-factor isn't just supported by TI sensors, though it isn't always called
> > > n-factor. Maxim (eg MAX6581) uses the term "ideality factor", though they
> > > also refer to the factor as "N" in the datasheet.
> > >
> > > So question is if we make this ti,n-factor and maxim,n-factor, or if we make
> > > it generic and define some kind of generic units. Thoughts ? My personal
> > > preference would be a generic definition, but is not a strong preference.
> >
> > generic if the units are generic. Though if the register value is
> > opaque to s/w, then maybe register value is fine.
> >
> > > In regard to units, the n-factor is, as the name says, a factor. Default
> > > value is 1.008. The value range for MAX6581 is 0.999 to 1.030. For TMP421
> > > it is 0.706542 to 1.747977. So the scondary question is if the value
> > > written should be the register value (as proposed here) or the absolute
> > > factor (eg in micro-units).
> >
> > A range, but the register value can only be 0 or 1?
> >
> No, register values are 0x0 .. 0x1f for MAX6581, and 0x0 .. 0xff for TMP421.

Okay, then the schema below is wrong.

> > > > Needs a type reference too.
> > > >
> > > > > +        description: |
> > > > > +          The value (two's complement) to be programmed in the channel specific N correction register.
> > > > > +          For remote channels only.
> > > > > +        items:
> > > > > +          minimum: 0
> > > > > +          maximum: 1
> > > > > +
> > > > > +    required:
> > > > > +      - reg
> > > > > +
> > > > > +    additionalProperties: false
> > > > > +
> > > > >  examples:
> > > > >    - |
> > > > >      i2c {
> > > > > @@ -41,3 +78,32 @@ examples:
> > > > >          reg = <0x4c>;
> > > > >        };
> > > > >      };
> > > > > +  - |
> > > > > +    i2c {
> > > > > +      #address-cells = <1>;
> > > > > +      #size-cells = <0>;
> > > > > +
> > > > > +      sensor@4c {
> > > > > +        compatible = "ti,tmp422";
> > > > > +        reg = <0x4c>;
> > > > > +        #address-cells = <1>;
> > > > > +        #size-cells = <0>;
> > > > > +
> > > > > +        input@0 {
> > > > > +          reg = <0x0>;
> > > > > +          n-factor = <0x1>;
> > > > > +          label = "local";
> > > > > +        };
> > >
> > > In the context or other sensors, question here is if we can make the
> > > bindings generic. We have been discussing this for NCT7802Y. The main
> > > question for me is how to handle different sensor types. TMP421 is
> > > easy because it only has one type of sensors, but there are other
> > > devices which also have, for example, voltage and/or current sensors.
> > > NCT7802 is an example for that. We just had a set of bindings for that
> > > chip proposed at
> > > https://patchwork.kernel.org/project/linux-hwmon/patch/20210921004627.2786132-1-osk@google.com/
> > >
> > > Would it be possible to determine a generic scheme that works for all
> > > chips ? I can see two problems:
> > > - How to express sensor types. The NCT7802 submission proposes another level
> > >   of indirection, ie
> > >
> > >   temperature-sensors {
> > > > > +
> > > > > +        input@1 {
> > > > > +          reg = <0x1>;
> > > > > +          n-factor = <0x0>;
> > > > > +          label = "somelabel";
> > > > > +        };
> > > > > +
> > > > > +        input@2 {
> > > > > +          reg = <0x2>;
> > > > > +          status = "disabled";
> > > > > +        };
> > > > > +      };
> > > > > +    };
> > >     };
> >
> > I think the function should be within the node. Otherwise, the
> > addressing becomes weird (e.g. input@3 is under current-sensors or
> > something) with seemingly separate address spaces.
> >
>
> Sorry, can you translate that for a DT non-expert ? Or, in other words,
> how would / should one express a chip with sets of, say, current-sensors,
> voltage sensors, and temperature sensors. Each would have a different
> number of channels and different parameters.

If each kind of sensor is a different number space (e.g. 0-2), then
how you have it with 2 levels of nodes is appropriate. If you only
have one set of channel or input numbers, then they should all have
the same parent node. So is it current sensors 0,1,2 and temperature
sensors 0,1,2, or just input channels 0,1,2,3,4,5?

> > > The second question is how to express sensor index. One option is the solution
> > > suggested here, ie to use reg=<> as sensor index. The second is the solution
> > > suggested in the 7802 bindings, where the (chip specific) name is used as
> > > sensor index.
> > >
> > > +            temperature-sensors {
> > > +                ltd {
> > > +                  status = "disabled";
> > > +                };
> > > +
> > > +                rtd1 {
> > > +                  status = "okay";
> > > +                  type = <4> /* thermistor */;
> >
> > 'type' is a bit generic. We don't want the same property name to
> > possibly have multiple definitions.
> >
> How about sensor-type ?

Sure. And you are going to define a common set of type numbers?

Rob