[PATCH v2 0/2] hwmon: (adt7475) duty cycle configuration

[PATCH v2 0/2] 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.

Apologies for sending the quick v2 but the v1 I sent was an earlier local
iteration than what I meant to send out.

Chris Packham (2):
  dt-bindings: hwmon: Document adt7475 PWM initial duty cycle
  hwmon: (adt7475) Add support for configuring initial PWM duty cycle

 .../devicetree/bindings/hwmon/adt7475.yaml    | 27 ++++++++-
 drivers/hwmon/adt7475.c                       | 58 +++++++++++++++++++
 2 files changed, 84 insertions(+), 1 deletion(-)

-- 
2.43.2

[PATCH v2 1/2] dt-bindings: hwmon: Document adt7475 PWM initial duty cycle

[Chris Packham]

Add documentation for the pwm-initial-duty-cycle and
pwm-initial-frequency properties. These allow the starting state of the
PWM outputs to be set to cater for hardware designs where undesirable
amounts of noise is created by the default hardware state.

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

Notes:
    Changes in v2:
    - Document 0 as a valid value (leaves hardware as-is)

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

diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
index 051c976ab711..97deda082b4a 100644
--- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml
+++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
@@ -51,6 +51,30 @@ properties:
       enum: [0, 1]
       default: 1
 
+  adi,pwm-initial-duty-cycle:
+    description: |
+      Configures the initial duty cycle for the PWM outputs. The hardware
+      default is 100% but this may cause unwanted fan noise at startup. Set
+      this to a value from 0 (0% duty cycle) to 255 (100% duty cycle).
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 3
+    maxItems: 3
+    items:
+      minimum: 0
+      maximum: 255
+      default: 255
+
+  adi,pwm-initial-frequency:
+    description: |
+      Configures the initial frequency for the PWM outputs. A value of 0
+      will leave the PWM frequency as-is.
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 3
+    maxItems: 3
+    items:
+      enum: [0, 11, 14, 22, 29, 35, 44, 58, 88, 22500]
+      default: 35
+
 patternProperties:
   "^adi,bypass-attenuator-in[0-4]$":
     description: |
@@ -80,7 +104,6 @@ patternProperties:
       - therm#
       - smbalert#
       - gpio
-
 required:
   - compatible
   - reg
@@ -99,6 +122,8 @@ examples:
         adi,bypass-attenuator-in0 = <1>;
         adi,bypass-attenuator-in1 = <0>;
         adi,pwm-active-state = <1 0 1>;
+        adi,pwm-initial-duty-cycle = <128 0 128>;
+        adi,pwm-initial-frequency = <22500 0 22500>;
         adi,pin10-function = "smbalert#";
         adi,pin14-function = "tach4";
       };
-- 
2.43.2

[PATCH v2 2/2] hwmon: (adt7475) Add support for configuring initial PWM duty cycle

[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 take an initial PWM
duty cycle and frequency via device properties.

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

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

diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
index 4224ffb30483..14bd618488f8 100644
--- a/drivers/hwmon/adt7475.c
+++ b/drivers/hwmon/adt7475.c
@@ -1662,6 +1662,56 @@ static int adt7475_set_pwm_polarity(struct i2c_client *client)
 	return 0;
 }
 
