Re: [PATCH v4 3/3] pwm: argon-fan-hat: Add Argon40 Fan HAT support

[Marek Vasut <marek.vasut@mailbox.org>]

On 6/23/25 9:53 PM, Uwe Kleine-König wrote:
> Hello Marek,

Hello Uwe,

> On Mon, Jun 23, 2025 at 07:30:33PM +0200, Marek Vasut wrote:
>> On 6/23/25 11:11 AM, Uwe Kleine-König wrote:
>>> when I replied to v3 this v4 was already on the list which I missed. My
>>> concern applies here, too, though.
>>>
>>> On Sat, Jun 21, 2025 at 07:19:56PM +0200, Marek Vasut wrote:
>>>> +static void argon_fan_hat_i2c_shutdown(struct i2c_client *i2c)
>>>> +{
>>>> +	argon_fan_hat_write(i2c, 100);
>>>> +}
>>>
>>> If you drop this, I'm willing to apply.
>>
>> Dropping this would make the hardware which uses this device more
>> susceptible to thermal damage, e.g. in case it gets stuck during reboot and
>> does not boot Linux afterward. I don't want to risk such thermal damage.
> 
> We agree here. But the right place to address this is the pwm-fan
> driver. A PWM is supposed to do exactly and only what its consumer wants
> it to do (in the limits set by hardware). Officially a PWM driver
> doesn't know the polarity of a fan, so `argon_fan_hat_write(i2c, 100)`
> might fully enable or complete disable the fan. The fan-driver knows the
> polarity. The PWM driver doesn't even know that it controls a fan. And
> the next guy takes the argon device and controls a motor with it --- and
> wonders that the vehicle gives full-speed at shutdown.

I suspect this cannot happen without non-standard hardware change of 
this device, see the link which shows what this device is, it is an 
integrated PWM+fan device:

Argon Fan HAT https://argon40.com/products/argon-fan-hat

> So I hope we also agree that the pwm-fan driver (or an even more generic
> place if possible that applies to all fan drivers) is the right layer to
> fix this. And note that the pwm-fan driver already has such a decision
> implemented, it's just the wrong one from your POV as it disables the
> fan at shutdown. For me this is another confirmation that having a
> shutdown callback in the PWM driver is wrong. The two affected drivers
> shouldn't fight about what is the right policy.

I would fully agree with this argument for a generic PWM controller, but 
this isn't one, this is a combined PWM+fan device.

The PWM driver is the last one that is being shut down, it is being shut 
down after the pwm-fan driver. If the pwm-fan driver fails for whatever 
reason, the PWM driver -- in this case driver for a combined PWM+fan 
device -- should make sure that the hardware does not melt. So I would 
argue that, for this specific device, the shutdown hook here is correct.

I would propose to keep the shutdown hook here, and extend the pwm-fan 
driver to be aligned with the behavior of the shutdown hook here. Does 
that work for you ?