On Mon, Oct 04, 2021 at 09:36:23AM +0200, Krzysztof Adamski wrote:

Dnia Sat, Oct 02, 2021 at 07:22:19AM -0700, Guenter Roeck napisał(a):

quoted

On Thu, Sep 30, 2021 at 09:19:49AM +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/ti,tmp421.yaml  | 66 +++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/ti,tmp421.yaml b/Documentation/devicetree/bindings/hwmon/ti,tmp421.yaml
index 47040ace4f73..0d4ea2209500 100644
--- a/Documentation/devicetree/bindings/hwmon/ti,tmp421.yaml
+++ b/Documentation/devicetree/bindings/hwmon/ti,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])$":

Was there agreement on "input" ? It is a somewhat odd name for a temperature
sensor. If that name can be used to distinguish child sensor types, it might
make sense to have a well defined name to state that this is a temperature
sensor.

Nope, no conclusion on that, yet, thus I did not change that and I was
still using the same approach I had on v1. For me it can be a "channel@X", a
"temperature@X".. whatever you decide.

My question was more on mandating a single string instead of letting
users decide. I don't care either if it isn't used for anything in
particular, but you specifically mandate "input" as the only valid
string. I am not a DT expert, but it seems to me that mandating the
content of that string and then not using it other than to ensure that
the user really specified "input" doesn't make much sense to me.
Having said that, if this is the DT way of things, it is ok with
me.

However I'm in favor of some generic name, like "channel" or "input",
and using some "type property", if required, instead of calling the
nodes "temperatue@X", "voltage@X".

It does open up a nother dimension for multi-type sensor chips, though,

For a chip with voltage and temperature sensors:

	temperature@0 {
		reg = <0>;
	};

	voltage@0 {
		reg = <0>;
	};

vs:

	temperature-sensors {
		xxx@0 {
			reg = <0>;
		};
	};

	voltage-sensors {
		xxx@0 {
			reg = <0>;
		};
	};

This is way out of my league in terms of what is appropriate,
except that "xxx" isn't always easy to determine if the string is fixed
as you suggest. What should it be for a sensor measuring an output voltage ?

	input@0 {
		reg = <0>;
		label = "output voltage";
	};

Anyway, maybe Rob has an idea how to name this properly.

Guenter

quoted

quoted

+    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

Which of the supported chips has 4 remote channels ?

True, there is no TMP424. I will fix that in v4.

quoted

quoted

+        items:
+          minimum: 0
+          maximum: 4
+
+      label:
+        description: |
+          A descriptive name for this channel, like "ambient" or "psu".
+
+      n-factor:

n-factor or "ti,n-factor" ? The unit is chip specific, after all.

Or ti,nfactor, as used by tmp513? Again, there was no clear conclusion
so I didn't change that. Let me know what is your decision and I will
obey that.

Not my call to make about nfactor or n-factor, really. I'll leave that
for Rob to decide.

quoted

quoted

+        description: |
+          The value (two's complement) to be programmed in the channel specific N correction register.

[ side note: Since the value is just a register value, "two's complement" seems
  unnecessary and confusing; in the context of the DT description it doesn't
  really matter what the register values actually mean. ]

quoted

quoted

+          For remote channels only.
+        items:
+          minimum: 0
+          maximum: 1

Is this the correct value range ? The value range (in integer form) is
-128 .. 127 (or 0 .. 255 as unsigned), not 0..1.

True, I must have misunderstood this minimum/maximum and confused it
with the number of items or something. Now, since DT does not really
handle signed values and considers everything an unsigned, should I use
0..255 or -128..127?

I suspect it should be 0..255. After all, the values reflect register values,
not their meaning. But I don't really know. Rob ?

Guenter