+static int adt7475_set_pwm_initial_freq(struct i2c_client *client)
+{
+	int ret, out, i;
+	u32 freqs[ADT7475_PWM_COUNT];
+	int data;
+
+	ret = device_property_read_u32_array(&client->dev,
+					     "adi,pwm-initial-frequency", freqs,
+					     ARRAY_SIZE(freqs));
+	if (ret)
+		return ret;
+
+	for (i = 0; i < ADT7475_PWM_COUNT; i++) {
+		if (!freqs[i])
+			continue;
+		out = find_closest(freqs[i], pwmfreq_table, ARRAY_SIZE(pwmfreq_table));
+		data = adt7475_read(TEMP_TRANGE_REG(i));
+		if (data < 0)
+			return data;
+		data &= ~0xf;
+		data |= out;
+
+		ret = i2c_smbus_write_byte_data(client, TEMP_TRANGE_REG(i), data);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int adt7475_set_pwm_initial_duty(struct i2c_client *client)
+{
+	int ret, i;
+	u32 dutys[ADT7475_PWM_COUNT];
+
+	ret = device_property_read_u32_array(&client->dev,
+					     "adi,pwm-initial-duty-cycle", dutys,
+					     ARRAY_SIZE(dutys));
+	if (ret)
+		return ret;
+
+	for (i = 0; i < ADT7475_PWM_COUNT; i++) {
+		ret = i2c_smbus_write_byte_data(client, PWM_MAX_REG(i), dutys[i] & 0xff);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int adt7475_probe(struct i2c_client *client)
 {
 	enum chips chip;
@@ -1778,6 +1828,14 @@ static int adt7475_probe(struct i2c_client *client)
 	if (ret && ret != -EINVAL)
 		dev_warn(&client->dev, "Error configuring pwm polarity\n");
 
+	ret = adt7475_set_pwm_initial_freq(client);
+	if (ret && ret != -EINVAL)
+		dev_warn(&client->dev, "Error configuring pwm frequency\n");
+
+	ret = adt7475_set_pwm_initial_duty(client);
+	if (ret && ret != -EINVAL)
+		dev_warn(&client->dev, "Error configuring pwm duty cycle\n");
+
 	/* Start monitoring */
 	switch (chip) {
 	case adt7475:
-- 
2.43.2

Re: [PATCH v2 1/2] dt-bindings: hwmon: Document adt7475 PWM initial duty cycle

[Krzysztof Kozlowski]

On 08/05/2024 23:55, Chris Packham wrote:
> Add documentation for the pwm-initial-duty-cycle and
> pwm-initial-frequency properties. These allow the starting state of the
> PWM outputs to be set to cater for hardware designs where undesirable
> amounts of noise is created by the default hardware state.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> 
> Notes:
>     Changes in v2:
>     - Document 0 as a valid value (leaves hardware as-is)
> 
>  .../devicetree/bindings/hwmon/adt7475.yaml    | 27 ++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> index 051c976ab711..97deda082b4a 100644
> --- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> @@ -51,6 +51,30 @@ properties:
>        enum: [0, 1]
>        default: 1
>  
> +  adi,pwm-initial-duty-cycle:
> +    description: |
> +      Configures the initial duty cycle for the PWM outputs. The hardware
> +      default is 100% but this may cause unwanted fan noise at startup. Set
> +      this to a value from 0 (0% duty cycle) to 255 (100% duty cycle).
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 3
> +    maxItems: 3
> +    items:
> +      minimum: 0
> +      maximum: 255
> +      default: 255
> +
> +  adi,pwm-initial-frequency:

Frequency usually has some units, so use appropriate unit suffix and
drop $ref.  Maybe that's just target-rpm property?

But isn't this duplicating previous property? This is fan controller,
not PWM provider (in any case you miss proper $refs to pwm.yaml or
fan-common.yaml), so the only thing you initially want to configure is
the fan rotation, not specific PWM waveform. If you you want to
configure specific PWM waveform, then it's a PWM provider... but it is
not... Confused.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: hwmon: Document adt7475 PWM initial duty cycle

[Guenter Roeck]

On Thu, May 09, 2024 at 09:06:49AM +0200, Krzysztof Kozlowski wrote:
> On 08/05/2024 23:55, Chris Packham wrote:
> > Add documentation for the pwm-initial-duty-cycle and
> > pwm-initial-frequency properties. These allow the starting state of the
> > PWM outputs to be set to cater for hardware designs where undesirable
> > amounts of noise is created by the default hardware state.
> > 
> > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> > ---
> > 
> > Notes:
> >     Changes in v2:
> >     - Document 0 as a valid value (leaves hardware as-is)
> > 
> >  .../devicetree/bindings/hwmon/adt7475.yaml    | 27 ++++++++++++++++++-
> >  1 file changed, 26 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> > index 051c976ab711..97deda082b4a 100644
> > --- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> > +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> > @@ -51,6 +51,30 @@ properties:
> >        enum: [0, 1]
> >        default: 1
> >  
> > +  adi,pwm-initial-duty-cycle:
> > +    description: |
> > +      Configures the initial duty cycle for the PWM outputs. The hardware
> > +      default is 100% but this may cause unwanted fan noise at startup. Set
> > +      this to a value from 0 (0% duty cycle) to 255 (100% duty cycle).
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +    minItems: 3
> > +    maxItems: 3
> > +    items:
> > +      minimum: 0
> > +      maximum: 255
> > +      default: 255
> > +
> > +  adi,pwm-initial-frequency:
> 
> Frequency usually has some units, so use appropriate unit suffix and
> drop $ref.  Maybe that's just target-rpm property?
> 
We are talking pwm here, not rpm.

> But isn't this duplicating previous property? This is fan controller,
> not PWM provider (in any case you miss proper $refs to pwm.yaml or
> fan-common.yaml), so the only thing you initially want to configure is
> the fan rotation, not specific PWM waveform. If you you want to
> configure specific PWM waveform, then it's a PWM provider... but it is
> not... Confused.
> 

As I have said before ... almost all fan controllers have pwm outputs to
control the fans, because that is how fans are controlled. So, in your
terminology, pretty much all fan controllers are also pwm providers.

At the same time, I resist the push to implement pwm controller code in
fan controller drivers because that would just add a lot of code for no good
reason other than "because". I guess we'll have to find a means to extract
pwm related configuration data such as this one from devicetree without
actually implementing a full blown pwm controller driver.

Guenter

Re: [PATCH v2 1/2] dt-bindings: hwmon: Document adt7475 PWM initial duty cycle

[Chris Packham]

Hi Krzysztof,

On 9/05/24 19:06, Krzysztof Kozlowski wrote:
> On 08/05/2024 23:55, Chris Packham wrote:
>> Add documentation for the pwm-initial-duty-cycle and
>> pwm-initial-frequency properties. These allow the starting state of the
>> PWM outputs to be set to cater for hardware designs where undesirable
>> amounts of noise is created by the default hardware state.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
>>
>> Notes:
>>      Changes in v2:
>>      - Document 0 as a valid value (leaves hardware as-is)
>>
>>   .../devicetree/bindings/hwmon/adt7475.yaml    | 27 ++++++++++++++++++-
>>   1 file changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
>> index 051c976ab711..97deda082b4a 100644
>> --- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml
>> +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
>> @@ -51,6 +51,30 @@ properties:
>>         enum: [0, 1]
>>         default: 1
>>   
>> +  adi,pwm-initial-duty-cycle:
>> +    description: |
>> +      Configures the initial duty cycle for the PWM outputs. The hardware
>> +      default is 100% but this may cause unwanted fan noise at startup. Set
>> +      this to a value from 0 (0% duty cycle) to 255 (100% duty cycle).
>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>> +    minItems: 3
>> +    maxItems: 3
>> +    items:
>> +      minimum: 0
>> +      maximum: 255
>> +      default: 255
>> +
>> +  adi,pwm-initial-frequency:
> Frequency usually has some units, so use appropriate unit suffix and
> drop $ref.  Maybe that's just target-rpm property?
>
> But isn't this duplicating previous property? This is fan controller,
> not PWM provider (in any case you miss proper $refs to pwm.yaml or
> fan-common.yaml), so the only thing you initially want to configure is
> the fan rotation, not specific PWM waveform. If you you want to
> configure specific PWM waveform, then it's a PWM provider... but it is
> not... Confused.

There's two things going on here. There's a PWM duty cycle which is 
configurable from 0% to 100%. It might be nice if this was expressed as 
a percentage instead of 0-255 but I went with the latter because that's 
how the sysfs ABI for the duty cycle works.

The frequency (which I'll call adi,pwm-initial-frequency-hz in v3) 
affects how that duty cycle is presented to the fans. So you could still 
have a duty cycle of 50% at any frequency. What frequency is best 
depends on the kind of fans being used. In my particular case the lower 
frequencies end up with the fans oscillating annoyingly so I use the 
highest setting.

>
> Best regards,
> Krzysztof
>

Re: [PATCH v2 1/2] dt-bindings: hwmon: Document adt7475 PWM initial duty cycle

[Guenter Roeck]

Chris,

On Thu, May 09, 2024 at 06:19:12PM +0000, Chris Packham wrote:
> Hi Krzysztof,
> 
> On 9/05/24 19:06, Krzysztof Kozlowski wrote:
> > On 08/05/2024 23:55, Chris Packham wrote:
> >> Add documentation for the pwm-initial-duty-cycle and
> >> pwm-initial-frequency properties. These allow the starting state of the
> >> PWM outputs to be set to cater for hardware designs where undesirable
> >> amounts of noise is created by the default hardware state.
> >>
> >> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> >> ---
> >>
> >> Notes:
> >>      Changes in v2:
> >>      - Document 0 as a valid value (leaves hardware as-is)
> >>
> >>   .../devicetree/bindings/hwmon/adt7475.yaml    | 27 ++++++++++++++++++-
> >>   1 file changed, 26 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> >> index 051c976ab711..97deda082b4a 100644
> >> --- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> >> +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> >> @@ -51,6 +51,30 @@ properties:
> >>         enum: [0, 1]
> >>         default: 1
> >>   
> >> +  adi,pwm-initial-duty-cycle:
> >> +    description: |
> >> +      Configures the initial duty cycle for the PWM outputs. The hardware
> >> +      default is 100% but this may cause unwanted fan noise at startup. Set
> >> +      this to a value from 0 (0% duty cycle) to 255 (100% duty cycle).
> >> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> >> +    minItems: 3
> >> +    maxItems: 3
> >> +    items:
> >> +      minimum: 0
> >> +      maximum: 255
> >> +      default: 255
> >> +
> >> +  adi,pwm-initial-frequency:
> > Frequency usually has some units, so use appropriate unit suffix and
> > drop $ref.  Maybe that's just target-rpm property?
> >
> > But isn't this duplicating previous property? This is fan controller,
> > not PWM provider (in any case you miss proper $refs to pwm.yaml or
> > fan-common.yaml), so the only thing you initially want to configure is
> > the fan rotation, not specific PWM waveform. If you you want to
> > configure specific PWM waveform, then it's a PWM provider... but it is
> > not... Confused.
> 
> There's two things going on here. There's a PWM duty cycle which is 
> configurable from 0% to 100%. It might be nice if this was expressed as 
> a percentage instead of 0-255 but I went with the latter because that's 
> how the sysfs ABI for the duty cycle works.
> 
> The frequency (which I'll call adi,pwm-initial-frequency-hz in v3) 
> affects how that duty cycle is presented to the fans. So you could still 
> have a duty cycle of 50% at any frequency. What frequency is best 
> depends on the kind of fans being used. In my particular case the lower 
> frequencies end up with the fans oscillating annoyingly so I use the 
> highest setting.
> 

My udnerstanding is that we are supposed to use standard pwm provider
properties. The property description is provider specicic, so I think
we can pretty much just make it up.

Essentially you'd first define a pwm provider which defines all the
pwm parameters needed, such as pwm freqency, default duty cycle,
and flags such as PWM_POLARITY_INVERTED. You'd then add something like

	pwms = <&pwm index frequency duty_cycle ... flags>;

to the node for each fan, and be done.

That doesn't mean that we would actually have to register the chip
as pwm provider with the pwm subsystem; all we would have to do is to
interpret the property values.

Hope thie helps,
Guenter

Re: [PATCH v2 1/2] dt-bindings: hwmon: Document adt7475 PWM initial duty cycle

[Chris Packham]

On 10/05/24 15:36, Guenter Roeck wrote:
> Chris,
>
> On Thu, May 09, 2024 at 06:19:12PM +0000, Chris Packham wrote:
>> Hi Krzysztof,
>>
>> On 9/05/24 19:06, Krzysztof Kozlowski wrote:
>>> On 08/05/2024 23:55, Chris Packham wrote:
>>>> Add documentation for the pwm-initial-duty-cycle and
>>>> pwm-initial-frequency properties. These allow the starting state of the
>>>> PWM outputs to be set to cater for hardware designs where undesirable
>>>> amounts of noise is created by the default hardware state.
>>>>
>>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>>> ---
>>>>
>>>> Notes:
>>>>       Changes in v2:
>>>>       - Document 0 as a valid value (leaves hardware as-is)
>>>>
>>>>    .../devicetree/bindings/hwmon/adt7475.yaml    | 27 ++++++++++++++++++-
>>>>    1 file changed, 26 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
>>>> index 051c976ab711..97deda082b4a 100644
>>>> --- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml
>>>> +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
>>>> @@ -51,6 +51,30 @@ properties:
>>>>          enum: [0, 1]
>>>>          default: 1
>>>>    
>>>> +  adi,pwm-initial-duty-cycle:
>>>> +    description: |
>>>> +      Configures the initial duty cycle for the PWM outputs. The hardware
>>>> +      default is 100% but this may cause unwanted fan noise at startup. Set
>>>> +      this to a value from 0 (0% duty cycle) to 255 (100% duty cycle).
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>>>> +    minItems: 3
>>>> +    maxItems: 3
>>>> +    items:
>>>> +      minimum: 0
>>>> +      maximum: 255
>>>> +      default: 255
>>>> +
>>>> +  adi,pwm-initial-frequency:
>>> Frequency usually has some units, so use appropriate unit suffix and
>>> drop $ref.  Maybe that's just target-rpm property?
>>>
>>> But isn't this duplicating previous property? This is fan controller,
>>> not PWM provider (in any case you miss proper $refs to pwm.yaml or
>>> fan-common.yaml), so the only thing you initially want to configure is
>>> the fan rotation, not specific PWM waveform. If you you want to
>>> configure specific PWM waveform, then it's a PWM provider... but it is
>>> not... Confused.
>> There's two things going on here. There's a PWM duty cycle which is
>> configurable from 0% to 100%. It might be nice if this was expressed as
>> a percentage instead of 0-255 but I went with the latter because that's
>> how the sysfs ABI for the duty cycle works.
>>
>> The frequency (which I'll call adi,pwm-initial-frequency-hz in v3)
>> affects how that duty cycle is presented to the fans. So you could still
>> have a duty cycle of 50% at any frequency. What frequency is best
>> depends on the kind of fans being used. In my particular case the lower
>> frequencies end up with the fans oscillating annoyingly so I use the
>> highest setting.
>>
> My udnerstanding is that we are supposed to use standard pwm provider
> properties. The property description is provider specicic, so I think
> we can pretty much just make it up.
>
> Essentially you'd first define a pwm provider which defines all the
> pwm parameters needed, such as pwm freqency, default duty cycle,
> and flags such as PWM_POLARITY_INVERTED. You'd then add something like
>
> 	pwms = <&pwm index frequency duty_cycle ... flags>;
>
> to the node for each fan, and be done.
>
> That doesn't mean that we would actually have to register the chip
> as pwm provider with the pwm subsystem; all we would have to do is to
> interpret the property values.

We've already got the pwm-active-state as a separate property so that 
might be tricky to deal with, I guess it could be deprecated in favour 
of something else. Looking at pwm.yaml and fan-common.yaml I can't quite 
see how that'd help here. Were you thinking maybe something like

pwm: hwmon@2e {
     compatible = "adi,adt7476";
     reg = <0x2e>;
     #pwm-cells = <4>;
     fan-0 {
         pwms = <&pwm 0 255 22500 PWM_POLARITY_INVERTED>;
         pwm-names = "PWM1";
         tach-ch = <0>;
     };
     fan-1 {
         // controlled by pwm 0
         tach-ch = <1>
     };
     fan-0 {
         pwms = <&pwm 2 255 22500 PWM_POLARITY_INVERTED>;
         pwm-names = "PWM3";
         tach-ch <2>;
     };
     fan-1 {
         // controlled by pwm 2
         tach-ch = <3>
     };
};

Re: [PATCH v2 1/2] dt-bindings: hwmon: Document adt7475 PWM initial duty cycle

[Guenter Roeck]

On 5/10/24 08:51, Chris Packham wrote:
> 
> On 10/05/24 15:36, Guenter Roeck wrote:
>> Chris,
>>
>> On Thu, May 09, 2024 at 06:19:12PM +0000, Chris Packham wrote:
>>> Hi Krzysztof,
>>>
>>> On 9/05/24 19:06, Krzysztof Kozlowski wrote:
>>>> On 08/05/2024 23:55, Chris Packham wrote:
>>>>> Add documentation for the pwm-initial-duty-cycle and
>>>>> pwm-initial-frequency properties. These allow the starting state of the
>>>>> PWM outputs to be set to cater for hardware designs where undesirable
>>>>> amounts of noise is created by the default hardware state.
>>>>>
>>>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>>>> ---
>>>>>
>>>>> Notes:
>>>>>        Changes in v2:
>>>>>        - Document 0 as a valid value (leaves hardware as-is)
>>>>>
>>>>>     .../devicetree/bindings/hwmon/adt7475.yaml    | 27 ++++++++++++++++++-
>>>>>     1 file changed, 26 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
>>>>> index 051c976ab711..97deda082b4a 100644
>>>>> --- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml
>>>>> +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
>>>>> @@ -51,6 +51,30 @@ properties:
>>>>>           enum: [0, 1]
>>>>>           default: 1
>>>>>     
>>>>> +  adi,pwm-initial-duty-cycle:
>>>>> +    description: |
>>>>> +      Configures the initial duty cycle for the PWM outputs. The hardware
>>>>> +      default is 100% but this may cause unwanted fan noise at startup. Set
>>>>> +      this to a value from 0 (0% duty cycle) to 255 (100% duty cycle).
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>>>>> +    minItems: 3
>>>>> +    maxItems: 3
>>>>> +    items:
>>>>> +      minimum: 0
>>>>> +      maximum: 255
>>>>> +      default: 255
>>>>> +
>>>>> +  adi,pwm-initial-frequency:
>>>> Frequency usually has some units, so use appropriate unit suffix and
>>>> drop $ref.  Maybe that's just target-rpm property?
>>>>
>>>> But isn't this duplicating previous property? This is fan controller,
>>>> not PWM provider (in any case you miss proper $refs to pwm.yaml or
>>>> fan-common.yaml), so the only thing you initially want to configure is
>>>> the fan rotation, not specific PWM waveform. If you you want to
>>>> configure specific PWM waveform, then it's a PWM provider... but it is
>>>> not... Confused.
>>> There's two things going on here. There's a PWM duty cycle which is
>>> configurable from 0% to 100%. It might be nice if this was expressed as
>>> a percentage instead of 0-255 but I went with the latter because that's
>>> how the sysfs ABI for the duty cycle works.
>>>
>>> The frequency (which I'll call adi,pwm-initial-frequency-hz in v3)
>>> affects how that duty cycle is presented to the fans. So you could still
>>> have a duty cycle of 50% at any frequency. What frequency is best
>>> depends on the kind of fans being used. In my particular case the lower
>>> frequencies end up with the fans oscillating annoyingly so I use the
>>> highest setting.
>>>
>> My udnerstanding is that we are supposed to use standard pwm provider
>> properties. The property description is provider specicic, so I think
>> we can pretty much just make it up.
>>
>> Essentially you'd first define a pwm provider which defines all the
>> pwm parameters needed, such as pwm freqency, default duty cycle,
>> and flags such as PWM_POLARITY_INVERTED. You'd then add something like
>>
>> 	pwms = <&pwm index frequency duty_cycle ... flags>;
>>
>> to the node for each fan, and be done.
>>
>> That doesn't mean that we would actually have to register the chip
>> as pwm provider with the pwm subsystem; all we would have to do is to
>> interpret the property values.
> 
> We've already got the pwm-active-state as a separate property so that
> might be tricky to deal with, I guess it could be deprecated in favour
> of something else. Looking at pwm.yaml and fan-common.yaml I can't quite
> see how that'd help here. Were you thinking maybe something like
> 
> pwm: hwmon@2e {
>       compatible = "adi,adt7476";
>       reg = <0x2e>;
>       #pwm-cells = <4>;
>       fan-0 {
>           pwms = <&pwm 0 255 22500 PWM_POLARITY_INVERTED>;
>           pwm-names = "PWM1";
>           tach-ch = <0>;
>       };
>       fan-1 {
>           // controlled by pwm 0
>           tach-ch = <1>
>       };
>       fan-0 {
>           pwms = <&pwm 2 255 22500 PWM_POLARITY_INVERTED>;
>           pwm-names = "PWM3";
>           tach-ch <2>;
>       };
>       fan-1 {
>           // controlled by pwm 2
>           tach-ch = <3>

I think that would have to be

	...
	fan-0 {
		pwms = <&pwm 0 255 22500 PWM_POLARITY_INVERTED>;
		tach-ch = <1 2>;
	};
	fan-1 {
		tach-ch = <3>
	};
	...

Context: pwm-names is optional and does not add value here unless I am missing
something. Also, if I understand the bindings correctly, all tachometer channels
controlled by a single pwm are supposed to be listed in a single node. With the
above, you'd then have fan1, fan2, and fan3 plus pwm1 and pwm3 (pwm2 would be
disabled/unused).

Code-wise, I think you'd then call
	
	struct of_phandle_args args;
	...
	err = of_parse_phandle_with_args(np, "pwms", "#pwm-cells", 0, &args)

with np pointing to the fan node. This should return the parameters in 'args'.

However, unless you have a use case, I'd suggest not to implement support for
"multiple fans controlled by single pwm" since that would require extra
code and you would not actually be able to test it. A mandatory 1:1 mapping
is fine with me. Support for 1:n mapping can be implemented if / when there
is a use case. The same is true for registering the driver with the pwm
subsystem - that would only be necessary if anyone ever uses one of the
pwm channels for non-fan use.

That makes me wonder if we actually need tach-ch in the first place or if
something like

	fan-0 {
		pwms = <&pwm 0 255 22500 PWM_POLARITY_INVERTED>;
	};
	fan-1 {
		pwms = <&pwm 1 255 22500 0>;
	};
	...
	
would do for this chip.

Thanks,
Guenter

Re: [PATCH v2 1/2] dt-bindings: hwmon: Document adt7475 PWM initial duty cycle

[Chris Packham]

On 13/05/24 04:58, Guenter Roeck wrote:
> On 5/10/24 08:51, Chris Packham wrote:
>>
>> On 10/05/24 15:36, Guenter Roeck wrote:
>>> Chris,
>>>
>>> On Thu, May 09, 2024 at 06:19:12PM +0000, Chris Packham wrote:
>>>> Hi Krzysztof,
>>>>
>>>> On 9/05/24 19:06, Krzysztof Kozlowski wrote:
>>>>> On 08/05/2024 23:55, Chris Packham wrote:
>>>>>> Add documentation for the pwm-initial-duty-cycle and
>>>>>> pwm-initial-frequency properties. These allow the starting state 
>>>>>> of the
>>>>>> PWM outputs to be set to cater for hardware designs where 
>>>>>> undesirable
>>>>>> amounts of noise is created by the default hardware state.
>>>>>>
>>>>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>>>>> ---
>>>>>>
>>>>>> Notes:
>>>>>>        Changes in v2:
>>>>>>        - Document 0 as a valid value (leaves hardware as-is)
>>>>>>
>>>>>>     .../devicetree/bindings/hwmon/adt7475.yaml    | 27 
>>>>>> ++++++++++++++++++-
>>>>>>     1 file changed, 26 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml 
>>>>>> b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
>>>>>> index 051c976ab711..97deda082b4a 100644
>>>>>> --- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
>>>>>> @@ -51,6 +51,30 @@ properties:
>>>>>>           enum: [0, 1]
>>>>>>           default: 1
>>>>>>     +  adi,pwm-initial-duty-cycle:
>>>>>> +    description: |
>>>>>> +      Configures the initial duty cycle for the PWM outputs. The 
>>>>>> hardware
>>>>>> +      default is 100% but this may cause unwanted fan noise at 
>>>>>> startup. Set
>>>>>> +      this to a value from 0 (0% duty cycle) to 255 (100% duty 
>>>>>> cycle).
>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>>>>>> +    minItems: 3
>>>>>> +    maxItems: 3
>>>>>> +    items:
>>>>>> +      minimum: 0
>>>>>> +      maximum: 255
>>>>>> +      default: 255
>>>>>> +
>>>>>> +  adi,pwm-initial-frequency:
>>>>> Frequency usually has some units, so use appropriate unit suffix and
>>>>> drop $ref.  Maybe that's just target-rpm property?
>>>>>
>>>>> But isn't this duplicating previous property? This is fan controller,
>>>>> not PWM provider (in any case you miss proper $refs to pwm.yaml or
>>>>> fan-common.yaml), so the only thing you initially want to 
>>>>> configure is
>>>>> the fan rotation, not specific PWM waveform. If you you want to
>>>>> configure specific PWM waveform, then it's a PWM provider... but 
>>>>> it is
>>>>> not... Confused.
>>>> There's two things going on here. There's a PWM duty cycle which is
>>>> configurable from 0% to 100%. It might be nice if this was 
>>>> expressed as
>>>> a percentage instead of 0-255 but I went with the latter because 
>>>> that's
>>>> how the sysfs ABI for the duty cycle works.
>>>>
>>>> The frequency (which I'll call adi,pwm-initial-frequency-hz in v3)
>>>> affects how that duty cycle is presented to the fans. So you could 
>>>> still
>>>> have a duty cycle of 50% at any frequency. What frequency is best
>>>> depends on the kind of fans being used. In my particular case the 
>>>> lower
>>>> frequencies end up with the fans oscillating annoyingly so I use the
>>>> highest setting.
>>>>
>>> My udnerstanding is that we are supposed to use standard pwm provider
>>> properties. The property description is provider specicic, so I think
>>> we can pretty much just make it up.
>>>
>>> Essentially you'd first define a pwm provider which defines all the
>>> pwm parameters needed, such as pwm freqency, default duty cycle,
>>> and flags such as PWM_POLARITY_INVERTED. You'd then add something like
>>>
>>>     pwms = <&pwm index frequency duty_cycle ... flags>;
>>>
>>> to the node for each fan, and be done.
>>>
>>> That doesn't mean that we would actually have to register the chip
>>> as pwm provider with the pwm subsystem; all we would have to do is to
>>> interpret the property values.
>>
>> We've already got the pwm-active-state as a separate property so that
>> might be tricky to deal with, I guess it could be deprecated in favour
>> of something else. Looking at pwm.yaml and fan-common.yaml I can't quite
>> see how that'd help here. Were you thinking maybe something like
>>
>> pwm: hwmon@2e {
>>       compatible = "adi,adt7476";
>>       reg = <0x2e>;
>>       #pwm-cells = <4>;
>>       fan-0 {
>>           pwms = <&pwm 0 255 22500 PWM_POLARITY_INVERTED>;
>>           pwm-names = "PWM1";
>>           tach-ch = <0>;
>>       };
>>       fan-1 {
>>           // controlled by pwm 0
>>           tach-ch = <1>
>>       };
>>       fan-0 {
>>           pwms = <&pwm 2 255 22500 PWM_POLARITY_INVERTED>;
>>           pwm-names = "PWM3";
>>           tach-ch <2>;
>>       };
>>       fan-1 {
>>           // controlled by pwm 2
>>           tach-ch = <3>
>
> I think that would have to be
>
>     ...
>     fan-0 {
>         pwms = <&pwm 0 255 22500 PWM_POLARITY_INVERTED>;
>         tach-ch = <1 2>;
>     };
>     fan-1 {
>         tach-ch = <3>
>     };
>     ...
>
> Context: pwm-names is optional and does not add value here unless I am 
> missing
> something. Also, if I understand the bindings correctly, all 
> tachometer channels
> controlled by a single pwm are supposed to be listed in a single node. 
> With the
> above, you'd then have fan1, fan2, and fan3 plus pwm1 and pwm3 (pwm2 
> would be
> disabled/unused).
>
> Code-wise, I think you'd then call
>
>     struct of_phandle_args args;
>     ...
>     err = of_parse_phandle_with_args(np, "pwms", "#pwm-cells", 0, &args)
>
> with np pointing to the fan node. This should return the parameters in 
> 'args'.

On that point. How would I explain in the bindings that cell 2 is the 
duty cycle, cell 3 is the frequency and cell 4 is the flags?

The other complication is that one of the systems I have is x86 so I 
need to express this with the ACPI Device Properties compatibility code. 
I think I can figure out the ACPI table stuff but I can't call 
of_parse_phandle_with_args() directly.

>
> However, unless you have a use case, I'd suggest not to implement 
> support for
> "multiple fans controlled by single pwm" since that would require extra
> code and you would not actually be able to test it. A mandatory 1:1 
> mapping
> is fine with me. Support for 1:n mapping can be implemented if / when 
> there
> is a use case. 

The system I'm dealing with has exactly that. But we don't adjust the 
fan RPM directly so I think we're OK (just maybe some comments so people 
aren't confused by missing fans). The ADT7476 will adjust the PWM duty 
cycle based on the temperature, the fan RPM is just something we report 
(and generate an alarm if it goes too low).

> The same is true for registering the driver with the pwm
> subsystem - that would only be necessary if anyone ever uses one of the
> pwm channels for non-fan use.

Agreed. I won't plumb anything into the pwm subsystem. Although it would 
be kind of neat to see a LED that changes as the system gets hotter, 
kind of like an electronic thermochromic crystal.

>
> That makes me wonder if we actually need tach-ch in the first place or if
> something like
>
>     fan-0 {
>         pwms = <&pwm 0 255 22500 PWM_POLARITY_INVERTED>;
>     };
>     fan-1 {
>         pwms = <&pwm 1 255 22500 0>;
>     };
>     ...
> would do for this chip. 

Yeah that'd be fine for me.

Re: [PATCH v2 1/2] dt-bindings: hwmon: Document adt7475 PWM initial duty cycle

[Conor Dooley]

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

On Fri, May 17, 2024 at 01:09:03AM +0000, Chris Packham wrote:
> 
> On 13/05/24 04:58, Guenter Roeck wrote:
> > On 5/10/24 08:51, Chris Packham wrote:
> >>
> >> On 10/05/24 15:36, Guenter Roeck wrote:
> >>> Chris,
> >>>
> >>> On Thu, May 09, 2024 at 06:19:12PM +0000, Chris Packham wrote:
> >>>> Hi Krzysztof,
> >>>>
> >>>> On 9/05/24 19:06, Krzysztof Kozlowski wrote:
> >>>>> On 08/05/2024 23:55, Chris Packham wrote:
> >>>>>> Add documentation for the pwm-initial-duty-cycle and
> >>>>>> pwm-initial-frequency properties. These allow the starting state 
> >>>>>> of the
> >>>>>> PWM outputs to be set to cater for hardware designs where 
> >>>>>> undesirable
> >>>>>> amounts of noise is created by the default hardware state.
> >>>>>>
> >>>>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> >>>>>> ---
> >>>>>>
> >>>>>> Notes:
> >>>>>>        Changes in v2:
> >>>>>>        - Document 0 as a valid value (leaves hardware as-is)
> >>>>>>
> >>>>>>     .../devicetree/bindings/hwmon/adt7475.yaml    | 27 
> >>>>>> ++++++++++++++++++-
> >>>>>>     1 file changed, 26 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml 
> >>>>>> b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> >>>>>> index 051c976ab711..97deda082b4a 100644
> >>>>>> --- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> >>>>>> +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> >>>>>> @@ -51,6 +51,30 @@ properties:
> >>>>>>           enum: [0, 1]
> >>>>>>           default: 1
> >>>>>>     +  adi,pwm-initial-duty-cycle:
> >>>>>> +    description: |
> >>>>>> +      Configures the initial duty cycle for the PWM outputs. The 
> >>>>>> hardware
> >>>>>> +      default is 100% but this may cause unwanted fan noise at 
> >>>>>> startup. Set
> >>>>>> +      this to a value from 0 (0% duty cycle) to 255 (100% duty 
> >>>>>> cycle).
> >>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> >>>>>> +    minItems: 3
> >>>>>> +    maxItems: 3
> >>>>>> +    items:
> >>>>>> +      minimum: 0
> >>>>>> +      maximum: 255
> >>>>>> +      default: 255
> >>>>>> +
> >>>>>> +  adi,pwm-initial-frequency:
> >>>>> Frequency usually has some units, so use appropriate unit suffix and
> >>>>> drop $ref.  Maybe that's just target-rpm property?
> >>>>>
> >>>>> But isn't this duplicating previous property? This is fan controller,
> >>>>> not PWM provider (in any case you miss proper $refs to pwm.yaml or
> >>>>> fan-common.yaml), so the only thing you initially want to 
> >>>>> configure is
> >>>>> the fan rotation, not specific PWM waveform. If you you want to
> >>>>> configure specific PWM waveform, then it's a PWM provider... but 
> >>>>> it is
> >>>>> not... Confused.
> >>>> There's two things going on here. There's a PWM duty cycle which is
> >>>> configurable from 0% to 100%. It might be nice if this was 
> >>>> expressed as
> >>>> a percentage instead of 0-255 but I went with the latter because 
> >>>> that's
> >>>> how the sysfs ABI for the duty cycle works.
> >>>>
> >>>> The frequency (which I'll call adi,pwm-initial-frequency-hz in v3)
> >>>> affects how that duty cycle is presented to the fans. So you could 
> >>>> still
> >>>> have a duty cycle of 50% at any frequency. What frequency is best
> >>>> depends on the kind of fans being used. In my particular case the 
> >>>> lower
> >>>> frequencies end up with the fans oscillating annoyingly so I use the
> >>>> highest setting.
> >>>>
> >>> My udnerstanding is that we are supposed to use standard pwm provider
> >>> properties. The property description is provider specicic, so I think
> >>> we can pretty much just make it up.
> >>>
> >>> Essentially you'd first define a pwm provider which defines all the
> >>> pwm parameters needed, such as pwm freqency, default duty cycle,
> >>> and flags such as PWM_POLARITY_INVERTED. You'd then add something like
> >>>
> >>>     pwms = <&pwm index frequency duty_cycle ... flags>;
> >>>
> >>> to the node for each fan, and be done.
> >>>
> >>> That doesn't mean that we would actually have to register the chip
> >>> as pwm provider with the pwm subsystem; all we would have to do is to
> >>> interpret the property values.
> >>
> >> We've already got the pwm-active-state as a separate property so that
> >> might be tricky to deal with, I guess it could be deprecated in favour
> >> of something else. Looking at pwm.yaml and fan-common.yaml I can't quite
> >> see how that'd help here. Were you thinking maybe something like
> >>
> >> pwm: hwmon@2e {
> >>       compatible = "adi,adt7476";
> >>       reg = <0x2e>;
> >>       #pwm-cells = <4>;
> >>       fan-0 {
> >>           pwms = <&pwm 0 255 22500 PWM_POLARITY_INVERTED>;
> >>           pwm-names = "PWM1";
> >>           tach-ch = <0>;
> >>       };
> >>       fan-1 {
> >>           // controlled by pwm 0
> >>           tach-ch = <1>
> >>       };
> >>       fan-0 {
> >>           pwms = <&pwm 2 255 22500 PWM_POLARITY_INVERTED>;
> >>           pwm-names = "PWM3";
> >>           tach-ch <2>;
> >>       };
> >>       fan-1 {
> >>           // controlled by pwm 2
> >>           tach-ch = <3>
> >
> > I think that would have to be
> >
> >     ...
> >     fan-0 {
> >         pwms = <&pwm 0 255 22500 PWM_POLARITY_INVERTED>;
> >         tach-ch = <1 2>;
> >     };
> >     fan-1 {
> >         tach-ch = <3>
> >     };
> >     ...
> >
> > Context: pwm-names is optional and does not add value here unless I am 
> > missing
> > something. Also, if I understand the bindings correctly, all 
> > tachometer channels
> > controlled by a single pwm are supposed to be listed in a single node. 
> > With the
> > above, you'd then have fan1, fan2, and fan3 plus pwm1 and pwm3 (pwm2 
> > would be
> > disabled/unused).
> >
> > Code-wise, I think you'd then call
> >
> >     struct of_phandle_args args;
> >     ...
> >     err = of_parse_phandle_with_args(np, "pwms", "#pwm-cells", 0, &args)
> >
> > with np pointing to the fan node. This should return the parameters in 
> > 'args'.
> 
> On that point. How would I explain in the bindings that cell 2 is the 
> duty cycle, cell 3 is the frequency and cell 4 is the flags?

In the pwm-cells property in the pwm provider binding . You might want to
order it as <index freq flags duty> as usually that's the ordering done
in most (all?) pwm provider bindings that I have seen.
The pwm bindings I think are really unhelpful though - they all say "see
pwm.yaml for info on the cells in #pwm-cells, but then pwm.yaml has no
information. The information is actually in pwm.text, but the binding
conversion did s/pwm.text/pwm.yaml/ in pwm controller bindings.
I'll send a patch that fixes up pwm.yaml.

> 
> The other complication is that one of the systems I have is x86 so I 
> need to express this with the ACPI Device Properties compatibility code. 
> I think I can figure out the ACPI table stuff but I can't call 
> of_parse_phandle_with_args() directly.
> 
> >
> > However, unless you have a use case, I'd suggest not to implement 
> > support for
> > "multiple fans controlled by single pwm" since that would require extra
> > code and you would not actually be able to test it. A mandatory 1:1 
> > mapping
> > is fine with me. Support for 1:n mapping can be implemented if / when 
> > there
> > is a use case. 
> 
> The system I'm dealing with has exactly that. But we don't adjust the 
> fan RPM directly so I think we're OK (just maybe some comments so people 
> aren't confused by missing fans). The ADT7476 will adjust the PWM duty 
> cycle based on the temperature, the fan RPM is just something we report 
> (and generate an alarm if it goes too low).
> 
> > The same is true for registering the driver with the pwm
> > subsystem - that would only be necessary if anyone ever uses one of the
> > pwm channels for non-fan use.
> 
> Agreed. I won't plumb anything into the pwm subsystem. Although it would 
> be kind of neat to see a LED that changes as the system gets hotter, 
> kind of like an electronic thermochromic crystal.
> 
> >
> > That makes me wonder if we actually need tach-ch in the first place or if
> > something like
> >
> >     fan-0 {
> >         pwms = <&pwm 0 255 22500 PWM_POLARITY_INVERTED>;
> >     };
> >     fan-1 {
> >         pwms = <&pwm 1 255 22500 0>;
> >     };
> >     ...
> > would do for this chip. 
> 
> Yeah that'd be fine for me.

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

Re: [PATCH v2 1/2] dt-bindings: hwmon: Document adt7475 PWM initial duty cycle

[Conor Dooley]

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

On Fri, May 17, 2024 at 06:00:06PM +0100, Conor Dooley wrote:
> > On that point. How would I explain in the bindings that cell 2 is the 
> > duty cycle, cell 3 is the frequency and cell 4 is the flags?
> 
> In the pwm-cells property in the pwm provider binding . You might want to
> order it as <index freq flags duty> as usually that's the ordering done
> in most (all?) pwm provider bindings that I have seen.
> The pwm bindings I think are really unhelpful though - they all say "see
> pwm.yaml for info on the cells in #pwm-cells, but then pwm.yaml has no
> information. The information is actually in pwm.text, but the binding
> conversion did s/pwm.text/pwm.yaml/ in pwm controller bindings.
> I'll send a patch that fixes up pwm.yaml.

Possibly cell 4 should be standardised as the period for all pwm
providers and then all you'd have to do for your provider is set
#pwm-cells:
  minItems: 4

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

Re: [PATCH v2 1/2] dt-bindings: hwmon: Document adt7475 PWM initial duty cycle

[Guenter Roeck]

On 5/17/24 10:02, Conor Dooley wrote:
> On Fri, May 17, 2024 at 06:00:06PM +0100, Conor Dooley wrote:
>>> On that point. How would I explain in the bindings that cell 2 is the
>>> duty cycle, cell 3 is the frequency and cell 4 is the flags?
>>
>> In the pwm-cells property in the pwm provider binding . You might want to
>> order it as <index freq flags duty> as usually that's the ordering done
>> in most (all?) pwm provider bindings that I have seen.
>> The pwm bindings I think are really unhelpful though - they all say "see
>> pwm.yaml for info on the cells in #pwm-cells, but then pwm.yaml has no
>> information. The information is actually in pwm.text, but the binding
>> conversion did s/pwm.text/pwm.yaml/ in pwm controller bindings.
>> I'll send a patch that fixes up pwm.yaml.
> 
> Possibly cell 4 should be standardised as the period for all pwm
> providers and then all you'd have to do for your provider is set
> #pwm-cells:
>    minItems: 4


The chip (and other chips using pwm outputs to control fans) have additional
configuration parameters such as the minimum and maximum permitted pwm duty
cycles, or the startup timeout for various pwm outputs. I may be missing
something, but I don't see any such bindings in pwm.txt or pwm.yaml.

That is probably (likely ?) not needed for Chris' application, but it is an
overall concern since presumably similar bindings should be used for other
fan controllers.

In this context, I think we'll need nested bindings, because the controller
also supports temperature and voltage monitoring. Ultimately we'll also need
tach-ch because the controller specifically supports controlling multiple fans
from a single pwm channel and needs to be configured accordingly, at least
for automatic fan control.

Thanks,
Guenter

Re: [PATCH v2 1/2] dt-bindings: hwmon: Document adt7475 PWM initial duty cycle

[Conor Dooley]

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

On Fri, May 17, 2024 at 06:02:33PM +0100, Conor Dooley wrote:
> On Fri, May 17, 2024 at 06:00:06PM +0100, Conor Dooley wrote:
> > > On that point. How would I explain in the bindings that cell 2 is the 
> > > duty cycle, cell 3 is the frequency and cell 4 is the flags?
> > 
> > In the pwm-cells property in the pwm provider binding . You might want to
> > order it as <index freq flags duty> as usually that's the ordering done
> > in most (all?) pwm provider bindings that I have seen.
> > The pwm bindings I think are really unhelpful though - they all say "see
> > pwm.yaml for info on the cells in #pwm-cells, but then pwm.yaml has no
> > information. The information is actually in pwm.text, but the binding
> > conversion did s/pwm.text/pwm.yaml/ in pwm controller bindings.
> > I'll send a patch that fixes up pwm.yaml.
> 
> Possibly cell 4 should be standardised as the period for all pwm
> providers and then all you'd have to do for your provider is set
> #pwm-cells:
>   minItems: 4

`const: 4`, d'oh.



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