[PATCH v2 0/7] Add camera access keys, IdeaPad driver improvements

[PATCH v2 0/7] Add camera access keys, IdeaPad driver improvements

[Eray Orçunus]

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
-- 
2.34.1

[PATCH v2 1/7] Revert "platform/x86: ideapad-laptop: check for touchpad support in _CFG"

[Eray Orçunus]

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>
---
 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;
 
-- 
2.34.1

Re: [PATCH v2 1/7] Revert "platform/x86: ideapad-laptop: check for touchpad support in _CFG"

[Hans de Goede]

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;
>  

[PATCH v2 2/7] HID: add mapping for camera access keys

[Eray Orçunus]

HUTRR72 added 3 new usage codes for keys that are supposed to enable,
disable and toggle camera access. These are useful, considering many
laptops today have key(s) for toggling access to camera.

This patch adds new key definitions for KEY_CAMERA_ACCESS_ENABLE,
KEY_CAMERA_ACCESS_DISABLE and KEY_CAMERA_ACCESS_TOGGLE. Additionally
hid-debug is adjusted to recognize this new usage codes as well.

Signed-off-by: Eray Orçunus <erayorcunus@gmail.com>
Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-debug.c                | 3 +++
 drivers/hid/hid-input.c                | 3 +++
 include/uapi/linux/input-event-codes.h | 3 +++
 3 files changed, 9 insertions(+)

diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index f48d3534e020..991f880fdbd4 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -936,6 +936,9 @@ static const char *keys[KEY_MAX + 1] = {
 	[KEY_ASSISTANT] = "Assistant",
 	[KEY_KBD_LAYOUT_NEXT] = "KbdLayoutNext",
 	[KEY_EMOJI_PICKER] = "EmojiPicker",
+	[KEY_CAMERA_ACCESS_ENABLE] = "CameraAccessEnable",
+	[KEY_CAMERA_ACCESS_DISABLE] = "CameraAccessDisable",
+	[KEY_CAMERA_ACCESS_TOGGLE] = "CameraAccessToggle",
 	[KEY_DICTATE] = "Dictate",
 	[KEY_BRIGHTNESS_MIN] = "BrightnessMin",
 	[KEY_BRIGHTNESS_MAX] = "BrightnessMax",
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index f197aed6444a..f8e6513e77b8 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -995,6 +995,9 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
 		case 0x0cd: map_key_clear(KEY_PLAYPAUSE);	break;
 		case 0x0cf: map_key_clear(KEY_VOICECOMMAND);	break;
 
+		case 0x0d5: map_key_clear(KEY_CAMERA_ACCESS_ENABLE);		break;
+		case 0x0d6: map_key_clear(KEY_CAMERA_ACCESS_DISABLE);		break;
+		case 0x0d7: map_key_clear(KEY_CAMERA_ACCESS_TOGGLE);		break;
 		case 0x0d8: map_key_clear(KEY_DICTATE);		break;
 		case 0x0d9: map_key_clear(KEY_EMOJI_PICKER);	break;
 
diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
index 7989d9483ea7..ef392d0f943f 100644
--- a/include/uapi/linux/input-event-codes.h
+++ b/include/uapi/linux/input-event-codes.h
@@ -614,6 +614,9 @@
 #define KEY_KBD_LAYOUT_NEXT	0x248	/* AC Next Keyboard Layout Select */
 #define KEY_EMOJI_PICKER	0x249	/* Show/hide emoji picker (HUTRR101) */
 #define KEY_DICTATE		0x24a	/* Start or Stop Voice Dictation Session (HUTRR99) */
+#define KEY_CAMERA_ACCESS_ENABLE	0x24b	/* Enables programmatic access to camera devices. (HUTRR72) */
+#define KEY_CAMERA_ACCESS_DISABLE	0x24c	/* Disables programmatic access to camera devices. (HUTRR72) */
+#define KEY_CAMERA_ACCESS_TOGGLE	0x24d	/* Toggles the current state of the camera access control. (HUTRR72) */
 
 #define KEY_BRIGHTNESS_MIN		0x250	/* Set Brightness to Minimum */
 #define KEY_BRIGHTNESS_MAX		0x251	/* Set Brightness to Maximum */
-- 
2.34.1

Re: [PATCH v2 2/7] HID: add mapping for camera access keys

[Jiri Kosina]

On Sat, 29 Oct 2022, Eray Orçunus wrote:

> HUTRR72 added 3 new usage codes for keys that are supposed to enable,
> disable and toggle camera access. These are useful, considering many
> laptops today have key(s) for toggling access to camera.
> 
> This patch adds new key definitions for KEY_CAMERA_ACCESS_ENABLE,
> KEY_CAMERA_ACCESS_DISABLE and KEY_CAMERA_ACCESS_TOGGLE. Additionally
> hid-debug is adjusted to recognize this new usage codes as well.
> 
> Signed-off-by: Eray Orçunus <erayorcunus@gmail.com>
> Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Acked-by: Jiri Kosina <jkosina@suse.cz>

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v2 2/7] HID: add mapping for camera access keys

