[PATCH] platform/x86: lg-laptop: Support for battery charge limit on newer models

[PATCH] platform/x86: lg-laptop: Support for battery charge limit on newer models

[Matan Ziv-Av]

Add support for the difference between various models:

- Use dmi to detect laptop model.
- 2019 and newer models use _wmbb method to set battery charge limit.

Signed-off-by: Matan Ziv-Av <matan@svgalib.org>
---
 drivers/platform/x86/lg-laptop.c | 75 ++++++++++++++++++++++++++++----
 1 file changed, 66 insertions(+), 9 deletions(-)

diff --git a/drivers/platform/x86/lg-laptop.c b/drivers/platform/x86/lg-laptop.c
index 20145b539335..12b0257c0bdd 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/dmi.h>
 #include <linux/input.h>
 #include <linux/input/sparse-keymap.h>
 #include <linux/kernel.h>
@@ -69,6 +70,8 @@ static u32 inited;
 #define INIT_INPUT_ACPI         0x04
 #define INIT_SPARSE_KEYMAP      0x80
 
+static int battery_limit_use_wmbb;
+
 static const struct key_entry wmi_keymap[] = {
 	{KE_KEY, 0x70, {KEY_F15} },	 /* LG control panel (F1) */
 	{KE_KEY, 0x74, {KEY_F13} },	 /* Touchpad toggle (F5) */
@@ -461,7 +464,10 @@ static ssize_t battery_care_limit_store(struct device *dev,
 	if (value == 100 || value == 80) {
 		union acpi_object *r;
 
-		r = lg_wmab(WM_BATT_LIMIT, WM_SET, value);
+		if (!battery_limit_use_wmbb)
+			r = lg_wmab(WM_BATT_LIMIT, WM_SET, value);
+		else
+			r = lg_wmbb(WMBB_BATT_LIMIT, WM_SET, value);
 		if (!r)
 			return -EIO;
 
@@ -479,16 +485,29 @@ static ssize_t battery_care_limit_show(struct device *dev,
 	unsigned int status;
 	union acpi_object *r;
 
-	r = lg_wmab(WM_BATT_LIMIT, WM_GET, 0);
-	if (!r)
-		return -EIO;
+	if (!battery_limit_use_wmbb) {
+		r = lg_wmab(WM_BATT_LIMIT, WM_GET, 0);
+		if (!r)
+			return -EIO;
 
-	if (r->type != ACPI_TYPE_INTEGER) {
-		kfree(r);
-		return -EIO;
-	}
+		if (r->type != ACPI_TYPE_INTEGER) {
+			kfree(r);
+			return -EIO;
+		}
 
-	status = r->integer.value;
+		status = r->integer.value;
+	} else {
+		r = lg_wmbb(WMBB_BATT_LIMIT, WM_GET, 0);
+		if (!r)
+			return -EIO;
+
+		if (r->type != ACPI_TYPE_BUFFER) {
+			kfree(r);
+			return -EIO;
+		}
+
+		status = r->buffer.pointer[0x10];
+	}
 	kfree(r);
 	if (status != 80 && status != 100)
 		status = 0;
@@ -602,6 +621,8 @@ static struct platform_driver pf_driver = {
 static int acpi_add(struct acpi_device *device)
 {
 	int ret;
+	const char *product;
+	int year = 2017;
 
 	if (pf_device)
 		return 0;
@@ -619,6 +640,42 @@ static int acpi_add(struct acpi_device *device)
 		pr_err("unable to register platform device\n");
 		goto out_platform_registered;
 	}
+	product = dmi_get_system_info(DMI_PRODUCT_NAME);
+	if (strlen(product) > 4)
+		switch (product[4]) {
+		case '5':
+		case '6':
+			year = 2016;
+			break;
+		case '7':
+			year = 2017;
+			break;
+		case '8':
+			year = 2018;
+			break;
+		case '9':
+			year = 2019;
+			break;
+		case '0':
+			if (strlen(product) > 5)
+				switch (product[5]) {
+				case 'N':
+					year = 2020;
+					break;
+				case 'P':
+					year = 2021;
+					break;
+				default:
+					year = 2022;
+				}
+			break;
+		default:
+			year = 2019;
+		}
+	pr_info("product: %s  year: %d\n", product, year);
+
+	if (year >= 2019)
+		battery_limit_use_wmbb = 1;
 
 	ret = sysfs_create_group(&pf_device->dev.kobj, &dev_attribute_group);
 	if (ret)
-- 
2.31.1


-- 
Matan.

Re: [PATCH] platform/x86: lg-laptop: Support for battery charge limit on newer models

[Hans de Goede]

Hi,

On 8/14/21 12:11 AM, Matan Ziv-Av wrote:
> 
> Add support for the difference between various models:
> 
> - Use dmi to detect laptop model.
> - 2019 and newer models use _wmbb method to set battery charge limit.
> 
> Signed-off-by: Matan Ziv-Av <matan@svgalib.org>

Thank you for the patch, some small comments inline.


> ---
>  drivers/platform/x86/lg-laptop.c | 75 ++++++++++++++++++++++++++++----
>  1 file changed, 66 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/platform/x86/lg-laptop.c b/drivers/platform/x86/lg-laptop.c
> index 20145b539335..12b0257c0bdd 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/dmi.h>
>  #include <linux/input.h>
>  #include <linux/input/sparse-keymap.h>
>  #include <linux/kernel.h>
> @@ -69,6 +70,8 @@ static u32 inited;
>  #define INIT_INPUT_ACPI         0x04
>  #define INIT_SPARSE_KEYMAP      0x80
>  
> +static int battery_limit_use_wmbb;
> +
>  static const struct key_entry wmi_keymap[] = {
>  	{KE_KEY, 0x70, {KEY_F15} },	 /* LG control panel (F1) */
>  	{KE_KEY, 0x74, {KEY_F13} },	 /* Touchpad toggle (F5) */
> @@ -461,7 +464,10 @@ static ssize_t battery_care_limit_store(struct device *dev,
>  	if (value == 100 || value == 80) {
>  		union acpi_object *r;
>  
> -		r = lg_wmab(WM_BATT_LIMIT, WM_SET, value);
> +		if (!battery_limit_use_wmbb)
> +			r = lg_wmab(WM_BATT_LIMIT, WM_SET, value);
> +		else
> +			r = lg_wmbb(WMBB_BATT_LIMIT, WM_SET, value);

Please drop the ! from the if condition and swap the 2 branches.

>  		if (!r)
>  			return -EIO;
>  
> @@ -479,16 +485,29 @@ static ssize_t battery_care_limit_show(struct device *dev,
>  	unsigned int status;
>  	union acpi_object *r;
>  
> -	r = lg_wmab(WM_BATT_LIMIT, WM_GET, 0);
> -	if (!r)
> -		return -EIO;
> +	if (!battery_limit_use_wmbb) {
> +		r = lg_wmab(WM_BATT_LIMIT, WM_GET, 0);
> +		if (!r)
> +			return -EIO;
>  
> -	if (r->type != ACPI_TYPE_INTEGER) {
> -		kfree(r);
> -		return -EIO;
> -	}
> +		if (r->type != ACPI_TYPE_INTEGER) {
> +			kfree(r);
> +			return -EIO;
> +		}
>  
> -	status = r->integer.value;
> +		status = r->integer.value;
> +	} else {
> +		r = lg_wmbb(WMBB_BATT_LIMIT, WM_GET, 0);
> +		if (!r)
> +			return -EIO;
> +
> +		if (r->type != ACPI_TYPE_BUFFER) {
> +			kfree(r);
> +			return -EIO;
> +		}
> +
> +		status = r->buffer.pointer[0x10];
> +	}

Idem (Please drop the ! from the if condition and swap the 2 branches).

>  	kfree(r);
>  	if (status != 80 && status != 100)
>  		status = 0;
> @@ -602,6 +621,8 @@ static struct platform_driver pf_driver = {
>  static int acpi_add(struct acpi_device *device)
>  {
>  	int ret;
> +	const char *product;
> +	int year = 2017;
>  
>  	if (pf_device)
>  		return 0;
> @@ -619,6 +640,42 @@ static int acpi_add(struct acpi_device *device)
>  		pr_err("unable to register platform device\n");
>  		goto out_platform_registered;
>  	}
> +	product = dmi_get_system_info(DMI_PRODUCT_NAME);
> +	if (strlen(product) > 4)
> +		switch (product[4]) {
> +		case '5':
> +		case '6':
> +			year = 2016;
> +			break;
> +		case '7':
> +			year = 2017;
> +			break;
> +		case '8':
> +			year = 2018;
> +			break;
> +		case '9':
> +			year = 2019;
> +			break;
> +		case '0':
> +			if (strlen(product) > 5)
> +				switch (product[5]) {
> +				case 'N':
> +					year = 2020;
> +					break;
> +				case 'P':
> +					year = 2021;
> +					break;
> +				default:
> +					year = 2022;
> +				}
> +			break;
> +		default:
> +			year = 2019;
> +		}
> +	pr_info("product: %s  year: %d\n", product, year);
> +
> +	if (year >= 2019)
> +		battery_limit_use_wmbb = 1;
>  
>  	ret = sysfs_create_group(&pf_device->dev.kobj, &dev_attribute_group);
>  	if (ret)

This does not feel very robust how about doing a strstr for "201" and if that
fails for "202" to find the year ?

Regards,

Hans

p.s.

While reviewing this I also took a quick look at the existing lg-laptop.c
and the wmi_keymap stood out to me, specifically:

        {KE_KEY, 0x74, {KEY_F13} },      /* Touchpad toggle (F5) */

If that key just sends this event and does not actually change the
touchpad settings, IOW userspace is supposed to react this (e.g.
filter out touchpad events in software after the toggle), then the
correct key to send here would be KEY_F21, this has been the standard
key-code to send for this for a while now and GNOME and KDE will
automatically do the right thing when sending that, including a
nice on-screen-display (OSD)notifcation (like when changing the volume)
indicating the new (software) state (on or off) of the touchpad.

If the hw does actually handle the touchpad on/off itself
(I see there also is a touchpad-led?) then the right thing to do
would be to send f22 (Touchpad toggle off-to-on) and f23
(Touchpad toggle on-to-off). This assumes that you can figure
out the new touchpad state. When receiving f22 / f23 GNOME will
display the OSD without making any other settings changes.

Also see: /lib/udev/hwdb.d/60-keyboard.hwdb


        {KE_KEY, 0x10000000, {KEY_F16} },/* Keyboard backlight (F8) - pressing
                                          * this key both sends an event and
                                          * changes backlight level.
                                          */

If this hotkey changes the kbd-backlight level "in hardware"
then it should not send a key-press instead you should specify

led_classdev.flags = LED_BRIGHT_HW_CHANGED

For the kbd-backlight led_classdev and then call:

	led_classdev_notify_brightness_hw_changed(&kbd_backlight, new_backlight_level);

When receiving the event. upower will pick the event send by this up
and then notify interested parties such as e.g. gnome-settings-daemon
which will then show a nice OSD with the new backlight level similar
to how it is done for e.g. volume controls.


If you can also send patches to change these 2 things, so that lg-laptop
conforms with the standard userspace APIs used for this that would be great.

Re: [PATCH] platform/x86: lg-laptop: Support for battery charge limit on newer models

[Matan Ziv-Av]

On Wed, 18 Aug 2021, Hans de Goede wrote:

> Hi,
> 
> On 8/14/21 12:11 AM, Matan Ziv-Av wrote:
> > 
> > Add support for the difference between various models:
> > 
> > - Use dmi to detect laptop model.
> > - 2019 and newer models use _wmbb method to set battery charge limit.
> > 
> > Signed-off-by: Matan Ziv-Av <matan@svgalib.org>
> 
> Thank you for the patch, some small comments inline.
> 
> 
> Please drop the ! from the if condition and swap the 2 branches.

Fixed.

> > +	product = dmi_get_system_info(DMI_PRODUCT_NAME);
> > +	if (strlen(product) > 4)
> > +		switch (product[4]) {
> > +		case '5':
> > +		case '6':
> > +			year = 2016;
> > +			break;
> > +		case '7':
> > +			year = 2017;
> > +			break;
> > +		case '8':
> > +			year = 2018;
> > +			break;
> > +		case '9':
> > +			year = 2019;
> > +			break;
> > +		case '0':
> > +			if (strlen(product) > 5)
> > +				switch (product[5]) {
> > +				case 'N':
> > +					year = 2020;
> > +					break;
> > +				case 'P':
> > +					year = 2021;
> > +					break;
> > +				default:
> > +					year = 2022;
> > +				}
> > +			break;
> > +		default:
> > +			year = 2019;
> > +		}
> > +	pr_info("product: %s  year: %d\n", product, year);
> > +
> > +	if (year >= 2019)
> > +		battery_limit_use_wmbb = 1;
> 
> This does not feel very robust how about doing a strstr for "201" and if that
> fails for "202" to find the year ?

Unfortunately, this is not so simple.

Some example model numbers:

15Z960-A.AA75U1
15Z980-R.AAS9U1
14T990-U.AAS8U1
17Z90P-K.AAB8U1

First two digits represent screen size. Third letter device type. Fifth 
digit is the last digit of the model year (up to 2021, where it is 0 and 
the sixth letter indicates the model year).

> Regards,
> 
> Hans
> 
> p.s.
> 
> While reviewing this I also took a quick look at the existing lg-laptop.c
> and the wmi_keymap stood out to me, specifically:
> 
>         {KE_KEY, 0x74, {KEY_F13} },      /* Touchpad toggle (F5) */
> 
> If that key just sends this event and does not actually change the
> touchpad settings, IOW userspace is supposed to react this (e.g.
> filter out touchpad events in software after the toggle), then the
> correct key to send here would be KEY_F21, this has been the standard
> key-code to send for this for a while now and GNOME and KDE will
> automatically do the right thing when sending that, including a
> nice on-screen-display (OSD)notifcation (like when changing the volume)
> indicating the new (software) state (on or off) of the touchpad.
> 
> If the hw does actually handle the touchpad on/off itself
> (I see there also is a touchpad-led?) then the right thing to do
> would be to send f22 (Touchpad toggle off-to-on) and f23
> (Touchpad toggle on-to-off). This assumes that you can figure
> out the new touchpad state. When receiving f22 / f23 GNOME will
> display the OSD without making any other settings changes.
>
> Also see: /lib/udev/hwdb.d/60-keyboard.hwdb
> 
> 
>         {KE_KEY, 0x10000000, {KEY_F16} },/* Keyboard backlight (F8) - pressing
>                                           * this key both sends an event and
>                                           * changes backlight level.
>                                           */
> 
> If this hotkey changes the kbd-backlight level "in hardware"
> then it should not send a key-press instead you should specify
> 
> led_classdev.flags = LED_BRIGHT_HW_CHANGED
> 
> For the kbd-backlight led_classdev and then call:
> 
> 	led_classdev_notify_brightness_hw_changed(&kbd_backlight, new_backlight_level);
> 
> When receiving the event. upower will pick the event send by this up
> and then notify interested parties such as e.g. gnome-settings-daemon
> which will then show a nice OSD with the new backlight level similar
> to how it is done for e.g. volume controls.
> 
> 
> If you can also send patches to change these 2 things, so that lg-laptop
> conforms with the standard userspace APIs used for this that would be great.

I sent patches for this (in a separate thread). But in my testing, this 
does not happen automatically and I did not find yet how to configure 
udev/upower/kde to display this notification.




-- 
Matan.

Re: [PATCH] platform/x86: lg-laptop: Support for battery charge limit on newer models

[Barnabás Pőcze]

Hi


2021. augusztus 18., szerda 15:36 keltezéssel, Matan Ziv-Av írta:
> [...]
> > If this hotkey changes the kbd-backlight level "in hardware"
> > then it should not send a key-press instead you should specify
> >
> > led_classdev.flags = LED_BRIGHT_HW_CHANGED
> >
> > For the kbd-backlight led_classdev and then call:
> >
> > 	led_classdev_notify_brightness_hw_changed(&kbd_backlight, new_backlight_level);
> >
> > When receiving the event. upower will pick the event send by this up
> > and then notify interested parties such as e.g. gnome-settings-daemon
> > which will then show a nice OSD with the new backlight level similar
> > to how it is done for e.g. volume controls.
> >
> >
> > If you can also send patches to change these 2 things, so that lg-laptop
> > conforms with the standard userspace APIs used for this that would be great.
>
> I sent patches for this (in a separate thread). But in my testing, this
> does not happen automatically and I did not find yet how to configure
> udev/upower/kde to display this notification.
>

You can check the events by running something like:

  dbus-monitor --system "type='signal',interface='org.freedesktop.UPower.KbdBacklight',member='BrightnessChanged'"

The gnome-settings-daemon watches these signals and instructs the GNOME shell to display
an OSD about the new keyboard brightness. Well, at least on my machine. A quick search in
the repository[1] suggests that KDE's PowerDevil is also listening for these signals.

I am not sure now, but I seem to recall that UPower needed to be restarted to see a LED
that appeared after it was started when I was testing these LED brightness change notifications.

[1]: https://invent.kde.org/plasma/powerdevil


Best regards,
Barnabás Pőcze

Re: [PATCH] platform/x86: lg-laptop: Support for battery charge limit on newer models

[Hans de Goede]

Hi,

On 8/18/21 5:01 PM, Barnabás Pőcze wrote:
> Hi
> 
> 
> 2021. augusztus 18., szerda 15:36 keltezéssel, Matan Ziv-Av írta:
>> [...]
>>> If this hotkey changes the kbd-backlight level "in hardware"
>>> then it should not send a key-press instead you should specify
>>>
>>> led_classdev.flags = LED_BRIGHT_HW_CHANGED
>>>
>>> For the kbd-backlight led_classdev and then call:
>>>
>>> 	led_classdev_notify_brightness_hw_changed(&kbd_backlight, new_backlight_level);
>>>
>>> When receiving the event. upower will pick the event send by this up
>>> and then notify interested parties such as e.g. gnome-settings-daemon
>>> which will then show a nice OSD with the new backlight level similar
>>> to how it is done for e.g. volume controls.
>>>
>>>
>>> If you can also send patches to change these 2 things, so that lg-laptop
>>> conforms with the standard userspace APIs used for this that would be great.
>>
>> I sent patches for this (in a separate thread). But in my testing, this
>> does not happen automatically and I did not find yet how to configure
>> udev/upower/kde to display this notification.
>>
> 
> You can check the events by running something like:
> 
>   dbus-monitor --system "type='signal',interface='org.freedesktop.UPower.KbdBacklight',member='BrightnessChanged'"

Right, one important remark though, for some reason when the brightness_hw_changed
API was added to the LED subsystem it was decided to make this a Kconfig
option, so in order for this to work you must also make sure to have:

CONFIG_LEDS_BRIGHTNESS_HW_CHANGED=y

in your .config .

Regards,

Hans

Re: [PATCH] platform/x86: lg-laptop: Support for battery charge limit on newer models

[Hans de Goede]

Hi,

On 8/18/21 3:36 PM, Matan Ziv-Av wrote:
> On Wed, 18 Aug 2021, Hans de Goede wrote:
> 
>> Hi,
>>
>> On 8/14/21 12:11 AM, Matan Ziv-Av wrote:
>>>
>>> Add support for the difference between various models:
>>>
>>> - Use dmi to detect laptop model.
>>> - 2019 and newer models use _wmbb method to set battery charge limit.
>>>
>>> Signed-off-by: Matan Ziv-Av <matan@svgalib.org>
>>
>> Thank you for the patch, some small comments inline.
>>
>>
>> Please drop the ! from the if condition and swap the 2 branches.
> 
> Fixed.

Thanks.

>>> +	product = dmi_get_system_info(DMI_PRODUCT_NAME);
>>> +	if (strlen(product) > 4)
>>> +		switch (product[4]) {
>>> +		case '5':
>>> +		case '6':
>>> +			year = 2016;
>>> +			break;
>>> +		case '7':
>>> +			year = 2017;
>>> +			break;
>>> +		case '8':
>>> +			year = 2018;
>>> +			break;
>>> +		case '9':
>>> +			year = 2019;
>>> +			break;
>>> +		case '0':
>>> +			if (strlen(product) > 5)
>>> +				switch (product[5]) {
>>> +				case 'N':
>>> +					year = 2020;
>>> +					break;
>>> +				case 'P':
>>> +					year = 2021;
>>> +					break;
>>> +				default:
>>> +					year = 2022;
>>> +				}
>>> +			break;
>>> +		default:
>>> +			year = 2019;
>>> +		}
>>> +	pr_info("product: %s  year: %d\n", product, year);
>>> +
>>> +	if (year >= 2019)
>>> +		battery_limit_use_wmbb = 1;
>>
>> This does not feel very robust how about doing a strstr for "201" and if that
>> fails for "202" to find the year ?
> 
> Unfortunately, this is not so simple.
> 
> Some example model numbers:
> 
> 15Z960-A.AA75U1
> 15Z980-R.AAS9U1
> 14T990-U.AAS8U1
> 17Z90P-K.AAB8U1
> 
> First two digits represent screen size. Third letter device type. Fifth 
> digit is the last digit of the model year (up to 2021, where it is 0 and 
> the sixth letter indicates the model year).

Ok, then I'm just going to trust that you as the maintainer of the lg-laptop
driver know best and take the patch with the DMI string parsing left as
is (as you did in your new version).

>> p.s.
>>
>> While reviewing this I also took a quick look at the existing lg-laptop.c
>> and the wmi_keymap stood out to me, specifically:
>>
>>         {KE_KEY, 0x74, {KEY_F13} },      /* Touchpad toggle (F5) */
>>
>> If that key just sends this event and does not actually change the
>> touchpad settings, IOW userspace is supposed to react this (e.g.
>> filter out touchpad events in software after the toggle), then the
>> correct key to send here would be KEY_F21, this has been the standard
>> key-code to send for this for a while now and GNOME and KDE will
>> automatically do the right thing when sending that, including a
>> nice on-screen-display (OSD)notifcation (like when changing the volume)
>> indicating the new (software) state (on or off) of the touchpad.
>>
>> If the hw does actually handle the touchpad on/off itself
>> (I see there also is a touchpad-led?) then the right thing to do
>> would be to send f22 (Touchpad toggle off-to-on) and f23
>> (Touchpad toggle on-to-off). This assumes that you can figure
>> out the new touchpad state. When receiving f22 / f23 GNOME will
>> display the OSD without making any other settings changes.
>>
>> Also see: /lib/udev/hwdb.d/60-keyboard.hwdb
>>
>>
>>         {KE_KEY, 0x10000000, {KEY_F16} },/* Keyboard backlight (F8) - pressing
>>                                           * this key both sends an event and
>>                                           * changes backlight level.
>>                                           */
>>
>> If this hotkey changes the kbd-backlight level "in hardware"
>> then it should not send a key-press instead you should specify
>>
>> led_classdev.flags = LED_BRIGHT_HW_CHANGED
>>
>> For the kbd-backlight led_classdev and then call:
>>
>> 	led_classdev_notify_brightness_hw_changed(&kbd_backlight, new_backlight_level);
>>
>> When receiving the event. upower will pick the event send by this up
>> and then notify interested parties such as e.g. gnome-settings-daemon
>> which will then show a nice OSD with the new backlight level similar
>> to how it is done for e.g. volume controls.
>>
>>
>> If you can also send patches to change these 2 things, so that lg-laptop
>> conforms with the standard userspace APIs used for this that would be great.
> 
> I sent patches for this (in a separate thread).

Great, thank you.

Regards,

Hans