[PATCH v4 1/2] dt-bindings: hwmon: pwm-fan: Document default-pwm property

[PATCH v4 1/2] dt-bindings: hwmon: pwm-fan: Document default-pwm property

[Peter Korsgaard]

The pwm-fan driver uses full PWM (255) duty cycle at startup, which may not
always be desirable because of noise or power consumption peaks, so add an
optional "default-pwm" property that can be used to specify a custom default
PWM duty cycle (0..255).

This is somewhat similar to target-rpm from fan-common.yaml, but that cannot
be used here as target-rpm specifies the target fan speed, whereas this is
the default pwm to set when the device is instantiated - And the resulting
fan RPM resulting from a given PWM duty cycle is fan dependent.

Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
---
Changes since v3:
 - Fix example syntax
 - Extend description of why target-rpm cannot be used

Changes since v2:
 - Recreated/resent

Changes since v1:
 - Rename to default-pwm

 Documentation/devicetree/bindings/hwmon/pwm-fan.yaml | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml b/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
index 8b4ed5ee962f..873c4c32e608 100644
--- a/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
+++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
@@ -20,6 +20,13 @@ properties:
     items:
       maximum: 255
 
+  default-pwm:
+    description: Default PWM duty cycle value to use at startup
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 255
+    default: 255
+
   fan-supply:
     description: Phandle to the regulator that provides power to the fan.
 
