Hi Marek,
On Mon, 23 Jun 2025 at 22:44, Marek Vasut <marek.vasut@xxxxxxxxxxx> wrote:
> On 6/23/25 9:53 PM, Uwe Kleine-König wrote:
> > 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 ?
Perhaps modelling it as a pwm-driver was not the right thing to do?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
