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

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

[-- Attachment #1: Type: text/plain, Size: 4106 bytes --]

On Thu, 28 Aug 2025, Ilpo Järvinen wrote:

> On Fri, 22 Aug 2025, Daniel wrote:
> 
> > 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 call
> > 
> > 	lg_wmab(dev, WM_FAN_MODE, WM_SET, x)
> > 
> > the new mode is read from either the upper nibble or the lower nibble of x,
> > depending on the value of some other EC register.  Crucially, when WMAB
> > is called twice (once with the fan mode in the upper nibble, once with
> > it in the lower nibble), the result of the second call can overwrite
> > the first call.

Oh, one more thing. I think this would warrant a Fixes tag pointing to the 
original commit.

--
 i.

> > Fix this by calling WMAB once, with the fan mode set in both nibbles.
> > As a bonus, the driver now supports the "Performance" mode seen in
> > the Windows LG Control Center app (less aggressive CPU throttling, but
> > louder fan noise and shorter battery life).  I can confirm that with
> > this patch writing/reading the fan mode works as expected on my laptop,
> > although I haven't tested it on any other LG laptops.
> > 
> > Also, correct the documentation to reflect that a value of 0 corresponds
> > to the default mode (what the LG app calls "Optimal") and a value of 1
> > corresponds to the silent mode.
> >
> > Tested-by: Daniel <dany97@live.ca>
> > Signed-off-by: Daniel <dany97@live.ca>
> > ---
> >  .../admin-guide/laptops/lg-laptop.rst         |  4 ++--
> >  drivers/platform/x86/lg-laptop.c              | 22 +++++--------------
> >  2 files changed, 8 insertions(+), 18 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..b8de6e568 100644
> > --- a/drivers/platform/x86/lg-laptop.c
> > +++ b/drivers/platform/x86/lg-laptop.c
> > @@ -274,29 +274,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);
> > +	r = lg_wmab(dev, WM_FAN_MODE, WM_SET, (value << 4) | value);
> 
> Is it okay to remove preserving the other bits?
> 
> Please name these field with defined GENMASK() and then use FIELD_PREP() 
> here for both fields.
> 
> >  	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);
> >  	kfree(r);
> >  
> >  	return count;
> > @@ -317,7 +307,7 @@ static ssize_t fan_mode_show(struct device *dev,
> >  		return -EIO;
> >  	}
> >  
> > -	status = r->integer.value & 0x01;
> > +	status = r->integer.value & 0x03;
> 
> This looks also like a field so should be named with a define?
> 
> >  	kfree(r);
> >  
> >  	return sysfs_emit(buffer, "%d\n", status);
> > 
> 
>