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

[PATCH v3 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. I'm reasonably happy with the devicetree support but
I'm struggling to figure out how to make this work for ACPI (which is actually
the platform I need this on). I figure the ASL is something like

    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" },
            },
            ToUUID (UUID_HIERARCHICAL_DATA_EXTENSION),
            Package () {
                Package () { "fan-0", "FAN0" },
            }
        })
        Name (FAN0, Package () {
            ToUUID (UUID_DEVICE_PROPERTIES),
    	    Package () { 0, 22500, PWM_POLARITY_INVERTED, 255 },
        })
    }

But I've got no idea how to parse that out keeping as much code in common with
the devicetree version.

I'm sending this series out now to get some feedback ton the DT code and
hopefully someone will give me some suggestions for dealing with the ACPI case.

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    | 27 +++++++-
 drivers/hwmon/adt7475.c                       | 63 +++++++++++++++++++
 2 files changed, 88 insertions(+), 2 deletions(-)

-- 
2.45.1

[PATCH v3 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>
---

Notes:
    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    | 25 ++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
index 051c976ab711..99bd689ae0cd 100644
--- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml
+++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
@@ -51,6 +51,15 @@ 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 frequency in hertz - 0, 11, 14, 22, 29, 35, 44, 58, 88, 22500
+      - 2: PWM flags 0 or PWM_POLARITY_INVERTED
+      - 3: The default pwm duty cycle - 0-255
+
 patternProperties:
   "^adi,bypass-attenuator-in[0-4]$":
     description: |
@@ -81,6 +90,10 @@ patternProperties:
       - smbalert#
       - gpio
 
+  "^fan-[0-9]+$":
+    $ref: fan-common.yaml#
+    unevaluatedProperties: false
+
 required:
   - compatible
   - reg
@@ -89,11 +102,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 +115,14 @@ examples:
         adi,pwm-active-state = <1 0 1>;
         adi,pin10-function = "smbalert#";
         adi,pin14-function = "tach4";
+        #pwm-cells = <4>;
+
+        fan-0 {
+          pwms = <&pwm 0 22500 PWM_POLARITY_INVERTED 255>;
+        };
+
+        fan-1 {
+          pwms = <&pwm 2 22500 PWM_POLARITY_INVERTED 255>;
+        };
       };
     };
-- 
2.45.1

