[PATCH v2 1/2] dt-bindings: hwmon: pwm-fan: Document start from stopped state properties

[PATCH v2 1/2] dt-bindings: hwmon: pwm-fan: Document start from stopped state properties

[Marek Vasut]

Delta AFC0612DB-F00 fan has to be set to at least 30% PWM duty cycle
to spin up from a stopped state, and can be afterward throttled down to
lower PWM duty cycle. Introduce support for operating such fans which
need to start at higher PWM duty cycle first and can slow down next.

Document two new DT properties, "fan-stop-to-start-percent" and
"fan-stop-to-start-usec". The former describes the minimum percent
of fan RPM at which it will surely spin up from stopped state. This
value can be found in the fan datasheet and can be converted to PWM
duty cycle easily. The "fan-stop-to-start-usec" describes the minimum
time in microseconds for which the fan has to be set to stopped state
start RPM for the fan to surely spin up.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Jean Delvare <jdelvare@suse.com>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: linux-hwmon@vger.kernel.org
---
V2: - Rename fan-dead-stop-start-percent to fan-stop-to-start-percent
    - Rename fan-dead-stop-start-usec to fan-stop-to-start-us
---
 Documentation/devicetree/bindings/hwmon/pwm-fan.yaml | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml b/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
index 4e5abf7580cc6..f4911e3fdb5b5 100644
--- a/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
+++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
@@ -31,6 +31,17 @@ properties:
       it must be self resetting edge interrupts.
     maxItems: 1
 
+  fan-stop-to-start-percent:
+    description:
+      Minimum fan RPM in percent to start when stopped.
+    minimum: 0
+    maximum: 100
+
+  fan-stop-to-start-us:
+    description:
+      Time to wait in microseconds after start when stopped.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
   pulses-per-revolution:
     description:
       Define the number of pulses per fan revolution for each tachometer
-- 
2.45.2

[PATCH v2 2/2] hwmon: (pwm-fan) Introduce start from stopped state handling

[Marek Vasut]

Delta AFC0612DB-F00 fan has to be set to at least 30% PWM duty cycle
to spin up from a stopped state, and can be afterward throttled down to
lower PWM duty cycle. Introduce support for operating such fans which
need to start at higher PWM duty cycle first and can slow down next.

Introduce two new DT properties, "fan-stop-to-start-percent" and
"fan-stop-to-start-us". The former describes the minimum percent
of fan RPM at which it will surely spin up from stopped state. This
value can be found in the fan datasheet and can be converted to PWM
duty cycle easily. The "fan-stop-to-start-us" describes the minimum
time in microseconds for which the fan has to be set to stopped state
start RPM for the fan to surely spin up.

Adjust the PWM setting code such that if the PWM duty cycle is below
the minimum duty cycle needed by the fan to spin up from stopped state,
then first set the PWM duty cycle to the minimum duty cycle needed
by the fan to spin up from stopped state, then wait the time necessary
for the fan to spin up from stopped state, and finally set the PWM duty
cycle to the one desired by user.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Jean Delvare <jdelvare@suse.com>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: linux-hwmon@vger.kernel.org
---
V2: - Rename fan-dead-stop-start-percent to fan-stop-to-start-percent
    - Rename fan-dead-stop-start-usec to fan-stop-to-start-us
    - Rename variables containing dead_stop to contain stopped
---
 drivers/hwmon/pwm-fan.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index c434db4656e7d..27f449b65f285 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -7,6 +7,7 @@
  * Author: Kamil Debski <k.debski@samsung.com>
  */
 
+#include <linux/delay.h>
 #include <linux/hwmon.h>
 #include <linux/interrupt.h>
 #include <linux/mod_devicetable.h>
@@ -60,6 +61,9 @@ struct pwm_fan_ctx {
 
 	struct hwmon_chip_info info;
 	struct hwmon_channel_info fan_channel;
+
+	u64 pwm_duty_cycle_from_stopped;
+	u32 pwm_usec_from_stopped;
 };
 
 /* This handler assumes self resetting edge triggered interrupt. */
@@ -199,7 +203,9 @@ static int pwm_fan_power_off(struct pwm_fan_ctx *ctx, bool force_disable)
 static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
 {
 	struct pwm_state *state = &ctx->pwm_state;
+	unsigned long final_pwm = pwm;
 	unsigned long period;
+	bool update = false;
 	int ret = 0;
 
 	if (pwm > 0) {
@@ -208,11 +214,22 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
 			return 0;
 
 		period = state->period;
-		state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
+		update = state->duty_cycle < ctx->pwm_duty_cycle_from_stopped;
+		if (update)
+			state->duty_cycle = ctx->pwm_duty_cycle_from_stopped;
+		else
+			state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
 		ret = pwm_apply_might_sleep(ctx->pwm, state);
 		if (ret)
 			return ret;
 		ret = pwm_fan_power_on(ctx);
+		if (!ret && update) {
+			pwm = final_pwm;
+			state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
+			usleep_range(ctx->pwm_usec_from_stopped,
+				     ctx->pwm_usec_from_stopped * 2);
+			ret = pwm_apply_might_sleep(ctx->pwm, state);
+		}
 	} else {
 		ret = pwm_fan_power_off(ctx, false);
 	}
@@ -480,6 +497,7 @@ static int pwm_fan_probe(struct platform_device *pdev)
 	struct device *hwmon;
 	int ret;
 	const struct hwmon_channel_info **channels;
+	u32 pwm_min_from_stopped = 0;
 	u32 *fan_channel_config;
 	int channel_count = 1;	/* We always have a PWM channel. */
 	int i;
@@ -620,6 +638,19 @@ static int pwm_fan_probe(struct platform_device *pdev)
 		channels[1] = &ctx->fan_channel;
 	}
 
