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

[Krzysztof Adamski <krzysztof.adamski@nokia.com>]

Dnia Tue, Oct 05, 2021 at 07:14:57AM -0700, Guenter Roeck napisał(a):
>> > > +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.

I guess the reason I have used "input@X" was, again, because I based my
idea of how DT bindings should look like on the ina3221 as it seemed
like the most similar to my case. This chip, however, current/voltage
monitor so "input" makes more sense there, indeed.

In general, while I do like consistency, we are not defining a binding
for all hwmon sensors now as each of them, anyways, has its own binding
documentation so some variation is allowed, I think (and inevitable as
we won't change existing bindings).

So, I do mandate that the child nodes for *this* device should be named
accordingly but this doesn't mean other hwmon devices can't use others names
that makes more sense to them. While I could make the code that will
iterate over all child nodes and will ignore their names, I do think it
is better to specify the strict naming convention that everybody will
use (again, for *this* device), instead. I see several advantages of
this:
  - consistency in different DTs
  - easier extensibility - if we introduce something that will require
    adding some other non-channel related nodes, we don't have to worry
    that much about the name clashing
  - it makes verifying the DT correctness easier

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

Out of those, I strongly prefer the first one. But, again, we don't have
to define one kind of binding for ALL hwmon sensors (but I do think its
better if we can be generic).
So the biggest question to me is whether temperature and voltage should
have separate "address spaces". If we, indeed, want to have
temperature@0 and voltage@0, then using specific names, instead of
generic, like "channel", does make sense. But then, again, I don't see a
problem with one driver having a binding with "channel@X", while other
having "temperature@X" and "voltage@X".

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

I think that if all the "channels" of the device are of the same type,
it doesn't matter. If we have some inputs and some outputs, we should
have either:

channel@0 {
	reg = <0>;
	type = "input";   // or something like that, maybe a numeric value
	with defines
}
channel@1 {
	reg = <1>;
	type = "output";   // or something like that, maybe a numeric value
	with defines
}

Or:

input@0 {
  reg = <0>;
}

output@1 {
  reg = <1>;
}

But, again, TMP42X doesn't need any of that anyways :)

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

Rob? :)

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

The DT blob will only contain 0..255. DTC has a syntactic sugar that
lets you specify the value as negative and will convert it to 2s
complement for you. So those two are exactly the same:

n-factor = <(-10)>;
n-factor = <0xfffffff6>;

 From the code perspective, however, all 3 most significant bytes are
ignored so 0xfffffff6 is the same as 0xf6.

Krzysztof