[PATCH v4 1/3] dt-bindings: vendor-prefixes: Document Argon40

[PATCH v4 1/3] dt-bindings: vendor-prefixes: Document Argon40

[Marek Vasut]

Argon 40 Technologies Limited is a SBC expansion board vendor.
Document the prefix. For details see https://argon40.com .

Acked-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: "Uwe Kleine-König" <ukleinek@kernel.org>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: linux-pwm@vger.kernel.org
Cc: linux-renesas-soc@vger.kernel.org
---
V2: Add AB from Conor
V3: No change
V4: No change
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 3f23f6f93b62..27e2ad136931 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -149,6 +149,8 @@ patternProperties:
     description: Arctic Sand
   "^arcx,.*":
     description: arcx Inc. / Archronix Inc.
+  "^argon40,.*":
+    description: Argon 40 Technologies Limited
   "^ariaboard,.*":
     description: Shanghai Novotech Co., Ltd. (Ariaboard)
   "^aries,.*":
-- 
2.47.2

[PATCH v4 2/3] dt-bindings: pwm: argon40,fan-hat: Document Argon40 Fan HAT

[Marek Vasut]

Document trivial PWM on Argon40 Fan HAT, which is a RaspberryPi
blower fan hat which can be controlled over I2C.

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: "Uwe Kleine-König" <ukleinek@kernel.org>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: linux-pwm@vger.kernel.org
Cc: linux-renesas-soc@vger.kernel.org
---
V2: Implement dedicated binding document
V3: Update the description and pwm-cells
V4: Drop | from description and fix up pwm-cells = <2> in example
---
 .../bindings/pwm/argon40,fan-hat.yaml         | 48 +++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/argon40,fan-hat.yaml

diff --git a/Documentation/devicetree/bindings/pwm/argon40,fan-hat.yaml b/Documentation/devicetree/bindings/pwm/argon40,fan-hat.yaml
new file mode 100644
index 000000000000..7dbc7c2cd802
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/argon40,fan-hat.yaml
@@ -0,0 +1,48 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/argon40,fan-hat.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Argon40 Fan HAT PWM controller
+
+maintainers:
+  - Marek Vasut <marek.vasut+renesas@mailbox.org>
+
+description:
+  The trivial PWM on Argon40 Fan HAT, which is a RaspberryPi blower fan
+  hat which can be controlled over I2C, generates a fixed 30 kHz period
+  PWM signal with configurable 0..100% duty cycle to control the fan
+  speed.
+
+allOf:
+  - $ref: pwm.yaml#
+
+properties:
+  compatible:
+    const: argon40,fan-hat
+
+  reg:
+    maxItems: 1
+
+  "#pwm-cells":
+    const: 3
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      pwm@1a {
+        compatible = "argon40,fan-hat";
+        reg = <0x1a>;
+        #pwm-cells = <3>;
+      };
+    };
-- 
2.47.2

Re: [PATCH v4 2/3] dt-bindings: pwm: argon40,fan-hat: Document Argon40 Fan HAT

[Rob Herring (Arm)]

On Sat, 21 Jun 2025 19:19:55 +0200, Marek Vasut wrote:
> Document trivial PWM on Argon40 Fan HAT, which is a RaspberryPi
> blower fan hat which can be controlled over I2C.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> ---
> Cc: "Uwe Kleine-König" <ukleinek@kernel.org>
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> Cc: Rob Herring <robh@kernel.org>
> Cc: devicetree@vger.kernel.org
> Cc: linux-pwm@vger.kernel.org
> Cc: linux-renesas-soc@vger.kernel.org
> ---
> V2: Implement dedicated binding document
> V3: Update the description and pwm-cells
> V4: Drop | from description and fix up pwm-cells = <2> in example
> ---
>  .../bindings/pwm/argon40,fan-hat.yaml         | 48 +++++++++++++++++++
>  1 file changed, 48 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/argon40,fan-hat.yaml
> 

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

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

[Marek Vasut]

