[PATCH v7 0/3] hwmon: (adt7475) duty cycle configuration

[PATCH v7 0/3] hwmon: (adt7475) duty cycle configuration

[Chris Packham]

I have a system that has very over spec'd fans so the amount of noise when they
run at 100% duty cycle is considerable. We have userspace monitoring tools that
will configure appropriate fan control parameters but there is a bit of a delay
between the kernel loading the driver and the userland tools catching up to
configure the fan control. This series adds device properties that allow the
PWM duty cycle to be specified via device properties so the PWM duty cycle can
be reduced as soon as possible.

This series attempts to setup the adt7475 as a pwm provider so that we can
specify these properties. The devicetree support was reasonably straight
forward (example usage is in the binding patch). I struggled to get the ACPI
version working well and in the end the code had to distinguish between the
of_node and other case. The ASL I've ended up with is

    Device (ADT0)
    {
        Name (_HID, "PRP0001")
        Name (_CRS, ResourceTemplate () {
            I2cSerialBusV2 (0x2E, ControllerInitiated,
                            100000, AddressingMode7Bit,
                            "^^CH00", 0x00,
                            ResourceConsumer, , Exclusive, )
        })
        Name (_DSD, Package () {
            ToUUID (UUID_DEVICE_PROPERTIES),
            Package () {
                Package () { "compatible", "adi,adt7476" },
                Package () { "#pwm-cells", 4 },
            },
        })
        Device (FAN0)
        {
            Name (_ADR, 0)
            Name (_DSD, Package () {
                ToUUID (UUID_DEVICE_PROPERTIES),
                Package () {
                    Package () { "pwms", Package () { 0, 44444, 1, 22222 } },
                }
            })
        }
        Device (FAN1)
        {
            Name (_ADR, 0)
            Name (_DSD, Package () {
                ToUUID (UUID_DEVICE_PROPERTIES),
                Package () {
                    Package () { "pwms", Package () { 2, 44444, 1, 22222 } },
                }
            })
        }
    }

If had to introduce a code path that parses that because try as I might I could
not convince fwnode_property_get_reference_args() to fetch the information out
of the ACPI data. If I've missed something obvious please let me know.

Chris Packham (3):
  dt-bindings: hwmon: Add adt7475 fan/pwm properties
  dt-bindings: hwmon: adt7475: Deprecate adi,pwm-active-state
  hwmon: (adt7475) Add support for configuring initial PWM state

 .../devicetree/bindings/hwmon/adt7475.yaml    |  37 ++++-
 drivers/hwmon/adt7475.c                       | 131 ++++++++++++++++++
 2 files changed, 166 insertions(+), 2 deletions(-)

-- 
2.45.2

[PATCH v7 1/3] dt-bindings: hwmon: Add adt7475 fan/pwm properties

[Chris Packham]

Add fan child nodes that allow describing the connections for the
ADT7475 to the fans it controls. This also allows setting some
initial values for the pwm duty cycle and frequency.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---

Notes:
    Changes in v7:
    - None
    Changes in v6:
    - Collect r-by from Rob
    Changes in v5:
    - Use nanoseconds for PWM frequency and duty cycle as per existing
      conventions for PWMs
    - Set flags to 0 in example to match adi,pwm-active-state setting
    Changes in v4:
    - 0 is not a valid frequency value
    Changes in v3:
    - Use the pwm provider/consumer bindings
    Changes in v2:
    - Document 0 as a valid value (leaves hardware as-is)

 .../devicetree/bindings/hwmon/adt7475.yaml    | 35 ++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
index 051c976ab711..df2b5b889e4d 100644
--- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml
+++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
@@ -51,6 +51,24 @@ properties:
       enum: [0, 1]
       default: 1
 
+  "#pwm-cells":
+    const: 4
+    description: |
+      Number of cells in a PWM specifier.
+      - 0: The PWM channel
+      - 1: The PWM period in nanoseconds
+           - 90909091 (11 Hz)
+           - 71428571 (14 Hz)
+           - 45454545 (22 Hz)
+           - 34482759 (29 Hz)
+           - 28571429 (35 Hz)
+           - 22727273 (44 Hz)
+           - 17241379 (58 Hz)
+           - 11363636 (88 Hz)
+           - 44444 (22 kHz)
+      - 2: PWM flags 0 or PWM_POLARITY_INVERTED
+      - 3: The default PWM duty cycle in nanoseconds
+
 patternProperties:
   "^adi,bypass-attenuator-in[0-4]$":
     description: |
@@ -81,6 +99,10 @@ patternProperties:
       - smbalert#
       - gpio
 
+  "^fan-[0-9]+$":
+    $ref: fan-common.yaml#
+    unevaluatedProperties: false
+
 required:
   - compatible
   - reg
