Re: [RFC PATCH 2/3] platform/x86: lenovo-wmi-other: Add Legion Go fan RPM fallback

Hi Aditya,

On Sat, 2026-08-22 at 03:17 +0530, Aditya Dash wrote:
> Some Legion Go firmware does not mark fan 1 RPM as readable in Capability
> Data, although Other Mode feature 0x04030001 returns the current RPM. As a
> result, lenovo-wmi-other hides fan1_input.
> 
> When the normal VALID and GET flags are missing on a supported product,
> try one read from the Other Mode feature. Expose fan1_input as read-only
> if the read succeeds and does not return 0xffffffff. Treat a later
> 0xffffffff reply as an unavailable reading.

U32_MAX

> 
> Assisted-by: Pi:gpt-5.6-sol
> Signed-off-by: Aditya Dash <[email protected]>
> ---
>  Documentation/wmi/devices/lenovo-wmi-other.rst |  4 ++++
>  drivers/platform/x86/lenovo/wmi-other.c        | 14 +++++++++++++-
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/wmi/devices/lenovo-wmi-other.rst b/Documentation/wmi/devices/lenovo-wmi-other.rst
> index 75f2deaaef16..c93e9e6f3fd0 100644
> --- a/Documentation/wmi/devices/lenovo-wmi-other.rst
> +++ b/Documentation/wmi/devices/lenovo-wmi-other.rst
> @@ -56,6 +56,10 @@ On supported Legion Go models, Other Mode feature ``0x04020000`` controls
>  Full Speed mode in firmware. The driver exposes it as ``pwm1_enable``. Value 0
>  enables Full Speed, and value 2 returns fan control to firmware.
>  
> +Some Legion Go firmware does not advertise fan 1 RPM through Capability
> +Data. In that case, the driver reads Other Mode feature ``0x04030001`` for
> +``fan1_input``. Value ``0xffffffff`` means that RPM is unavailable.
> +

The documentation does not mean to record the implementation details.
Drop the paragraph and tell your LLM not to ramble.

>  LENOVO_CAPABILITY_DATA_01
>  -------------------------
>  
> diff --git a/drivers/platform/x86/lenovo/wmi-other.c b/drivers/platform/x86/lenovo/wmi-other.c
> index c180933e1d18..b4be7739b243 100644
> --- a/drivers/platform/x86/lenovo/wmi-other.c
> +++ b/drivers/platform/x86/lenovo/wmi-other.c
> @@ -98,6 +98,7 @@ enum lwmi_feature_id_psu {
>  #define LWMI_FAN_ID(x) ((x) + LWMI_FAN_ID_BASE)
>  
>  #define LWMI_FAN_DIV 100
> +#define LWMI_FAN_RPM_NORMAL_SUPPORT (LWMI_SUPP_VALID | LWMI_SUPP_GET)

Inline it.

>  
>  #define LWMI_CHARGE_BEHAVIOR_DISCHARGE	0x00
>  #define LWMI_CHARGE_BEHAVIOR_AUTO	0x01
> @@ -195,6 +196,7 @@ struct lwmi_om_priv {
>  
>  	struct lwmi_fan_info fan_info[LWMI_FAN_NR];
>  	bool fullspeed_supported;
> +	bool fan0_input_fallback;

You LLM was over-engineering things. Shouldn't overriding fan_info being
enough?

   priv->fan_info[0].supported |= (LWMI_SUPP_VALID | LWMI_SUPP_GET);

>  
>  	struct {
>  		bool capdata00_collected : 1;
> @@ -343,6 +345,8 @@ static umode_t lwmi_om_hwmon_is_visible(const void *drvdata, enum hwmon_sensor_t
>  		return 0644;
>  
>  	if (type == hwmon_fan) {
> +		if (channel == 0 && priv->fan0_input_fallback && attr == hwmon_fan_input)
> +			return 0444;
>  		if (!(priv->fan_info[channel].supported & LWMI_SUPP_VALID))
>  			return 0;
>  
> @@ -419,6 +423,8 @@ static int lwmi_om_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>  			err = lwmi_om_fan_get_set(priv, channel, &retval, false);
>  			if (err)
>  				return err;
> +			if (channel == 0 && priv->fan0_input_fallback && retval == U32_MAX)
> +				return -EIO;

Don't over-engineer it. You can safely interpret U32_MAX as -EIO in any
cases.

Therefore, please move the check into lwmi_om_fan_get_set().

>  
>  			*val = retval;
>  			return 0;
> @@ -542,6 +548,7 @@ static const struct hwmon_chip_info lwmi_om_hwmon_chip_info = {
>   */
>  static void lwmi_om_hwmon_add(struct lwmi_om_priv *priv)
>  {
> +	u32 rpm;
>  	long enable;
>  	int i, valid;
>  
> @@ -563,6 +570,11 @@ static void lwmi_om_hwmon_add(struct lwmi_om_priv *priv)
>  
>  	priv->fullspeed_supported =
>  		lwmi_fan_supported() && !lwmi_om_fullspeed_get(priv, &enable);
> +	priv->fan0_input_fallback =
> +		lwmi_fan_supported() &&

???

Don't call it again. Simply consult priv->fullspeed_supported.

> +		(priv->fan_info[0].supported & LWMI_FAN_RPM_NORMAL_SUPPORT) !=
> +		LWMI_FAN_RPM_NORMAL_SUPPORT &&

!(priv->fan_info[0].supported & LWMI_SUPP_VALID) ||
!(priv->fan_info[0].supported & LWMI_SUPP_GET)

> +		!lwmi_om_fan_get_set(priv, 0, &rpm, false) && rpm != U32_MAX;

And that's why moving the check into lwmi_om_fan_get_set() makes things
easier.

Thanks,
Rong

>  
>  	valid = 0;
>  	for (i = 0; i < LWMI_FAN_NR; i++) {
> @@ -580,7 +592,7 @@ static void lwmi_om_hwmon_add(struct lwmi_om_priv *priv)
>  		}
>  	}
>  
> -	if (valid == 0 && !priv->fullspeed_supported) {
> +	if (valid == 0 && !priv->fullspeed_supported && !priv->fan0_input_fallback) {
>  		dev_warn(&priv->wdev->dev,
>  			 "fan reporting/tuning is unsupported on this device\n");
>  		return;