Re: [PATCH v3] platform/x86: lg-laptop: Fix WMAB call in fan_mode_store()

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Fri, 19 Sep 2025, Daniel wrote:

> > Is it good to reuse the input side define here for response side or
> > should you have another with a more specific name? (This is put to
> > status named variable so my natural expectation is that the field's
> > name is somehow related to that.)
> 
> The fan mode really is sent back to us in *_LOWER and *_UPPER form,
> exactly like how we send the fan mode.  (Only, the lower and upper
> portions are guaranteed to be identical, so we just read the lower.)
> Hence why I did not add a new define for the response side.
> 
> Should I add another define FAN_MODE_RESPONSE?

Ah okay so the variable name is misleading then as it's really "mode" 
that is returned, not really "status"? I think this is fine then although 
I'd prefer to have also another patch on top of this which would rename 
that variable to mode.

> -- >8 --
> Subject: [PATCH v4] platform/x86: lg-laptop: Fix WMAB call in fan_mode_store()
> 
> On my LG Gram 16Z95P-K.AA75A8 (2022), writes to
> /sys/devices/platform/lg-laptop/fan_mode have no effect and reads always
> report a status of 0.
> 
> Disassembling the relevant ACPI tables reveals that in the WMAB call to
> set the fan mode, the new mode is read either from bits 0,1 or bits 4,5
> (depending on the value of some other EC register).  Thus when we call
> WMAB twice, first with bits 4,5 zero, then bits 0,1 zero, the second
> call undoes the effect of the first call.
> 
> Fix this by calling WMAB once, with the mode set in bits 0,1 and 4,5.
> When the fan mode is returned from WMAB it always has this form, so
> there is no need to preserve the other bits.  As a bonus, the driver
> now supports the "Performance" fan mode seen in the LG-provided Windows
> control app, which provides less aggressive CPU throttling but louder
> fan noise and shorter battery life.
> 
> I can confirm with this patch reading/writing the fan mode now works
> as expected on my laptop, although I have not tested it on any other
> LG laptop.
> 
> Also, correct the documentation to reflect that 0 corresponds to the
> default mode (what the Windows app calls "Optimal") and 1 corresponds
> to the silent mode.
> 
> Signed-off-by: Daniel Lee <dany97@live.ca>
> Tested-by: Daniel Lee <dany97@live.ca>
> Fixes: dbf0c5a6b1f8e7bec5e17baa60a1e04c28d90f9b ("platform/x86: Add LG Gram laptop special features driver")
> ---
> V1 -> V2: Replace bitops with GENMASK() and FIELD_PREP()
> V2 -> V3: Add parentheses next to function name in summary line
>           Use full name in signoff
> V3 -> V4: Add include for linux/bitfield.h
>           Remove "FIELD" from bitmask macro names
> 
>  .../admin-guide/laptops/lg-laptop.rst         |  4 +--
>  drivers/platform/x86/lg-laptop.c              | 30 ++++++++-----------
>  2 files changed, 14 insertions(+), 20 deletions(-)
> 
> diff --git a/Documentation/admin-guide/laptops/lg-laptop.rst b/Documentation/admin-guide/laptops/lg-laptop.rst
> index 67fd6932c..c4dd534f9 100644
> --- a/Documentation/admin-guide/laptops/lg-laptop.rst
> +++ b/Documentation/admin-guide/laptops/lg-laptop.rst
> @@ -48,8 +48,8 @@ This value is reset to 100 when the kernel boots.
>  Fan mode
>  --------
>  
> -Writing 1/0 to /sys/devices/platform/lg-laptop/fan_mode disables/enables
> -the fan silent mode.
> +Writing 0/1/2 to /sys/devices/platform/lg-laptop/fan_mode sets fan mode to
> +Optimal/Silent/Performance respectively.
>  
>  
>  USB charge
> diff --git a/drivers/platform/x86/lg-laptop.c b/drivers/platform/x86/lg-laptop.c
> index 4b57102c7..0ef179f7a 100644
> --- a/drivers/platform/x86/lg-laptop.c
> +++ b/drivers/platform/x86/lg-laptop.c
> @@ -8,6 +8,7 @@
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
>  #include <linux/acpi.h>
> +#include <linux/bitfield.h>
>  #include <linux/bits.h>
>  #include <linux/device.h>
>  #include <linux/dev_printk.h>
> @@ -75,6 +76,9 @@ MODULE_PARM_DESC(fw_debug, "Enable printing of firmware debug messages");
>  #define WMBB_USB_CHARGE 0x10B
>  #define WMBB_BATT_LIMIT 0x10C
>  
> +#define FAN_MODE_LOWER GENMASK(1, 0)
> +#define FAN_MODE_UPPER GENMASK(5, 4)
> +
>  #define PLATFORM_NAME   "lg-laptop"
>  
>  MODULE_ALIAS("wmi:" WMI_EVENT_GUID0);
> @@ -274,29 +278,19 @@ static ssize_t fan_mode_store(struct device *dev,
>  			      struct device_attribute *attr,
>  			      const char *buffer, size_t count)
>  {
> -	bool value;
> +	unsigned long value;
>  	union acpi_object *r;
> -	u32 m;
>  	int ret;
>  
> -	ret = kstrtobool(buffer, &value);
> +	ret = kstrtoul(buffer, 10, &value);
>  	if (ret)
>  		return ret;
> +	if (value >= 3)
> +		return -EINVAL;
>  
> -	r = lg_wmab(dev, WM_FAN_MODE, WM_GET, 0);
> -	if (!r)
> -		return -EIO;
> -
> -	if (r->type != ACPI_TYPE_INTEGER) {
> -		kfree(r);
> -		return -EIO;
> -	}
> -
> -	m = r->integer.value;
> -	kfree(r);
> -	r = lg_wmab(dev, WM_FAN_MODE, WM_SET, (m & 0xffffff0f) | (value << 4));
> -	kfree(r);
> -	r = lg_wmab(dev, WM_FAN_MODE, WM_SET, (m & 0xfffffff0) | value);
> +	r = lg_wmab(dev, WM_FAN_MODE, WM_SET,
> +		FIELD_PREP(FAN_MODE_LOWER, value) |
> +		FIELD_PREP(FAN_MODE_UPPER, value));
>  	kfree(r);
>  
>  	return count;
> @@ -317,7 +311,7 @@ static ssize_t fan_mode_show(struct device *dev,
>  		return -EIO;
>  	}
>  
> -	status = r->integer.value & 0x01;
> +	status = FIELD_GET(FAN_MODE_LOWER, r->integer.value);
>  	kfree(r);
>  
>  	return sysfs_emit(buffer, "%d\n", status);
> 

-- 
 i.