All of
 help / color / mirror / Atom feed
From: Guenter Roeck <>
To: "Jan Kundrát" <>
Cc: "Václav Kubernát" <>,
	"Jean Delvare" <>,
	"Jonathan Corbet" <>,,,
Subject: Re: [PATCH v5 2/5] hwmon: (max31790) Fix and split pwm*_enable
Date: Wed, 19 May 2021 06:55:42 -0700	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On 5/19/21 2:10 AM, Jan Kundrát wrote:
>> As it turns out, even the current code doesn't really work for fans 7..12.
>>         sr = get_tach_period(data->fan_dynamics[channel]);
>> However, the data->fan_dynamics array has only 6 entries, not 12, so
>> reading fan[7-12]_input will result in bad/random values.
> Hi Guenter, I'm Vaclav's colleague. The chip can indeed reconfigure each PWMOUT pin either as a PWM output or as a TACH input, but that's not something that's correctly implemented in the current code, and we have no use for that either (and we cannot test that on our PCBs easily, we do not have the manufacturer's eval kit).
> It looks to me that the original bug is that the current docs mention 12 fan inputs. Would you be OK with a patch series which fixes the docs so that the chip always exports 6 TACH inputs and 6 PWMOUT channels?
That would not be appropriate. The chip does support 12 fan inputs,
so that is not a bug. Its support has a bug, and the datasheet is kind
of vague when it comes to details, but that doesn't mean we can just
remove its support.

I see two bugs in the current code:
- pwm values should be read from the duty cycle registers,
   not from the target duty cycle registers.
- fan[1-12]_input needs to use a correct divider value.
   Unfortunately the datasheet is a bit vague when it comes
   to deciding which divider value to use, so the best we can
   do is going to use the values from fan[1-6].

Fixing this will require two patches, which should come first.
Let me know if you want to do that; if not I'll write patches
in the next few days.

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.


  reply	other threads:[~2021-05-19 13:55 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-18 21:16 [PATCH v5 2/5] hwmon: (max31790) Fix and split pwm*_enable Guenter Roeck
2021-05-19  9:10 ` Jan Kundrát
2021-05-19 13:55   ` Guenter Roeck [this message]
2021-05-20 11:29     ` Jan Kundrát
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:

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

  git send-email \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.