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

[PATCH v4 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, 22500, 1, 50 } },
                }
            })
        }
        Device (FAN1)
        {
            Name (_ADR, 0)
            Name (_DSD, Package () {
                ToUUID (UUID_DEVICE_PROPERTIES),
                Package () {
                    Package () { "pwms", Package () { 2, 22500, 1, 50 } },
                }
            })
        }
    }

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

-- 
2.45.1

[PATCH v4 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:
    I realise there is still some discussion about how to express the
    frequency and duty cycle. I have a personal preference for using hertz
    for the frequency and 0-255 for the duty cycle but if the consensus is
    to express these things some other way I'm fine with doing some math.
    
    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    | 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..bfef4c803bf7 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 - 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 v4 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 bfef4c803bf7..ed18984c2529 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 v4 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 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 | 112 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 112 insertions(+)

diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
index 4224ffb30483..b5c58e3cda03 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,112 @@ 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_reference_args(struct fwnode_handle *fwnode,
+						       struct adt7475_pwm_config *cfg)
+{
+	int ret;
+	struct fwnode_reference_args args = {};
+
+	ret = fwnode_property_get_reference_args(fwnode, "pwms", "#pwm-cells", 0, 0, &args);
+	if (ret)
+		return ret;
+
+	if (args.nargs != 4) {
+		fwnode_handle_put(args.fwnode);
+		return -EINVAL;
+	}
+
+	cfg->index = args.args[0];
+	cfg->freq = find_closest(args.args[1], pwmfreq_table, ARRAY_SIZE(pwmfreq_table));
+	cfg->flags = args.args[2];
+	cfg->duty = clamp_val(args.args[3], 0, 0xFF);
+
+	fwnode_handle_put(args.fwnode);
+
+	return 0;
+}
+
+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;
+
+	cfg->index = args[0];
+	cfg->freq = find_closest(args[1], pwmfreq_table, ARRAY_SIZE(pwmfreq_table));
+	cfg->flags = args[2];
+	cfg->duty = clamp_val(args[3], 0, 0xFF);
+
+	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 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 +1886,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 v4 1/3] dt-bindings: hwmon: Add adt7475 fan/pwm properties

[Conor Dooley]

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

On Wed, May 29, 2024 at 10:56:36AM +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:
>     I realise there is still some discussion about how to express the
>     frequency and duty cycle. I have a personal preference for using hertz
>     for the frequency and 0-255 for the duty cycle but if the consensus is
>     to express these things some other way I'm fine with doing some math.

Probably worth carrying a link to it here:
https://lore.kernel.org/all/4de798f3-069e-4028-a5b5-5e6a639277e3@alliedtelesis.co.nz/

I asked Uwe to take a look & it's on his todo list.

Thanks,
Conor.

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

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

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

[Rob Herring (Arm)]

On Wed, 29 May 2024 10:56:37 +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>
> ---
> 
> Notes:
>     Changes in v3:
>     - New
> 
>  Documentation/devicetree/bindings/hwmon/adt7475.yaml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

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

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

[Conor Dooley]

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

Uwe,

On Wed, May 29, 2024 at 08:58:41AM +0100, Conor Dooley wrote:
> On Wed, May 29, 2024 at 10:56:36AM +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:
> >     I realise there is still some discussion about how to express the
> >     frequency and duty cycle. I have a personal preference for using hertz
> >     for the frequency and 0-255 for the duty cycle but if the consensus is
> >     to express these things some other way I'm fine with doing some math.
> 
> Probably worth carrying a link to it here:
> https://lore.kernel.org/all/4de798f3-069e-4028-a5b5-5e6a639277e3@alliedtelesis.co.nz/
> 
> I asked Uwe to take a look & it's on his todo list.

This is an attempt to bump this up on your todo list a bit!

Thanks,
Conor.

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



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

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

[Uwe Kleine-König]

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

Hello Chris,

sorry for taking so long to respond. Don't take it personal, I'm way
behind my maintainer dutys in general.

