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):
> > On Thu, Sep 30, 2021 at 09:19:49AM +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@xxxxxxxxx>
> > > ---
> > > .../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
> > > + 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.
>
> >
> > > + 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.
>
> >
> > > + 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. ]
> > > + 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