Add trivial PWM driver for Argon40 Fan HAT, which is a RaspberryPi
blower fan hat which can be controlled over I2C. Model this device
as a PWM, so the pwm-fan can be attached to it and handle thermal
zones and RPM management in a generic manner.

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: "Uwe Kleine-König" <ukleinek@kernel.org>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: linux-pwm@vger.kernel.org
Cc: linux-renesas-soc@vger.kernel.org
---
V2: - Switch to waveform ops
    - Add shutdown hook to force the fan to maximum RPM on shutdown
      instead of stopping it, to be on the safe side
V3: - Find the 30 kHz fixed period PWM, use that
    - Add comments
    - Consolidate argon_fan_hat_write()
V4: - Add if (wf->duty_length_ns > ARGON40_FAN_HAT_PERIOD_NS) overflow check
    - Use i2c_smbus_write_byte_data()
    - Rename struct pwm_chip *pc to struct pwm_chip *chip
    - Remove tab alignment from argon_fan_hat_pwm_ops {}
    - Define ARGON40_FAN_HAT_REG_DUTY_CYCLE macro
---
 drivers/pwm/Kconfig             |   9 +++
 drivers/pwm/Makefile            |   1 +
 drivers/pwm/pwm-argon-fan-hat.c | 120 ++++++++++++++++++++++++++++++++
 3 files changed, 130 insertions(+)
 create mode 100644 drivers/pwm/pwm-argon-fan-hat.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 6e113f8b4baf..3ef1757502eb 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -66,6 +66,15 @@ config PWM_APPLE
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-apple.
 
+config PWM_ARGON_FAN_HAT
+	tristate "Argon40 Fan HAT support"
+	depends on I2C && OF
+	help
+	  Generic PWM framework driver for Argon40 Fan HAT.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-argon-fan-hat.
+
 config PWM_ATMEL
 	tristate "Atmel PWM support"
 	depends on ARCH_AT91 || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 96160f4257fc..ff4f47e5fb7a 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -3,6 +3,7 @@ obj-$(CONFIG_PWM)		+= core.o
 obj-$(CONFIG_PWM_AB8500)	+= pwm-ab8500.o
 obj-$(CONFIG_PWM_ADP5585)	+= pwm-adp5585.o
 obj-$(CONFIG_PWM_APPLE)		+= pwm-apple.o
+obj-$(CONFIG_PWM_ARGON_FAN_HAT)	+= pwm-argon-fan-hat.o
 obj-$(CONFIG_PWM_ATMEL)		+= pwm-atmel.o
 obj-$(CONFIG_PWM_ATMEL_HLCDC_PWM)	+= pwm-atmel-hlcdc.o
 obj-$(CONFIG_PWM_ATMEL_TCB)	+= pwm-atmel-tcb.o