On Wed, May 29, 2024 at 10:56:36AM +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:
>     I realise there is still some discussion about how to express the
>     frequency and duty cycle. I have a personal preference for using hertz
>     for the frequency and 0-255 for the duty cycle but if the consensus is
>     to express these things some other way I'm fine with doing some math.
>     
>     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    | 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..bfef4c803bf7 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 - 11, 14, 22, 29, 35, 44, 58, 88, 22500

Nack, don't deviate from how PWMs are usually referenced. So specify the
period in nanoseconds, not Hertz.

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

I'd be ok with that, however please add support for that in the pwm core
and then just use that.

You wrote that you find it irritating that the duty is specified in
nanoseconds and not a percentage. The reason for that is historic. Also
it gives a more precise specification (at least compared to the naive
representation of the integer percentage as an integer type).
For the rework of how PWM waveforms are represented I picked
"duty_length" as variable name for this value, in the hope this is less
confusing.

untested prototype for a 4th member in pwm specifiers:

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 5c1d20985148..f732235df12d 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -251,7 +251,7 @@ int pwm_adjust_config(struct pwm_device *pwm)
 	 * duty cycle of 0.
 	 */
 	if (!state.period) {
-		state.duty_cycle = 0;
+		state.duty_cycle = pargs.duty_length;
 		state.period = pargs.period;
 		state.polarity = pargs.polarity;
 
@@ -437,6 +437,10 @@ of_pwm_xlate_with_flags(struct pwm_chip *chip, const struct of_phandle_args *arg
 	if (args->args_count > 2 && args->args[2] & PWM_POLARITY_INVERTED)
 		pwm->args.polarity = PWM_POLARITY_INVERSED;
 
+	pwm->args.duty_length = 0;
+	if (args->args_count > 3)
+		pwm->args.duty_length = args->args[3];
+
 	return pwm;
 }
 EXPORT_SYMBOL_GPL(of_pwm_xlate_with_flags);
@@ -457,6 +461,10 @@ of_pwm_single_xlate(struct pwm_chip *chip, const struct of_phandle_args *args)
 	if (args->args_count > 1 && args->args[1] & PWM_POLARITY_INVERTED)
 		pwm->args.polarity = PWM_POLARITY_INVERSED;
 
+	pwm->args.duty_length = 0;
+	if (args->args_count > 2)
+		pwm->args.duty_length = args->args[2];
+
 	return pwm;
 }
 EXPORT_SYMBOL_GPL(of_pwm_single_xlate);
@@ -1353,6 +1361,9 @@ static struct pwm_device *acpi_pwm_get(const struct fwnode_handle *fwnode)
 	if (args.nargs > 2 && args.args[2] & PWM_POLARITY_INVERTED)
 		pwm->args.polarity = PWM_POLARITY_INVERSED;
 
+	/* Maybe extend this to apply args.args[3] if args.nargs > 3? */
+	pwm->args.duty_cycle = 0;
+
 	return pwm;
 }
 
@@ -1514,6 +1525,7 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id)
 
 	pwm->args.period = chosen->period;
 	pwm->args.polarity = chosen->polarity;
