[PATCH v2 0/2] hwmon: (amc6821) Add PWM polarity configuration with OF

[PATCH v2 0/2] hwmon: (amc6821) Add PWM polarity configuration with OF

[Francesco Dolcini]

From: Francesco Dolcini <francesco.dolcini@toradex.com>

Add support for configuring the PWM polarity of the amc6821 fan controller.

Francesco Dolcini (2):
  dt-bindings: hwmon: amc6821: add PWM polarity
  hwmon: (amc6821) Add PWM polarity configuration with OF

 .../devicetree/bindings/hwmon/ti,amc6821.yaml     |  8 ++++++++
 drivers/hwmon/amc6821.c                           | 15 ++++++++++-----
 2 files changed, 18 insertions(+), 5 deletions(-)

-- 
2.39.5

[PATCH v2 1/2] dt-bindings: hwmon: amc6821: add PWM polarity

[Francesco Dolcini]

From: Francesco Dolcini <francesco.dolcini@toradex.com>

Add property to describe the PWM-Out pin polarity.

Link: https://www.ti.com/lit/gpn/amc6821
Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
---
v2: no changes
v1: https://lore.kernel.org/all/20250218165633.106867-2-francesco@dolcini.it/
---
 Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml b/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml
index 5d33f1a23d03..11604aa41b3e 100644
--- a/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml
+++ b/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml
@@ -28,6 +28,14 @@ properties:
   i2c-mux:
     type: object
 
+  ti,pwm-inverted:
+    type: boolean
+    description:
+      Set to make the PWM-Out pin go high (with an external pull-up resistor)
+      for 100% duty cycle (suitable for driving the fan using a NMOS device),
+      when not set the PWM-Out pin goes low for 100% duty cycle (suitable for
+      driving the fan using a PMOS device).
+
 required:
   - compatible
   - reg
-- 
2.39.5

Re: [PATCH v2 1/2] dt-bindings: hwmon: amc6821: add PWM polarity

[Rob Herring]

On Mon, Feb 24, 2025 at 07:08:00PM +0100, Francesco Dolcini wrote:
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> 
> Add property to describe the PWM-Out pin polarity.

Why doesn't the invert support in the pwm binding work for you? Yes, I 
read the discussion, but don't remember the conclusion and you need to 
justify it here.

> 
> Link: https://www.ti.com/lit/gpn/amc6821
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> ---
> v2: no changes
> v1: https://lore.kernel.org/all/20250218165633.106867-2-francesco@dolcini.it/
> ---
>  Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml b/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml
> index 5d33f1a23d03..11604aa41b3e 100644
> --- a/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml
> @@ -28,6 +28,14 @@ properties:
>    i2c-mux:
>      type: object
>  
> +  ti,pwm-inverted:
> +    type: boolean
> +    description:
> +      Set to make the PWM-Out pin go high (with an external pull-up resistor)
> +      for 100% duty cycle (suitable for driving the fan using a NMOS device),
> +      when not set the PWM-Out pin goes low for 100% duty cycle (suitable for
> +      driving the fan using a PMOS device).
> +
>  required:
>    - compatible
>    - reg
> -- 
> 2.39.5
> 

Re: [PATCH v2 1/2] dt-bindings: hwmon: amc6821: add PWM polarity

[Francesco Dolcini]

On Wed, Feb 26, 2025 at 07:49:22AM -0600, Rob Herring wrote:
> On Mon, Feb 24, 2025 at 07:08:00PM +0100, Francesco Dolcini wrote:
> > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > 
> > Add property to describe the PWM-Out pin polarity.
> 
> Why doesn't the invert support in the pwm binding work for you? Yes, I 
> read the discussion, but don't remember the conclusion and you need to 
> justify it here.

This chip is not a PWM controller, it is a FAN controller.

The HW has a PWM pin output that is used to control the fan, but the
device is not modelled as a PWM controller (correctly, given that is not
such a device) and the OS does not control the PWM, the chip reads the
temperature and decide the PWM duty cycle accordingly in an autonomous
way.

It's just the same that was done in commit ed39ff506adb ("dt-bindings:
hwmon: Document adt7475 pwm-active-state property").

Francesco

Re: [PATCH v2 1/2] dt-bindings: hwmon: amc6821: add PWM polarity

[Francesco Dolcini]

Hello Rob and all,

On Wed, Feb 26, 2025 at 02:58:06PM +0100, Francesco Dolcini wrote:
> On Wed, Feb 26, 2025 at 07:49:22AM -0600, Rob Herring wrote:
> > On Mon, Feb 24, 2025 at 07:08:00PM +0100, Francesco Dolcini wrote:
> > > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > 
> > > Add property to describe the PWM-Out pin polarity.
> > 
> > Why doesn't the invert support in the pwm binding work for you? Yes, I 
> > read the discussion, but don't remember the conclusion and you need to 
> > justify it here.
> 
> This chip is not a PWM controller, it is a FAN controller.
> 
> The HW has a PWM pin output that is used to control the fan, but the
> device is not modelled as a PWM controller (correctly, given that is not
> such a device) and the OS does not control the PWM, the chip reads the
> temperature and decide the PWM duty cycle accordingly in an autonomous
> way.

Can you advise on how to move this forward? Is my explanation good
enough or some more clarification is needed? Should I send a v3
incorporating such a comment into the commit message? Anything else?

