Re: [PATCH 2/2] platform/x86: hp-wmi: implement fan keep-alive

[Krishna Chomal <krishna.chomal108@gmail.com>]

On Mon, Dec 29, 2025 at 03:21:42PM +0200, Ilpo Järvinen wrote:
[snip]
>> +/*
>> + * 90s delay to prevent the firmware from resetting fan mode after fixed
>> + * 120s timeout
>> + */
>> +#define KEEP_ALIVE_DELAY	90
>
>For any time related define, please include its unit in the name so a
>person reading code can immediately know it.
>

Renamed to KEEP_ALIVE_DELAY_SECS v2.

[snip]
>> @@ -2159,23 +2174,36 @@ static int hp_wmi_hwmon_enforce_ctx(struct hp_wmi_hwmon_priv *ctx)
>>  	case PWM_MODE_MAX:
>>  		if (is_victus_s_thermal_profile())
>>  			hp_wmi_get_fan_count_userdefine_trigger();
>> -		return hp_wmi_fan_speed_max_set(1);
>> +		ret = hp_wmi_fan_speed_max_set(1);
>> +		break;
>>  	case PWM_MODE_MANUAL:
>>  		if (!is_victus_s_thermal_profile())
>>  			return -EOPNOTSUPP;
>> -		return hp_wmi_fan_speed_set(ctx, PWM_TO_RPM(ctx->pwm, ctx));
>> +		ret = hp_wmi_fan_speed_set(ctx, PWM_TO_RPM(ctx->pwm, ctx));
>> +		break;
>>  	case PWM_MODE_AUTO:
>>  		if (is_victus_s_thermal_profile()) {
>>  			hp_wmi_get_fan_count_userdefine_trigger();
>> -			return hp_wmi_fan_speed_max_reset(ctx);
>> +			ret = hp_wmi_fan_speed_max_reset(ctx);
>>  		} else {
>> -			return hp_wmi_fan_speed_max_set(0);
>> +			ret = hp_wmi_fan_speed_max_set(0);
>>  		}
>> +		break;
>>  	default:
>>  		/* shouldn't happen */
>>  		return -EINVAL;
>>  	}
>>
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* Reschedule keep-alive work based on the new state */
>> +	if (ctx->mode == PWM_MODE_MAX || ctx->mode == PWM_MODE_MANUAL)
>> +		schedule_delayed_work(&ctx->keep_alive_dwork,
>> +				      secs_to_jiffies(KEEP_ALIVE_DELAY));
>> +	else
>> +		cancel_delayed_work_sync(&ctx->keep_alive_dwork);
>
>This is now duplicating the switch/case, just add these to the existing
>cases.
>
>You may want to introduce ret variable already in PATCH 1/2 and add a note
>there that an upcoming change is going to add keep-alive so the return
>value has to be stored temporarily.
>

Yes, handled the re-scheduling in switch/case itself in v2 and introduced
the temporary ret variable in patch 1/2.

>> +
>>  	return 0;
>>  }
>>
>> @@ -2321,6 +2349,20 @@ static const struct hwmon_chip_info chip_info = {
>>  	.info = info,
>>  };
>>
>> +static void hp_wmi_hwmon_keep_alive_handler(struct work_struct *work)
>> +{
>> +	struct delayed_work *dwork;
>> +	struct hp_wmi_hwmon_priv *ctx;
>> +
>> +	dwork = to_delayed_work(work);
>> +	ctx = container_of(dwork, struct hp_wmi_hwmon_priv, keep_alive_dwork);
>> +	/*
>> +	 * Re-apply the current hwmon context settings.
>> +	 * NOTE: hp_wmi_hwmon_enforce_ctx will handle the re-scheduling.
>> +	 */
>> +	hp_wmi_hwmon_enforce_ctx(ctx);
>
>(I only now understand why you named this function like this, and I still
>don't think it's a good name for it as you've other callers besides this
>one real "enforcing" case.)
>

Yes named to "apply_fan_settings" in patch 1/2 of this series.