+	pwm->args.duty_length = 0;
 
 	return pwm;
 }
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index f8c2dc12dbd3..678a97706eac 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -41,6 +41,7 @@ enum pwm_polarity {
  */
 struct pwm_args {
 	u64 period;
+	u32 duty_length;
 	enum pwm_polarity polarity;
 };
 
(I think it doesn't make sense to use a u64 here. At least the oftree
values are only 32 bit wide. I didn't check the ACPI part, if that is
only 32 bit wide, too, it would make sense to make period use a 32 bit
type, too.)

Best regards
Uwe

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

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

[Chris Packham]

Hi Uwe,

On 5/07/24 21:09, Uwe Kleine-König wrote:
> Hello Chris,
>
> sorry for taking so long to respond. Don't take it personal, I'm way
> behind my maintainer dutys in general.
>
> On Wed, May 29, 2024 at 10:56:36AM +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:
>>      I realise there is still some discussion about how to express the
>>      frequency and duty cycle. I have a personal preference for using hertz
>>      for the frequency and 0-255 for the duty cycle but if the consensus is
>>      to express these things some other way I'm fine with doing some math.
>>      
>>      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    | 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..bfef4c803bf7 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 - 11, 14, 22, 29, 35, 44, 58, 88, 22500
> Nack, don't deviate from how PWMs are usually referenced. So specify the
> period in nanoseconds, not Hertz.

OK no problem. The math is easy enough. I might end up over documenting 
things for the acceptable PWM period so someone can weigh in on that.

>> +      - 2: PWM flags 0 or PWM_POLARITY_INVERTED
>> +      - 3: The default pwm duty cycle - 0-255
> I'd be ok with that, however please add support for that in the pwm core
> and then just use that.
>
> You wrote that you find it irritating that the duty is specified in
> nanoseconds and not a percentage. The reason for that is historic. Also
> it gives a more precise specification (at least compared to the naive
> representation of the integer percentage as an integer type).
> For the rework of how PWM waveforms are represented I picked
> "duty_length" as variable name for this value, in the hope this is less
> confusing.
>
> untested prototype for a 4th member in pwm specifiers:

Right now the adt7475 is not a pwm_chip and I'm not really planning on 
making it one. My goal with this was just to make it possible for the 
kernel to keep the fans running quietly before userland can take over 
and start doing proper configuration/monitoring. The best I can do is 
make sure that the devicetree binding is done in such a way that it 
could grow pwm_chip capabilities in the future.

>
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 5c1d20985148..f732235df12d 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -251,7 +251,7 @@ int pwm_adjust_config(struct pwm_device *pwm)
>   	 * duty cycle of 0.
>   	 */
>   	if (!state.period) {
> -		state.duty_cycle = 0;
> +		state.duty_cycle = pargs.duty_length;
>   		state.period = pargs.period;
>   		state.polarity = pargs.polarity;
>   
> @@ -437,6 +437,10 @@ of_pwm_xlate_with_flags(struct pwm_chip *chip, const struct of_phandle_args *arg
>   	if (args->args_count > 2 && args->args[2] & PWM_POLARITY_INVERTED)
>   		pwm->args.polarity = PWM_POLARITY_INVERSED;
>   
> +	pwm->args.duty_length = 0;
> +	if (args->args_count > 3)
> +		pwm->args.duty_length = args->args[3];
> +
>   	return pwm;
>   }
>   EXPORT_SYMBOL_GPL(of_pwm_xlate_with_flags);
> @@ -457,6 +461,10 @@ of_pwm_single_xlate(struct pwm_chip *chip, const struct of_phandle_args *args)
>   	if (args->args_count > 1 && args->args[1] & PWM_POLARITY_INVERTED)
>   		pwm->args.polarity = PWM_POLARITY_INVERSED;
>   
> +	pwm->args.duty_length = 0;
> +	if (args->args_count > 2)
> +		pwm->args.duty_length = args->args[2];
> +
>   	return pwm;
>   }
>   EXPORT_SYMBOL_GPL(of_pwm_single_xlate);
> @@ -1353,6 +1361,9 @@ static struct pwm_device *acpi_pwm_get(const struct fwnode_handle *fwnode)
>   	if (args.nargs > 2 && args.args[2] & PWM_POLARITY_INVERTED)
>   		pwm->args.polarity = PWM_POLARITY_INVERSED;
>   
> +	/* Maybe extend this to apply args.args[3] if args.nargs > 3? */
> +	pwm->args.duty_cycle = 0;
> +
>   	return pwm;
>   }
>   
> @@ -1514,6 +1525,7 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id)
>   
>   	pwm->args.period = chosen->period;
>   	pwm->args.polarity = chosen->polarity;
> +	pwm->args.duty_length = 0;
>   
>   	return pwm;
>   }
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index f8c2dc12dbd3..678a97706eac 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -41,6 +41,7 @@ enum pwm_polarity {
>    */
>   struct pwm_args {
>   	u64 period;
> +	u32 duty_length;
>   	enum pwm_polarity polarity;
>   };
>   
> (I think it doesn't make sense to use a u64 here. At least the oftree
> values are only 32 bit wide. I didn't check the ACPI part, if that is
> only 32 bit wide, too, it would make sense to make period use a 32 bit
> type, too.)
>
> Best regards
> Uwe