Re: [PATCH 2/2] hwmon: pwm-fan: stop using legacy PWM functions and some cleanups

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

On Tue, Dec 15, 2020 at 10:20:31AM +0100, Uwe Kleine-König wrote:
> pwm_apply_state() does what the legacy functions pwm_config() and
> pwm_{en,dis}able() do in a single function call. This simplifies error
> handling and is more efficient for new-style PWM hardware drivers.
> 
> Instead of repeatedly querying the PWM framework about the initial PWM
> configuration, cache the settings in driver data.
> 
> Also use __set_pwm() in .probe() to have the algorithm calculating the PWM
> state in a single place.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Can you rebase this patch on top of hwmon-next ?

Thanks,
Guenter

> ---
>  drivers/hwmon/pwm-fan.c | 47 +++++++++++++++++------------------------
>  1 file changed, 19 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> index ec171f2b684a..4ccad5a87019 100644
> --- a/drivers/hwmon/pwm-fan.c
> +++ b/drivers/hwmon/pwm-fan.c
> @@ -25,6 +25,7 @@
>  struct pwm_fan_ctx {
>  	struct mutex lock;
>  	struct pwm_device *pwm;
> +	struct pwm_state pwm_state;
>  	struct regulator *reg_en;
>  
>  	int irq;
> @@ -73,18 +74,17 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
>  {
>  	unsigned long period;
>  	int ret = 0;
> -	struct pwm_state state = { };
> +	struct pwm_state *state = &ctx->pwm_state;
>  
>  	mutex_lock(&ctx->lock);
>  	if (ctx->pwm_value == pwm)
>  		goto exit_set_pwm_err;
>  
> -	pwm_init_state(ctx->pwm, &state);
> -	period = ctx->pwm->args.period;
> -	state.duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
> -	state.enabled = pwm ? true : false;
> +	period = state->period;
> +	state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
> +	state->enabled = pwm ? true : false;
>  
> -	ret = pwm_apply_state(ctx->pwm, &state);
> +	ret = pwm_apply_state(ctx->pwm, state);
>  	if (!ret)
>  		ctx->pwm_value = pwm;
>  exit_set_pwm_err:
> @@ -274,7 +274,9 @@ static void pwm_fan_regulator_disable(void *data)
>  static void pwm_fan_pwm_disable(void *__ctx)
>  {
>  	struct pwm_fan_ctx *ctx = __ctx;
> -	pwm_disable(ctx->pwm);
> +
> +	ctx->pwm_state.enabled = false;
> +	pwm_apply_state(ctx->pwm, &ctx->pwm_state);
>  	del_timer_sync(&ctx->rpm_timer);
>  }
>  
> @@ -285,7 +287,6 @@ static int pwm_fan_probe(struct platform_device *pdev)
>  	struct pwm_fan_ctx *ctx;
>  	struct device *hwmon;
>  	int ret;
> -	struct pwm_state state = { };
>  	u32 ppr = 2;
>  
>  	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> @@ -324,22 +325,20 @@ static int pwm_fan_probe(struct platform_device *pdev)
>  
>  	ctx->pwm_value = MAX_PWM;
>  
> -	pwm_init_state(ctx->pwm, &state);
> +	pwm_init_state(ctx->pwm, &ctx->pwm_state);
> +
>  	/*
>  	 * __set_pwm assumes that MAX_PWM * (period - 1) fits into an unsigned
>  	 * long. Check this here to prevent the fan running at a too low
>  	 * frequency.
>  	 */
> -	if (state.period > ULONG_MAX / MAX_PWM + 1) {
> +	if (ctx->pwm_state.period > ULONG_MAX / MAX_PWM + 1) {
>  		dev_err(dev, "Configured period too big\n");
>  		return -EINVAL;
>  	}
>  
>  	/* Set duty cycle to maximum allowed and enable PWM output */
> -	state.duty_cycle = ctx->pwm->args.period - 1;
> -	state.enabled = true;
> -
> -	ret = pwm_apply_state(ctx->pwm, &state);
> +	ret = __set_pwm(ctx, MAX_PWM);
>  	if (ret) {
>  		dev_err(dev, "Failed to configure PWM: %d\n", ret);
>  		return ret;
> @@ -399,17 +398,16 @@ static int pwm_fan_probe(struct platform_device *pdev)
>  static int pwm_fan_disable(struct device *dev)
>  {
>  	struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> -	struct pwm_args args;
>  	int ret;
>  
> -	pwm_get_args(ctx->pwm, &args);
> -
>  	if (ctx->pwm_value) {
> -		ret = pwm_config(ctx->pwm, 0, args.period);
> +		/* keep ctx->pwm_state unmodified for pwm_fan_resume() */
> +		struct pwm_state state = ctx->pwm_state;
> +		state.duty_cycle = 0;
> +		state.enabled = false;
> +		ret = pwm_apply_state(ctx->pwm, &state);
>  		if (ret < 0)
>  			return ret;
> -
> -		pwm_disable(ctx->pwm);
>  	}
>  
>  	if (ctx->reg_en) {
> @@ -437,8 +435,6 @@ static int pwm_fan_suspend(struct device *dev)
>  static int pwm_fan_resume(struct device *dev)
>  {
>  	struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> -	struct pwm_args pargs;
> -	unsigned long duty;
>  	int ret;
>  
>  	if (ctx->reg_en) {
> @@ -452,12 +448,7 @@ static int pwm_fan_resume(struct device *dev)
>  	if (ctx->pwm_value == 0)
>  		return 0;
>  
> -	pwm_get_args(ctx->pwm, &pargs);
> -	duty = DIV_ROUND_UP_ULL(ctx->pwm_value * (pargs.period - 1), MAX_PWM);
> -	ret = pwm_config(ctx->pwm, duty, pargs.period);
> -	if (ret)
> -		return ret;
> -	return pwm_enable(ctx->pwm);
> +	return pwm_apply_state(ctx->pwm, &ctx->pwm_state);
>  }
>  #endif
>