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

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

 Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml | 8 ++++++++
 drivers/hwmon/amc6821.c                                 | 7 +++++--
 2 files changed, 13 insertions(+), 2 deletions(-)

-- 
2.39.5

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

[PATCH v1 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.

Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
---
 drivers/hwmon/amc6821.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
index 1e3c6acd8974..1ea2d97eebca 100644
--- a/drivers/hwmon/amc6821.c
+++ b/drivers/hwmon/amc6821.c
@@ -845,7 +845,7 @@ 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;
 	int err;
@@ -864,6 +864,9 @@ static int amc6821_init_client(struct amc6821_data *data)
 		if (err)
 			return err;
 
+		if (of_property_read_bool(client->dev.of_node, "ti,pwm-inverted"))
+			pwminv = 1;
+
 		err = regmap_update_bits(regmap, AMC6821_REG_CONF1,
 					 AMC6821_CONF1_THERMOVIE | AMC6821_CONF1_FANIE |
 					 AMC6821_CONF1_START | AMC6821_CONF1_PWMINV,
@@ -916,7 +919,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

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

[Quentin Schulz]

Hi Francesco,

On 2/18/25 5:56 PM, Francesco Dolcini wrote:
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> 
> Add support to configure the PWM-Out pin polarity based on a device
> tree property.
> 
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> ---
>   drivers/hwmon/amc6821.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
> index 1e3c6acd8974..1ea2d97eebca 100644
> --- a/drivers/hwmon/amc6821.c
> +++ b/drivers/hwmon/amc6821.c
> @@ -845,7 +845,7 @@ 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;
>   	int err;
> @@ -864,6 +864,9 @@ static int amc6821_init_client(struct amc6821_data *data)
>   		if (err)
>   			return err;
>   
> +		if (of_property_read_bool(client->dev.of_node, "ti,pwm-inverted"))

I know that the AMC6821 is doing a lot of smart things, but this really 
tickled me. PWM controllers actually do support that already via 
PWM_POLARITY_INVERTED flag for example. See 
Documentation/devicetree/bindings/hwmon/adt7475.yaml which seems to be 
another HWMON driver which acts as a PWM controller. I'm not sure this 
is relevant, applicable or desired but I wanted to highlight this.

> +			pwminv = 1;
> +

This is silently overriding the module parameter.

I don't think this is a good idea, at the very least not silently.

I would suggest to add some logic in the probe function to set this 
value and check its consistency.

Something like:

"""
diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
index 1e3c6acd89740..3a13a914e2bbb 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;	/* -1 not modified by the user, 0 default PWM 
output, 1 inverted PWM output */
  module_param(pwminv, int, 0444);

  static int init = 1; /*Power-on initialization.*/
@@ -904,6 +904,7 @@ static int amc6821_probe(struct i2c_client *client)
  	struct amc6821_data *data;
  	struct device *hwmon_dev;
  	struct regmap *regmap;
+	bool pwminv_dt;
  	int err;

  	data = devm_kzalloc(dev, sizeof(struct amc6821_data), GFP_KERNEL);
@@ -916,6 +917,18 @@ static int amc6821_probe(struct i2c_client *client)
  				     "Failed to initialize regmap\n");
  	data->regmap = regmap;

+	pwminv_dt = of_property_read_bool(client->dev.of_node, "ti,pwm-inverted");
+
+	if (pwminv == -1) {
+		pwminv = pwminv_dt;
+	} else if (is_of_node(client->dev.fwnode)) {
+		if ((!pwminv_dt && pwminv) || (pwminv_dt && pwminv == 0)) {
+			dev_err(dev,
+				"Polarity of PWM output passed by module parameter (pwminv=%d) 
differs from the one provided through the Device Tree, ignoring Device 
Tree value\n",
+				pwminv);
+		}
+	}
+
  	err = amc6821_init_client(data);
  	if (err)
  		return err;
"""

maybe? Note that I have neither compiled nor tested this code.

This also changes the precedence compared to the patch you sent, I think 
we may want the module param to override the DT property if there's a 
conflict.

What do you think?

Cheers,
Quentin

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

[Francesco Dolcini]

Hello Quentin,

On Wed, Feb 19, 2025 at 11:08:43AM +0100, Quentin Schulz wrote:
> On 2/18/25 5:56 PM, Francesco Dolcini wrote:
> > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > 
> > Add support to configure the PWM-Out pin polarity based on a device
> > tree property.
> > 
> > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > ---
> >   drivers/hwmon/amc6821.c | 7 +++++--
> >   1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
> > index 1e3c6acd8974..1ea2d97eebca 100644
> > --- a/drivers/hwmon/amc6821.c
> > +++ b/drivers/hwmon/amc6821.c
> > @@ -845,7 +845,7 @@ 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;
> >   	int err;
> > @@ -864,6 +864,9 @@ static int amc6821_init_client(struct amc6821_data *data)
> >   		if (err)
> >   			return err;
> > +		if (of_property_read_bool(client->dev.of_node, "ti,pwm-inverted"))
> 
> I know that the AMC6821 is doing a lot of smart things, but this really
> tickled me. PWM controllers actually do support that already via
> PWM_POLARITY_INVERTED flag for example. See
> Documentation/devicetree/bindings/hwmon/adt7475.yaml which seems to be
> another HWMON driver which acts as a PWM controller. I'm not sure this is
> relevant, applicable or desired but I wanted to highlight this.

From the DT binding point of view, it seems to implement the same I am
proposing here with adi,pwm-active-state property.

Do you have anything more specific in mind?

> 
> > +			pwminv = 1;
> > +
> 
> This is silently overriding the module parameter.
> 
> I don't think this is a good idea, at the very least not silently.

I was thinking at the same, and in the end I do have proposed this
solution in any case.

Let's look at the 2 use cases in which the DT property and the module
parameter are different.

## 1

module parameter pwminv=0
ti,pwm-inverted DT property present

=> we enable the PWM inversion

I think this is fair, if someone has a DT based system we need to assume
that the DT is correct. This is a HW configuration, not a module
parameter.

## 2

module parameter pwminv=1
ti,pwm-inverted DT property absent

=> we enable the PWM inversion

In this case the module parameter is overriding the DT. It means that
someone explicitly set pwminv=1 module parameter. I think is fair to
fulfill the module parameter request in this case, overriding the DT

> I would suggest to add some logic in the probe function to set this value
> and check its consistency.

With that said I can implement something around the lines you proposed,
if you still think is worth doing it. I would personally just keep the
priority on the module parameter over the DT and add an info print on what
is actually configured by the driver (not checking if they are
different).
	
Or I can just add a dev_info() telling the user about the actual PWM
polarity used, making this more transparent, without changing the logic
proposed here.

Francesco

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

[Quentin Schulz]

Hi Francesco,

On 2/19/25 11:33 AM, Francesco Dolcini wrote:
> Hello Quentin,
> 
> On Wed, Feb 19, 2025 at 11:08:43AM +0100, Quentin Schulz wrote:
>> On 2/18/25 5:56 PM, Francesco Dolcini wrote:
>>> From: Francesco Dolcini <francesco.dolcini@toradex.com>
>>>
>>> Add support to configure the PWM-Out pin polarity based on a device
>>> tree property.
>>>
>>> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
>>> ---
>>>    drivers/hwmon/amc6821.c | 7 +++++--
>>>    1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
>>> index 1e3c6acd8974..1ea2d97eebca 100644
>>> --- a/drivers/hwmon/amc6821.c
>>> +++ b/drivers/hwmon/amc6821.c
>>> @@ -845,7 +845,7 @@ 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;
>>>    	int err;
>>> @@ -864,6 +864,9 @@ static int amc6821_init_client(struct amc6821_data *data)
>>>    		if (err)
>>>    			return err;
>>> +		if (of_property_read_bool(client->dev.of_node, "ti,pwm-inverted"))
>>
>> I know that the AMC6821 is doing a lot of smart things, but this really
>> tickled me. PWM controllers actually do support that already via
>> PWM_POLARITY_INVERTED flag for example. See
>> Documentation/devicetree/bindings/hwmon/adt7475.yaml which seems to be
>> another HWMON driver which acts as a PWM controller. I'm not sure this is
>> relevant, applicable or desired but I wanted to highlight this.
> 
>  From the DT binding point of view, it seems to implement the same I am
> proposing here with adi,pwm-active-state property.
> 

Ah! It seems like I read only the part that agreed with the idea I had 
in mind :)

> Do you have anything more specific in mind?
> 

Yes, #pwm-cells just below in the binding. You can then see that the 
third cell in a PWM specifier is for the polarity. If I didn't misread 
once more, I believe that what's in adi,pwm-active-state is ignored 
based on the content of the PWM flags in a PWM cell specifier, c.f. 
adt7475_set_pwm_polarity followed by adt7475_fan_pwm_config in 
adt7475_probe. I would have assumed that having the polarity inverted in 
adi,pwm-active-state would mean that the meaning of the flag in the PWM 
cell specifier would be inverted as well, meaning 0 -> inverted, 
PWM_POLARITY_INVERTED -> doubly inverted so "normal" polarity.

adt7475_fan_pwm_config was added a few years after adt7475_set_pwm_polarity.

>>
>>> +			pwminv = 1;
>>> +
>>
>> This is silently overriding the module parameter.
>>
>> I don't think this is a good idea, at the very least not silently.
> 
> I was thinking at the same, and in the end I do have proposed this
> solution in any case.
> 
> Let's look at the 2 use cases in which the DT property and the module
> parameter are different.
> 
> ## 1
> 
> module parameter pwminv=0
> ti,pwm-inverted DT property present
> 
> => we enable the PWM inversion
> 
> I think this is fair, if someone has a DT based system we need to assume
> that the DT is correct. This is a HW configuration, not a module
> parameter.
> 
> ## 2
> 
> module parameter pwminv=1
> ti,pwm-inverted DT property absent
> 
> => we enable the PWM inversion
> 
> In this case the module parameter is overriding the DT. It means that
> someone explicitly set pwminv=1 module parameter. I think is fair to
> fulfill the module parameter request in this case, overriding the DT
> 

Why are we not assuming the DT is correct here as well? I don't like 
that the behavior is different depending on the presence of the DT 
property. Its absence should carry as much weight as its presence. If 
you don't want that to be the case, we can always have another property like

ti,pwm-polarity = <0>; /* normal polarity */

or

ti,pwm-polarity = <PWM_POLARITY_INVERTED>;

and then the absence of the DT property is a "weak" normal polarity for 
which we shouldn't print the error message if it differs from the module 
param. But honestly, I don't think the DT people will be happy with that 
suggestion :)

