[5/6] platform/x86: ideapad-laptop: Expose camera_power only if supported
Commit Message
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(-)
Comments
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
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
@@ -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);