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

[Guenter Roeck <linux@roeck-us.net>]

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.

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

> 
> 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";
> > +        };
> > +      };
> > +    };
    };

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 */;
+                };
+            };

I personally don't have a strong opinion either way, but I would like to see
a single solution for all sensor chips.

Rob, do you have a preference ?

Thanks,
Guenter