Re: [PATCH 2/2] hwmon: (pwm-fan) Introduce start from dead stop handling

[Guenter Roeck <linux@roeck-us.net>]

On 11/5/24 05:52, Marek Vasut wrote:
> Delta AFC0612DB-F00 fan has to be set to at least 30% PWM duty cycle
> to spin up from a dead stop, 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-dead-stop-start-percent" and
> "fan-dead-stop-start-usec". The former describes the minimum percent
> of fan RPM at which it will surely spin up from dead stop. This value
> can be found in the fan datasheet and can be converted to PWM duty
> cycle easily. The "fan-dead-stop-start-usec" describes the minimum
> time in microseconds for which the fan has to be set to dead stop
> 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 dead stop,
> then first set the PWM duty cycle to the minimum duty cycle needed
> by the fan to spin up from dead stop, then wait the time necessary
> for the fan to spin up from dead stop, and finally set the PWM duty
> cycle to the one desired by user.
> 

As with the other patch, I don't think "dead" adds any value anywhere.

Guenter

> 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
> ---
>   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..264b7ddf8bb40 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_dead_stop;
> +	u32 pwm_usec_from_dead_stop;
>   };
>   
>   /* 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_dead_stop;
> +		if (update)
> +			state->duty_cycle = ctx->pwm_duty_cycle_from_dead_stop;
> +		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_dead_stop,
> +				     ctx->pwm_usec_from_dead_stop * 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_dead_stop = 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-dead-stop-start-percent",
> +				   &pwm_min_from_dead_stop);
> +	if (!ret && pwm_min_from_dead_stop) {
> +		ctx->pwm_duty_cycle_from_dead_stop =
> +			DIV_ROUND_UP(pwm_min_from_dead_stop *
> +				     (ctx->pwm_state.period - 1),
> +				     100);
> +	}
> +	ret = of_property_read_u32(dev->of_node, "fan-dead-stop-start-usec",
> +				   &ctx->pwm_usec_from_dead_stop);
> +	if (ret)
> +		ctx->pwm_usec_from_dead_stop = 250000;
> +
>   	ctx->info.ops = &pwm_fan_hwmon_ops;
>   	ctx->info.info = channels;
>