| Message ID | 20221029120311.11152-1-erayorcunus@gmail.com |
|---|---|
| Headers | show |
| Series | Add camera access keys, IdeaPad driver improvements | expand |
Hi Eray, Sorry for the long silence, I have not done any pdx86 patch review the last 2 weeks due to personal circumstances. On 11/9/22 13:58, Eray Orçunus wrote: > On 11/08/22 06:56, Ike Panhc wrote: >> On 10/29/22 20:03, Eray Orçunus wrote: >>> Nowadays many laptops have camera access keys, yet there is no usage codes >>> mapped to them, even though it's introduced in HUTRR72. Start point of >>> this patch series was adding it and making IdeaPads send it to userspace. >>> But later I discovered that camera_power attribute of ideapad-laptop >>> driver on my IdeaPad 520-15IKB doesn't work, so I can't toggle it with >>> that. I managed to find a way to check whether an IdeaPad supports >>> camera_power attribute (which sends VPCCMD_W_CAMERA to EC), don't expose >>> it to sysfs so userspace will know that it can't toggle camera access via >>> camera_power, in my case, after receiving KEY_CAMERA_ACCESS_TOGGLE. >>> >>> Along the way I discovered that old IdeaPads, like S10-3, may not be able >>> to toggle their touchpad as a regression of a commit aimed for newer >>> IdeaPads, so I reverted it. >>> >>> Also I noticed that I can get/set the state of my keyboard light, >>> so one of the patches also adds supports for this kind of keyboard lights, >>> which I call "partially supported keyboard lights". I expect that commit >>> to add keyboard light support for 520-15IKB, 330-17ICH, 5 (15) and more. >>> Currently only tested on 520-15IKB. >> >> Thanks. Also test on my ideapad s410 and it looks good. >> >> Acked-by: Ike Panhc <ike.pan@canonical.com> > > > Thank you :) > > I need some advice since I'm new in here, sadly another patch has been > merged to ideapad-laptop along the way and currently it's not possible to > merge patch #7, does that mean I should send v3 of my patch series? No that is not necessary, I can rework it to apply on top of the other patch. But TBH I think we really need to work on a different solution for the problem with the touchpad issues with ideapad-laptop we cannot just keep adding touchpad and/or DMI ids because the driver is breaking touchpad functionality left and right. I will send out an email after this one to all authors of recent patches which all do "priv->features.touchpad_ctrl_via_ec = 0" in some way. With a request to gather some more info of why exactly this is necessary and to see if we cannot come up with a more generic fix. > And whom should I wait for merge, x86 platform drivers maintainers? I'm the x86 platform drivers maintainer. I believe it makes sense to merge this series through the x86 platform drivers git tree. I need to coordinate the merging of patch 2/7 with wDmitry (the input subsystem maintainer) I'll send him an email about this. After that I can likely merge patches 2-6. For the touchpad patches I would first like to get a better handle on how to fix things more generic. Specifically patch 1/7 will cause priv->features.touchpad_ctrl_via_ec to get set to 1 on more models and since that is causing issues I don't think that is a good idea (even though the patch does make sense) and for 7/7 I hope to come up with something more generic. If you can run the tests from the touchpad mail soon that would really help! Note I do plan to send 7/7 out as a fix for 6.1 if we run out of time wrt coming up with a recent fix. Getting at least some fix out the door is also why I already merged the other patch using the DMI ids. > I think that is the only subsystem whose maintainers haven't replied yet. Correct, but I have replied now :) Regards, Hans
Hi! On 11/9/22 19:38, Hans de Goede wrote: > Hi Eray, > > Sorry for the long silence, I have not done any pdx86 patch review > the last 2 weeks due to personal circumstances. Oh, I wasn't even aware I had to wait for pdx86 review, and Ike Panhc just sent his Acked-By anyway, no problem at all. > On 11/9/22 13:58, Eray Orçunus wrote: > > On 11/08/22 06:56, Ike Panhc wrote: > >> > >> Thanks. Also test on my ideapad s410 and it looks good. > >> > >> Acked-by: Ike Panhc <ike.pan@canonical.com> > > > > > > Thank you :) > > > > I need some advice since I'm new in here, sadly another patch has been > > merged to ideapad-laptop along the way and currently it's not possible to > > merge patch #7, does that mean I should send v3 of my patch series? > > No that is not necessary, I can rework it to apply on top of the other > patch. Oh, that's great, thank you. > For the touchpad patches I would first like to get > a better handle on how to fix things more generic. > > Specifically patch 1/7 will cause priv->features.touchpad_ctrl_via_ec > to get set to 1 on more models and since that is causing issues > I don't think that is a good idea (even though the patch does > make sense) and for 7/7 I hope to come up with something > more generic. > > If you can run the tests from the touchpad mail soon that > would really help! That sounds great! I will try to help as much as I can. And yeah, I couldn't guess patch 1 can cause a regression on some IdeaPads. > > I think that is the only subsystem whose maintainers haven't replied yet. > > Correct, but I have replied now :) Hehe, this reply was very informative, thank you :) Best, Eray
Hi, On 10/29/22 14:03, Eray Orçunus wrote: > Last 8 bit of _CFG started being used in later IdeaPads, thus 30th bit > doesn't always show whether device supports touchpad or touchpad switch. > Remove checking bit 30 of _CFG, so older IdeaPads like S10-3 can switch > touchpad again via touchpad attribute. > > This reverts commit b3ed1b7fe3786c8fe795c16ca07cf3bda67b652f. > > Signed-off-by: Eray Orçunus <erayorcunus@gmail.com> Thank you for your patch, I've applied this patch to my review-hans branch: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans Note it will show up in my review-hans branch once I've pushed my local branch there, which might take a while. Once I've run some tests on this branch the patches there will be added to the platform-drivers-x86/for-next branch and eventually will be included in the pdx86 pull-request to Linus for the next merge-window. Regards, Hans > --- > drivers/platform/x86/ideapad-laptop.c | 14 +++++--------- > 1 file changed, 5 insertions(+), 9 deletions(-) > > diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c > index e7a1299e3776..b67bac457a7a 100644 > --- a/drivers/platform/x86/ideapad-laptop.c > +++ b/drivers/platform/x86/ideapad-laptop.c > @@ -46,11 +46,10 @@ static const char *const ideapad_wmi_fnesc_events[] = { > #endif > > enum { > - CFG_CAP_BT_BIT = 16, > - CFG_CAP_3G_BIT = 17, > - CFG_CAP_WIFI_BIT = 18, > - CFG_CAP_CAM_BIT = 19, > - CFG_CAP_TOUCHPAD_BIT = 30, > + CFG_CAP_BT_BIT = 16, > + CFG_CAP_3G_BIT = 17, > + CFG_CAP_WIFI_BIT = 18, > + CFG_CAP_CAM_BIT = 19, > }; > > enum { > @@ -367,8 +366,6 @@ static int debugfs_cfg_show(struct seq_file *s, void *data) > seq_puts(s, " wifi"); > if (test_bit(CFG_CAP_CAM_BIT, &priv->cfg)) > seq_puts(s, " camera"); > - if (test_bit(CFG_CAP_TOUCHPAD_BIT, &priv->cfg)) > - seq_puts(s, " touchpad"); > seq_puts(s, "\n"); > > seq_puts(s, "Graphics: "); > @@ -661,8 +658,7 @@ static umode_t ideapad_is_visible(struct kobject *kobj, > else if (attr == &dev_attr_fn_lock.attr) > supported = priv->features.fn_lock; > else if (attr == &dev_attr_touchpad.attr) > - supported = priv->features.touchpad_ctrl_via_ec && > - test_bit(CFG_CAP_TOUCHPAD_BIT, &priv->cfg); > + supported = priv->features.touchpad_ctrl_via_ec; > else if (attr == &dev_attr_usb_charging.attr) > supported = priv->features.usb_charging; >
Hi, On 10/29/22 14:03, Eray Orçunus wrote: > Later IdeaPads report various things in last 8 bits of _CFG, at least > 5 of them represent supported on-screen-displays. Add those bit numbers > to the enum, and use CFG_OSD_ as prefix of their names. Also expose > the values of these bits to debugfs, since they can be useful. > > Signed-off-by: Eray Orçunus <erayorcunus@gmail.com> Thank you for your patch, I've applied this patch to my review-hans branch: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans Note it will show up in my review-hans branch once I've pushed my local branch there, which might take a while. Once I've run some tests on this branch the patches there will be added to the platform-drivers-x86/for-next branch and eventually will be included in the pdx86 pull-request to Linus for the next merge-window. Regards, Hans > --- > drivers/platform/x86/ideapad-laptop.c | 33 +++++++++++++++++++++++---- > 1 file changed, 29 insertions(+), 4 deletions(-) > > diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c > index 0ef40b88b240..f3d4f2beda07 100644 > --- a/drivers/platform/x86/ideapad-laptop.c > +++ b/drivers/platform/x86/ideapad-laptop.c > @@ -46,10 +46,22 @@ static const char *const ideapad_wmi_fnesc_events[] = { > #endif > > enum { > - CFG_CAP_BT_BIT = 16, > - CFG_CAP_3G_BIT = 17, > - CFG_CAP_WIFI_BIT = 18, > - CFG_CAP_CAM_BIT = 19, > + CFG_CAP_BT_BIT = 16, > + CFG_CAP_3G_BIT = 17, > + CFG_CAP_WIFI_BIT = 18, > + CFG_CAP_CAM_BIT = 19, > + > + /* > + * These are OnScreenDisplay support bits that can be useful to determine > + * whether a hotkey exists/should show OSD. But they aren't particularly > + * meaningful since they were introduced later, i.e. 2010 IdeaPads > + * don't have these, but they still have had OSD for hotkeys. > + */ > + CFG_OSD_NUMLK_BIT = 27, > + CFG_OSD_CAPSLK_BIT = 28, > + CFG_OSD_MICMUTE_BIT = 29, > + CFG_OSD_TOUCHPAD_BIT = 30, > + CFG_OSD_CAM_BIT = 31, > }; > > enum { > @@ -368,6 +380,19 @@ static int debugfs_cfg_show(struct seq_file *s, void *data) > seq_puts(s, " camera"); > seq_puts(s, "\n"); > > + seq_puts(s, "OSD support:"); > + if (test_bit(CFG_OSD_NUMLK_BIT, &priv->cfg)) > + seq_puts(s, " num-lock"); > + if (test_bit(CFG_OSD_CAPSLK_BIT, &priv->cfg)) > + seq_puts(s, " caps-lock"); > + if (test_bit(CFG_OSD_MICMUTE_BIT, &priv->cfg)) > + seq_puts(s, " mic-mute"); > + if (test_bit(CFG_OSD_TOUCHPAD_BIT, &priv->cfg)) > + seq_puts(s, " touchpad"); > + if (test_bit(CFG_OSD_CAM_BIT, &priv->cfg)) > + seq_puts(s, " camera"); > + seq_puts(s, "\n"); > + > seq_puts(s, "Graphics: "); > switch (priv->cfg & 0x700) { > case 0x100:
Hi Eray, On 10/29/22 14:03, Eray Orçunus wrote: > IdeaPads with HALS_KBD_BL_SUPPORT_BIT have full keyboard light support, > and they send an event via ACPI on light state change. Whereas some > IdeaPads that don't have this bit set, i.e. 520-15ikb, 330-17ich and > 5 (15), don't send an event, yet they still support switching keyboard > light via KBLO object on DSDT. Detect these IdeaPads with searching for > KBLO object, set their kbd_bl_partial to true and register led device > for them. Tested on 520-15ikb. > > Signed-off-by: Eray Orçunus <erayorcunus@gmail.com> > --- > drivers/platform/x86/ideapad-laptop.c | 79 ++++++++++++++++++++++++--- > 1 file changed, 70 insertions(+), 9 deletions(-) > > diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c > index e8c088e7a53d..b34fbc4d741c 100644 > --- a/drivers/platform/x86/ideapad-laptop.c > +++ b/drivers/platform/x86/ideapad-laptop.c > @@ -149,6 +149,7 @@ struct ideapad_private { > bool fn_lock : 1; > bool hw_rfkill_switch : 1; > bool kbd_bl : 1; > + bool kbd_bl_partial : 1; > bool cam_ctrl_via_ec : 1; > bool touchpad_ctrl_via_ec : 1; > bool usb_charging : 1; > @@ -157,6 +158,9 @@ struct ideapad_private { > bool initialized; > struct led_classdev led; > unsigned int last_brightness; > + /* Below are used only if kbd_bl_partial is set */ > + acpi_handle lfcm_mutex; > + acpi_handle kblo_obj; > } kbd_bl; > }; > > @@ -1300,19 +1304,52 @@ static void ideapad_backlight_notify_brightness(struct ideapad_private *priv) > backlight_force_update(priv->blightdev, BACKLIGHT_UPDATE_HOTKEY); > } > > +#define IDEAPAD_ACPI_MUTEX_TIMEOUT 1500 > + > /* > * keyboard backlight > */ > static int ideapad_kbd_bl_brightness_get(struct ideapad_private *priv) > { > - unsigned long hals; > + unsigned long ret_val; > int err; > + acpi_status status; > > - err = eval_hals(priv->adev->handle, &hals); > + /* > + * Some IdeaPads with partially implemented keyboard lights don't give > + * us the light state on HALS_KBD_BL_STATE_BIT in the return value of HALS, > + * i.e. 5 (15) and 330-17ich. Fortunately we know how to gather it. > + * Even if it won't work, we will still give HALS a try, because > + * some IdeaPads with kbd_bl_partial, i.e. 520-15ikb, > + * correctly sets HALS_KBD_BL_STATE_BIT in HALS return value. > + */ > + > + if (priv->features.kbd_bl_partial && > + priv->kbd_bl.lfcm_mutex != NULL && priv->kbd_bl.kblo_obj != NULL) { IMHO it would be better to only set kbd_bl_partial when both handles are not NULL, then you can drop the handle checks here. > + > + status = acpi_acquire_mutex(priv->kbd_bl.lfcm_mutex, NULL, > + IDEAPAD_ACPI_MUTEX_TIMEOUT); > + > + if (ACPI_SUCCESS(status)) { This code now ends up still going through the normal kbd-bl path when it fails to acquire the mutex. Instead it should do: if (ACPI_FAILURE(status)) return -EIO; And then have the rest of the code one indentation level less deep. > + err = eval_int(priv->kbd_bl.kblo_obj, NULL, &ret_val); > + > + status = acpi_release_mutex(priv->kbd_bl.lfcm_mutex, NULL); > + if (ACPI_FAILURE(status)) > + dev_err(&priv->platform_device->dev, > + "Failed to release LFCM mutex"); I'm pretty sure that the ACPI core will already log an error if things fail, I would change this to just a single line: acpi_release_mutex(priv->kbd_bl.lfcm_mutex, NULL); > + > + if (err) > + return err; > + > + return !!ret_val; !!ret_val turns it into a boolean, does that mean it is always either on or off with no level in between ? > + } > + } > + > + err = eval_hals(priv->adev->handle, &ret_val); > if (err) > return err; > > - return !!test_bit(HALS_KBD_BL_STATE_BIT, &hals); > + return !!test_bit(HALS_KBD_BL_STATE_BIT, &ret_val); > } > > static enum led_brightness ideapad_kbd_bl_led_cdev_brightness_get(struct led_classdev *led_cdev) > @@ -1329,7 +1366,8 @@ static int ideapad_kbd_bl_brightness_set(struct ideapad_private *priv, unsigned > if (err) > return err; > > - priv->kbd_bl.last_brightness = brightness; > + if (!priv->features.kbd_bl_partial) > + priv->kbd_bl.last_brightness = brightness; > > return 0; > } I don't understand this change, you change ideapad_kbd_bl_brightness_get() to do an int eval of KBLO, but here you now still do a exec_sals(SALS_KBD_BL_ON / SALS_KBD_BL_OFF) ? Also there is no reason not to update last_brightness here ... > @@ -1349,6 +1387,9 @@ static void ideapad_kbd_bl_notify(struct ideapad_private *priv) > if (!priv->kbd_bl.initialized) > return; > > + if (priv->features.kbd_bl_partial) > + return; > + Why? If we do happen to get a notify on one of these devices and the brightness has changed, then it would be good to let userspace know and if never get a notify then this function won't run so we don't need the if. > brightness = ideapad_kbd_bl_brightness_get(priv); > if (brightness < 0) > return; > @@ -1371,17 +1412,20 @@ static int ideapad_kbd_bl_init(struct ideapad_private *priv) > if (WARN_ON(priv->kbd_bl.initialized)) > return -EEXIST; > > - brightness = ideapad_kbd_bl_brightness_get(priv); > - if (brightness < 0) > - return brightness; > + /* IdeaPads with kbd_bl_partial don't have keyboard backlight event */ > + if (!priv->features.kbd_bl_partial) { > + brightness = ideapad_kbd_bl_brightness_get(priv); > + if (brightness < 0) > + return brightness; > > - priv->kbd_bl.last_brightness = brightness; > + priv->kbd_bl.last_brightness = brightness; > + priv->kbd_bl.led.flags = LED_BRIGHT_HW_CHANGED; > + } Again no need to for the if here. Setting last_brightness and advertising LED_BRIGHT_HW_CHANGED unconditionally won't hurt and not making this difference keeps the code simpler. > > priv->kbd_bl.led.name = "platform::" LED_FUNCTION_KBD_BACKLIGHT; > priv->kbd_bl.led.max_brightness = 1; > priv->kbd_bl.led.brightness_get = ideapad_kbd_bl_led_cdev_brightness_get; > priv->kbd_bl.led.brightness_set_blocking = ideapad_kbd_bl_led_cdev_brightness_set; > - priv->kbd_bl.led.flags = LED_BRIGHT_HW_CHANGED; > > err = led_classdev_register(&priv->platform_device->dev, &priv->kbd_bl.led); > if (err) > @@ -1594,8 +1638,25 @@ static void ideapad_check_features(struct ideapad_private *priv) > if (test_bit(HALS_FNLOCK_SUPPORT_BIT, &val)) > priv->features.fn_lock = true; > > + /* > + * IdeaPads with HALS_KBD_BL_SUPPORT_BIT have full keyboard > + * light support, and they send an event via ACPI on light > + * state change. Whereas some IdeaPads, at least 520-15ikb > + * and 5 (15), don't send an event, yet they still have > + * KBLO object. In this case, set kbd_bl_partial to true > + * and cache the LFCM mutex, it might be useful while > + * getting the brightness. > + */ > + > if (test_bit(HALS_KBD_BL_SUPPORT_BIT, &val)) > priv->features.kbd_bl = true; > + else if (ACPI_SUCCESS(acpi_get_handle(handle, "^KBLO", > + &priv->kbd_bl.kblo_obj))) { As mentioned above I would change this to: else if (ACPI_SUCCESS(acpi_get_handle(handle, "^KBLO", &priv->kbd_bl.kblo_obj)) && ACPI_SUCCESS(acpi_get_handle(handle, "^LFCM", &priv->kbd_bl.lfcm_mutex))) { > + priv->features.kbd_bl = true; > + priv->features.kbd_bl_partial = true; > + (void)acpi_get_handle(handle, "^LFCM", > + &priv->kbd_bl.lfcm_mutex); And then drop the acpi_get_handle() here. > + } > > if (test_bit(HALS_USB_CHARGING_SUPPORT_BIT, &val)) > priv->features.usb_charging = true; Regards, Hans
Hi, On 10/29/22 14:03, Eray Orçunus wrote: > My 520-15IKB (2017) with SYNA2B33 doesn't have working VPCCMD_W_TOUCHPAD command - > it's the touchpad program switches the touchpad instead on Windows. Considering > all IdeaPads with SYNA2B33 touchpad produced in 2017/2018, it's very likely that > none of the IdeaPads with SYNA2B33 support touchpad switching via EC. So let's > add SYNA2B33 to the touchpads not switchable via EC. > > Signed-off-by: Eray Orçunus <erayorcunus@gmail.com> As already discussed in the other thread I'm not sure this is the best way to go about this, lets continue discussing this in the other thread. Regards, Hans > --- > drivers/platform/x86/ideapad-laptop.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c > index b34fbc4d741c..937126c62a14 100644 > --- a/drivers/platform/x86/ideapad-laptop.c > +++ b/drivers/platform/x86/ideapad-laptop.c > @@ -1621,8 +1621,12 @@ static void ideapad_check_features(struct ideapad_private *priv) > "Could not find PCI* node in the namespace\n"); > } > > - /* Most ideapads with ELAN0634 touchpad don't use EC touchpad switch */ > - priv->features.touchpad_ctrl_via_ec = !acpi_dev_present("ELAN0634", NULL, -1); > + /* > + * Most ideapads with ELAN0634 and SYNA2B33 touchpads don't use > + * EC touchpad switch > + */ > + priv->features.touchpad_ctrl_via_ec = !acpi_dev_present("ELAN0634", NULL, -1) && > + !acpi_dev_present("SYNA2B33", NULL, -1); > > if (!read_ec_data(handle, VPCCMD_R_FAN, &val)) > priv->features.fan_mode = true;
Nowadays many laptops have camera access keys, yet there is no usage codes mapped to them, even though it's introduced in HUTRR72. Start point of this patch series was adding it and making IdeaPads send it to userspace. But later I discovered that camera_power attribute of ideapad-laptop driver on my IdeaPad 520-15IKB doesn't work, so I can't toggle it with that. I managed to find a way to check whether an IdeaPad supports camera_power attribute (which sends VPCCMD_W_CAMERA to EC), don't expose it to sysfs so userspace will know that it can't toggle camera access via camera_power, in my case, after receiving KEY_CAMERA_ACCESS_TOGGLE. Along the way I discovered that old IdeaPads, like S10-3, may not be able to toggle their touchpad as a regression of a commit aimed for newer IdeaPads, so I reverted it. Also I noticed that I can get/set the state of my keyboard light, so one of the patches also adds supports for this kind of keyboard lights, which I call "partially supported keyboard lights". I expect that commit to add keyboard light support for 520-15IKB, 330-17ICH, 5 (15) and more. Currently only tested on 520-15IKB. --- Changes in v2: - Added Dmitry Torokhov's Acked-By to patch 2 - Applied Barnabás Pőcze's recommendations to patch 5: - strncmp -> strstarts - static global "CAM" string -> inlined "CAM" string - move new variables to the scope they're used, and order them - Added patch 7, which removes "touchpad" attr for SYNA2B33 Eray Orçunus (7): Revert "platform/x86: ideapad-laptop: check for touchpad support in _CFG" HID: add mapping for camera access keys platform/x86: ideapad-laptop: Report KEY_CAMERA_ACCESS_TOGGLE instead of KEY_CAMERA platform/x86: ideapad-laptop: Add new _CFG bit numbers for future use platform/x86: ideapad-laptop: Expose camera_power only if supported platform/x86: ideapad-laptop: Keyboard backlight support for more IdeaPads platform/x86: ideapad-laptop: Don't expose touchpad attr on IdeaPads with SYNA2B33 drivers/hid/hid-debug.c | 3 + drivers/hid/hid-input.c | 3 + drivers/platform/x86/ideapad-laptop.c | 170 ++++++++++++++++++++++--- include/uapi/linux/input-event-codes.h | 3 + 4 files changed, 162 insertions(+), 17 deletions(-) base-commit: d9db04c1dec6189413701c52b9498a7a56c96445