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

[Hans de Goede <hdegoede@redhat.com>]

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.