@@ -89,11 +111,12 @@ additionalProperties: false
 
 examples:
   - |
+    #include <dt-bindings/pwm/pwm.h>
     i2c {
       #address-cells = <1>;
       #size-cells = <0>;
 
-      hwmon@2e {
+      pwm: hwmon@2e {
         compatible = "adi,adt7476";
         reg = <0x2e>;
         adi,bypass-attenuator-in0 = <1>;
@@ -101,5 +124,15 @@ examples:
         adi,pwm-active-state = <1 0 1>;
         adi,pin10-function = "smbalert#";
         adi,pin14-function = "tach4";
+        #pwm-cells = <4>;
+
+        /* PWMs at 22.5 kHz frequency, 50% duty*/
+        fan-0 {
+          pwms = <&pwm 0 44444 0 22222>;
+        };
+
+        fan-1 {
+          pwms = <&pwm 2 44444 0 22222>;
+        };
       };
     };
-- 
2.45.2

[PATCH v7 2/3] dt-bindings: hwmon: adt7475: Deprecate adi,pwm-active-state

[Chris Packham]

Now that we have fan child nodes that can specify flags for the PWM
outputs we no longer need the adi,pwm-active-state property.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Acked-by: Rob Herring (Arm) <robh@kernel.org>
---

Notes:
    Changes in v6 & v7:
    - None
    Changes in v5:
    - Add ack from Rob
    Changes in v4:
    - None
    Changes in v3:
    - New

 Documentation/devicetree/bindings/hwmon/adt7475.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
index df2b5b889e4d..79e8d62fa3b3 100644
--- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml
+++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
@@ -45,6 +45,7 @@ properties:
       the pwm uses a logic low output for 100% duty cycle. If set to 1 the pwm
       uses a logic high output for 100% duty cycle.
     $ref: /schemas/types.yaml#/definitions/uint32-array
+    deprecated: true
     minItems: 3
     maxItems: 3
     items:
@@ -121,7 +122,6 @@ examples:
         reg = <0x2e>;
         adi,bypass-attenuator-in0 = <1>;
         adi,bypass-attenuator-in1 = <0>;
-        adi,pwm-active-state = <1 0 1>;
         adi,pin10-function = "smbalert#";
         adi,pin14-function = "tach4";
         #pwm-cells = <4>;
-- 
2.45.2

[PATCH v7 3/3] hwmon: (adt7475) Add support for configuring initial PWM state

[Chris Packham]

By default the PWM duty cycle in hardware is 100%. On some systems this
can cause unwanted fan noise. Add the ability to specify the fan
connections and initial state of the PWMs via device properties.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    Changes in v7:
    - Use plain / for freq_hz since this can't overflow. Use div_u64 to
      avoid overflow when calculating duty.
    Changes in v6:
    - Use do_div() instead of plain /
    - Use a helper function to avoid repetition between the of and non-of
      code paths.
    Changes in v5:
    - Deal with PWM frequency and duty cycle being specified in nanoseconds
    Changes in v4:
    - Support DT and ACPI fwnodes
    - Put PWM into manual mode
    Changes in v3:
    - Use the pwm provider/consumer bindings
    Changes in v2:
    - Use correct device property string for frequency
    - Allow -EINVAL and only warn on error
    - Use a frequency of 0 to indicate that the hardware should be left as-is

 drivers/hwmon/adt7475.c | 131 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 131 insertions(+)

diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
index 4224ffb30483..2037a4b57031 100644
--- a/drivers/hwmon/adt7475.c
+++ b/drivers/hwmon/adt7475.c
@@ -21,6 +21,8 @@
 #include <linux/of.h>
 #include <linux/util_macros.h>
 
+#include <dt-bindings/pwm/pwm.h>
+
 /* Indexes for the sysfs hooks */
 
 #define INPUT		0
@@ -1662,6 +1664,131 @@ static int adt7475_set_pwm_polarity(struct i2c_client *client)
 	return 0;
 }
 