+	ret = of_property_read_u32(dev->of_node, "fan-stop-to-start-percent",
+				   &pwm_min_from_stopped);
+	if (!ret && pwm_min_from_stopped) {
+		ctx->pwm_duty_cycle_from_stopped =
+			DIV_ROUND_UP(pwm_min_from_stopped *
+				     (ctx->pwm_state.period - 1),
+				     100);
+	}
+	ret = of_property_read_u32(dev->of_node, "fan-stop-to-start-us",
+				   &ctx->pwm_usec_from_stopped);
+	if (ret)
+		ctx->pwm_usec_from_stopped = 250000;
+
 	ctx->info.ops = &pwm_fan_hwmon_ops;
 	ctx->info.info = channels;
 
-- 
2.45.2

Re: [PATCH v2 2/2] hwmon: (pwm-fan) Introduce start from stopped state handling

[Guenter Roeck]

On 11/5/24 18:14, Marek Vasut wrote:
> Delta AFC0612DB-F00 fan has to be set to at least 30% PWM duty cycle
> to spin up from a stopped state, and can be afterward throttled down to
> lower PWM duty cycle. Introduce support for operating such fans which
> need to start at higher PWM duty cycle first and can slow down next.
> 
> Introduce two new DT properties, "fan-stop-to-start-percent" and
> "fan-stop-to-start-us". The former describes the minimum percent
> of fan RPM at which it will surely spin up from stopped state. This
> value can be found in the fan datasheet and can be converted to PWM
> duty cycle easily. The "fan-stop-to-start-us" describes the minimum
> time in microseconds for which the fan has to be set to stopped state
> start RPM for the fan to surely spin up.
> 
> Adjust the PWM setting code such that if the PWM duty cycle is below
> the minimum duty cycle needed by the fan to spin up from stopped state,
> then first set the PWM duty cycle to the minimum duty cycle needed
> by the fan to spin up from stopped state, then wait the time necessary
> for the fan to spin up from stopped state, and finally set the PWM duty
> cycle to the one desired by user.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Jean Delvare <jdelvare@suse.com>
> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> Cc: Rob Herring <robh@kernel.org>
> Cc: devicetree@vger.kernel.org
> Cc: linux-hwmon@vger.kernel.org
> ---
> V2: - Rename fan-dead-stop-start-percent to fan-stop-to-start-percent
>      - Rename fan-dead-stop-start-usec to fan-stop-to-start-us
>      - Rename variables containing dead_stop to contain stopped
> ---
>   drivers/hwmon/pwm-fan.c | 33 ++++++++++++++++++++++++++++++++-
>   1 file changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> index c434db4656e7d..27f449b65f285 100644
> --- a/drivers/hwmon/pwm-fan.c
> +++ b/drivers/hwmon/pwm-fan.c
> @@ -7,6 +7,7 @@
>    * Author: Kamil Debski <k.debski@samsung.com>
>    */
>   
> +#include <linux/delay.h>
>   #include <linux/hwmon.h>
>   #include <linux/interrupt.h>
>   #include <linux/mod_devicetable.h>
> @@ -60,6 +61,9 @@ struct pwm_fan_ctx {
>   
>   	struct hwmon_chip_info info;
>   	struct hwmon_channel_info fan_channel;
> +
> +	u64 pwm_duty_cycle_from_stopped;
> +	u32 pwm_usec_from_stopped;
>   };
>   
>   /* This handler assumes self resetting edge triggered interrupt. */
> @@ -199,7 +203,9 @@ static int pwm_fan_power_off(struct pwm_fan_ctx *ctx, bool force_disable)
>   static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
>   {
>   	struct pwm_state *state = &ctx->pwm_state;
> +	unsigned long final_pwm = pwm;
>   	unsigned long period;
> +	bool update = false;
>   	int ret = 0;
>   
>   	if (pwm > 0) {
> @@ -208,11 +214,22 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
>   			return 0;
>   
>   		period = state->period;
> -		state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
> +		update = state->duty_cycle < ctx->pwm_duty_cycle_from_stopped;
> +		if (update)
> +			state->duty_cycle = ctx->pwm_duty_cycle_from_stopped;
> +		else
> +			state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
>   		ret = pwm_apply_might_sleep(ctx->pwm, state);
>   		if (ret)
>   			return ret;
>   		ret = pwm_fan_power_on(ctx);
> +		if (!ret && update) {
> +			pwm = final_pwm;
> +			state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
> +			usleep_range(ctx->pwm_usec_from_stopped,
> +				     ctx->pwm_usec_from_stopped * 2);
> +			ret = pwm_apply_might_sleep(ctx->pwm, state);
> +		}
>   	} else {
>   		ret = pwm_fan_power_off(ctx, false);
>   	}
> @@ -480,6 +497,7 @@ static int pwm_fan_probe(struct platform_device *pdev)
>   	struct device *hwmon;
>   	int ret;
>   	const struct hwmon_channel_info **channels;
> +	u32 pwm_min_from_stopped = 0;
>   	u32 *fan_channel_config;
>   	int channel_count = 1;	/* We always have a PWM channel. */
>   	int i;
> @@ -620,6 +638,19 @@ static int pwm_fan_probe(struct platform_device *pdev)
>   		channels[1] = &ctx->fan_channel;
>   	}
>   
> +	ret = of_property_read_u32(dev->of_node, "fan-stop-to-start-percent",
> +				   &pwm_min_from_stopped);
> +	if (!ret && pwm_min_from_stopped) {
> +		ctx->pwm_duty_cycle_from_stopped =
> +			DIV_ROUND_UP(pwm_min_from_stopped *
> +				     (ctx->pwm_state.period - 1),
> +				     100);

Since "period" is u64, this results in the "ERROR: modpost: "__aeabi_uldivmod"
[drivers/hwmon/pwm-fan.ko] undefined!" error as reported by 0-day. Or at least
I think that is the problem. I'd suggest to try building the driver on a 32-bit
system to be sure.

Guenter

> +	}
> +	ret = of_property_read_u32(dev->of_node, "fan-stop-to-start-us",
> +				   &ctx->pwm_usec_from_stopped);
> +	if (ret)
> +		ctx->pwm_usec_from_stopped = 250000;
> +
>   	ctx->info.ops = &pwm_fan_hwmon_ops;
>   	ctx->info.info = channels;
>   

