Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: Guenter Roeck <groeck7@gmail.com>
To:     =?UTF-8?Q?Jan_Kundr=c3=a1t?= <jan.kundrat@cesnet.cz>
Cc:     =?UTF-8?B?VsOhY2xhdiBLdWJlcm7DoXQ=?= <kubernat@cesnet.cz>,
        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
References: <20210518211609.GA3532746@roeck-us.net>
 <6f256c72-df4d-4f9a-ba5f-eabfd9f2365f@cesnet.cz>
From:   Guenter Roeck <linux@roeck-us.net>
Subject: Re: [PATCH v5 2/5] hwmon: (max31790) Fix and split pwm*_enable
Message-ID: <76619e11-3999-1e89-de93-fb5942970844@roeck-us.net>
Date:   Wed, 19 May 2021 06:55:42 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.1
MIME-Version: 1.0
In-Reply-To: <6f256c72-df4d-4f9a-ba5f-eabfd9f2365f@cesnet.cz>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

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.

Guenter