Hi Werner,np
Sorry for the late reply.
The parent device can be a hid device, a wmi device, a platform device etc. so I thought it would be nice to have some unified way for identification.
On 2/22/24 2:14 PM, Werner Sembach wrote:
Hi,Ack this sounds good.
Thanks everyone for the exhaustive feedback. And at least this thread is a good comprehesive reference for the future ^^.
To recap the hopefully final UAPI for complex RGB lighting devices:
- By default there is a singular /sys/class/leds/* entry that treats the device as if it was a single zone RGB keyboard backlight with no special effects.
- There is an accompanying misc device with the sysfs attributes "name", "device_type", "firmware_version_string", "serial_number" for device identification and "use_leds_uapi" that defaults to 1.You don't need a char misc device here, you can just make this sysfs attributes on the LED class device's parent device by using device_driver.dev_groups. Please don't use a char misc device just to attach sysfs attributes to it.
Also I'm a bit unsure about most of these attributes, "use_leds_uapi" was discussed before
and makes sense. But at least for HID devices the rest of this info is already available
in sysfs attributes on the HID devices (things like vendor and product id) and since the
userspace code needs per device code to drive the kbd anyways it can also have device
specific code to retrieve all this info, so the other sysfs attributes just feel like
duplicating information. Also there already are a ton of existing hidraw userspace rgbkbd
drivers which already get this info from other places.
Ack
- If set to 0 the /sys/class/leds/* entry disappears. The driver should keep the last state the backlight was in active if possible.Ack, if this finds it way into some documentation (which it should) please make it
- If set 1 it appears again. The driver should bring it back to a static 1 zone setting while avoiding flicker if possible.
clear that this is about the "use_leds_uapi" sysfs attribute.
- If the device is not controllable by for example hidraw, the misc device might also implement additional ioctls or sysfs attributes to allow a more complex low level control for the keyboard backlight. This is will be a highly vendor specific UAPI.IMHO this is the only case where actually using a misc device makes sense, so that
you have a chardev to do the ioctls on. misc-device-s should really only be used
when you need a chardev under /dev . Since you don't need the chardev for the e.g.
hidraw case you should not use a miscdev there IMHO.
Ok, I will see when I start implementing.
- The actual logic interacting with this low level UAPI is implemented by a userspace driverSee above, I don't think we want the misc device for the hidraw case, at which
Implementation wise: For the creation of the misc device with the use_leds_uapi switch a helper function/macro might be useful? Wonder if it should go into leds.h, led-class-multicolor.h, or a new header file?
point I think the helper becomes unnecessary since just a single sysfs write
callback is necessary.
Also for adding new sysfs attributes it is strongly encouraged to use
device_driver.dev_groups so that the device core handled registering /
unregistering the sysfs attributes which fixes a bunch of races; and
using device_driver.dev_groups does not mix well with a helper as you've
suggested.
Thanks for the feedback,
- Out of my head it would look something like this:I think overall the plan sounds good, with my main suggested change
led_classdev_add_optional_misc_control(
struct led_classdev *led_cdev,
char* name,
char* device_type,
char* firmware_version_string,
char* serial_number,
void (*deregister_led)(struct led_classdev *led_cdev),
void (*reregister_led)(struct led_classdev *led_cdev))
Let me know your thoughts and hopefully I can start implementing it soon for one of our devices.
being to not use an unnecessary misc device for the hid-raw case.
Regards,
Hans