| Message ID | 20221026190106.28441-6-erayorcunus@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add camera access keys, IdeaPad driver improvements | expand |
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
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);
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(-)