LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Guenter Roeck <linux@roeck-us.net>
To: Oskar Senft <osk@google.com>
Cc: linux-hwmon@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, Jean Delvare <jdelvare@suse.com>,
	Rob Herring <robh+dt@kernel.org>
Subject: Re: [PATCH] dt-bindings: hwmon: Add nct7802 bindings
Date: Tue, 14 Sep 2021 10:33:57 -0700	[thread overview]
Message-ID: <c410eba9-f6cf-4dbf-797f-48afde9c1898@roeck-us.net> (raw)
In-Reply-To: <CABoTLcQfS5-UL92NR9vbc2YrGJv3oQPYCqAm-diNoq-tkHP_hQ@mail.gmail.com>

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


  reply	other threads:[~2021-09-14 17:34 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-10 13:03 Oskar Senft
2021-09-10 16:10 ` Guenter Roeck
2021-09-10 20:44   ` Oskar Senft
2021-09-10 21:28     ` Guenter Roeck
2021-09-14 12:41       ` Oskar Senft
2021-09-14 15:08         ` Guenter Roeck
2021-09-14 17:11           ` Oskar Senft
2021-09-14 17:33             ` Guenter Roeck [this message]
2021-09-16 19:19               ` Oskar Senft
2021-09-16 19:34                 ` Guenter Roeck
2021-09-16 19:53                   ` Oskar Senft
2021-09-16 20:07                     ` Guenter Roeck
2021-09-16 20:10                       ` Oskar Senft
2021-09-17  3:09                         ` Oskar Senft
2021-09-17  3:29                           ` Guenter Roeck
2021-09-12  4:03     ` Guenter Roeck
2021-09-10 21:03 ` Rob Herring

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c410eba9-f6cf-4dbf-797f-48afde9c1898@roeck-us.net \
    --to=linux@roeck-us.net \
    --cc=devicetree@vger.kernel.org \
    --cc=jdelvare@suse.com \
    --cc=linux-hwmon@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=osk@google.com \
    --cc=robh+dt@kernel.org \
    --subject='Re: [PATCH] dt-bindings: hwmon: Add nct7802 bindings' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).