Message-ID: <c410eba9-f6cf-4dbf-797f-48afde9c1898@roeck-us.net>
Date:   Tue, 14 Sep 2021 10:33:57 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Oskar Senft <osk@...gle.com>
Cc:     linux-hwmon@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, Jean Delvare <jdelvare@...e.com>,
        Rob Herring <robh+dt@...nel.org>
Subject: Re: [PATCH] dt-bindings: hwmon: Add nct7802 bindings

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

Powered by blists - more mailing lists