>> I would suggest to add some logic in the probe function to set this value
>> and check its consistency.
> 
> With that said I can implement something around the lines you proposed,
> if you still think is worth doing it. I would personally just keep the
> priority on the module parameter over the DT and add an info print on what
> is actually configured by the driver (not checking if they are
> different).
> 	

Module params over DT is fine with me, I just want consistency here, so 
if it's always the case, fine :)

Not really sure we need a dev_info, that's pretty verbose. I liked 
dev_err for when both settings differ.

Cheers,
Quentin

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

[Guenter Roeck]

On 2/19/25 02:08, Quentin Schulz wrote:
> Hi Francesco,
> 
> On 2/18/25 5:56 PM, Francesco Dolcini wrote:
>> From: Francesco Dolcini <francesco.dolcini@toradex.com>
>>
>> Add support to configure the PWM-Out pin polarity based on a device
>> tree property.
>>
>> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
>> ---
>>   drivers/hwmon/amc6821.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
>> index 1e3c6acd8974..1ea2d97eebca 100644
>> --- a/drivers/hwmon/amc6821.c
>> +++ b/drivers/hwmon/amc6821.c
>> @@ -845,7 +845,7 @@ 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;
>>       int err;
>> @@ -864,6 +864,9 @@ static int amc6821_init_client(struct amc6821_data *data)
>>           if (err)
>>               return err;
>> +        if (of_property_read_bool(client->dev.of_node, "ti,pwm-inverted"))
> 
> I know that the AMC6821 is doing a lot of smart things, but this really tickled me. PWM controllers actually do support that already via PWM_POLARITY_INVERTED flag for example. See Documentation/devicetree/bindings/hwmon/adt7475.yaml which seems to be another HWMON driver which acts as a PWM controller. I'm not sure this is relevant, applicable or desired but I wanted to highlight this.
> 
>> +            pwminv = 1;
>> +
> 
> This is silently overriding the module parameter.
> 
> I don't think this is a good idea, at the very least not silently.
> 
> I would suggest to add some logic in the probe function to set this value and check its consistency.
> 
> Something like:
> 
> """
> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
> index 1e3c6acd89740..3a13a914e2bbb 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;    /* -1 not modified by the user, 0 default PWM output, 1 inverted PWM output */
>   module_param(pwminv, int, 0444);
> 
>   static int init = 1; /*Power-on initialization.*/
> @@ -904,6 +904,7 @@ static int amc6821_probe(struct i2c_client *client)
>       struct amc6821_data *data;
>       struct device *hwmon_dev;
>       struct regmap *regmap;
> +    bool pwminv_dt;
>       int err;
> 
>       data = devm_kzalloc(dev, sizeof(struct amc6821_data), GFP_KERNEL);
> @@ -916,6 +917,18 @@ static int amc6821_probe(struct i2c_client *client)
>                        "Failed to initialize regmap\n");
>       data->regmap = regmap;
> 
> +    pwminv_dt = of_property_read_bool(client->dev.of_node, "ti,pwm-inverted");
> +
> +    if (pwminv == -1) {
> +        pwminv = pwminv_dt;

A devicetree property, associated with a single instance of the driver,
overriding a module parameter affecting all instances ? This is a no-go.

Guenter

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

[Francesco Dolcini]

On Wed, Feb 19, 2025 at 05:46:10AM -0800, Guenter Roeck wrote:
> On 2/19/25 02:08, Quentin Schulz wrote:
> > Hi Francesco,
> > 
> > On 2/18/25 5:56 PM, Francesco Dolcini wrote:
> > > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > 
> > > Add support to configure the PWM-Out pin polarity based on a device
> > > tree property.
> > > 
> > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > ---
> > >   drivers/hwmon/amc6821.c | 7 +++++--
> > >   1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
> > > index 1e3c6acd8974..1ea2d97eebca 100644
> > > --- a/drivers/hwmon/amc6821.c
> > > +++ b/drivers/hwmon/amc6821.c
> > > @@ -845,7 +845,7 @@ 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;
> > >       int err;
> > > @@ -864,6 +864,9 @@ static int amc6821_init_client(struct amc6821_data *data)
> > >           if (err)
> > >               return err;
> > > +        if (of_property_read_bool(client->dev.of_node, "ti,pwm-inverted"))
> > 
> > I know that the AMC6821 is doing a lot of smart things, but this really tickled me. PWM controllers actually do support that already via PWM_POLARITY_INVERTED flag for example. See Documentation/devicetree/bindings/hwmon/adt7475.yaml which seems to be another HWMON driver which acts as a PWM controller. I'm not sure this is relevant, applicable or desired but I wanted to highlight this.
> > 
> > > +            pwminv = 1;
> > > +
> > 
> > This is silently overriding the module parameter.
> > 
> > I don't think this is a good idea, at the very least not silently.
> > 
> > I would suggest to add some logic in the probe function to set this value and check its consistency.
> > 
> > Something like:
> > 
> > """
> > diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
> > index 1e3c6acd89740..3a13a914e2bbb 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;    /* -1 not modified by the user, 0 default PWM output, 1 inverted PWM output */
> >   module_param(pwminv, int, 0444);
> > 
> >   static int init = 1; /*Power-on initialization.*/
> > @@ -904,6 +904,7 @@ static int amc6821_probe(struct i2c_client *client)
> >       struct amc6821_data *data;
> >       struct device *hwmon_dev;
> >       struct regmap *regmap;
> > +    bool pwminv_dt;
> >       int err;
> > 
> >       data = devm_kzalloc(dev, sizeof(struct amc6821_data), GFP_KERNEL);
> > @@ -916,6 +917,18 @@ static int amc6821_probe(struct i2c_client *client)
> >                        "Failed to initialize regmap\n");
> >       data->regmap = regmap;
> > 
> > +    pwminv_dt = of_property_read_bool(client->dev.of_node, "ti,pwm-inverted");
> > +
> > +    if (pwminv == -1) {
> > +        pwminv = pwminv_dt;
> 
> A devicetree property, associated with a single instance of the driver,
> overriding a module parameter affecting all instances ? This is a no-go.

I will rework the patch in such a way that the module parameter, when
specified, takes always the precedence over the DT code, works for you
Guenter ?

Francesco

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

[Francesco Dolcini]

Hi Quentin,

On Wed, Feb 19, 2025 at 12:12:24PM +0100, Quentin Schulz wrote:
> On 2/19/25 11:33 AM, Francesco Dolcini wrote:
> > On Wed, Feb 19, 2025 at 11:08:43AM +0100, Quentin Schulz wrote:
> > > On 2/18/25 5:56 PM, Francesco Dolcini wrote:
> > > > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > > 
> > > > Add support to configure the PWM-Out pin polarity based on a device
> > > > tree property.
> > > > 
> > > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > > ---
> > > >    drivers/hwmon/amc6821.c | 7 +++++--
> > > >    1 file changed, 5 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
> > > > index 1e3c6acd8974..1ea2d97eebca 100644
> > > > --- a/drivers/hwmon/amc6821.c
> > > > +++ b/drivers/hwmon/amc6821.c
> > > > @@ -845,7 +845,7 @@ 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;
> > > >    	int err;
> > > > @@ -864,6 +864,9 @@ static int amc6821_init_client(struct amc6821_data *data)
> > > >    		if (err)
> > > >    			return err;
> > > > +		if (of_property_read_bool(client->dev.of_node, "ti,pwm-inverted"))
> > > 
> > > I know that the AMC6821 is doing a lot of smart things, but this really
> > > tickled me. PWM controllers actually do support that already via
> > > PWM_POLARITY_INVERTED flag for example. See
> > > Documentation/devicetree/bindings/hwmon/adt7475.yaml which seems to be
> > > another HWMON driver which acts as a PWM controller. I'm not sure this is
> > > relevant, applicable or desired but I wanted to highlight this.
> > 
> >  From the DT binding point of view, it seems to implement the same I am
> > proposing here with adi,pwm-active-state property.
> > 
> 
> Ah! It seems like I read only the part that agreed with the idea I had in
> mind :)
> 
> > Do you have anything more specific in mind?
> > 
> 
> Yes, #pwm-cells just below in the binding. You can then see that the third
> cell in a PWM specifier is for the polarity. If I didn't misread once more,
> I believe that what's in adi,pwm-active-state is ignored based on the
> content of the PWM flags in a PWM cell specifier, c.f.
> adt7475_set_pwm_polarity followed by adt7475_fan_pwm_config in
> adt7475_probe. I would have assumed that having the polarity inverted in
> adi,pwm-active-state would mean that the meaning of the flag in the PWM cell
> specifier would be inverted as well, meaning 0 -> inverted,
> PWM_POLARITY_INVERTED -> doubly inverted so "normal" polarity.
> 
> adt7475_fan_pwm_config was added a few years after adt7475_set_pwm_polarity.

I think this is out of scope for this patch. The amc6821 can control the
fan PWM stand-alone, this change has nothing to do with the generic pwm
framework, this is required to have the PWM out pin correctly driven by
the fan controller chip.

> Module params over DT is fine with me, I just want consistency here, so if
> it's always the case, fine :)

Ok, I'll implement it this way.

Francesco