diff --git a/drivers/pwm/pwm-argon-fan-hat.c b/drivers/pwm/pwm-argon-fan-hat.c
new file mode 100644
index 000000000000..1bf07c769497
--- /dev/null
+++ b/drivers/pwm/pwm-argon-fan-hat.c
@@ -0,0 +1,120 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2025 Marek Vasut
+ *
+ * Limitations:
+ * - no support for offset/polarity
+ * - fixed 30 kHz period
+ *
+ * Argon Fan HAT https://argon40.com/products/argon-fan-hat
+ */
+
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/pwm.h>
+
+#define ARGON40_FAN_HAT_PERIOD_NS	33333	/* ~30 kHz */
+
+#define ARGON40_FAN_HAT_REG_DUTY_CYCLE	0x80
+
+static int argon_fan_hat_round_waveform_tohw(struct pwm_chip *chip,
+					     struct pwm_device *pwm,
+					     const struct pwm_waveform *wf,
+					     void *_wfhw)
+{
+	u8 *wfhw = _wfhw;
+
+	if (wf->duty_length_ns > ARGON40_FAN_HAT_PERIOD_NS)
+		*wfhw = 100;
+	else
+		*wfhw = mul_u64_u64_div_u64(wf->duty_length_ns, 100, ARGON40_FAN_HAT_PERIOD_NS);
+
+	return 0;
+}
+
+static int argon_fan_hat_round_waveform_fromhw(struct pwm_chip *chip,
+					       struct pwm_device *pwm,
+					       const void *_wfhw,
+					       struct pwm_waveform *wf)
+{
+	const u8 *wfhw = _wfhw;
+
+	wf->period_length_ns = ARGON40_FAN_HAT_PERIOD_NS;
+	wf->duty_length_ns = DIV64_U64_ROUND_UP(wf->period_length_ns * *wfhw, 100);
+	wf->duty_offset_ns = 0;
+
+	return 0;
+}
+
+static int argon_fan_hat_write(struct i2c_client *i2c, const u8 wfhw)
+{
+	return i2c_smbus_write_byte_data(i2c, ARGON40_FAN_HAT_REG_DUTY_CYCLE, wfhw);
+}
+
+static int argon_fan_hat_write_waveform(struct pwm_chip *chip,
+					struct pwm_device *pwm,
+					const void *_wfhw)
+{
+	struct i2c_client *i2c = pwmchip_get_drvdata(chip);
+	const u8 *wfhw = _wfhw;
+
+	return argon_fan_hat_write(i2c, *wfhw);
+}
+
+static const struct pwm_ops argon_fan_hat_pwm_ops = {
+	.sizeof_wfhw = sizeof(u8),
+	.round_waveform_fromhw = argon_fan_hat_round_waveform_fromhw,
+	.round_waveform_tohw = argon_fan_hat_round_waveform_tohw,
+	.write_waveform = argon_fan_hat_write_waveform,
+	/*
+	 * The controller does not provide any way to read info back,
+	 * reading from the controller stops the fan, therefore there
+	 * is no .read_waveform here.
+	 */
+};
+
+static int argon_fan_hat_i2c_probe(struct i2c_client *i2c)
+{
+	struct pwm_chip *chip = devm_pwmchip_alloc(&i2c->dev, 1, 0);
+	int ret;
+
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+
+	chip->ops = &argon_fan_hat_pwm_ops;
+	pwmchip_set_drvdata(chip, i2c);
+
+	ret = devm_pwmchip_add(&i2c->dev, chip);
+	if (ret)
+		return dev_err_probe(&i2c->dev, ret, "Could not add PWM chip\n");
+
+	return 0;
+}
+
+static void argon_fan_hat_i2c_shutdown(struct i2c_client *i2c)
+{
+	argon_fan_hat_write(i2c, 100);
+}
+
+static const struct of_device_id argon_fan_hat_dt_ids[] = {
+	{ .compatible = "argon40,fan-hat" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, argon_fan_hat_dt_ids);
+
+static struct i2c_driver argon_fan_hat_driver = {
+	.driver = {
+		.name = "argon-fan-hat",
+		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
+		.of_match_table = argon_fan_hat_dt_ids,
+	},
+	.probe = argon_fan_hat_i2c_probe,
+	.shutdown = argon_fan_hat_i2c_shutdown,
+};
+
+module_i2c_driver(argon_fan_hat_driver);
+
+MODULE_AUTHOR("Marek Vasut <marek.vasut+renesas@mailbox.org>");
+MODULE_DESCRIPTION("Argon40 Fan HAT");
+MODULE_LICENSE("GPL");
-- 
2.47.2

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 347 bytes --]

Hello,

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.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Marek Vasut]

On 6/23/25 11:11 AM, Uwe Kleine-König wrote:
> Hello,

Hello Uwe,

> 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.

-- 
Best regards,
Marek Vasut

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1786 bytes --]

Hello Marek,

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.

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.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Marek Vasut]

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 ?

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

[Geert Uytterhoeven]

Hi Marek,

On Mon, 23 Jun 2025 at 22:44, Marek Vasut <marek.vasut@mailbox.org> 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@linux-m68k.org

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

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 4253 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]