Re: [PATCH] dt-bindings: hwmon: Add nct7802 bindings

[Guenter Roeck]

On 9/14/21 10:11 AM, Oskar Senft wrote:
> Hi Guenter
>
> > > Following the example from tmp421, this could then be like this:
> >
> > Something like that, only we'll need something to distinguish
> > temperature sensors from other sensor types, eg voltage or current.
> > Maybe a "type" property. I'd suggest "sensor-type", but we have
> > non-sensor attributes such as fan count and pwm values which should
> > be covered as well. But it looks like a good start for a set of
> > generic sensor properties.
> Would it be acceptable to simply number the sensors and document which
> sensor has which number?
>
> Something like this:
> 0 = LTD
> 1 = RTD1
> 2 = RTD2
> 3 = RTD3
> 4 = FAN1
> 5 = FAN2
> 6 = FAN3
That might be a possibility, though it would have to be well defined
for each chip (nct7802 also has voltage sensors). We'll have to discuss
this with Rob.

Personally I think I would prefer using a type qualifier - that seems
cleaner. But that is really a matter of opinion.

> Would we also want to be able to define PWMs? From what I can tell the
> driver does not support running individual pins in GPIO mode, right?
> So I'm not quite clear what "disabling PWM" would actually mean.
The ABI states that fans should run at full speed in that case,
though that may be chip dependent (some chips stop the fan if pwm
control is turned off).

> Anyway, if we simply go by "sensor number", that would mean that we'd
> have different attributes depending on the sensor number. Would that
> be ok?
That is a question for Rob to answer.

> Also, I'm sorry, I think I just realized that in "voltage mode" we
> don't seem to get a temperature reading. I hadn't actually looked
> through more of the datasheet except for the single MODE register
> before. But I don't think this makes a difference for what I proposed
> so far?

We don't ? I thought this reflects temperature measurement with a
transistor instead of a diode (which would be current based).
Hard to say - the datasheet is a bit vague in that regard.

> > >          /* LTD */
> > >          input@0 {
> > >              reg = <0x0>;
> > >              status = "okay";
> >
> > Not sure what the default is here ('okay' or 'disabled').
> > We'd also need to define what to do if there is no data
> > for a given sensor.
> I think I'd like to keep previous behavior unmodified. From what I can
> tell previous behavior was:
> - xTDs enabled by default
> - RTD modes unmodified, i.e. defaulting to whatever the HW comes up with
>
> The NCT7802Y can self-program from an EEPROM, so I assume we should
> honor the "power-up configuration" obtained from there? I.e. if no
> configuration is provided in the device tree, the driver should use
> whatever configuration the chip has when the driver is loaded.
Definitely yes. My question was more what to do if the information
in devicetree nodes is incomplete.

Thanks,
Guenter

> > >              label = "voltage mode";
> >
> > That isn't the idea for "label", as "label" would be expected to
> > show up as tempX_label (and a label of "voltage mode" would be odd).
> > The label should indicate where the sensor is located on a board,
> > such as "inlet" or "outlet".
> Yes, absolutely. This was a bad example on my part. In my
> understanding "label" is just a string that we pass through.
>
> Oskar.