Thanks,
Francesco

Re: [PATCH v2 1/2] dt-bindings: hwmon: amc6821: add PWM polarity

[Guenter Roeck]

On 3/19/25 03:12, Francesco Dolcini wrote:
> Hello Rob and all,
> 
> On Wed, Feb 26, 2025 at 02:58:06PM +0100, Francesco Dolcini wrote:
>> On Wed, Feb 26, 2025 at 07:49:22AM -0600, Rob Herring wrote:
>>> On Mon, Feb 24, 2025 at 07:08:00PM +0100, Francesco Dolcini wrote:
>>>> From: Francesco Dolcini <francesco.dolcini@toradex.com>
>>>>
>>>> Add property to describe the PWM-Out pin polarity.
>>>
>>> Why doesn't the invert support in the pwm binding work for you? Yes, I
>>> read the discussion, but don't remember the conclusion and you need to
>>> justify it here.
>>
>> This chip is not a PWM controller, it is a FAN controller.
>>
>> The HW has a PWM pin output that is used to control the fan, but the
>> device is not modelled as a PWM controller (correctly, given that is not
>> such a device) and the OS does not control the PWM, the chip reads the
>> temperature and decide the PWM duty cycle accordingly in an autonomous
>> way.
> 
> Can you advise on how to move this forward? Is my explanation good
> enough or some more clarification is needed? Should I send a v3
> incorporating such a comment into the commit message? Anything else?
> 

DT maintainers insist that pwm properties are described using pwm cells.
That does not mean that the driver has to implement a pwm controller
(and I would resist doing that, because it is pointless), just that
the chip's pwm properties are described this way. That is indeed a deviation
from older devicetree files, where "inverted" properties were acceptable.

I don't know if there is a means to avoid that. Some devicetree files
don't mention pwm in the property name. See nxp,inverted-out or
atmel,lcdcon-backlight-inverted for examples. I suspect that is no longer
acceptable, though. The easiest would probably be to define optional
minimal pwm bindings for the chip. Unless I am missing something, that
would just be the pwm polarity, so you would have a single pwm cell.
Something like

   '#pwm-cells':
     const: 1
     description: |
       Number of cells in a PWM specifier.
       - cell 0: The PWM polarity: 0 or PWM_POLARITY_INVERTED

and then something like

     fan_controller: fan@18 {
	compatible = "ti,amc6821";
	reg = <0x18>;
	#pwm-cells = <1>;

	pwms = <&fan_controller PWM_POLARITY_INVERTED>;
     };	

That may require some tweaking though; I think #pwm-cells may apply to the
children of a property, not to the property itself.

Guenter

[PATCH v2 2/2] hwmon: (amc6821) Add PWM polarity configuration with OF

[Francesco Dolcini]

From: Francesco Dolcini <francesco.dolcini@toradex.com>

Add support to configure the PWM-Out pin polarity based on a device
tree property.

The driver has a module option to set the PWM polarity (normal=0,
inverted=1), when specified it always takes the precedence over the DT
configuration.

Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
---
v2: pwminv module parameter takes always the precedence over the DT property
v1: https://lore.kernel.org/all/20250218165633.106867-3-francesco@dolcini.it/
---
 drivers/hwmon/amc6821.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
index 1e3c6acd8974..317741995df2 100644
--- a/drivers/hwmon/amc6821.c
+++ b/drivers/hwmon/amc6821.c
@@ -37,7 +37,7 @@ static const unsigned short normal_i2c[] = {0x18, 0x19, 0x1a, 0x2c, 0x2d, 0x2e,
  * Insmod parameters
  */
 
-static int pwminv;	/*Inverted PWM output. */
+static int pwminv = -1; /*Inverted PWM output. */
 module_param(pwminv, int, 0444);
 
 static int init = 1; /*Power-on initialization.*/
@@ -845,9 +845,10 @@ static int amc6821_detect(struct i2c_client *client, struct i2c_board_info *info
 	return 0;
 }
 
-static int amc6821_init_client(struct amc6821_data *data)
+static int amc6821_init_client(struct i2c_client *client, struct amc6821_data *data)
 {
 	struct regmap *regmap = data->regmap;
+	u32 regval;
 	int err;
 
 	if (init) {
@@ -864,11 +865,15 @@ static int amc6821_init_client(struct amc6821_data *data)
 		if (err)
 			return err;
 
+		regval = AMC6821_CONF1_START;
+		if ((pwminv < 0 && of_property_read_bool(client->dev.of_node, "ti,pwm-inverted")) ||
+		    pwminv > 0)
+			regval |= AMC6821_CONF1_PWMINV;
+
 		err = regmap_update_bits(regmap, AMC6821_REG_CONF1,
 					 AMC6821_CONF1_THERMOVIE | AMC6821_CONF1_FANIE |
 					 AMC6821_CONF1_START | AMC6821_CONF1_PWMINV,
-					 AMC6821_CONF1_START |
-					 (pwminv ? AMC6821_CONF1_PWMINV : 0));
+					 regval);
 		if (err)
 			return err;
 	}
@@ -916,7 +921,7 @@ static int amc6821_probe(struct i2c_client *client)
 				     "Failed to initialize regmap\n");
 	data->regmap = regmap;
 
-	err = amc6821_init_client(data);
+	err = amc6821_init_client(client, data);
 	if (err)
 		return err;
 
-- 
2.39.5