+struct adt7475_pwm_config {
+	int index;
+	int freq;
+	int flags;
+	int duty;
+};
+
+static int _adt7475_pwm_properties_parse_args(u32 args[4], struct adt7475_pwm_config *cfg)
+{
+	int freq_hz;
+	int duty;
+
+	if (args[1] == 0)
+		return -EINVAL;
+
+	freq_hz = 1000000000UL / args[1];
+	if (args[3] >= args[1])
+		duty = 255;
+	else
+		duty = div_u64(255ULL * args[3], args[1]);
+
+	cfg->index = args[0];
+	cfg->freq = find_closest(freq_hz, pwmfreq_table, ARRAY_SIZE(pwmfreq_table));
+	cfg->flags = args[2];
+	cfg->duty = duty;
+
+	return 0;
+}
+
+static int adt7475_pwm_properties_parse_reference_args(struct fwnode_handle *fwnode,
+						       struct adt7475_pwm_config *cfg)
+{
+	int ret, i;
+	struct fwnode_reference_args rargs = {};
+	u32 args[4] = {};
+
+	ret = fwnode_property_get_reference_args(fwnode, "pwms", "#pwm-cells", 0, 0, &rargs);
+	if (ret)
+		return ret;
+
+	if (rargs.nargs != 4) {
+		fwnode_handle_put(rargs.fwnode);
+		return -EINVAL;
+	}
+
+	for (i = 0; i < 4; i++)
+		args[i] = rargs.args[i];
+
+	ret = _adt7475_pwm_properties_parse_args(args, cfg);
+
+	fwnode_handle_put(rargs.fwnode);
+
+	return ret;
+}
+
+static int adt7475_pwm_properties_parse_args(struct fwnode_handle *fwnode,
+					     struct adt7475_pwm_config *cfg)
+{
+	int ret;
+	u32 args[4] = {};
+
+	ret = fwnode_property_read_u32_array(fwnode, "pwms", args, ARRAY_SIZE(args));
+	if (ret)
+		return ret;
+
+	return _adt7475_pwm_properties_parse_args(args, cfg);
+
+}
+
+static int adt7475_fan_pwm_config(struct i2c_client *client)
+{
+	struct adt7475_data *data = i2c_get_clientdata(client);
+	struct fwnode_handle *child;
+	struct adt7475_pwm_config cfg = {};
+	int ret;
+
+	device_for_each_child_node(&client->dev, child) {
+		if (!fwnode_property_present(child, "pwms"))
+			continue;
+
+		if (is_of_node(child))
+			ret = adt7475_pwm_properties_parse_reference_args(child, &cfg);
+		else
+			ret = adt7475_pwm_properties_parse_args(child, &cfg);
+
+		if (cfg.index >= ADT7475_PWM_COUNT)
+			return -EINVAL;
+
+		ret = adt7475_read(PWM_CONFIG_REG(cfg.index));
+		if (ret < 0)
+			return ret;
+		data->pwm[CONTROL][cfg.index] = ret;
+		if (cfg.flags & PWM_POLARITY_INVERTED)
+			data->pwm[CONTROL][cfg.index] |= BIT(4);
+		else
+			data->pwm[CONTROL][cfg.index] &= ~BIT(4);
+
+		/* Force to manual mode so PWM values take effect */
+		data->pwm[CONTROL][cfg.index] &= ~0xE0;
+		data->pwm[CONTROL][cfg.index] |= 0x07 << 5;
+
+		ret = i2c_smbus_write_byte_data(client, PWM_CONFIG_REG(cfg.index),
+						data->pwm[CONTROL][cfg.index]);
+		if (ret)
+			return ret;
+
+		data->pwm[INPUT][cfg.index] = cfg.duty;
+		ret = i2c_smbus_write_byte_data(client, PWM_REG(cfg.index),
+						data->pwm[INPUT][cfg.index]);
+		if (ret)
+			return ret;
+
+		data->range[cfg.index] = adt7475_read(TEMP_TRANGE_REG(cfg.index));
+		data->range[cfg.index] &= ~0xf;
+		data->range[cfg.index] |= cfg.freq;
+
+		ret = i2c_smbus_write_byte_data(client, TEMP_TRANGE_REG(cfg.index),
+						data->range[cfg.index]);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int adt7475_probe(struct i2c_client *client)
 {
 	enum chips chip;
@@ -1778,6 +1905,10 @@ static int adt7475_probe(struct i2c_client *client)
 	if (ret && ret != -EINVAL)
 		dev_warn(&client->dev, "Error configuring pwm polarity\n");
 
+	ret = adt7475_fan_pwm_config(client);
+	if (ret)
+		dev_warn(&client->dev, "Error %d configuring fan/pwm\n", ret);
+
 	/* Start monitoring */
 	switch (chip) {
 	case adt7475:
-- 
2.45.2

Re: [PATCH v7 1/3] dt-bindings: hwmon: Add adt7475 fan/pwm properties

[Guenter Roeck]

On Tue, Jul 23, 2024 at 10:17:35AM +1200, Chris Packham wrote:
> Add fan child nodes that allow describing the connections for the
> ADT7475 to the fans it controls. This also allows setting some
> initial values for the pwm duty cycle and frequency.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

Applied.

Thanks,
Guenter

Re: [PATCH v7 2/3] dt-bindings: hwmon: adt7475: Deprecate adi,pwm-active-state

[Guenter Roeck]

On Tue, Jul 23, 2024 at 10:17:36AM +1200, Chris Packham wrote:
> Now that we have fan child nodes that can specify flags for the PWM
> outputs we no longer need the adi,pwm-active-state property.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Acked-by: Rob Herring (Arm) <robh@kernel.org>

Applied.

Thanks,
Guenter

Re: [PATCH v7 3/3] hwmon: (adt7475) Add support for configuring initial PWM state

[Guenter Roeck]

On Tue, Jul 23, 2024 at 10:17:37AM +1200, Chris Packham wrote:
> By default the PWM duty cycle in hardware is 100%. On some systems this
> can cause unwanted fan noise. Add the ability to specify the fan
> connections and initial state of the PWMs via device properties.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

CHECK: Blank lines aren't necessary before a close brace '}'
#207: FILE: drivers/hwmon/adt7475.c:1734:
+
+}

Never mind, applied after fixing the above.

Thanks,
Guenter

Re: [PATCH v7 3/3] hwmon: (adt7475) Add support for configuring initial PWM state

[Chris Packham]

On 26/07/24 02:09, Guenter Roeck wrote:
> On Tue, Jul 23, 2024 at 10:17:37AM +1200, Chris Packham wrote:
>> By default the PWM duty cycle in hardware is 100%. On some systems this
>> can cause unwanted fan noise. Add the ability to specify the fan
>> connections and initial state of the PWMs via device properties.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> CHECK: Blank lines aren't necessary before a close brace '}'
> #207: FILE: drivers/hwmon/adt7475.c:1734:
> +
> +}
>
> Never mind, applied after fixing the above.

