On Tue, Sep 21, 2021 at 3:52 PM Guenter Roeck [off-list ref] wrote:

On Tue, Sep 21, 2021 at 02:06:18PM -0500, Rob Herring wrote:

quoted

On Tue, Sep 21, 2021 at 7:58 AM Guenter Roeck [off-list ref] wrote:

quoted

On Mon, Sep 20, 2021 at 05:24:09PM -0500, Rob Herring wrote:

quoted

On Tue, Sep 07, 2021 at 03:46:14PM +0200, Krzysztof Adamski wrote:

quoted

Add binding description for the per temperature channel configuration
like labels and n-factor.

Signed-off-by: Krzysztof Adamski <redacted>
---
 .../devicetree/bindings/hwmon/tmp421.yaml     | 66 +++++++++++++++++++
 1 file changed, 66 insertions(+)

I'd keep this separate...

quoted

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.

quoted

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.

quoted

quoted

quoted

Needs a type reference too.

quoted

+        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 {

quoted

quoted

+
+        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?

quoted

quoted

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