Re: [PATCH v2 2/2] hwmon: (pwm-fan) Introduce start from stopped state handling

[Marek Vasut]

On 11/6/24 5:26 AM, Guenter Roeck wrote:

[...]

>> @@ -480,6 +497,7 @@ static int pwm_fan_probe(struct platform_device 
>> *pdev)
>>       struct device *hwmon;
>>       int ret;
>>       const struct hwmon_channel_info **channels;
>> +    u32 pwm_min_from_stopped = 0;
>>       u32 *fan_channel_config;
>>       int channel_count = 1;    /* We always have a PWM channel. */
>>       int i;
>> @@ -620,6 +638,19 @@ static int pwm_fan_probe(struct platform_device 
>> *pdev)
>>           channels[1] = &ctx->fan_channel;
>>       }
>> +    ret = of_property_read_u32(dev->of_node, "fan-stop-to-start- 
>> percent",
>> +                   &pwm_min_from_stopped);
>> +    if (!ret && pwm_min_from_stopped) {
>> +        ctx->pwm_duty_cycle_from_stopped =
>> +            DIV_ROUND_UP(pwm_min_from_stopped *
>> +                     (ctx->pwm_state.period - 1),
>> +                     100);
> 
> Since "period" is u64, this results in the "ERROR: modpost: 
> "__aeabi_uldivmod"
> [drivers/hwmon/pwm-fan.ko] undefined!" error as reported by 0-day. Or at 
> least
> I think that is the problem. I'd suggest to try building the driver on a 
> 32-bit
> system to be sure.
Ah, right, fixed in V3. Thanks !

Re: [PATCH v2 1/2] dt-bindings: hwmon: pwm-fan: Document start from stopped state properties

[Rob Herring (Arm)]

On Wed, 06 Nov 2024 03:14:36 +0100, Marek Vasut wrote:
> Delta AFC0612DB-F00 fan has to be set to at least 30% PWM duty cycle
> to spin up from a stopped state, and can be afterward throttled down to
> lower PWM duty cycle. Introduce support for operating such fans which
> need to start at higher PWM duty cycle first and can slow down next.
> 
> Document two new DT properties, "fan-stop-to-start-percent" and
> "fan-stop-to-start-usec". The former describes the minimum percent
> of fan RPM at which it will surely spin up from stopped state. This
> value can be found in the fan datasheet and can be converted to PWM
> duty cycle easily. The "fan-stop-to-start-usec" describes the minimum
> time in microseconds for which the fan has to be set to stopped state
> start RPM for the fan to surely spin up.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Jean Delvare <jdelvare@suse.com>
> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> Cc: Rob Herring <robh@kernel.org>
> Cc: devicetree@vger.kernel.org
> Cc: linux-hwmon@vger.kernel.org
> ---
> V2: - Rename fan-dead-stop-start-percent to fan-stop-to-start-percent
>     - Rename fan-dead-stop-start-usec to fan-stop-to-start-us
> ---
>  Documentation/devicetree/bindings/hwmon/pwm-fan.yaml | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml: properties:fan-stop-to-start-us: '$ref' should not be valid under {'const': '$ref'}
	hint: Standard unit suffix properties don't need a type $ref
	from schema $id: http://devicetree.org/meta-schemas/core.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241106021559.175105-1-marex@denx.de

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.