Hello Marek,
On Mon, Jun 23, 2025 at 10:44:28PM +0200, Marek Vasut 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
I know people using hardware in different ways than specified by the
vendor, so this is a weak argument.
> > 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.
You model it as PWM + fan, and that's fine. I don't want special sauce
in the PWM driver part. And you don't need special sauce in the PWM
driver part to ensure the fan to spin up at shutdown.
> The PWM driver is the last one that is being shut down, it is being shut
> down after the pwm-fan driver.
Did you notice that the i2c bus driver is shut down still later? I
suggest to ensure there that the fan isn't disabled. /s
> 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.
The most likely problem in the pwm-fan driver is that the i2c bus is
broken, the pwm driver doesn't help here either. How far do you want to
go? Force the driver to =y in .config? Probe it even when it's missing
in the device tree? Provide the fan device if PWM_FAN is disabled?
From a software architecture POV splitting responsibilities to different
components is the right thing to do. This helps to keep maintainability
efforts within bounds, doesn't surprise developers that research the fan
behaviour and so check the fan driver, and increase reusability and
solving problems only once. Also the next PWM driver author creating an
i2c based PWM driver won't copy the shutdown hook from the argon40
driver which probably is even more wrong for that one.
> 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 ?
I stand to my request: Drop the shutdown hook and adapt the pwm-fan
driver to your needs.
Best regards
Uwe
Attachment:
signature.asc
Description: PGP signature