[PATCH v3 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>
---

Notes:
    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 99bd689ae0cd..3d4f9266f0e3 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:
@@ -112,7 +113,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.1

[PATCH v3 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 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 | 63 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
index 4224ffb30483..2b8f39c1d76f 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,63 @@ static int adt7475_set_pwm_polarity(struct i2c_client *client)
 	return 0;
 }
 
+static int adt7475_fan_pwm_config(struct i2c_client *client)
+{
+	struct adt7475_data *data = i2c_get_clientdata(client);
+	struct fwnode_handle *child;
+	struct of_phandle_args args = {};
+	int ret, pwm, duty, freq, flags;
+	u8 val;
+
+	device_for_each_child_node(&client->dev, child) {
+		if (!is_of_node(child))
+			continue;
+
+		ret = of_parse_phandle_with_args(to_of_node(child), "pwms", "#pwm-cells", 0, &args);
+		if (ret)
+			return ret;
+
+		if (args.args_count != 4)
+			return -EINVAL;
+
+		pwm = args.args[0];
+		freq = find_closest(args.args[1], pwmfreq_table, ARRAY_SIZE(pwmfreq_table));
+		flags = args.args[2];
+		duty = clamp_val(args.args[3], 0, 0xFF);
+
+		if (pwm >= ADT7475_PWM_COUNT)
+			return -EINVAL;
+
+		ret = adt7475_read(PWM_CONFIG_REG(pwm));
+		if (ret < 0)
+			return ret;
+		val = ret;
+		if (flags & PWM_POLARITY_INVERTED)
+			val |= BIT(4);
+		else
+			val &= ~BIT(4);
+
+		ret = i2c_smbus_write_byte_data(client, PWM_CONFIG_REG(pwm), val);
+		if (ret)
+			return ret;
+
+		data->pwm[pwm][0] = duty;
+		ret = i2c_smbus_write_byte_data(client, PWM_REG(pwm), data->pwm[pwm][0]);
+		if (ret)
+			return ret;
+
+		data->range[pwm] = adt7475_read(TEMP_TRANGE_REG(pwm));
+		data->range[pwm] &= ~0xf;
+		data->range[pwm] |= freq;
+
+		ret = i2c_smbus_write_byte_data(client, TEMP_TRANGE_REG(pwm), data->range[pwm]);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int adt7475_probe(struct i2c_client *client)
 {
 	enum chips chip;
@@ -1778,6 +1837,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.1

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

[Conor Dooley]

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

On Mon, May 20, 2024 at 03:03:19PM +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>
> ---
> 
> Notes:
>     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    | 25 ++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> index 051c976ab711..99bd689ae0cd 100644
> --- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> @@ -51,6 +51,15 @@ 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 frequency in hertz - 0, 11, 14, 22, 29, 35, 44, 58, 88, 22500

The standard binding is period in nanoseconds, not frequency in Hz.
What's gained from deviating from that?

> +      - 2: PWM flags 0 or PWM_POLARITY_INVERTED
> +      - 3: The default pwm duty cycle - 0-255

Same here I guess, why not match the units used for the period for the
duty cycle?

Cheers,
Conor.

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

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

[Guenter Roeck]

On 5/20/24 09:49, Conor Dooley wrote:
> On Mon, May 20, 2024 at 03:03:19PM +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>
>> ---
>>
>> Notes:
>>      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    | 25 ++++++++++++++++++-
>>   1 file changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
>> index 051c976ab711..99bd689ae0cd 100644
>> --- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml
>> +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
>> @@ -51,6 +51,15 @@ 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 frequency in hertz - 0, 11, 14, 22, 29, 35, 44, 58, 88, 22500
> 
> The standard binding is period in nanoseconds, not frequency in Hz.
> What's gained from deviating from that?
> 

I'd strongly suspect that Chris didn't know and didn't expect it,
just like me.

>> +      - 2: PWM flags 0 or PWM_POLARITY_INVERTED
>> +      - 3: The default pwm duty cycle - 0-255
> 
> Same here I guess, why not match the units used for the period for the
> duty cycle?
> 

Same here. I am used to pwm frequency in Hz and duty cycle as percentage.
Using the period instead is somewhat unusual, and I must admit that I
have never encountered it while dealing with a variety of fan controllers.

Just to make sure I understand this correctly - duty cycles would
be (rounded):

Hz	ns
11	90,909,091
14	71,428,571
22	45,454,545
29:	34,482,759
35:	28,571,429
44:	22,727,273
58:	17,241,379
88:	11,363,636
22500	44,444

Examples for duty cycles would be

20%: 0,2s or 200,000,000
50%: 0.5s or 500,000,000
80%: 0.8s or 800,000,000

Is that correct ?

Thanks,
Guenter

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

[Conor Dooley]

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

On Mon, May 20, 2024 at 12:03:42PM -0700, Guenter Roeck wrote:
> On 5/20/24 09:49, Conor Dooley wrote:
> > On Mon, May 20, 2024 at 03:03:19PM +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>
> > > ---
> > > 
> > > Notes:
> > >      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    | 25 ++++++++++++++++++-
> > >   1 file changed, 24 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> > > index 051c976ab711..99bd689ae0cd 100644
> > > --- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> > > +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> > > @@ -51,6 +51,15 @@ 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 frequency in hertz - 0, 11, 14, 22, 29, 35, 44, 58, 88, 22500
> > 
> > The standard binding is period in nanoseconds, not frequency in Hz.
> > What's gained from deviating from that?
> > 
> 
> I'd strongly suspect that Chris didn't know and didn't expect it,
> just like me.

I did point out on v2 that the information on the standard binding was
in pwm.txt, but I also said "order it has <index freq flags duty>" which
likely didn't help. I did however CC you both on a patch the other day
where I fixed pwm.yaml so that it actually contained the information on
what the cells represented.
> > > +      - 2: PWM flags 0 or PWM_POLARITY_INVERTED
> > > +      - 3: The default pwm duty cycle - 0-255
> > 
> > Same here I guess, why not match the units used for the period for the
> > duty cycle?
> > 
> 
> Same here. I am used to pwm frequency in Hz and duty cycle as percentage.
> Using the period instead is somewhat unusual, and I must admit that I
> have never encountered it while dealing with a variety of fan controllers.

If it is what makes sense to use, because it's what everyone and their
mother documents, then sure. My rationale I suppose was threefold:
- Consistency with the period
- Time would be more portable between providers, if 8 bits of precision
  is deemed insufficient for some providers.
- Last & least, the PWM APIs in the kernel use time for the dutycycle

If you're going to use percentages rather than time, would it not
make more sense to either use percent itself with a 0-100 range or use
basis points if percent doesn't provide sufficient granularity?

I think it'd be good of Uwe chimed in, given we're discussing a PWM
provider binding after all.

> Just to make sure I understand this correctly - duty cycles would

s/duty cycles/periods/

> be (rounded):
> 
> Hz	ns
> 11	90,909,091
> 14	71,428,571
> 22	45,454,545
> 29:	34,482,759
> 35:	28,571,429
> 44:	22,727,273
> 58:	17,241,379
> 88:	11,363,636
> 22500	44,444
> 
> Examples for duty cycles would be
> 
> 20%: 0,2s or 200,000,000
> 50%: 0.5s or 500,000,000
> 80%: 0.8s or 800,000,000
> 
> Is that correct ?

Assuming a 1 second period, those look as expected.

Cheers,
Conor.


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

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

[Chris Packham]

On 21/05/24 07:41, Conor Dooley wrote:
> On Mon, May 20, 2024 at 12:03:42PM -0700, Guenter Roeck wrote:
>> On 5/20/24 09:49, Conor Dooley wrote:
>>> On Mon, May 20, 2024 at 03:03:19PM +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>
>>>> ---
>>>>
>>>> Notes:
>>>>       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    | 25 ++++++++++++++++++-
>>>>    1 file changed, 24 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
>>>> index 051c976ab711..99bd689ae0cd 100644
>>>> --- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml
>>>> +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
>>>> @@ -51,6 +51,15 @@ 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 frequency in hertz - 0, 11, 14, 22, 29, 35, 44, 58, 88, 22500
>>> The standard binding is period in nanoseconds, not frequency in Hz.
>>> What's gained from deviating from that?
>>>
>> I'd strongly suspect that Chris didn't know and didn't expect it,
>> just like me.
> I did point out on v2 that the information on the standard binding was
> in pwm.txt, but I also said "order it has <index freq flags duty>" which
> likely didn't help. I did however CC you both on a patch the other day
> where I fixed pwm.yaml so that it actually contained the information on
> what the cells represented.

I did see that patch thanks. And I did adjust the order of the 
parameters I had based on it.

I personally found expressing the frequency in seconds confusing which 
the is main reason why I stuck with hertz. I also couldn't grok how to 
express the duty cycle if I did express the frequency in nanoseconds. I 
think maybe I'm a bit confused as to how to express a generic PWM period 
when the controller I'm dealing with has a frequency and a duty cycle.

>>>> +      - 2: PWM flags 0 or PWM_POLARITY_INVERTED
>>>> +      - 3: The default pwm duty cycle - 0-255
>>> Same here I guess, why not match the units used for the period for the
>>> duty cycle?
>>>
>> Same here. I am used to pwm frequency in Hz and duty cycle as percentage.
>> Using the period instead is somewhat unusual, and I must admit that I
>> have never encountered it while dealing with a variety of fan controllers.
My expectation is that a duty cycle is a percentage. The only reason I'm 
using 0-255 instead of 0-100 is because that matches the sysfs ABI. But 
I'd happily change to a true percentage (if I can figure out the integer 
math), we'd lose a little bit of precision but nothing anyone would 
really notice.
> If it is what makes sense to use, because it's what everyone and their
> mother documents, then sure. My rationale I suppose was threefold:
> - Consistency with the period
> - Time would be more portable between providers, if 8 bits of precision
>    is deemed insufficient for some providers.
> - Last & least, the PWM APIs in the kernel use time for the dutycycle
>
> If you're going to use percentages rather than time, would it not
> make more sense to either use percent itself with a 0-100 range or use
> basis points if percent doesn't provide sufficient granularity?
>
> I think it'd be good of Uwe chimed in, given we're discussing a PWM
> provider binding after all.

I'd also point out I'm more than happy to go back to my v2 approach of 
adding specific properties for the frequency and duty cycle.

>> Just to make sure I understand this correctly - duty cycles would
> s/duty cycles/periods/
>
>> be (rounded):
>>
>> Hz	ns
>> 11	90,909,091
>> 14	71,428,571
>> 22	45,454,545
>> 29:	34,482,759
>> 35:	28,571,429
>> 44:	22,727,273
>> 58:	17,241,379
>> 88:	11,363,636
>> 22500	44,444
>>
>> Examples for duty cycles would be
>>
>> 20%: 0,2s or 200,000,000
>> 50%: 0.5s or 500,000,000
>> 80%: 0.8s or 800,000,000
>>
>> Is that correct ?
> Assuming a 1 second period, those look as expected.

To me that just makes things harder to understand. If you were reading 
the ADT7475 datasheet you'd see things like the supported frequencies of 
11, 14, ... , 88, 22500. Having to express that in nanoseconds requires 
making a bit of a leap. The duty cycle percentage to nanoseconds is more 
straight forward but seems unnecessarily obscure to me.

>
> Cheers,
> Conor.
>