[Hans de Goede]

Hi Dmitry,

On 10/29/22 14:03, Eray Orçunus wrote:
> HUTRR72 added 3 new usage codes for keys that are supposed to enable,
> disable and toggle camera access. These are useful, considering many
> laptops today have key(s) for toggling access to camera.
> 
> This patch adds new key definitions for KEY_CAMERA_ACCESS_ENABLE,
> KEY_CAMERA_ACCESS_DISABLE and KEY_CAMERA_ACCESS_TOGGLE. Additionally
> hid-debug is adjusted to recognize this new usage codes as well.
> 
> Signed-off-by: Eray Orçunus <erayorcunus@gmail.com>
> Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

I have rejected the drivers/platform/x86 patch which depends
on this, because it changes current behavior, potentially
breaking userspace.

Since this means I won't be taking any patches depending on
this I believe it is best if this is merged through the input tree.

Note this also has a:

Acked-by: Jiri Kosina <jkosina@suse.cz>

tag given in this email thread.

Regards,

Hans





> ---
>  drivers/hid/hid-debug.c                | 3 +++
>  drivers/hid/hid-input.c                | 3 +++
>  include/uapi/linux/input-event-codes.h | 3 +++
>  3 files changed, 9 insertions(+)
> 
> diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
> index f48d3534e020..991f880fdbd4 100644
> --- a/drivers/hid/hid-debug.c
> +++ b/drivers/hid/hid-debug.c
> @@ -936,6 +936,9 @@ static const char *keys[KEY_MAX + 1] = {
>  	[KEY_ASSISTANT] = "Assistant",
>  	[KEY_KBD_LAYOUT_NEXT] = "KbdLayoutNext",
>  	[KEY_EMOJI_PICKER] = "EmojiPicker",
> +	[KEY_CAMERA_ACCESS_ENABLE] = "CameraAccessEnable",
> +	[KEY_CAMERA_ACCESS_DISABLE] = "CameraAccessDisable",
> +	[KEY_CAMERA_ACCESS_TOGGLE] = "CameraAccessToggle",
>  	[KEY_DICTATE] = "Dictate",
>  	[KEY_BRIGHTNESS_MIN] = "BrightnessMin",
>  	[KEY_BRIGHTNESS_MAX] = "BrightnessMax",
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index f197aed6444a..f8e6513e77b8 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -995,6 +995,9 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
>  		case 0x0cd: map_key_clear(KEY_PLAYPAUSE);	break;
>  		case 0x0cf: map_key_clear(KEY_VOICECOMMAND);	break;
>  
> +		case 0x0d5: map_key_clear(KEY_CAMERA_ACCESS_ENABLE);		break;
> +		case 0x0d6: map_key_clear(KEY_CAMERA_ACCESS_DISABLE);		break;
> +		case 0x0d7: map_key_clear(KEY_CAMERA_ACCESS_TOGGLE);		break;
>  		case 0x0d8: map_key_clear(KEY_DICTATE);		break;
>  		case 0x0d9: map_key_clear(KEY_EMOJI_PICKER);	break;
>  
> diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
> index 7989d9483ea7..ef392d0f943f 100644
> --- a/include/uapi/linux/input-event-codes.h
> +++ b/include/uapi/linux/input-event-codes.h
> @@ -614,6 +614,9 @@
>  #define KEY_KBD_LAYOUT_NEXT	0x248	/* AC Next Keyboard Layout Select */
>  #define KEY_EMOJI_PICKER	0x249	/* Show/hide emoji picker (HUTRR101) */
>  #define KEY_DICTATE		0x24a	/* Start or Stop Voice Dictation Session (HUTRR99) */
> +#define KEY_CAMERA_ACCESS_ENABLE	0x24b	/* Enables programmatic access to camera devices. (HUTRR72) */
> +#define KEY_CAMERA_ACCESS_DISABLE	0x24c	/* Disables programmatic access to camera devices. (HUTRR72) */
> +#define KEY_CAMERA_ACCESS_TOGGLE	0x24d	/* Toggles the current state of the camera access control. (HUTRR72) */
>  
>  #define KEY_BRIGHTNESS_MIN		0x250	/* Set Brightness to Minimum */
>  #define KEY_BRIGHTNESS_MAX		0x251	/* Set Brightness to Maximum */

Re: [PATCH v2 2/7] HID: add mapping for camera access keys

[Dmitry Torokhov]

On Tue, Nov 15, 2022 at 09:33:49PM +0100, Hans de Goede wrote:
> Hi Dmitry,
> 
> On 10/29/22 14:03, Eray Orçunus wrote:
> > HUTRR72 added 3 new usage codes for keys that are supposed to enable,
> > disable and toggle camera access. These are useful, considering many
> > laptops today have key(s) for toggling access to camera.
> > 
> > This patch adds new key definitions for KEY_CAMERA_ACCESS_ENABLE,
> > KEY_CAMERA_ACCESS_DISABLE and KEY_CAMERA_ACCESS_TOGGLE. Additionally
> > hid-debug is adjusted to recognize this new usage codes as well.
> > 
> > Signed-off-by: Eray Orçunus <erayorcunus@gmail.com>
> > Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> I have rejected the drivers/platform/x86 patch which depends
> on this, because it changes current behavior, potentially
> breaking userspace.
> 
> Since this means I won't be taking any patches depending on
> this I believe it is best if this is merged through the input tree.
> 
> Note this also has a:
> 
> Acked-by: Jiri Kosina <jkosina@suse.cz>
> 
> tag given in this email thread.

OK, I picked it up.

Thanks.

-- 
Dmitry

Re: [PATCH v2 2/7] HID: add mapping for camera access keys

[Dmitry Torokhov]

Hi Eray,

On Sat, Oct 29, 2022 at 5:04 AM Eray Orçunus <erayorcunus@gmail.com> wrote:
>
> HUTRR72 added 3 new usage codes for keys that are supposed to enable,
> disable and toggle camera access. These are useful, considering many
> laptops today have key(s) for toggling access to camera.
>
> This patch adds new key definitions for KEY_CAMERA_ACCESS_ENABLE,
> KEY_CAMERA_ACCESS_DISABLE and KEY_CAMERA_ACCESS_TOGGLE. Additionally
> hid-debug is adjusted to recognize this new usage codes as well.
>
> Signed-off-by: Eray Orçunus <erayorcunus@gmail.com>
> Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/hid/hid-debug.c                | 3 +++
>  drivers/hid/hid-input.c                | 3 +++
>  include/uapi/linux/input-event-codes.h | 3 +++
>  3 files changed, 9 insertions(+)
>
> diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
> index f48d3534e020..991f880fdbd4 100644
> --- a/drivers/hid/hid-debug.c
> +++ b/drivers/hid/hid-debug.c
> @@ -936,6 +936,9 @@ static const char *keys[KEY_MAX + 1] = {
>         [KEY_ASSISTANT] = "Assistant",
>         [KEY_KBD_LAYOUT_NEXT] = "KbdLayoutNext",
>         [KEY_EMOJI_PICKER] = "EmojiPicker",
> +       [KEY_CAMERA_ACCESS_ENABLE] = "CameraAccessEnable",
> +       [KEY_CAMERA_ACCESS_DISABLE] = "CameraAccessDisable",
> +       [KEY_CAMERA_ACCESS_TOGGLE] = "CameraAccessToggle",
>         [KEY_DICTATE] = "Dictate",
>         [KEY_BRIGHTNESS_MIN] = "BrightnessMin",
>         [KEY_BRIGHTNESS_MAX] = "BrightnessMax",
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index f197aed6444a..f8e6513e77b8 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -995,6 +995,9 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
>                 case 0x0cd: map_key_clear(KEY_PLAYPAUSE);       break;
>                 case 0x0cf: map_key_clear(KEY_VOICECOMMAND);    break;
>
> +               case 0x0d5: map_key_clear(KEY_CAMERA_ACCESS_ENABLE);            break;
> +               case 0x0d6: map_key_clear(KEY_CAMERA_ACCESS_DISABLE);           break;
> +               case 0x0d7: map_key_clear(KEY_CAMERA_ACCESS_TOGGLE);            break;

I was looking at the HID Usages table spec and it came to my attention
that HUTRR72 actually established 0x76, 0x77 and 0x78 as the usages for
then new camera access controls:

https://www.usb.org/sites/default/files/hutrr72_-_usages_to_control_camera_access_0.pdf

Where did 0xd5, 0xd6 and 0xd7 came from?

It looks like we need to update the hid-input mappings as the are
clashing with game recording controls from HUTRR64:

https://www.usb.org/sites/default/files/hutrr64b_-_game_recording_controllers_0.pdf

Thanks.

-- 
Dmitry

[PATCH v2 3/7] platform/x86: ideapad-laptop: Report KEY_CAMERA_ACCESS_TOGGLE instead of KEY_CAMERA

[Eray Orçunus]

Reporting KEY_CAMERA when pressing camera switch key is wrong, since
KEY_CAMERA is supposed to be used for taking snapshot. Change it with
KEY_CAMERA_ACCESS_TOGGLE, so user-space can act correctly.

This patch needs KEY_CAMERA_ACCESS_TOGGLE to be defined, thus depends on
"HID: add mapping for camera access keys" patch.

Signed-off-by: Eray Orçunus <erayorcunus@gmail.com>
---
 drivers/platform/x86/ideapad-laptop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index b67bac457a7a..0ef40b88b240 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -1038,7 +1038,7 @@ static void ideapad_sysfs_exit(struct ideapad_private *priv)
  */
 static const struct key_entry ideapad_keymap[] = {
 	{ KE_KEY,   6, { KEY_SWITCHVIDEOMODE } },
-	{ KE_KEY,   7, { KEY_CAMERA } },
+	{ KE_KEY,   7, { KEY_CAMERA_ACCESS_TOGGLE } },
 	{ KE_KEY,   8, { KEY_MICMUTE } },
 	{ KE_KEY,  11, { KEY_F16 } },
 	{ KE_KEY,  13, { KEY_WLAN } },
-- 
2.34.1

Re: [PATCH v2 3/7] platform/x86: ideapad-laptop: Report KEY_CAMERA_ACCESS_TOGGLE instead of KEY_CAMERA

[Hans de Goede]

Hi Eray,

On 10/29/22 14:03, Eray Orçunus wrote:
> Reporting KEY_CAMERA when pressing camera switch key is wrong, since
> KEY_CAMERA is supposed to be used for taking snapshot. Change it with
> KEY_CAMERA_ACCESS_TOGGLE, so user-space can act correctly.

I'm afraid that we cannot just go and change the emitted key-code,
users might already have setup key-bindings to the existing
KEY_CAMERA code. Also key-codes >= 240 (decimal!) don't work in
X11 based desktop-environments. And even under Wayland they still
have issues, see e.g.:

https://gitlab.gnome.org/GNOME/gtk/-/issues/4353

So I'm afraid that I cannot take this patch.

Note that since a sparse_keymap is used, you can always override
this on your own systems by using a /etc/udev/hwdb.d/61-keyboard.hwdb
file, see: /lib/udev/hwdb.d/60-keyboard.hwdb for the syntax to
map a (sparse) scancode to the KEY_FOO code of your choice.

Regards,

Hans






> 
> This patch needs KEY_CAMERA_ACCESS_TOGGLE to be defined, thus depends on
> "HID: add mapping for camera access keys" patch.
> 
> Signed-off-by: Eray Orçunus <erayorcunus@gmail.com>
> ---
>  drivers/platform/x86/ideapad-laptop.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index b67bac457a7a..0ef40b88b240 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -1038,7 +1038,7 @@ static void ideapad_sysfs_exit(struct ideapad_private *priv)
>   */
>  static const struct key_entry ideapad_keymap[] = {
>  	{ KE_KEY,   6, { KEY_SWITCHVIDEOMODE } },
> -	{ KE_KEY,   7, { KEY_CAMERA } },
> +	{ KE_KEY,   7, { KEY_CAMERA_ACCESS_TOGGLE } },
>  	{ KE_KEY,   8, { KEY_MICMUTE } },
>  	{ KE_KEY,  11, { KEY_F16 } },
>  	{ KE_KEY,  13, { KEY_WLAN } },

[PATCH v2 4/7] platform/x86: ideapad-laptop: Add new _CFG bit numbers for future use

[Eray Orçunus]

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>
---
 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:
-- 
2.34.1

Re: [PATCH v2 4/7] platform/x86: ideapad-laptop: Add new _CFG bit numbers for future use

[Hans de Goede]

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:

[PATCH v2 5/7] platform/x86: ideapad-laptop: Expose camera_power only if supported

[Eray Orçunus]

IdeaPads dropped support for VPCCMD_W_CAMERA somewhere between 2014-2016,
none of the IdeaPads produced after that I tested supports it. Fortunately
I found a way to check it; if the DSDT has camera device(s) defined, it
shouldn't have working VPCCMD_W_CAMERA, thus camera_power shouldn't be
exposed to sysfs. To accomplish this, walk the ACPI namespace in
ideapad_check_features and check the devices starting with "CAM".
Tested on 520-15IKB and Legion Y520, which successfully didn't expose
the camera_power attribute.

Link: https://www.spinics.net/lists/platform-driver-x86/msg26147.html
Signed-off-by: Eray Orçunus <erayorcunus@gmail.com>
---
 drivers/platform/x86/ideapad-laptop.c | 52 ++++++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index f3d4f2beda07..e8c088e7a53d 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 cam_ctrl_via_ec      : 1;
 		bool touchpad_ctrl_via_ec : 1;
 		bool usb_charging         : 1;
 	} features;
@@ -163,6 +164,24 @@ static bool no_bt_rfkill;
 module_param(no_bt_rfkill, bool, 0444);
 MODULE_PARM_DESC(no_bt_rfkill, "No rfkill for bluetooth.");
 
+static acpi_status acpi_find_device_callback(acpi_handle handle, u32 level,
+					     void *context, void **return_value)
+{
+	struct acpi_buffer ret_buf;
+	char buffer[8];
+
+	ret_buf.length = sizeof(buffer);
+	ret_buf.pointer = buffer;
+
+	if (ACPI_SUCCESS(acpi_get_name(handle, ACPI_SINGLE_NAME, &ret_buf)))
+		if (strstarts(ret_buf.pointer, context)) {
+			*return_value = handle;
+			return AE_CTRL_TERMINATE;
+		}
+
+	return AE_OK;
+}
+
 /*
  * ACPI Helpers
  */
@@ -675,7 +694,7 @@ static umode_t ideapad_is_visible(struct kobject *kobj,
 	bool supported = true;
 
 	if (attr == &dev_attr_camera_power.attr)
-		supported = test_bit(CFG_CAP_CAM_BIT, &priv->cfg);
+		supported = priv->features.cam_ctrl_via_ec;
 	else if (attr == &dev_attr_conservation_mode.attr)
 		supported = priv->features.conservation_mode;
 	else if (attr == &dev_attr_fan_mode.attr)
@@ -1527,6 +1546,37 @@ static void ideapad_check_features(struct ideapad_private *priv)
 
 	priv->features.hw_rfkill_switch = dmi_check_system(hw_rfkill_list);
 
+	/*
+	 * Some IdeaPads have camera switch via EC (mostly older ones),
+	 * some don't. Fortunately we know that if DSDT contains device
+	 * object for the camera, camera isn't switchable via EC.
+	 * So, let's walk the namespace and try to find CAM* object.
+	 * If we can't find it, set cam_ctrl_via_ec to true.
+	 */
+
+	priv->features.cam_ctrl_via_ec = false;
+
+	if (test_bit(CFG_CAP_CAM_BIT, &priv->cfg)) {
+		acpi_handle temp_handle = NULL;
+		acpi_handle pci_handle;
+		acpi_status status;
+
+		status = acpi_get_handle(handle, "^^^", &pci_handle);
+		if (ACPI_SUCCESS(status)) {
+			status = acpi_walk_namespace(ACPI_TYPE_DEVICE, pci_handle,
+						     ACPI_UINT32_MAX,
+						     acpi_find_device_callback,
+						     NULL, "CAM",
+						     &temp_handle);
+
+			if (ACPI_SUCCESS(status) && temp_handle == NULL)
+				priv->features.cam_ctrl_via_ec = true;
+
+		} else
+			dev_warn(&priv->platform_device->dev,
+				"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);
 
-- 
2.34.1

Re: [PATCH v2 5/7] platform/x86: ideapad-laptop: Expose camera_power only if supported

[Hans de Goede]

Hi Eray,

On 10/29/22 14:03, Eray Orçunus wrote:
> IdeaPads dropped support for VPCCMD_W_CAMERA somewhere between 2014-2016,
> none of the IdeaPads produced after that I tested supports it. Fortunately
> I found a way to check it; if the DSDT has camera device(s) defined, it
> shouldn't have working VPCCMD_W_CAMERA, thus camera_power shouldn't be
> exposed to sysfs. To accomplish this, walk the ACPI namespace in
> ideapad_check_features and check the devices starting with "CAM".
> Tested on 520-15IKB and Legion Y520, which successfully didn't expose
> the camera_power attribute.
> 
> Link: https://www.spinics.net/lists/platform-driver-x86/msg26147.html
> Signed-off-by: Eray Orçunus <erayorcunus@gmail.com>
> ---
>  drivers/platform/x86/ideapad-laptop.c | 52 ++++++++++++++++++++++++++-
>  1 file changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index f3d4f2beda07..e8c088e7a53d 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 cam_ctrl_via_ec      : 1;
>  		bool touchpad_ctrl_via_ec : 1;
>  		bool usb_charging         : 1;
>  	} features;
> @@ -163,6 +164,24 @@ static bool no_bt_rfkill;
>  module_param(no_bt_rfkill, bool, 0444);
>  MODULE_PARM_DESC(no_bt_rfkill, "No rfkill for bluetooth.");
>  
> +static acpi_status acpi_find_device_callback(acpi_handle handle, u32 level,
> +					     void *context, void **return_value)
> +{
> +	struct acpi_buffer ret_buf;
> +	char buffer[8];
> +
> +	ret_buf.length = sizeof(buffer);
> +	ret_buf.pointer = buffer;
> +
> +	if (ACPI_SUCCESS(acpi_get_name(handle, ACPI_SINGLE_NAME, &ret_buf)))
> +		if (strstarts(ret_buf.pointer, context)) {
> +			*return_value = handle;
> +			return AE_CTRL_TERMINATE;
> +		}
> +
> +	return AE_OK;
> +}
> +
>  /*
>   * ACPI Helpers
>   */
> @@ -675,7 +694,7 @@ static umode_t ideapad_is_visible(struct kobject *kobj,
>  	bool supported = true;
>  
>  	if (attr == &dev_attr_camera_power.attr)
> -		supported = test_bit(CFG_CAP_CAM_BIT, &priv->cfg);
> +		supported = priv->features.cam_ctrl_via_ec;
>  	else if (attr == &dev_attr_conservation_mode.attr)
>  		supported = priv->features.conservation_mode;
>  	else if (attr == &dev_attr_fan_mode.attr)
> @@ -1527,6 +1546,37 @@ static void ideapad_check_features(struct ideapad_private *priv)
>  
>  	priv->features.hw_rfkill_switch = dmi_check_system(hw_rfkill_list);
>  
> +	/*
> +	 * Some IdeaPads have camera switch via EC (mostly older ones),
> +	 * some don't. Fortunately we know that if DSDT contains device
> +	 * object for the camera, camera isn't switchable via EC.
> +	 * So, let's walk the namespace and try to find CAM* object.
> +	 * If we can't find it, set cam_ctrl_via_ec to true.
> +	 */
> +
> +	priv->features.cam_ctrl_via_ec = false;

There is no need to explicitly set this to false since the entire
struct is allocated with kzalloc() and a bunch of other features
flags are also not explicitly set to false. Please drop this line.

> +
> +	if (test_bit(CFG_CAP_CAM_BIT, &priv->cfg)) {
> +		acpi_handle temp_handle = NULL;
> +		acpi_handle pci_handle;
> +		acpi_status status;
> +
> +		status = acpi_get_handle(handle, "^^^", &pci_handle);
> +		if (ACPI_SUCCESS(status)) {
> +			status = acpi_walk_namespace(ACPI_TYPE_DEVICE, pci_handle,
> +						     ACPI_UINT32_MAX,
> +						     acpi_find_device_callback,
> +						     NULL, "CAM",
> +						     &temp_handle);

Why not just use acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, ... ?

The PCI root is usually pretty much the only object under the root anyways
and this way you can avoid the acpi_get_handle() call + its error handling,
so using ACPI_ROOT_OBJECT would lead to a nice cleanup.

> +
> +			if (ACPI_SUCCESS(status) && temp_handle == NULL)
> +				priv->features.cam_ctrl_via_ec = true;
> +
> +		} else
> +			dev_warn(&priv->platform_device->dev,
> +				"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);
>  

Regards,

Hans

Re: [PATCH v2 5/7] platform/x86: ideapad-laptop: Expose camera_power only if supported

[Hans de Goede]

Hi Eray,

On 11/15/22 21:43, Hans de Goede wrote:
> Hi Eray,
> 
> On 10/29/22 14:03, Eray Orçunus wrote:
>> IdeaPads dropped support for VPCCMD_W_CAMERA somewhere between 2014-2016,
>> none of the IdeaPads produced after that I tested supports it. Fortunately
>> I found a way to check it; if the DSDT has camera device(s) defined, it
>> shouldn't have working VPCCMD_W_CAMERA, thus camera_power shouldn't be
>> exposed to sysfs. To accomplish this, walk the ACPI namespace in
>> ideapad_check_features and check the devices starting with "CAM".
>> Tested on 520-15IKB and Legion Y520, which successfully didn't expose
>> the camera_power attribute.
>>
>> Link: https://www.spinics.net/lists/platform-driver-x86/msg26147.html
>> Signed-off-by: Eray Orçunus <erayorcunus@gmail.com>
>> ---
>>  drivers/platform/x86/ideapad-laptop.c | 52 ++++++++++++++++++++++++++-
>>  1 file changed, 51 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
>> index f3d4f2beda07..e8c088e7a53d 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 cam_ctrl_via_ec      : 1;
>>  		bool touchpad_ctrl_via_ec : 1;
>>  		bool usb_charging         : 1;
>>  	} features;
>> @@ -163,6 +164,24 @@ static bool no_bt_rfkill;
>>  module_param(no_bt_rfkill, bool, 0444);
>>  MODULE_PARM_DESC(no_bt_rfkill, "No rfkill for bluetooth.");
>>  
>> +static acpi_status acpi_find_device_callback(acpi_handle handle, u32 level,
>> +					     void *context, void **return_value)
>> +{
>> +	struct acpi_buffer ret_buf;
>> +	char buffer[8];
>> +
>> +	ret_buf.length = sizeof(buffer);
>> +	ret_buf.pointer = buffer;
>> +
>> +	if (ACPI_SUCCESS(acpi_get_name(handle, ACPI_SINGLE_NAME, &ret_buf)))
>> +		if (strstarts(ret_buf.pointer, context)) {
>> +			*return_value = handle;
>> +			return AE_CTRL_TERMINATE;
>> +		}
>> +
>> +	return AE_OK;
>> +}
>> +
>>  /*
>>   * ACPI Helpers
>>   */
>> @@ -675,7 +694,7 @@ static umode_t ideapad_is_visible(struct kobject *kobj,
>>  	bool supported = true;
>>  
>>  	if (attr == &dev_attr_camera_power.attr)
>> -		supported = test_bit(CFG_CAP_CAM_BIT, &priv->cfg);
>> +		supported = priv->features.cam_ctrl_via_ec;
>>  	else if (attr == &dev_attr_conservation_mode.attr)
>>  		supported = priv->features.conservation_mode;
>>  	else if (attr == &dev_attr_fan_mode.attr)
>> @@ -1527,6 +1546,37 @@ static void ideapad_check_features(struct ideapad_private *priv)
>>  
>>  	priv->features.hw_rfkill_switch = dmi_check_system(hw_rfkill_list);
>>  
>> +	/*
>> +	 * Some IdeaPads have camera switch via EC (mostly older ones),
>> +	 * some don't. Fortunately we know that if DSDT contains device
>> +	 * object for the camera, camera isn't switchable via EC.
>> +	 * So, let's walk the namespace and try to find CAM* object.
>> +	 * If we can't find it, set cam_ctrl_via_ec to true.
>> +	 */
>> +
>> +	priv->features.cam_ctrl_via_ec = false;
> 
> There is no need to explicitly set this to false since the entire
> struct is allocated with kzalloc() and a bunch of other features
> flags are also not explicitly set to false. Please drop this line.
> 
>> +
>> +	if (test_bit(CFG_CAP_CAM_BIT, &priv->cfg)) {
>> +		acpi_handle temp_handle = NULL;
>> +		acpi_handle pci_handle;
>> +		acpi_status status;
>> +
>> +		status = acpi_get_handle(handle, "^^^", &pci_handle);
>> +		if (ACPI_SUCCESS(status)) {
>> +			status = acpi_walk_namespace(ACPI_TYPE_DEVICE, pci_handle,
>> +						     ACPI_UINT32_MAX,
>> +						     acpi_find_device_callback,
>> +						     NULL, "CAM",
>> +						     &temp_handle);
> 
> Why not just use acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, ... ?
> 
> The PCI root is usually pretty much the only object under the root anyways
> and this way you can avoid the acpi_get_handle() call + its error handling,
> so using ACPI_ROOT_OBJECT would lead to a nice cleanup.

Note when you send out a new version of this patch + patch 6/7,
please base it on top of my current review-hans branch since
a bunch of other ideapad-laptop changes have landed there.

Regards,

Hans

[PATCH v2 6/7] platform/x86: ideapad-laptop: Keyboard backlight support for more IdeaPads

[Eray Orçunus]

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) {
+
+		status = acpi_acquire_mutex(priv->kbd_bl.lfcm_mutex, NULL,
+					    IDEAPAD_ACPI_MUTEX_TIMEOUT);
+
+		if (ACPI_SUCCESS(status)) {
+			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");
+
+			if (err)
+				return err;
+
+			return !!ret_val;
+		}
+	}
+
+	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;
 }
@@ -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;
+
 	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;
+	}
 
 	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))) {
+				priv->features.kbd_bl = true;
+				priv->features.kbd_bl_partial = true;
+				(void)acpi_get_handle(handle, "^LFCM",
+						      &priv->kbd_bl.lfcm_mutex);
+			}
 
 			if (test_bit(HALS_USB_CHARGING_SUPPORT_BIT, &val))
 				priv->features.usb_charging = true;
-- 
2.34.1

Re: [PATCH v2 6/7] platform/x86: ideapad-laptop: Keyboard backlight support for more IdeaPads

[Hans de Goede]

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

[PATCH v2 7/7] platform/x86: ideapad-laptop: Don't expose touchpad attr on IdeaPads with SYNA2B33

[Eray Orçunus]

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>
---
 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;
-- 
2.34.1

Re: [PATCH v2 7/7] platform/x86: ideapad-laptop: Don't expose touchpad attr on IdeaPads with SYNA2B33

[Hans de Goede]

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;

Re: [PATCH v2 0/7] Add camera access keys, IdeaPad driver improvements

[Ike Panhc]

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>

Re: [PATCH v2 0/7] Add camera access keys, IdeaPad driver improvements

[Eray Orçunus]

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?
And whom should I wait for merge, x86 platform drivers maintainers?
I think that is the only subsystem whose maintainers haven't replied yet.

-eray

Re: [PATCH v2 0/7] Add camera access keys, IdeaPad driver improvements

[Hans de Goede]

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

Re: [PATCH v2 0/7] Add camera access keys, IdeaPad driver improvements

[Eray Orçunus]

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