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

[Rob Herring <robh@kernel.org>]

On Tue, Oct 05, 2021 at 07:14:57AM -0700, Guenter Roeck wrote:
> 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@nokia.com>
> > > > ---
> > > >  .../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.

Kind of a catch-22. Node names weren't consistent nor checked, so 
we can use them. 

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

Not valid because you have same address twice.

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

Didn't we already discuss this?

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

Does measuring a voltage have a direction?

> 
> 	input@0 {
> 		reg = <0>;
> 		label = "output voltage";
> 	};
> 
> Anyway, maybe Rob has an idea how to name this properly.

No, I don't have a sense of the range of h/w...

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

ti,n-factor

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

That's fine.

You can define it as a signed type, but the validation there is not 
working due to dts->dt.yaml losing the sign.

Rob