Re: [PATCH v3 2/2] hwmon: (emc2305) fix pwm never being able to set lower

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

On Tue, Dec 06, 2022 at 01:53:31PM +0800, Xingjiang Qiao wrote:
> There are fields 'last_hwmon_state' and 'last_thermal_state' in the
> structure 'emc2305_cdev_data', which respectively store the cooling state
> set by the 'hwmon' and 'thermal' subsystem, and the driver author hopes
> that if the state set by 'hwmon' is lower than the value set by 'thermal',
> the driver will just save it without actually setting the pwm. Currently,
> the 'last_thermal_state' also be updated by 'hwmon', which will cause the
> cooling state to never be set to a lower value. This patch fixes that.
> 
> Signed-off-by: Xingjiang Qiao <nanpuyue@gmail.com>

Applied, after renaming emc2305_set_cur_state_shim to __emc2305_set_cur_state.

Thanks,
Guenter

> ---
>  drivers/hwmon/emc2305.c | 37 ++++++++++++++++++++++++-------------
>  1 file changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/hwmon/emc2305.c b/drivers/hwmon/emc2305.c
> index 9a78ca22541e..e0f6eb8d72fc 100644
> --- a/drivers/hwmon/emc2305.c
> +++ b/drivers/hwmon/emc2305.c
> @@ -171,22 +171,12 @@ static int emc2305_get_max_state(struct thermal_cooling_device *cdev, unsigned l
>  	return 0;
>  }
>  
> -static int emc2305_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state)
> +static int emc2305_set_cur_state_shim(struct emc2305_data *data, int cdev_idx, unsigned long state)
>  {
> -	int cdev_idx, ret;
> -	struct emc2305_data *data = cdev->devdata;
> +	int ret;
>  	struct i2c_client *client = data->client;
>  	u8 val, i;
>  
> -	if (state > data->max_state)
> -		return -EINVAL;
> -
> -	cdev_idx =  emc2305_get_cdev_idx(cdev);
> -	if (cdev_idx < 0)
> -		return cdev_idx;
> -
> -	/* Save thermal state. */
> -	data->cdev_data[cdev_idx].last_thermal_state = state;
>  	state = max_t(unsigned long, state, data->cdev_data[cdev_idx].last_hwmon_state);
>  
>  	val = EMC2305_PWM_STATE2DUTY(state, data->max_state, EMC2305_FAN_MAX);
> @@ -211,6 +201,27 @@ static int emc2305_set_cur_state(struct thermal_cooling_device *cdev, unsigned l
>  	return 0;
>  }
>  
> +static int emc2305_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state)
> +{
> +	int cdev_idx, ret;
> +	struct emc2305_data *data = cdev->devdata;
> +
> +	if (state > data->max_state)
> +		return -EINVAL;
> +
> +	cdev_idx =  emc2305_get_cdev_idx(cdev);
> +	if (cdev_idx < 0)
> +		return cdev_idx;
> +
> +	/* Save thermal state. */
> +	data->cdev_data[cdev_idx].last_thermal_state = state;
> +	ret = emc2305_set_cur_state_shim(data, cdev_idx, state);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
>  static const struct thermal_cooling_device_ops emc2305_cooling_ops = {
>  	.get_max_state = emc2305_get_max_state,
>  	.get_cur_state = emc2305_get_cur_state,
> @@ -401,7 +412,7 @@ emc2305_write(struct device *dev, enum hwmon_sensor_types type, u32 attr, int ch
>  				 */
>  				if (data->cdev_data[cdev_idx].last_hwmon_state >=
>  				    data->cdev_data[cdev_idx].last_thermal_state)
> -					return emc2305_set_cur_state(data->cdev_data[cdev_idx].cdev,
> +					return emc2305_set_cur_state_shim(data, cdev_idx,
>  							data->cdev_data[cdev_idx].last_hwmon_state);
>  				return 0;
>  			}