@@ -100,6 +107,7 @@ examples:
     pwm-fan {
       compatible = "pwm-fan";
       pwms = <&pwm 0 40000 0>;
+      default-pwm = <75>;
       fan-supply = <&reg_fan>;
       interrupt-parent = <&gpio5>;
       interrupts = <1 IRQ_TYPE_EDGE_FALLING>;
-- 
2.39.5

[PATCH v4 2/2] hwmon: (pwm-fan): Make default PWM duty cycle configurable

[Peter Korsgaard]

The pwm-fan driver uses full PWM (255) duty cycle at startup, which may not
always be desirable because of noise or power consumption peaks, so support
an optional "default-pwm" property (0..255) that can be used to specify the
default PWM duty cycle.

Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
---
Changes since v3:
- Readded validation code from v2, reworded commit message

Changes since v2:
- Recreated/resent

Changes since v1:
- Rename property to default-pwm
- Drop u32 cast

 drivers/hwmon/pwm-fan.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index 53a1a968d00d..f49b5c2c64a9 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -499,6 +499,7 @@ static int pwm_fan_probe(struct platform_device *pdev)
 	const struct hwmon_channel_info **channels;
 	u32 pwm_min_from_stopped = 0;
 	u32 *fan_channel_config;
+	u32 default_pwm = MAX_PWM;
 	int channel_count = 1;	/* We always have a PWM channel. */
 	int i;
 
@@ -545,11 +546,17 @@ static int pwm_fan_probe(struct platform_device *pdev)
 
 	ctx->enable_mode = pwm_disable_reg_enable;
 
+	device_property_read_u32(dev, "default-pwm", &default_pwm);
+	if (default_pwm > MAX_PWM) {
+		dev_err(dev, "Invalid default-pwm: %u > %d\n", default_pwm, MAX_PWM);
+		return -EINVAL;
+	}
+
 	/*
-	 * Set duty cycle to maximum allowed and enable PWM output as well as
+	 * Set duty cycle to default and enable PWM output as well as
 	 * the regulator. In case of error nothing is changed
 	 */
-	ret = set_pwm(ctx, MAX_PWM);
+	ret = set_pwm(ctx, default_pwm);
 	if (ret) {
 		dev_err(dev, "Failed to configure PWM: %d\n", ret);
 		return ret;
-- 
2.39.5

Re: [PATCH v4 1/2] dt-bindings: hwmon: pwm-fan: Document default-pwm property

[Rob Herring]

On Fri, Jan 03, 2025 at 11:14:47AM +0100, Peter Korsgaard wrote:
> The pwm-fan driver uses full PWM (255) duty cycle at startup, which may not
> always be desirable because of noise or power consumption peaks, so add an
> optional "default-pwm" property that can be used to specify a custom default
> PWM duty cycle (0..255).
> 
> This is somewhat similar to target-rpm from fan-common.yaml, but that cannot
> be used here as target-rpm specifies the target fan speed, whereas this is
> the default pwm to set when the device is instantiated - And the resulting
> fan RPM resulting from a given PWM duty cycle is fan dependent.

I still don't agree. Quoting Guenter:

> The two values are also orthogonal. The fan rpm is fan dependent.
> Each fan will require a different pwm value to reach the target speed.
> Trying to use target-rpm to set a default pwm value would really
> not make much if any sense.

But RPM is ultimately what you care about and is the fan parameter 
that's universal yet independent of the underlying control. RPM is what 
determines noise and power consumption.

There's 2 cases to consider: you have a tach signal and know the fan RPM 
or you don't know the RPM. If you have a tach signal, we probably 
wouldn't be discussing this because target-rpm would be enough. So I'm 
assuming this is the case and you have no idea what RPM the fan runs at. 
The fan-common.yaml binding is a bit incomplete for this. What you need 
is some map of fan speed to PWM duty cycle as most likely it is not 
linear response. I think there are 2 options here:

Use the 'cooling-levels' property. Fan "speed" is the index of the 
array. So you just need a 'default-cooling-level' property that's the 
default index.

The other option is define an array of (fan RPM, PWM duty cycle) tuples. 
Then target-rpm can be used to select the entry. We already have 
something like this with 'gpio-fan,speed-map'.

There's also no definition of the minimum RPM or duty cycle in the 
pwm-fan binding. We have min-rpm in fan-common, but that doesn't work 
without a tach. A map would help here as well

This problem to me is similar to LEDs. Ultimately it's brightness that 
you care about, not the current or PWM duty cycle to get there.

Finally, whatever we end up with, it should go in fan-common.yaml. That 
supports PWMs too, so whatever we end up with is applicable to any PWM 
controlled fan.

Rob

Re: [PATCH v4 1/2] dt-bindings: hwmon: pwm-fan: Document default-pwm property

[Peter Korsgaard]

On 1/3/25 20:58, Rob Herring wrote:

> I still don't agree. Quoting Guenter:
> 
>> The two values are also orthogonal. The fan rpm is fan dependent.
>> Each fan will require a different pwm value to reach the target speed.
>> Trying to use target-rpm to set a default pwm value would really
>> not make much if any sense.

> But RPM is ultimately what you care about and is the fan parameter
> that's universal yet independent of the underlying control. RPM is what
> determines noise and power consumption.
> 
> There's 2 cases to consider: you have a tach signal and know the fan RPM
> or you don't know the RPM. If you have a tach signal, we probably
> wouldn't be discussing this because target-rpm would be enough. So I'm
> assuming this is the case and you have no idea what RPM the fan runs at.

Correct, no tacho.


> The fan-common.yaml binding is a bit incomplete for this. What you need
> is some map of fan speed to PWM duty cycle as most likely it is not
> linear response. I think there are 2 options here:
> 
> Use the 'cooling-levels' property. Fan "speed" is the index of the
> array. So you just need a 'default-cooling-level' property that's the
> default index.

I am not sure I what you mean with the RPM reference here? The 
cooling-levels support in the fan-pwm.c driver is a mapping between 
cooling levels and PWM values, NOT RPM value.


> The other option is define an array of (fan RPM, PWM duty cycle) tuples.
> Then target-rpm can be used to select the entry. We already have
> something like this with 'gpio-fan,speed-map'.

Where should these "invented" RPM values come from when there is no 
tacho signal? That sounds backwards / complicated for the very trivial 
"what should the default PWM value be at driver probe time" use case.


> There's also no definition of the minimum RPM or duty cycle in the
> pwm-fan binding. We have min-rpm in fan-common, but that doesn't work
> without a tach. A map would help here as well

The minimum PWM is presumably 0, E.G. signal always low?


> This problem to me is similar to LEDs. Ultimately it's brightness that
> you care about, not the current or PWM duty cycle to get there.

The use case (as described in the commit message) is to drive the fan 
less hard to limit noise and/or power consumption. The input to the fan 
drive control is a PWM setting, so it IMHO makes sense to specify that, 
as that is the interface provided by the fan-pwm driver - E.G. you boot 
up and tweak the pwm1 property in sysfs until you have a value that 
suits the noise/power consumption requirements and stick that value in 
the dts.


> Finally, whatever we end up with, it should go in fan-common.yaml. That
> supports PWMs too, so whatever we end up with is applicable to any PWM
> controlled fan.

What makes this "default-pwm" (or whatever it will be called) more 
generic than E.G. the recently added "fan-stop-to-start-percent" / 
"fan-stop-to-start-usec" properties added to pwm-fan.yaml by commit 
80bc64201e78 ("dt-bindings: hwmon: pwm-fan: Document start from stopped 
state properties")?

-- 
Bye, Peter Korsgaard

Re: [PATCH v4 1/2] dt-bindings: hwmon: pwm-fan: Document default-pwm property

[Rob Herring]

On Sun, Jan 05, 2025 at 05:10:42PM +0100, Peter Korsgaard wrote:
> On 1/3/25 20:58, Rob Herring wrote:
> 
> > I still don't agree. Quoting Guenter:
> > 
> > > The two values are also orthogonal. The fan rpm is fan dependent.
> > > Each fan will require a different pwm value to reach the target speed.
> > > Trying to use target-rpm to set a default pwm value would really
> > > not make much if any sense.
> 
> > But RPM is ultimately what you care about and is the fan parameter
> > that's universal yet independent of the underlying control. RPM is what
> > determines noise and power consumption.
> > 
> > There's 2 cases to consider: you have a tach signal and know the fan RPM
> > or you don't know the RPM. If you have a tach signal, we probably
> > wouldn't be discussing this because target-rpm would be enough. So I'm
> > assuming this is the case and you have no idea what RPM the fan runs at.
> 
> Correct, no tacho.
> 
> 
> > The fan-common.yaml binding is a bit incomplete for this. What you need
> > is some map of fan speed to PWM duty cycle as most likely it is not
> > linear response. I think there are 2 options here:
> > 
> > Use the 'cooling-levels' property. Fan "speed" is the index of the
> > array. So you just need a 'default-cooling-level' property that's the
> > default index.
> 
> I am not sure I what you mean with the RPM reference here? The
> cooling-levels support in the fan-pwm.c driver is a mapping between cooling
> levels and PWM values, NOT RPM value.

Did I say RPM anywhere for this option?

It is the index of the array that is meaningful to anything outside of 
the driver. The values are opaque. They are duty cycle in some cases 
and RPMs in other cases. The thermal subsystem knows nothing about PWM 
duty cycle nor RPMs. 

Defining a default-cooling-level would be useful to anyone, not just 
your usecase.

IOW, you are proposing:

default-pwm = <123>;

I'm proposing doing this instead:

cooling-levels = <0 123 255>;
default-cooling-level = <1>;


> > The other option is define an array of (fan RPM, PWM duty cycle) tuples.
> > Then target-rpm can be used to select the entry. We already have
> > something like this with 'gpio-fan,speed-map'.
> 
> Where should these "invented" RPM values come from when there is no tacho
> signal? That sounds backwards / complicated for the very trivial "what
> should the default PWM value be at driver probe time" use case.

Every fan has at least a maximum RPM spec'ed. Probably a minium too. 
For anything in between, you're correct that you don't know. I guess you 
just assume a linear response. 

> > There's also no definition of the minimum RPM or duty cycle in the
> > pwm-fan binding. We have min-rpm in fan-common, but that doesn't work
> > without a tach. A map would help here as well
> 
> The minimum PWM is presumably 0, E.G. signal always low?

I'm talking about the duty cycle needed to start the fan spinning and to 
keep it spinning. I'm sure that value is not 1 for any fan except one in 
a physics textbook (the only place friction does not exist).

Maybe the minimum is index 0 of cooling-levels? 


> > This problem to me is similar to LEDs. Ultimately it's brightness that
> > you care about, not the current or PWM duty cycle to get there.
> 
> The use case (as described in the commit message) is to drive the fan less
> hard to limit noise and/or power consumption. The input to the fan drive
> control is a PWM setting, so it IMHO makes sense to specify that, as that is
> the interface provided by the fan-pwm driver - E.G. you boot up and tweak
> the pwm1 property in sysfs until you have a value that suits the noise/power
> consumption requirements and stick that value in the dts.

I understand what you want. I'm trying to think ahead about what's the 
next thing someone will want to add to the binding. Just adding 1 
property at a time does not result in the best binding design. There's 
plenty of examples of that. Second, we have these fan bindings like 
pwm-fan which predate coming up with a common binding. Any further 
evolution of these bindings should not further diverge from the 
common binding. 

If your process do this once for a given platform, then having this in 
DT is fine. If the process is every user of the platform does this, then 
I don't think it should be in DT. Having users tweak the DT is not a 
great experience compared to just putting the setting in a file on 
the rootfs.

> > Finally, whatever we end up with, it should go in fan-common.yaml. That
> > supports PWMs too, so whatever we end up with is applicable to any PWM
> > controlled fan.
> 
> What makes this "default-pwm" (or whatever it will be called) more generic
> than E.G. the recently added "fan-stop-to-start-percent" /
> "fan-stop-to-start-usec" properties added to pwm-fan.yaml by commit
> 80bc64201e78 ("dt-bindings: hwmon: pwm-fan: Document start from stopped
> state properties")?

Nothing. Those should probably be moved.

Really, pwm-fan.yaml should reference fan-common.yaml and drop all the 
duplicate properties.

Rob

Re: [PATCH v4 1/2] dt-bindings: hwmon: pwm-fan: Document default-pwm property

[Guenter Roeck]

On 1/6/25 09:38, Rob Herring wrote:

>> The minimum PWM is presumably 0, E.G. signal always low?
> 
> I'm talking about the duty cycle needed to start the fan spinning and to
> keep it spinning. I'm sure that value is not 1 for any fan except one in
> a physics textbook (the only place friction does not exist).
> 
> Maybe the minimum is index 0 of cooling-levels?
> 

Fan controllers often have a separate value for the lowest cooling level
and for the PWM necessary to start a fan. The pwm associated with the lowest
cooling level may be lower than the pwm necessary to start a fan.

The fan controller would start a fan by applying the "start fan" pwm value
for a period of time, then reduce the pwm to the desired pwm value for
the running fan.

I'll be happy to dig up datasheet references if needed.

Guenter

Re: [PATCH v4 1/2] dt-bindings: hwmon: pwm-fan: Document default-pwm property

[Peter Korsgaard]

>>>>> "Rob" == Rob Herring <robh@kernel.org> writes:
On 1/6/25 18:38, Rob Herring wrote:

>> I am not sure I what you mean with the RPM reference here? The
>> cooling-levels support in the fan-pwm.c driver is a mapping between cooling
>> levels and PWM values, NOT RPM value.
> 
> Did I say RPM anywhere for this option?
> 
> It is the index of the array that is meaningful to anything outside of
> the driver. The values are opaque. They are duty cycle in some cases
> and RPMs in other cases. The thermal subsystem knows nothing about PWM
> duty cycle nor RPMs.
> 
> Defining a default-cooling-level would be useful to anyone, not just
> your usecase.
> 
> IOW, you are proposing:
> 
> default-pwm = <123>;
> 
> I'm proposing doing this instead:
> 
> cooling-levels = <0 123 255>;
> default-cooling-level = <1>;

I don't have CONFIG_THERMAL enabled in my builds (and don't know the
subsystem), but I see the pwm-fan driver has some logic to default to
the highest cooling level, it just forgets to actually set the PWM to
match it, so perhaps we can just fix that?

E.G. something like:

commit 02c8ba74eb7dddf210ceefa253385bc8e40f49ae
Author: Peter Korsgaard <peter@korsgaard.com>
Date:   Thu Jan 2 18:26:45 2025 +0100

    hwmon: (pwm-fan): Default to the Maximum cooling level if provided
    
    The pwm-fan driver uses full PWM (255) duty cycle at startup, which may not
    always be desirable because of noise or power consumption peaks.
    
    The driver optionally accept a list of "cooling-levels" for the thermal
    subsystem.  If provided, use the PWM value corresponding to the maximum
    cooling level rather than the full level.
    
    Signed-off-by: Peter Korsgaard <peter@korsgaard.com>

diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index 53a1a968d00d..33525096f1e7 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -499,6 +499,7 @@ static int pwm_fan_probe(struct platform_device *pdev)
 	const struct hwmon_channel_info **channels;
 	u32 pwm_min_from_stopped = 0;
 	u32 *fan_channel_config;
+	u32 default_pwm = MAX_PWM;
 	int channel_count = 1;	/* We always have a PWM channel. */
 	int i;
 
@@ -545,11 +546,18 @@ static int pwm_fan_probe(struct platform_device *pdev)
 
 	ctx->enable_mode = pwm_disable_reg_enable;
 
+	ret = pwm_fan_get_cooling_data(dev, ctx);
+	if (ret)
+		return ret;
+
+	if (ctx->pwm_fan_cooling_levels)
+		default_pwm = ctx->pwm_fan_cooling_levels[ctx->pwm_fan_max_state];
+
 	/*
-	 * Set duty cycle to maximum allowed and enable PWM output as well as
+	 * Set duty cycle to default and enable PWM output as well as
 	 * the regulator. In case of error nothing is changed
 	 */
-	ret = set_pwm(ctx, MAX_PWM);
+	ret = set_pwm(ctx, default_pwm);
 	if (ret) {
 		dev_err(dev, "Failed to configure PWM: %d\n", ret);
 		return ret;
@@ -661,10 +669,6 @@ static int pwm_fan_probe(struct platform_device *pdev)
 		return PTR_ERR(hwmon);
 	}
 
-	ret = pwm_fan_get_cooling_data(dev, ctx);
-	if (ret)
-		return ret;
-
 	ctx->pwm_fan_state = ctx->pwm_fan_max_state;
 	if (IS_ENABLED(CONFIG_THERMAL)) {
 		cdev = devm_thermal_of_cooling_device_register(dev,


Guenter, what do you say? This way we don't need any new device tree
properties. I personally find it less clear than a default-pwm property,
but oh well.

-- 
Bye, Peter Korsgaard

Re: [PATCH v4 1/2] dt-bindings: hwmon: pwm-fan: Document default-pwm property

[Guenter Roeck]

On 1/10/25 12:06, Peter Korsgaard wrote:
>>>>>> "Rob" == Rob Herring <robh@kernel.org> writes:
> On 1/6/25 18:38, Rob Herring wrote:
> 
>>> I am not sure I what you mean with the RPM reference here? The
>>> cooling-levels support in the fan-pwm.c driver is a mapping between cooling
>>> levels and PWM values, NOT RPM value.
>>
>> Did I say RPM anywhere for this option?
>>
>> It is the index of the array that is meaningful to anything outside of
>> the driver. The values are opaque. They are duty cycle in some cases
>> and RPMs in other cases. The thermal subsystem knows nothing about PWM
>> duty cycle nor RPMs.
>>
>> Defining a default-cooling-level would be useful to anyone, not just
>> your usecase.
>>
>> IOW, you are proposing:
>>
>> default-pwm = <123>;
>>
>> I'm proposing doing this instead:
>>
>> cooling-levels = <0 123 255>;
>> default-cooling-level = <1>;
> 
> I don't have CONFIG_THERMAL enabled in my builds (and don't know the
> subsystem), but I see the pwm-fan driver has some logic to default to
> the highest cooling level, it just forgets to actually set the PWM to
> match it, so perhaps we can just fix that?
> 
> E.G. something like:
> 
> commit 02c8ba74eb7dddf210ceefa253385bc8e40f49ae
> Author: Peter Korsgaard <peter@korsgaard.com>
> Date:   Thu Jan 2 18:26:45 2025 +0100
> 
>      hwmon: (pwm-fan): Default to the Maximum cooling level if provided
>      
>      The pwm-fan driver uses full PWM (255) duty cycle at startup, which may not
>      always be desirable because of noise or power consumption peaks.
>      
>      The driver optionally accept a list of "cooling-levels" for the thermal
>      subsystem.  If provided, use the PWM value corresponding to the maximum
>      cooling level rather than the full level.
>      
>      Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
> 
> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> index 53a1a968d00d..33525096f1e7 100644
> --- a/drivers/hwmon/pwm-fan.c
> +++ b/drivers/hwmon/pwm-fan.c
> @@ -499,6 +499,7 @@ static int pwm_fan_probe(struct platform_device *pdev)
>   	const struct hwmon_channel_info **channels;
>   	u32 pwm_min_from_stopped = 0;
>   	u32 *fan_channel_config;
> +	u32 default_pwm = MAX_PWM;
>   	int channel_count = 1;	/* We always have a PWM channel. */
>   	int i;
>   
> @@ -545,11 +546,18 @@ static int pwm_fan_probe(struct platform_device *pdev)
>   
>   	ctx->enable_mode = pwm_disable_reg_enable;
>   
> +	ret = pwm_fan_get_cooling_data(dev, ctx);
> +	if (ret)
> +		return ret;
> +
> +	if (ctx->pwm_fan_cooling_levels)
> +		default_pwm = ctx->pwm_fan_cooling_levels[ctx->pwm_fan_max_state];
> +
>   	/*
> -	 * Set duty cycle to maximum allowed and enable PWM output as well as
> +	 * Set duty cycle to default and enable PWM output as well as
>   	 * the regulator. In case of error nothing is changed
>   	 */
> -	ret = set_pwm(ctx, MAX_PWM);
> +	ret = set_pwm(ctx, default_pwm);
>   	if (ret) {
>   		dev_err(dev, "Failed to configure PWM: %d\n", ret);
>   		return ret;
> @@ -661,10 +669,6 @@ static int pwm_fan_probe(struct platform_device *pdev)
>   		return PTR_ERR(hwmon);
>   	}
>   
> -	ret = pwm_fan_get_cooling_data(dev, ctx);
> -	if (ret)
> -		return ret;
> -
>   	ctx->pwm_fan_state = ctx->pwm_fan_max_state;
>   	if (IS_ENABLED(CONFIG_THERMAL)) {
>   		cdev = devm_thermal_of_cooling_device_register(dev,
> 
> 
> Guenter, what do you say? This way we don't need any new device tree
> properties. I personally find it less clear than a default-pwm property,
> but oh well.
> 

I would not call that "default". It is more along the line of
"If available, use highest cooling level as maximum allowed".

Other than that, I don't like it, but since it looks like we
won't get approval for the devicetree property, I'd say go for it.

Guenter

Re: [PATCH v4 1/2] dt-bindings: hwmon: pwm-fan: Document default-pwm property

[Peter Korsgaard]

>>>>> "Guenter" == Guenter Roeck <linux@roeck-us.net> writes:

Hi,

 >> Guenter, what do you say? This way we don't need any new device tree
 >> properties. I personally find it less clear than a default-pwm property,
 >> but oh well.
 >> 

 > I would not call that "default". It is more along the line of
 > "If available, use highest cooling level as maximum allowed".

Sorry, what are you referring to exactly? The commit message? The change
is about the default/initial pwm setting, there is nothing disallowing
user space to increase it afterwards?


 > Other than that, I don't like it, but since it looks like we
 > won't get approval for the devicetree property, I'd say go for it.

Great, then we agree! I'll send a v5 then once it is clear what your
comment above refers to.

-- 
Bye, Peter Korsgaard

Re: [PATCH v4 1/2] dt-bindings: hwmon: pwm-fan: Document default-pwm property

[Guenter Roeck]

On 1/11/25 10:24, Peter Korsgaard wrote:
>>>>>> "Guenter" == Guenter Roeck <linux@roeck-us.net> writes:
> 
> Hi,
> 
>   >> Guenter, what do you say? This way we don't need any new device tree
>   >> properties. I personally find it less clear than a default-pwm property,
>   >> but oh well.
>   >>
> 
>   > I would not call that "default". It is more along the line of
>   > "If available, use highest cooling level as maximum allowed".
> 
> Sorry, what are you referring to exactly? The commit message? The change
> is about the default/initial pwm setting, there is nothing disallowing
> user space to increase it afterwards?
> 

 From the current probe function:

"Set duty cycle to maximum allowed". If you dislike the term "maximum allowed",
please consider something like "Set initial duty cycle to maximum or, if provided,
to the highest cooling level".

Guenter

Re: [PATCH v4 1/2] dt-bindings: hwmon: pwm-fan: Document default-pwm property

[Guenter Roeck]

On 1/3/25 11:58, Rob Herring wrote:
> On Fri, Jan 03, 2025 at 11:14:47AM +0100, Peter Korsgaard wrote:
>> The pwm-fan driver uses full PWM (255) duty cycle at startup, which may not
>> always be desirable because of noise or power consumption peaks, so add an
>> optional "default-pwm" property that can be used to specify a custom default
>> PWM duty cycle (0..255).
>>
>> This is somewhat similar to target-rpm from fan-common.yaml, but that cannot
>> be used here as target-rpm specifies the target fan speed, whereas this is
>> the default pwm to set when the device is instantiated - And the resulting
>> fan RPM resulting from a given PWM duty cycle is fan dependent.
> 
> I still don't agree. Quoting Guenter:
> 
>> The two values are also orthogonal. The fan rpm is fan dependent.
>> Each fan will require a different pwm value to reach the target speed.
>> Trying to use target-rpm to set a default pwm value would really
>> not make much if any sense.
> 

That is a mis-quote, really. I was talking about target-rpm. The property
to be added here is default-pwm, which is completely different.

> But RPM is ultimately what you care about and is the fan parameter
> that's universal yet independent of the underlying control. RPM is what
> determines noise and power consumption.
> 
> There's 2 cases to consider: you have a tach signal and know the fan RPM
> or you don't know the RPM. If you have a tach signal, we probably
> wouldn't be discussing this because target-rpm would be enough. So I'm
> assuming this is the case and you have no idea what RPM the fan runs at.
> The fan-common.yaml binding is a bit incomplete for this. What you need
> is some map of fan speed to PWM duty cycle as most likely it is not
> linear response. I think there are 2 options here:
> 
> Use the 'cooling-levels' property. Fan "speed" is the index of the
> array. So you just need a 'default-cooling-level' property that's the
> default index.
> 
> The other option is define an array of (fan RPM, PWM duty cycle) tuples.
> Then target-rpm can be used to select the entry. We already have
> something like this with 'gpio-fan,speed-map'.
> 

That won't work. Literally each individual fan (even the same model)
runs at a different rpm for a given pwm value, and I am sure the rpm
for a given pwm input changes depending on fan age and temperature.

Quoting from the NCT6796D-S datasheet, for an example:

"The default duty cycle is 7Fh, or 50% for SYSFANOUT, CPUFANOUT, AUXFANOUT0, AUXFANOUT1,
  AUXFANOUT2, AUXFANOUT3 and AUXFANOUT4.
  Note.The default speed of fan output is specified in registers CR[E0h] ~ CR[E4h] and CR[E7h] of Logical Device B,
  and the AUXFANOUT4 default speed of fan output is specified in register CR[E3h] of Logical Device D.
"

Note that the term "default speed" as used in the datasheet refers to pwm.
Again, from the datasheet:

CR E3h. AUXFAN4 Duty Cycle Register
                 ^^^^^^^^^^^^^^^^^^^

Setting the default value for such a register is what we are talking about here.
No rpm involved, just default pwm values. This is completely independent of rpm.

Guenter

Re: [PATCH v4 1/2] dt-bindings: hwmon: pwm-fan: Document default-pwm property

[Guenter Roeck]

On 1/3/25 11:58, Rob Herring wrote:

> 
> But RPM is ultimately what you care about and is the fan parameter
> that's universal yet independent of the underlying control. RPM is what
> determines noise and power consumption.
> 

I forgot to add: I would argue that airflow restrictions play a substantial
role for both power consumption and noise, especially when trying to achieve
a specific fan speed. For power consumption, I don't even see how rpm would
play a substantial role in the first place since power consumption depends
on pwm, not on the resulting rpm (or airflow).

Guenter