Hmm, odd checkpatch.pl doesn't complain for me

$ ./scripts/checkpatch.pl 
patches/adt-init-duty/v7/v7-0003-hwmon-adt7475-Add-support-for-configuring-initial.patch 

total: 0 errors, 0 warnings, 149 lines checked

patches/adt-init-duty/v7/v7-0003-hwmon-adt7475-Add-support-for-configuring-initial.patch 
has no obvious style problems and is ready for submission.

>
> Thanks,
> Guenter

Re: [PATCH v7 3/3] hwmon: (adt7475) Add support for configuring initial PWM state

[Guenter Roeck]

On 7/25/24 14:02, Chris Packham wrote:
> 
> On 26/07/24 02:09, Guenter Roeck wrote:
>> On Tue, Jul 23, 2024 at 10:17:37AM +1200, Chris Packham wrote:
>>> By default the PWM duty cycle in hardware is 100%. On some systems this
>>> can cause unwanted fan noise. Add the ability to specify the fan
>>> connections and initial state of the PWMs via device properties.
>>>
>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> CHECK: Blank lines aren't necessary before a close brace '}'
>> #207: FILE: drivers/hwmon/adt7475.c:1734:
>> +
>> +}
>>
>> Never mind, applied after fixing the above.
> 
> Hmm, odd checkpatch.pl doesn't complain for me
> 

This is a CHECK message. You did not use --strict.

Guenter

> $ ./scripts/checkpatch.pl
> patches/adt-init-duty/v7/v7-0003-hwmon-adt7475-Add-support-for-configuring-initial.patch
> 
> total: 0 errors, 0 warnings, 149 lines checked
> 
> patches/adt-init-duty/v7/v7-0003-hwmon-adt7475-Add-support-for-configuring-initial.patch
> has no obvious style problems and is ready for submission.
> 
>>
>> Thanks,
>> Guenter

Re: [PATCH v7 1/3] dt-bindings: hwmon: Add adt7475 fan/pwm properties

[Uwe Kleine-König]

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

Hello,

On Tue, Jul 23, 2024 at 10:17:35AM +1200, Chris Packham wrote:
> Add fan child nodes that allow describing the connections for the
> ADT7475 to the fans it controls. This also allows setting some
> initial values for the pwm duty cycle and frequency.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> ---
> 
> Notes:
>     Changes in v7:
>     - None
>     Changes in v6:
>     - Collect r-by from Rob
>     Changes in v5:
>     - Use nanoseconds for PWM frequency and duty cycle as per existing
>       conventions for PWMs
>     - Set flags to 0 in example to match adi,pwm-active-state setting
>     Changes in v4:
>     - 0 is not a valid frequency value
>     Changes in v3:
>     - Use the pwm provider/consumer bindings
>     Changes in v2:
>     - Document 0 as a valid value (leaves hardware as-is)
> 
>  .../devicetree/bindings/hwmon/adt7475.yaml    | 35 ++++++++++++++++++-
>  1 file changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> index 051c976ab711..df2b5b889e4d 100644
> --- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> @@ -51,6 +51,24 @@ properties:
>        enum: [0, 1]
>        default: 1
>  
> +  "#pwm-cells":
> +    const: 4

