| Message ID | 20240620082223.20178-2-dev@doubly.so (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86: asus-wmi: support a couple Zenbook 2023 features | expand |
On Thu, 20 Jun 2024, at 8:22 PM, Devin Bayer wrote: > Support the LED on F10 above the crossed out camera icon. > > Signed-off-by: Devin Bayer <dev@doubly.so> > --- > drivers/platform/x86/asus-wmi.c | 36 ++++++++++++++++++++++ > include/linux/platform_data/x86/asus-wmi.h | 2 ++ > 2 files changed, 38 insertions(+) > > diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c > index 3f9b6285c9a6..5585f15e7920 100644 > --- a/drivers/platform/x86/asus-wmi.c > +++ b/drivers/platform/x86/asus-wmi.c > @@ -73,6 +73,7 @@ module_param(fnlock_default, bool, 0444); > #define NOTIFY_LID_FLIP_ROG 0xbd > > #define ASUS_WMI_FNLOCK_BIOS_DISABLED BIT(0) > +#define ASUS_WMI_DEVID_CAMERA_LED 0x00060079 > > #define ASUS_MID_FAN_DESC "mid_fan" > #define ASUS_GPU_FAN_DESC "gpu_fan" > @@ -238,6 +239,7 @@ struct asus_wmi { > struct led_classdev lightbar_led; > int lightbar_led_wk; > struct led_classdev micmute_led; > + struct led_classdev camera_led; > struct workqueue_struct *led_workqueue; > struct work_struct tpd_led_work; > struct work_struct wlan_led_work; > @@ -1642,6 +1644,27 @@ static int micmute_led_set(struct led_classdev *led_cdev, > return err < 0 ? err : 0; > } > > +static enum led_brightness camera_led_get(struct led_classdev *led_cdev) > +{ > + struct asus_wmi *asus; > + u32 result; > + > + asus = container_of(led_cdev, struct asus_wmi, camera_led); > + asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_CAMERA_LED, &result); > + > + return result & ASUS_WMI_DSTS_BRIGHTNESS_MASK; > +} > + > +static int camera_led_set(struct led_classdev *led_cdev, > + enum led_brightness brightness) > +{ > + int state = brightness != LED_OFF; > + int err; > + > + err = asus_wmi_set_devstate(ASUS_WMI_DEVID_CAMERA_LED, state, NULL); > + return err < 0 ? err : 0; > +} > + > static void asus_wmi_led_exit(struct asus_wmi *asus) > { > led_classdev_unregister(&asus->kbd_led); > @@ -1649,6 +1672,7 @@ static void asus_wmi_led_exit(struct asus_wmi *asus) > led_classdev_unregister(&asus->wlan_led); > led_classdev_unregister(&asus->lightbar_led); > led_classdev_unregister(&asus->micmute_led); > + led_classdev_unregister(&asus->camera_led); > > if (asus->led_workqueue) > destroy_workqueue(asus->led_workqueue); > @@ -1740,6 +1764,18 @@ static int asus_wmi_led_init(struct asus_wmi *asus) > goto error; > } > > + if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_CAMERA_LED)) { > + asus->camera_led.name = "platform::camera"; What do other devices label their camera LED as? The one I could find appears to use `<vendor>::camera`. So maybe `asus::camera` would be better? This also keeps in line with `asus::kbd_backlight`. > + asus->camera_led.max_brightness = 1; > + asus->camera_led.brightness_get = camera_led_get; > + asus->camera_led.brightness_set_blocking = camera_led_set; > + > + rv = led_classdev_register(&asus->platform_device->dev, > + &asus->camera_led); > + if (rv) > + goto error; > + } > + > error: > if (rv) > asus_wmi_led_exit(asus); > diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h > index 3eb5cd6773ad..b3c35e33f1e7 100644 > --- a/include/linux/platform_data/x86/asus-wmi.h > +++ b/include/linux/platform_data/x86/asus-wmi.h > @@ -50,6 +50,8 @@ > #define ASUS_WMI_DEVID_LED5 0x00020015 > #define ASUS_WMI_DEVID_LED6 0x00020016 > #define ASUS_WMI_DEVID_MICMUTE_LED 0x00040017 > +#define ASUS_WMI_DEVID_CAMERA_LED_NEG 0x00060078 > +#define ASUS_WMI_DEVID_CAMERA_LED 0x00060079 > > /* Backlight and Brightness */ > #define ASUS_WMI_DEVID_ALS_ENABLE 0x00050001 /* Ambient Light Sensor */ > -- > 2.45.2 > I'll defer final review to Hans and Ilpo to be sure I've not missed anything, otherwise it LGTM pending the one comment above.
Thanks for the review, Luke. On 20/06/2024 23.40, Luke Jones wrote: >> >> + if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_CAMERA_LED)) { >> + asus->camera_led.name = "platform::camera"; > > What do other devices label their camera LED as? The one I could find appears to use `<vendor>::camera`. So maybe `asus::camera` would be better? This also keeps in line with `asus::kbd_backlight`. I reasoned it would be better to keep the name generic is so out of the box desktops could toggle the camera and the LED when KEY_CAMERA is pressed, just like with micmute and mute. But I'll submit a new version with just this patch and the name change. ~ Dev
On Fri, 21 Jun 2024, at 7:50 PM, Devin Bayer wrote: > > Thanks for the review, Luke. > > On 20/06/2024 23.40, Luke Jones wrote: > >> > >> + if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_CAMERA_LED)) { > >> + asus->camera_led.name = "platform::camera"; > > > > What do other devices label their camera LED as? The one I could find appears to use `<vendor>::camera`. So maybe `asus::camera` would be better? This also keeps in line with `asus::kbd_backlight`. > > I reasoned it would be better to keep the name generic is so out of the > box desktops could toggle the camera and the LED when KEY_CAMERA is > pressed, just like with micmute and mute. This might be true if one relies solely on the filesystem path, which in any case is a bad move and likely to cause the moon to drift away from earth eventually. Most Linux software will use the udev libraries available to filter devices according to any amount of criteria (and if they are not they *really should* - udev is pretty powerful and freeing. I've tried finding prior art again and there's just not a lot to go on. ".name = "platform" shows very little except a few micmute labels. "::cam" gets one entry. So my guess is this is still a very new thing or it's not important enough to be used.. In any case looking at the rest of the possible LED entries, mostly those following keyboard, the last part of the name being sensible is what counts the most (e.g "scrolllock", "camera"). This might be setting a precedent, and if so I'd be happy with "::camera" in the LED class conveying expectations well enough.
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c index 3f9b6285c9a6..5585f15e7920 100644 --- a/drivers/platform/x86/asus-wmi.c +++ b/drivers/platform/x86/asus-wmi.c @@ -73,6 +73,7 @@ module_param(fnlock_default, bool, 0444); #define NOTIFY_LID_FLIP_ROG 0xbd #define ASUS_WMI_FNLOCK_BIOS_DISABLED BIT(0) +#define ASUS_WMI_DEVID_CAMERA_LED 0x00060079 #define ASUS_MID_FAN_DESC "mid_fan" #define ASUS_GPU_FAN_DESC "gpu_fan" @@ -238,6 +239,7 @@ struct asus_wmi { struct led_classdev lightbar_led; int lightbar_led_wk; struct led_classdev micmute_led; + struct led_classdev camera_led; struct workqueue_struct *led_workqueue; struct work_struct tpd_led_work; struct work_struct wlan_led_work; @@ -1642,6 +1644,27 @@ static int micmute_led_set(struct led_classdev *led_cdev, return err < 0 ? err : 0; } +static enum led_brightness camera_led_get(struct led_classdev *led_cdev) +{ + struct asus_wmi *asus; + u32 result; + + asus = container_of(led_cdev, struct asus_wmi, camera_led); + asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_CAMERA_LED, &result); + + return result & ASUS_WMI_DSTS_BRIGHTNESS_MASK; +} + +static int camera_led_set(struct led_classdev *led_cdev, + enum led_brightness brightness) +{ + int state = brightness != LED_OFF; + int err; + + err = asus_wmi_set_devstate(ASUS_WMI_DEVID_CAMERA_LED, state, NULL); + return err < 0 ? err : 0; +} + static void asus_wmi_led_exit(struct asus_wmi *asus) { led_classdev_unregister(&asus->kbd_led); @@ -1649,6 +1672,7 @@ static void asus_wmi_led_exit(struct asus_wmi *asus) led_classdev_unregister(&asus->wlan_led); led_classdev_unregister(&asus->lightbar_led); led_classdev_unregister(&asus->micmute_led); + led_classdev_unregister(&asus->camera_led); if (asus->led_workqueue) destroy_workqueue(asus->led_workqueue); @@ -1740,6 +1764,18 @@ static int asus_wmi_led_init(struct asus_wmi *asus) goto error; } + if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_CAMERA_LED)) { + asus->camera_led.name = "platform::camera"; + asus->camera_led.max_brightness = 1; + asus->camera_led.brightness_get = camera_led_get; + asus->camera_led.brightness_set_blocking = camera_led_set; + + rv = led_classdev_register(&asus->platform_device->dev, + &asus->camera_led); + if (rv) + goto error; + } + error: if (rv) asus_wmi_led_exit(asus); diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h index 3eb5cd6773ad..b3c35e33f1e7 100644 --- a/include/linux/platform_data/x86/asus-wmi.h +++ b/include/linux/platform_data/x86/asus-wmi.h @@ -50,6 +50,8 @@ #define ASUS_WMI_DEVID_LED5 0x00020015 #define ASUS_WMI_DEVID_LED6 0x00020016 #define ASUS_WMI_DEVID_MICMUTE_LED 0x00040017 +#define ASUS_WMI_DEVID_CAMERA_LED_NEG 0x00060078 +#define ASUS_WMI_DEVID_CAMERA_LED 0x00060079 /* Backlight and Brightness */ #define ASUS_WMI_DEVID_ALS_ENABLE 0x00050001 /* Ambient Light Sensor */
Support the LED on F10 above the crossed out camera icon. Signed-off-by: Devin Bayer <dev@doubly.so> --- drivers/platform/x86/asus-wmi.c | 36 ++++++++++++++++++++++ include/linux/platform_data/x86/asus-wmi.h | 2 ++ 2 files changed, 38 insertions(+)