linux-doc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Jan Kundrát" <jan.kundrat@cesnet.cz>
To: "Guenter Roeck" <linux@roeck-us.net>,
	"Václav Kubernát" <kubernat@cesnet.cz>
Cc: Jean Delvare <jdelvare@suse.com>,
	Jonathan Corbet <corbet@lwn.net>, <linux-hwmon@vger.kernel.org>,
	<linux-doc@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v5 2/5] hwmon: (max31790) Fix and split pwm*_enable
Date: Thu, 20 May 2021 13:29:24 +0200	[thread overview]
Message-ID: <9bbdc7a7-f34d-4e3a-8e64-c20810456d11@cesnet.cz> (raw)
In-Reply-To: <76619e11-3999-1e89-de93-fb5942970844@roeck-us.net>

> As for fan[7-12]_enable, I don't even know if those can be enabled
> separately. I see two options: Drop those attributes entirely (
> assuming that those fan inputs are always enabled if the associated
> pins are configured as inputs), or align them with fan[1-6]_enable.

I think we need to decide first who provides the initial configuration for 
this chip. There's always at least six TACH inputs, and then there's six 
more pins where each can be either a PWM output or a TACH input. Who 
decides that? Is the kernel supposed to export six knobs to the userspace? 
So far, I've assumed that this should be driven via sysfs. But perhaps you 
would you like to rely on the FW (?) to set this up properly? (On our 
board, that would be a few random calls to `i2cset` from a U-Boot boot 
script. Not pretty, but doable. Just one more place to keep track of.)

It's proabably "tricky" to do this at runtime -- and I don't expect to see 
many boards where you have such a big freedom of reconnecting the actual 
fans once manufactured, anyway. So, either some DT parameters, or an 
autodetection based on whatever is in the registers at power up, which 
would make an explicit assumption that "something" has set up the nPWM/TACH 
bits properly in the Fan Configuration Register. OK, that might work, but 
the kernel must not ever reset that chip afterwards.

There's also the Fan Fault Mask register, which controls which fans 
propagate their failures to the nFAN_FAIL output pin. This one requires a 
semi-independent control than the nPWM/TACH bit above. It's feasible that 
not all TACH inputs have an actual fan connected, and this can well vary 
between products. For example, ours has just four fan connectors, so we 
don't want "failures" of fans 5 and 6 to assert the nFAN_FAIL pin. Also, 
there should be a check which prevents unmasking these failures for those 
TACH channels which are configured as PWM outputs. Or we can once again 
ignore this one and rely on the FW.

The current kernel code in max31790_read_fan() reads beyond the end of 
data->fan_dynamics, hitting the content of `fault_status` or `tach` fields 
instead, and therefore returning garbage. Not a big deal, just a missing % 
operator I guess, but to me, that's a pretty strong suggestion that nobody 
has used or even tested monitoring more than six fans on this chip, ever. 
(And yeah, the datasheet is not clear on how it's supposed to work anyway. 
Using a modulo is just a guess.)

Neither Vaclav nor me have any way of testing this feature -- hence my 
proposal to only improve what we need, and ignore TACH channels 7-12. But I 
guess it's not OK from your point of view?

With kind regards,
Jan

  reply	other threads:[~2021-05-20 11:31 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-18 21:16 Guenter Roeck
2021-05-19  9:10 ` Jan Kundrát
2021-05-19 13:55   ` Guenter Roeck
2021-05-20 11:29     ` Jan Kundrát [this message]
2021-05-20 11:50       ` Guenter Roeck
  -- strict thread matches above, loose matches on Subject: below --
2021-05-12  1:30 [PATCH v5 1/5] hwmon: (max31790) Rework to use regmap Václav Kubernát
2021-05-12  1:30 ` [PATCH v5 2/5] hwmon: (max31790) Fix and split pwm*_enable Václav Kubernát
2021-05-12  1:32   ` Václav Kubernát
2021-05-18 14:59     ` Guenter Roeck
2021-05-19 23:11   ` Guenter Roeck

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=9bbdc7a7-f34d-4e3a-8e64-c20810456d11@cesnet.cz \
    --to=jan.kundrat@cesnet.cz \
    --cc=corbet@lwn.net \
    --cc=jdelvare@suse.com \
    --cc=kubernat@cesnet.cz \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-hwmon@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --subject='Re: [PATCH v5 2/5] hwmon: (max31790) Fix and split pwm*_enable' \
    /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).