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

[PATCH 0/6] 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. 

Eray Orçunus (6):
  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

 drivers/hid/hid-debug.c                |   3 +
 drivers/hid/hid-input.c                |   3 +
 drivers/platform/x86/ideapad-laptop.c  | 163 ++++++++++++++++++++++---
 include/uapi/linux/input-event-codes.h |   3 +
 4 files changed, 157 insertions(+), 15 deletions(-)


base-commit: d9db04c1dec6189413701c52b9498a7a56c96445
-- 
2.34.1

[PATCH 1/6] 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

[PATCH 2/6] 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>
---
 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 2/6] HID: add mapping for camera access keys

[Dmitry Torokhov]

On Wed, Oct 26, 2022 at 10:01:02PM +0300, 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>

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

-- 
Dmitry

[PATCH 3/6] 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

[PATCH 4/6] 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

[PATCH 5/6] 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 | 53 ++++++++++++++++++++++++++-
 1 file changed, 52 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index f3d4f2beda07..65eea2e65bbe 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,26 @@ static bool no_bt_rfkill;
 module_param(no_bt_rfkill, bool, 0444);
 MODULE_PARM_DESC(no_bt_rfkill, "No rfkill for bluetooth.");
 
+static char *cam_device_prefix = "CAM";
+
+static acpi_status acpi_find_device_callback(acpi_handle handle, u32 level,
+					     void *context, void **return_value)
+{
+	char buffer[8];
+	struct acpi_buffer ret_buf;
+
+	ret_buf.length = sizeof(buffer);
+	ret_buf.pointer = buffer;
+
+	if (ACPI_SUCCESS(acpi_get_name(handle, ACPI_SINGLE_NAME, &ret_buf)))
+		if (strncmp(ret_buf.pointer, context, strlen(context)) == 0) {
+			*return_value = handle;
+			return AE_CTRL_TERMINATE;
+		}
+
+	return AE_OK;
+}
+
 /*
  * ACPI Helpers
  */
@@ -675,7 +696,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)
@@ -1523,10 +1544,40 @@ static const struct dmi_system_id hw_rfkill_list[] = {
 static void ideapad_check_features(struct ideapad_private *priv)
 {
 	acpi_handle handle = priv->adev->handle;
+	acpi_handle pci_handle;
+	acpi_handle temp_handle = NULL;
 	unsigned long val;
+	acpi_status status;
 
 	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)) {
+		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_device_prefix,
+						     &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 5/6] platform/x86: ideapad-laptop: Expose camera_power only if supported

[Barnabás Pőcze]

Hi


2022. október 26., szerda 21:01 keltezéssel, Eray Orçunus írta:

> 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 | 53 ++++++++++++++++++++++++++-
>  1 file changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index f3d4f2beda07..65eea2e65bbe 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,26 @@ static bool no_bt_rfkill;
>  module_param(no_bt_rfkill, bool, 0444);
>  MODULE_PARM_DESC(no_bt_rfkill, "No rfkill for bluetooth.");
> 
> +static char *cam_device_prefix = "CAM";
> +
> +static acpi_status acpi_find_device_callback(acpi_handle handle, u32 level,
> +					     void *context, void **return_value)
> +{
> +	char buffer[8];
> +	struct acpi_buffer ret_buf;
> +
> +	ret_buf.length = sizeof(buffer);
> +	ret_buf.pointer = buffer;
> +
> +	if (ACPI_SUCCESS(acpi_get_name(handle, ACPI_SINGLE_NAME, &ret_buf)))
> +		if (strncmp(ret_buf.pointer, context, strlen(context)) == 0) {

Please use `strstarts()` here. Is there any reason why you decided not to
simply "inline" the "CAM" string here (or even in the function call)?


> +			*return_value = handle;
> +			return AE_CTRL_TERMINATE;
> +		}
> +
> +	return AE_OK;
> +}
> +
>  /*
>   * ACPI Helpers
>   */
> @@ -675,7 +696,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)
> @@ -1523,10 +1544,40 @@ static const struct dmi_system_id hw_rfkill_list[] = {
>  static void ideapad_check_features(struct ideapad_private *priv)
>  {
>  	acpi_handle handle = priv->adev->handle;
> +	acpi_handle pci_handle;
> +	acpi_handle temp_handle = NULL;
>  	unsigned long val;
> +	acpi_status status;

It is a small thing, but I believe it is best to define these variables
in the block of that `if` since they are not used outside of it.


> 
>  	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)) {
> +		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_device_prefix,
> +						     &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
> 


Regards,
Barnabás Pőcze

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

[Eray Orçunus]

On Thu, 27 Oct 2022 19:43:29 +0000 Barnab=C3=A1s P=C5=91cze <pobrn@protonmail.com> wrote:

> Hi
> 
> 
> 2022. okt=C3=B3ber 26., szerda 21:01 keltez=C3=A9ssel, Eray Or=C3=A7unus =
> =C3=ADrta:
> 
> > IdeaPads dropped support for VPCCMD_W_CAMERA somewhere between 2014-2016,
> > none of the IdeaPads produced after that I tested supports it. Fortunatel=
> y
> > 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.
> >=20
> > Link: https://www.spinics.net/lists/platform-driver-x86/msg26147.html
> > Signed-off-by: Eray Or=C3=A7unus <erayorcunus@gmail.com>
> > ---
> >  drivers/platform/x86/ideapad-laptop.c | 53 ++++++++++++++++++++++++++-
> >  1 file changed, 52 insertions(+), 1 deletion(-)
> >=20
> > diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86=
> /ideapad-laptop.c
> > index f3d4f2beda07..65eea2e65bbe 100644
> > --- a/drivers/platform/x86/ideapad-laptop.c
> > +++ b/drivers/platform/x86/ideapad-laptop.c
> > @@ -149,6 +149,7 @@ struct ideapad_private {
> >  =09=09bool fn_lock              : 1;
> >  =09=09bool hw_rfkill_switch     : 1;
> >  =09=09bool kbd_bl               : 1;
> > +=09=09bool cam_ctrl_via_ec      : 1;
> >  =09=09bool touchpad_ctrl_via_ec : 1;
> >  =09=09bool usb_charging         : 1;
> >  =09} features;
> > @@ -163,6 +164,26 @@ static bool no_bt_rfkill;
> >  module_param(no_bt_rfkill, bool, 0444);
> >  MODULE_PARM_DESC(no_bt_rfkill, "No rfkill for bluetooth.");
> >=20
> > +static char *cam_device_prefix =3D "CAM";
> > +
> > +static acpi_status acpi_find_device_callback(acpi_handle handle, u32 lev=
> el,
> > +=09=09=09=09=09     void *context, void **return_value)
> > +{
> > +=09char buffer[8];
> > +=09struct acpi_buffer ret_buf;
> > +
> > +=09ret_buf.length =3D sizeof(buffer);
> > +=09ret_buf.pointer =3D buffer;
> > +
> > +=09if (ACPI_SUCCESS(acpi_get_name(handle, ACPI_SINGLE_NAME, &ret_buf)))
> > +=09=09if (strncmp(ret_buf.pointer, context, strlen(context)) =3D=3D 0) {
> 
> Please use `strstarts()` here. Is there any reason why you decided not to
> simply "inline" the "CAM" string here (or even in the function call)?

I may use this function to find other devices in future
(thus the name `acpi_find_device_callback`) and I've found a code in the kernel
which use static global initialization like that, so I decided to go for it in here.
But now I will create the "CAM" string inline, and I will also use `strstarts()`
(I didn't know such a function existed), thank you.

> 
> 
> > +=09=09=09*return_value =3D handle;
> > +=09=09=09return AE_CTRL_TERMINATE;
> > +=09=09}
> > +
> > +=09return AE_OK;
> > +}
> > +
> >  /*
> >   * ACPI Helpers
> >   */
> > @@ -675,7 +696,7 @@ static umode_t ideapad_is_visible(struct kobject *kob=
> j,
> >  =09bool supported =3D true;
> >=20
> >  =09if (attr =3D=3D &dev_attr_camera_power.attr)
> > -=09=09supported =3D test_bit(CFG_CAP_CAM_BIT, &priv->cfg);
> > +=09=09supported =3D priv->features.cam_ctrl_via_ec;
> >  =09else if (attr =3D=3D &dev_attr_conservation_mode.attr)
> >  =09=09supported =3D priv->features.conservation_mode;
> >  =09else if (attr =3D=3D &dev_attr_fan_mode.attr)
> > @@ -1523,10 +1544,40 @@ static const struct dmi_system_id hw_rfkill_list[=
> ] =3D {
> >  static void ideapad_check_features(struct ideapad_private *priv)
> >  {
> >  =09acpi_handle handle =3D priv->adev->handle;
> > +=09acpi_handle pci_handle;
> > +=09acpi_handle temp_handle =3D NULL;
> >  =09unsigned long val;
> > +=09acpi_status status;
> 
> It is a small thing, but I believe it is best to define these variables
> in the block of that `if` since they are not used outside of it.

Ok, will do in next revision, thank you.

-eray

[PATCH 6/6] 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 65eea2e65bbe..c30bbe0ca156 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;
 };
 
@@ -1302,19 +1306,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)
@@ -1331,7 +1368,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;
 }
@@ -1351,6 +1389,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;
@@ -1373,17 +1414,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)
@@ -1595,8 +1639,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