Re: [PATCH v4 5/6] hwmon: pwm-fan: Switch regulator dynamically

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

On Mon, May 23, 2022 at 01:05:12PM +0200, Alexander Stein wrote:
> This adds the enable attribute which is used to select if zero PWM duty
> means to switch off regulator and PWM or to keep them enabled but
> at inactive PWM output level.
> Depending on the select enable mode, turn off the regulator and PWM if
> the PWM duty is zero, or keep them enabled.
> This is especially important for fan using inverted PWM signal polarity.
> Having regulator supplied and PWM disabled, some PWM controllers provide
> the active, rather than inactive signal.
> 
> With this change the shutdown as well as suspend/resume paths require
> modifcations as well.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>

This patch is too complex to review as diff. Please resend the series with
the changes requested so I can apply and review the result.

Thanks,
Guenter

> ---
>  Documentation/hwmon/pwm-fan.rst |  12 ++
>  drivers/hwmon/pwm-fan.c         | 213 +++++++++++++++++++++-----------
>  2 files changed, 154 insertions(+), 71 deletions(-)
> 
> diff --git a/Documentation/hwmon/pwm-fan.rst b/Documentation/hwmon/pwm-fan.rst
> index 82fe96742fee..f77998b204ef 100644
> --- a/Documentation/hwmon/pwm-fan.rst
> +++ b/Documentation/hwmon/pwm-fan.rst
> @@ -18,3 +18,15 @@ the hwmon's sysfs interface.
>  
>  The fan rotation speed returned via the optional 'fan1_input' is extrapolated
>  from the sampled interrupts from the tachometer signal within 1 second.
> +
> +The driver provides the following sensor accesses in sysfs:
> +
> +=============== ======= =======================================================
> +fan1_input	ro	fan tachometer speed in RPM
> +pwm1_enable	rw	keep enable mode, defines behaviour when pwm1=0
> +			0 -> disable pwm and regulator
> +			1 -> enable pwm; if pwm==0, disable pwm, keep regulator enabled
> +			2 -> enable pwm; if pwm==0, keep pwm and regulator enabled
> +			3 -> enable pwm; if pwm==0, disable pwm and regulator
> +pwm1		rw	relative speed (0-255), 255=max. speed.
> +=============== ======= =======================================================
> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> index fcc1b7b55a65..e5d4b3b1cc49 100644
> --- a/drivers/hwmon/pwm-fan.c
> +++ b/drivers/hwmon/pwm-fan.c
> @@ -28,6 +28,13 @@ struct pwm_fan_tach {
>  	u8 pulses_per_revolution;
>  };
>  
> +enum pwm_fan_enable_mode {
> +	pwm_off_reg_off,
> +	pwm_disable_reg_enable,
> +	pwm_enable_reg_enable,
> +	pwm_disable_reg_disable,
> +};
> +
>  struct pwm_fan_ctx {
>  	struct device *dev;
>  
> @@ -35,6 +42,7 @@ struct pwm_fan_ctx {
>  	struct pwm_device *pwm;
>  	struct pwm_state pwm_state;
>  	struct regulator *reg_en;
> +	enum pwm_fan_enable_mode enable_mode;
>  	bool regulator_enabled;
>  	bool enabled;
>  
> @@ -86,6 +94,29 @@ static void sample_timer(struct timer_list *t)
>  	mod_timer(&ctx->rpm_timer, jiffies + HZ);
>  }
>  
> +static void pwm_fan_enable_mode_2_state(int enable_mode,
> +					struct pwm_state *state,
> +					bool *enable_regulator)
> +{
> +	switch (enable_mode) {
> +	case pwm_disable_reg_enable:
> +		/* disable pwm, keep regulator enabled */
> +		state->enabled = false;
> +		*enable_regulator = true;
> +		break;
> +	case pwm_enable_reg_enable:
> +		/* keep pwm and regulator enabled */
> +		state->enabled = true;
> +		*enable_regulator = true;
> +		break;
> +	case pwm_off_reg_off:
> +	case pwm_disable_reg_disable:
> +		/* disable pwm and regulator */
> +		state->enabled = false;
> +		*enable_regulator = false;
> +	}
> +}
> +
>  static int pwm_fan_switch_power(struct pwm_fan_ctx *ctx, bool on)
>  {
>  	int ret = 0;
> @@ -117,30 +148,46 @@ static int pwm_fan_power_on(struct pwm_fan_ctx *ctx)
>  	if (ctx->enabled)
>  		return 0;
>  
> +	ret = pwm_fan_switch_power(ctx, true);
> +	if (ret < 0) {
> +		dev_err(ctx->dev, "failed to enable power supply\n");
> +		return ret;
> +	}
> +
>  	state->enabled = true;
>  	ret = pwm_apply_state(ctx->pwm, state);
>  	if (ret) {
>  		dev_err(ctx->dev, "failed to enable PWM\n");
> -		goto err;
> +		goto disable_regulator;
>  	}
>  
>  	ctx->enabled = true;
>  
> -err:
> +	return 0;
> +
> +disable_regulator:
> +	pwm_fan_switch_power(ctx, false);
>  	return ret;
>  }
>  
>  static int pwm_fan_power_off(struct pwm_fan_ctx *ctx)
>  {
>  	struct pwm_state *state = &ctx->pwm_state;
> +	bool enable_regulator = false;
>  
>  	if (!ctx->enabled)
>  		return 0;
>  
> +	pwm_fan_enable_mode_2_state(ctx->enable_mode,
> +				    state,
> +				    &enable_regulator);
> +
>  	state->enabled = false;
>  	state->duty_cycle = 0;
>  	pwm_apply_state(ctx->pwm, state);
>  
> +	pwm_fan_switch_power(ctx, enable_regulator);
> +
>  	ctx->enabled = false;
>  
>  	return 0;
> @@ -153,6 +200,10 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
>  	int ret = 0;
>  
>  	if (pwm > 0) {
> +		if (ctx->enable_mode == pwm_off_reg_off)
> +			/* pwm-fan hard disabled */
> +			return 0;
> +
>  		period = state->period;
>  		state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
>  		ret = pwm_apply_state(ctx->pwm, state);
> @@ -190,20 +241,76 @@ static void pwm_fan_update_state(struct pwm_fan_ctx *ctx, unsigned long pwm)
>  	ctx->pwm_fan_state = i;
>  }
>  
> +static int pwm_fan_update_enable(struct pwm_fan_ctx *ctx, long val)
> +{
> +	int ret = 0;
> +	int old_val;
> +
> +	mutex_lock(&ctx->lock);
> +
> +	if (ctx->enable_mode == val)
> +		goto out;
> +
> +	old_val = ctx->enable_mode;
> +	ctx->enable_mode = val;
> +
> +	if (val == 0) {
> +		/* Disable pwm-fan unconditionally */
> +		ret = __set_pwm(ctx, 0);
> +		if (ret)
> +			ctx->enable_mode = old_val;
> +		pwm_fan_update_state(ctx, 0);
> +	} else {
> +		/*
> +		 * Change PWM and/or regulator state if currently disabled
> +		 * Nothing to do if currently enabled
> +		 */
> +		if (!ctx->enabled) {
> +			struct pwm_state *state = &ctx->pwm_state;
> +			bool enable_regulator = false;
> +
> +			state->duty_cycle = 0;
> +			pwm_fan_enable_mode_2_state(val,
> +						    state,
> +						    &enable_regulator);
> +
> +			pwm_apply_state(ctx->pwm, state);
> +			pwm_fan_switch_power(ctx, enable_regulator);
> +			pwm_fan_update_state(ctx, 0);
> +		}
> +	}
> +out:
> +	mutex_unlock(&ctx->lock);
> +
> +	return ret;
> +}
> +
>  static int pwm_fan_write(struct device *dev, enum hwmon_sensor_types type,
>  			 u32 attr, int channel, long val)
>  {
>  	struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
>  	int ret;
>  
> -	if (val < 0 || val > MAX_PWM)
> -		return -EINVAL;
> +	switch (attr) {
> +	case hwmon_pwm_input:
> +		if (val < 0 || val > MAX_PWM)
> +			return -EINVAL;
> +		ret = set_pwm(ctx, val);
> +		if (ret)
> +			return ret;
> +		pwm_fan_update_state(ctx, val);
> +		break;
> +	case hwmon_pwm_enable:
> +		if (val < 0 || val > 3)
> +			ret = -EINVAL;
> +		else
> +			ret = pwm_fan_update_enable(ctx, val);
>  
> -	ret = set_pwm(ctx, val);
> -	if (ret)
>  		return ret;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
>  
> -	pwm_fan_update_state(ctx, val);
>  	return 0;
>  }
>  
> @@ -214,9 +321,15 @@ static int pwm_fan_read(struct device *dev, enum hwmon_sensor_types type,
>  
>  	switch (type) {
>  	case hwmon_pwm:
> -		*val = ctx->pwm_value;
> -		return 0;
> -
> +		switch (attr) {
> +		case hwmon_pwm_input:
> +			*val = ctx->pwm_value;
> +			return 0;
> +		case hwmon_pwm_enable:
> +			*val = ctx->enable_mode;
> +			return 0;
> +		}
> +		return -EOPNOTSUPP;
>  	case hwmon_fan:
>  		*val = ctx->tachs[channel].rpm;
>  		return 0;
> @@ -345,20 +458,14 @@ static int pwm_fan_of_get_cooling_data(struct device *dev,
>  	return 0;
>  }
>  
> -static void pwm_fan_regulator_disable(void *data)
> -{
> -	struct pwm_fan_ctx *ctx = data;
> -
> -	pwm_fan_switch_power(ctx, false);
> -}
> -
> -static void pwm_fan_pwm_disable(void *__ctx)
> +static void pwm_fan_cleanup(void *__ctx)
>  {
>  	struct pwm_fan_ctx *ctx = __ctx;
>  
> -	ctx->pwm_state.enabled = false;
> -	pwm_apply_state(ctx->pwm, &ctx->pwm_state);
>  	del_timer_sync(&ctx->rpm_timer);
> +	/* Switch off everything */
> +	ctx->enable_mode = pwm_disable_reg_disable;
> +	pwm_fan_power_off(ctx);
>  }
>  
>  static int pwm_fan_probe(struct platform_device *pdev)
> @@ -392,16 +499,6 @@ static int pwm_fan_probe(struct platform_device *pdev)
>  			return PTR_ERR(ctx->reg_en);
>  
>  		ctx->reg_en = NULL;
> -	} else {
> -		ret = pwm_fan_switch_power(ctx, true);
> -		if (ret) {
> -			dev_err(dev, "Failed to enable fan supply: %d\n", ret);
> -			return ret;
> -		}
> -		ret = devm_add_action_or_reset(dev, pwm_fan_regulator_disable,
> -					       ctx);
> -		if (ret)
> -			return ret;
>  	}
>  
>  	pwm_init_state(ctx->pwm, &ctx->pwm_state);
> @@ -416,14 +513,19 @@ static int pwm_fan_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> -	/* Set duty cycle to maximum allowed and enable PWM output */
> +	ctx->enable_mode = pwm_disable_reg_enable;
> +
> +	/*
> +	 * Set duty cycle to maximum allowed and enable PWM output as well as
> +	 * the regulator. In case of error nothing is changed
> +	 */
>  	ret = set_pwm(ctx, MAX_PWM);
>  	if (ret) {
>  		dev_err(dev, "Failed to configure PWM: %d\n", ret);
>  		return ret;
>  	}
>  	timer_setup(&ctx->rpm_timer, sample_timer, 0);
> -	ret = devm_add_action_or_reset(dev, pwm_fan_pwm_disable, ctx);
> +	ret = devm_add_action_or_reset(dev, pwm_fan_cleanup, ctx);
>  	if (ret)
>  		return ret;
>  
> @@ -455,7 +557,7 @@ static int pwm_fan_probe(struct platform_device *pdev)
>  	if (!channels)
>  		return -ENOMEM;
>  
> -	channels[0] = HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT);
> +	channels[0] = HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT | HWMON_PWM_ENABLE);
>  
>  	for (i = 0; i < ctx->tach_count; i++) {
>  		struct pwm_fan_tach *tach = &ctx->tachs[i];
> @@ -529,57 +631,26 @@ static int pwm_fan_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -static int pwm_fan_disable(struct device *dev)
> -{
> -	struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> -	int ret;
> -
> -	if (ctx->pwm_value) {
> -		/* 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;
> -	}
> -
> -	ret = pwm_fan_switch_power(ctx, false);
> -	if (ret) {
> -		dev_err(dev, "Failed to disable fan supply: %d\n", ret);
> -		return ret;
> -	}
> -
> -	return 0;
> -}
> -
>  static void pwm_fan_shutdown(struct platform_device *pdev)
>  {
> -	pwm_fan_disable(&pdev->dev);
> +	struct pwm_fan_ctx *ctx = platform_get_drvdata(pdev);
> +
> +	pwm_fan_cleanup(ctx);
>  }
>  
>  #ifdef CONFIG_PM_SLEEP
>  static int pwm_fan_suspend(struct device *dev)
>  {
> -	return pwm_fan_disable(dev);
> +	struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> +
> +	return pwm_fan_power_off(ctx);
>  }
>  
>  static int pwm_fan_resume(struct device *dev)
>  {
>  	struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> -	int ret;
> -
> -	ret = pwm_fan_switch_power(ctx, true);
> -	if (ret) {
> -		dev_err(dev, "Failed to enable fan supply: %d\n", ret);
> -		return ret;
> -	}
> -
> -	if (ctx->pwm_value == 0)
> -		return 0;
>  
> -	return pwm_apply_state(ctx->pwm, &ctx->pwm_state);
> +	return set_pwm(ctx, ctx->pwm_value);
>  }
>  #endif
>