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

[Krishna Chomal]

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.