I asked to add support for #pwm-cells = <4> to the pwm core in reply to
v4 (see
https://lore.kernel.org/linux-pwm/drqvaon5lb2ei3jqofutbr6demibyfdhbmr24sva27gzpqdnon@fxa7rpl33iih/).

I'm unhappy to see this merged anyhow in combination with ad-hoc parsing
of the pwm properties in the driver :-\

> +    description: |
> +      Number of cells in a PWM specifier.
> +      - 0: The PWM channel
> +      - 1: The PWM period in nanoseconds
> +           - 90909091 (11 Hz)
> +           - 71428571 (14 Hz)
> +           - 45454545 (22 Hz)
> +           - 34482759 (29 Hz)
> +           - 28571429 (35 Hz)
> +           - 22727273 (44 Hz)
> +           - 17241379 (58 Hz)
> +           - 11363636 (88 Hz)
> +           - 44444 (22 kHz)
> +      - 2: PWM flags 0 or PWM_POLARITY_INVERTED
> +      - 3: The default PWM duty cycle in nanoseconds
> +

Best regards
Uwe

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

Re: [PATCH v7 1/3] dt-bindings: hwmon: Add adt7475 fan/pwm properties

[Chris Packham]

Hi Uwe,

On 28/05/2025 04:12, Uwe Kleine-König wrote:
> Hello,
>
> On Tue, Jul 23, 2024 at 10:17:35AM +1200, Chris Packham wrote:
>> Add fan child nodes that allow describing the connections for the
>> ADT7475 to the fans it controls. This also allows setting some
>> initial values for the pwm duty cycle and frequency.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
>> ---
>>
>> Notes:
>>      Changes in v7:
>>      - None
>>      Changes in v6:
>>      - Collect r-by from Rob
>>      Changes in v5:
>>      - Use nanoseconds for PWM frequency and duty cycle as per existing
>>        conventions for PWMs
>>      - Set flags to 0 in example to match adi,pwm-active-state setting
>>      Changes in v4:
>>      - 0 is not a valid frequency value
>>      Changes in v3:
>>      - Use the pwm provider/consumer bindings
>>      Changes in v2:
>>      - Document 0 as a valid value (leaves hardware as-is)
>>
>>   .../devicetree/bindings/hwmon/adt7475.yaml    | 35 ++++++++++++++++++-
>>   1 file changed, 34 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
>> index 051c976ab711..df2b5b889e4d 100644
>> --- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml
>> +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
>> @@ -51,6 +51,24 @@ properties:
>>         enum: [0, 1]
>>         default: 1
>>   
>> +  "#pwm-cells":
>> +    const: 4
> I asked to add support for #pwm-cells = <4> to the pwm core in reply to
> v4 (see
> https://lore.kernel.org/linux-pwm/drqvaon5lb2ei3jqofutbr6demibyfdhbmr24sva27gzpqdnon@fxa7rpl33iih/).
>
> I'm unhappy to see this merged anyhow in combination with ad-hoc parsing
> of the pwm properties in the driver :-\

As I mentioned at the time the adt7475 is not currently pwm_chip so I 
need the ad-hoc parsing in that driver. I'd be happy to take you 
prototype patch for pwm/core.c and polish it although I don't really 
have a good way of testing it.

>> +    description: |
>> +      Number of cells in a PWM specifier.
>> +      - 0: The PWM channel
>> +      - 1: The PWM period in nanoseconds
>> +           - 90909091 (11 Hz)
>> +           - 71428571 (14 Hz)
>> +           - 45454545 (22 Hz)
>> +           - 34482759 (29 Hz)
>> +           - 28571429 (35 Hz)
>> +           - 22727273 (44 Hz)
>> +           - 17241379 (58 Hz)
>> +           - 11363636 (88 Hz)
>> +           - 44444 (22 kHz)
>> +      - 2: PWM flags 0 or PWM_POLARITY_INVERTED
>> +      - 3: The default PWM duty cycle in nanoseconds
>> +
> Best regards
> Uwe

Re: [PATCH v7 1/3] dt-bindings: hwmon: Add adt7475 fan/pwm properties

[Uwe Kleine-König]

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

Hello Chris,

On Tue, May 27, 2025 at 08:24:56PM +0000, Chris Packham wrote:
> On 28/05/2025 04:12, Uwe Kleine-König wrote:
> > Hello,
> >
> > On Tue, Jul 23, 2024 at 10:17:35AM +1200, Chris Packham wrote:
> >> Add fan child nodes that allow describing the connections for the
> >> ADT7475 to the fans it controls. This also allows setting some
> >> initial values for the pwm duty cycle and frequency.
> >>
> >> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> >> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> >> ---
> >>
> >> Notes:
> >>      Changes in v7:
> >>      - None
> >>      Changes in v6:
> >>      - Collect r-by from Rob
> >>      Changes in v5:
> >>      - Use nanoseconds for PWM frequency and duty cycle as per existing
> >>        conventions for PWMs
> >>      - Set flags to 0 in example to match adi,pwm-active-state setting
> >>      Changes in v4:
> >>      - 0 is not a valid frequency value
> >>      Changes in v3:
> >>      - Use the pwm provider/consumer bindings
> >>      Changes in v2:
> >>      - Document 0 as a valid value (leaves hardware as-is)
> >>
> >>   .../devicetree/bindings/hwmon/adt7475.yaml    | 35 ++++++++++++++++++-
> >>   1 file changed, 34 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> >> index 051c976ab711..df2b5b889e4d 100644
> >> --- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> >> +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> >> @@ -51,6 +51,24 @@ properties:
> >>         enum: [0, 1]
> >>         default: 1
> >>   
> >> +  "#pwm-cells":
> >> +    const: 4
> > I asked to add support for #pwm-cells = <4> to the pwm core in reply to
> > v4 (see
> > https://lore.kernel.org/linux-pwm/drqvaon5lb2ei3jqofutbr6demibyfdhbmr24sva27gzpqdnon@fxa7rpl33iih/).
> >
> > I'm unhappy to see this merged anyhow in combination with ad-hoc parsing
> > of the pwm properties in the driver :-\
> 
> As I mentioned at the time the adt7475 is not currently pwm_chip so I 
> need the ad-hoc parsing in that driver. I'd be happy to take you 
> prototype patch for pwm/core.c and polish it although I don't really 
> have a good way of testing it.

It's more the deviation of the default binding for PWMs that I don't
like than the ad-hoc parsing. Ideally the adt7475 would provide a
pwmchip (as the binding suggests) and the fan would be formalized as a
pwm-fan. With the binding that was chosen here that option becomes more
ugly than necessary to implement.

If I understand correctly you need the default value for duty to
statically setup (or only initialize?) a fan, right? I'm not sure I like
extending #pwm-cells for a default duty value. Thinking about that a
while I'd prefer a binding that looks more like the clock configuration
stuff because actually having the period and flags as part of the
reference to the PWM to be used is also a bit strange. So I imagine
something like:

	mypwm: pwm {
		compatible = "...."
		#pwm-cells = <1>;
	};

	fan {
		compatible = "pwm-fan";
		pwms = <&mypwm 1>;
		assigned-pwms = <&mypwm>;
		assigned-pwm-default-period-lengths-ns = <40000>;
		assigned-pwm-default-flags = <PWM_POLARITY_INVERTED>;
	};

Then specifying a period (or later a duty cycle length) would be
optional and could be provided iff the device needs that for operation.

My mail was just me being frustrated about another special case that I'd
have to handle if I go into that direction. I should have been more
attentive to that development before it entered the mainline.

Best regards
Uwe

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

Re: [PATCH v7 1/3] dt-bindings: hwmon: Add adt7475 fan/pwm properties

[Chris Packham]

Hi Uwe,

On 28/05/2025 18:10, Uwe Kleine-König wrote:
> Hello Chris,
>
> On Tue, May 27, 2025 at 08:24:56PM +0000, Chris Packham wrote:
>> On 28/05/2025 04:12, Uwe Kleine-König wrote:
>>> Hello,
>>>
>>> On Tue, Jul 23, 2024 at 10:17:35AM +1200, Chris Packham wrote:
>>>> Add fan child nodes that allow describing the connections for the
>>>> ADT7475 to the fans it controls. This also allows setting some
>>>> initial values for the pwm duty cycle and frequency.
>>>>
>>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>>> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
>>>> ---
>>>>
>>>> Notes:
>>>>       Changes in v7:
>>>>       - None
>>>>       Changes in v6:
>>>>       - Collect r-by from Rob
>>>>       Changes in v5:
>>>>       - Use nanoseconds for PWM frequency and duty cycle as per existing
>>>>         conventions for PWMs
>>>>       - Set flags to 0 in example to match adi,pwm-active-state setting
>>>>       Changes in v4:
>>>>       - 0 is not a valid frequency value
>>>>       Changes in v3:
>>>>       - Use the pwm provider/consumer bindings
>>>>       Changes in v2:
>>>>       - Document 0 as a valid value (leaves hardware as-is)
>>>>
>>>>    .../devicetree/bindings/hwmon/adt7475.yaml    | 35 ++++++++++++++++++-
>>>>    1 file changed, 34 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
>>>> index 051c976ab711..df2b5b889e4d 100644
>>>> --- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml
>>>> +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
>>>> @@ -51,6 +51,24 @@ properties:
>>>>          enum: [0, 1]
>>>>          default: 1
>>>>    
>>>> +  "#pwm-cells":
>>>> +    const: 4
>>> I asked to add support for #pwm-cells = <4> to the pwm core in reply to
>>> v4 (see
>>> https://lore.kernel.org/linux-pwm/drqvaon5lb2ei3jqofutbr6demibyfdhbmr24sva27gzpqdnon@fxa7rpl33iih/).
>>>
>>> I'm unhappy to see this merged anyhow in combination with ad-hoc parsing
>>> of the pwm properties in the driver :-\
>> As I mentioned at the time the adt7475 is not currently pwm_chip so I
>> need the ad-hoc parsing in that driver. I'd be happy to take you
>> prototype patch for pwm/core.c and polish it although I don't really
>> have a good way of testing it.
> It's more the deviation of the default binding for PWMs that I don't
> like than the ad-hoc parsing. Ideally the adt7475 would provide a
> pwmchip (as the binding suggests) and the fan would be formalized as a
> pwm-fan. With the binding that was chosen here that option becomes more
> ugly than necessary to implement.
>
> If I understand correctly you need the default value for duty to
> statically setup (or only initialize?) a fan, right?
Correct.
> I'm not sure I like
> extending #pwm-cells for a default duty value. Thinking about that a
> while I'd prefer a binding that looks more like the clock configuration
> stuff because actually having the period and flags as part of the
> reference to the PWM to be used is also a bit strange. So I imagine
> something like:
>
> 	mypwm: pwm {
> 		compatible = "...."
> 		#pwm-cells = <1>;
> 	};
>
> 	fan {
> 		compatible = "pwm-fan";
> 		pwms = <&mypwm 1>;
> 		assigned-pwms = <&mypwm>;
> 		assigned-pwm-default-period-lengths-ns = <40000>;
> 		assigned-pwm-default-flags = <PWM_POLARITY_INVERTED>;
> 	};
>
> Then specifying a period (or later a duty cycle length) would be
> optional and could be provided iff the device needs that for operation.

The frequency and flags were already part of the standard #pwm-cells 
which I think is why I was encouraged to use them. I was also trying to 
get something that would work as an ACPI overlay which turned out to be 
really hard.

> My mail was just me being frustrated about another special case that I'd
> have to handle if I go into that direction. I should have been more
> attentive to that development before it entered the mainline.
I'd be happy to deprecate the 4 cell thing and replace it with 3 cell + 
vendor property for the default period if that helps.
>
> Best regards
> Uwe

Re: [PATCH v7 1/3] dt-bindings: hwmon: Add adt7475 fan/pwm properties

[Uwe Kleine-König]

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

Hello Chris,

On Wed, May 28, 2025 at 09:18:37PM +0000, Chris Packham wrote:
> On 28/05/2025 18:10, Uwe Kleine-König wrote:
> > If I understand correctly you need the default value for duty to
> > statically setup (or only initialize?) a fan, right?
> 
> Correct.
> 
> > I'm not sure I like
> > extending #pwm-cells for a default duty value. Thinking about that a
> > while I'd prefer a binding that looks more like the clock configuration
> > stuff because actually having the period and flags as part of the
> > reference to the PWM to be used is also a bit strange. So I imagine
> > something like:
> >
> > 	mypwm: pwm {
> > 		compatible = "...."
> > 		#pwm-cells = <1>;
> > 	};
> >
> > 	fan {
> > 		compatible = "pwm-fan";
> > 		pwms = <&mypwm 1>;
> > 		assigned-pwms = <&mypwm>;
> > 		assigned-pwm-default-period-lengths-ns = <40000>;
> > 		assigned-pwm-default-flags = <PWM_POLARITY_INVERTED>;
> > 	};
> >
> > Then specifying a period (or later a duty cycle length) would be
> > optional and could be provided iff the device needs that for operation.
> 
> The frequency and flags were already part of the standard #pwm-cells 
> which I think is why I was encouraged to use them.

Yeah, that part is fine. This might not be the long-term future, but
today that's the norm.

> I was also trying to get something that would work as an ACPI overlay
> which turned out to be really hard.

I don't know enough about ACPI to be helpful with this quest.

> > My mail was just me being frustrated about another special case that I'd
> > have to handle if I go into that direction. I should have been more
> > attentive to that development before it entered the mainline.
> 
> I'd be happy to deprecate the 4 cell thing and replace it with 3 cell + 
> vendor property for the default period if that helps.

I wonder how other similar devices determine the default duty cycle.
Isn't the norm to make the fan rotate at max speed and then when
userspace takes over it's speeded down?

Best regards
Uwe

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

Re: [PATCH v7 1/3] dt-bindings: hwmon: Add adt7475 fan/pwm properties

[Guenter Roeck]

On Wed, May 28, 2025 at 09:18:37PM +0000, Chris Packham wrote:
> >> As I mentioned at the time the adt7475 is not currently pwm_chip so I
> >> need the ad-hoc parsing in that driver. I'd be happy to take you
> >> prototype patch for pwm/core.c and polish it although I don't really
> >> have a good way of testing it.
> > It's more the deviation of the default binding for PWMs that I don't
> > like than the ad-hoc parsing. Ideally the adt7475 would provide a
> > pwmchip (as the binding suggests) and the fan would be formalized as a

We are not going to force each fan controller driver to register as pwm chip
just because it provides a pwm value to control the fans - even more so since
this gets really ugly if the chip can be programmed to either provide a voltage
output or a pwm value to control fan speed. Maybe the next requirement is that
fan controllers supporting voltage output to control fan speeds are supposed
to register themselves as regulators. I really don't want to go there.

Those are _not_ pwm controllers. They are special-purpose fan controllers.
Forcing them into the pwm framework from devicetree perspective is bad enough,
but forcing them to register as pwm controllers is a step too far.

Guenter

Re: [PATCH v7 1/3] dt-bindings: hwmon: Add adt7475 fan/pwm properties

[Chris Packham]

On 30/05/2025 21:38, Uwe Kleine-König wrote:
> Hello Chris,
>
> On Wed, May 28, 2025 at 09:18:37PM +0000, Chris Packham wrote:
>> On 28/05/2025 18:10, Uwe Kleine-König wrote:
>>> If I understand correctly you need the default value for duty to
>>> statically setup (or only initialize?) a fan, right?
>> Correct.
>>
>>> I'm not sure I like
>>> extending #pwm-cells for a default duty value. Thinking about that a
>>> while I'd prefer a binding that looks more like the clock configuration
>>> stuff because actually having the period and flags as part of the
>>> reference to the PWM to be used is also a bit strange. So I imagine
>>> something like:
>>>
>>> 	mypwm: pwm {
>>> 		compatible = "...."
>>> 		#pwm-cells = <1>;
>>> 	};
>>>
>>> 	fan {
>>> 		compatible = "pwm-fan";
>>> 		pwms = <&mypwm 1>;
>>> 		assigned-pwms = <&mypwm>;
>>> 		assigned-pwm-default-period-lengths-ns = <40000>;
>>> 		assigned-pwm-default-flags = <PWM_POLARITY_INVERTED>;
>>> 	};
>>>
>>> Then specifying a period (or later a duty cycle length) would be
>>> optional and could be provided iff the device needs that for operation.
>> The frequency and flags were already part of the standard #pwm-cells
>> which I think is why I was encouraged to use them.
> Yeah, that part is fine. This might not be the long-term future, but
> today that's the norm.
>
>> I was also trying to get something that would work as an ACPI overlay
>> which turned out to be really hard.
> I don't know enough about ACPI to be helpful with this quest.
>
>>> My mail was just me being frustrated about another special case that I'd
>>> have to handle if I go into that direction. I should have been more
>>> attentive to that development before it entered the mainline.
>> I'd be happy to deprecate the 4 cell thing and replace it with 3 cell +
>> vendor property for the default period if that helps.
> I wonder how other similar devices determine the default duty cycle.
> Isn't the norm to make the fan rotate at max speed and then when
> userspace takes over it's speeded down?

Yes that is the normal (and sensible thing do to). But occasionally 
hardware designers like to use incredibly over spec'd  fans that are 
just ridiculously noisy. On some products I've worked on we added basic 
fan control to u-boot so we could silence the fans early in the boot. I 
also gather that in the PC world the fan control is often done 
externally to the OS. In the specific case were I needed this 
functionality it was an embedded x86_64 so I had neither U-Boot nor a BMC.

Re: [PATCH v7 1/3] dt-bindings: hwmon: Add adt7475 fan/pwm properties

[Uwe Kleine-König]

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

Hello Chris,

On Mon, Jun 02, 2025 at 08:52:56PM +0000, Chris Packham wrote:
> On 30/05/2025 21:38, Uwe Kleine-König wrote:
> > I wonder how other similar devices determine the default duty cycle.
> > Isn't the norm to make the fan rotate at max speed and then when
> > userspace takes over it's speeded down?
> 
> Yes that is the normal (and sensible thing do to). But occasionally 
> hardware designers like to use incredibly over spec'd  fans that are 
> just ridiculously noisy. On some products I've worked on we added basic 
> fan control to u-boot so we could silence the fans early in the boot. I 
> also gather that in the PC world the fan control is often done 
> externally to the OS. In the specific case were I needed this 
> functionality it was an embedded x86_64 so I had neither U-Boot nor a BMC.

So you're saying that Linux is the first instance that is able to setup
the fan -- no BIOS, right?

I think that's quite normal and the fan is only noisy until userspace
takes over. So I still think that is what should happen here.

In case this is really too noisy, I'd prefer the driver to know itself
how to setup the fan and not put that policy into the device tree.

Best regards
Uwe

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