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