From: Matan Ziv-Av <matan@svgalib.org>
To: Hans de Goede <hdegoede@redhat.com>
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 16:36:02 +0300 (IDT) [thread overview]
Message-ID: <d8f5fb50-68d5-b331-3a56-e638e423d269@svgalib.org> (raw)
In-Reply-To: <7d2ea9fc-6942-d7c9-c6cf-61072dc13ba9@redhat.com>
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.
next prev parent reply other threads:[~2021-08-18 13:36 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 [this message]
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
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=d8f5fb50-68d5-b331-3a56-e638e423d269@svgalib.org \
--to=matan@svgalib.org \
--cc=hdegoede@redhat.com \
--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).