platform-driver-x86.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Hans de Goede <hdegoede@redhat.com>
To: Matan Ziv-Av <matan@svgalib.org>
Cc: Platform Driver <platform-driver-x86@vger.kernel.org>
Subject: Re: [PATCH] platform/x86: lg-laptop: Support for battery charge limit on newer models
Date: Wed, 18 Aug 2021 17:31:37 +0200	[thread overview]
Message-ID: <c7b789f9-7679-50e2-e8cf-a7d58103926e@redhat.com> (raw)
In-Reply-To: <d8f5fb50-68d5-b331-3a56-e638e423d269@svgalib.org>

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



      parent reply	other threads:[~2021-08-18 15:31 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-13 22:11 [PATCH] platform/x86: lg-laptop: Support for battery charge limit on newer models Matan Ziv-Av
2021-08-18  7:33 ` Hans de Goede
2021-08-18 13:36   ` Matan Ziv-Av
2021-08-18 15:01     ` Barnabás Pőcze
2021-08-18 15:30       ` Hans de Goede
2021-08-18 15:31     ` Hans de Goede [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c7b789f9-7679-50e2-e8cf-a7d58103926e@redhat.com \
    --to=hdegoede@redhat.com \
    --cc=matan@svgalib.org \
    --cc=platform-driver-x86@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).