Hi Hans,
Am 25.03.24 um 15:18 schrieb Hans de Goede:
Hi Werner,
On 3/19/24 4:18 PM, Werner Sembach wrote:
Hi Hans,
Am 18.03.24 um 12:11 schrieb Hans de Goede:
Hi Werner,
Sorry for the late reply.
np
On 2/22/24 2:14 PM, Werner Sembach wrote:
Hi,
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.
Ack this sounds good.
- 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.
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.
I think that some shared ioctl for identifying devices which need a misc-device
makes sense. But for existing hidraw supported keyboards there already is existing
userspace support, which already retreives this in a hidraw specific manner.
And I doubt that the existing userspace projects will add support for a new method
which is only available on new kernels, while they will also need to keep the old
method around to keep things working with older kernels.
So at least for the hidraw case I see little value in exporting the same info
in another way.
Ack on the no new device, but since we are adding use_leds_uapi to the parent
device anyway, having the identification on non-HID devices wie sysfs entries
would help even more in not creating a new device just to handle an ioctl.
Just to note here. When we go the LampArray route, everything is a HID device
anyway.
- If set to 0 the /sys/class/leds/* entry disappears. The driver should keep the last state the backlight was in active if possible.
- If set 1 it appears again. The driver should bring it back to a static 1 zone setting while avoiding flicker if possible.
Ack, if this finds it way into some documentation (which it should) please make it
clear that this is about the "use_leds_uapi" sysfs attribute.
Ack
- 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.
My train of thought was that it would be nice to have the use_leds_uapi at a somewhat unified location in sysfs. The parent device can be of any kind, like I mentioned above, and while for deactivating it can easily be found via /sys/class/leds/*/device/. For reactivating, the leds entry doesn't exist.
That is an interesting point. But I would expect any process/daemon which wants to
reactivate the in kernel LED class support to be the same process as which
deactivated it. So that daemon can simply canonicalize the /sys/class/leds/*/device
symlink or canonicalize the entire /sys/class/leds/*/device/use_leds_uapi path
and store the canonicalized version for when the daemon wants to reactivate things.
Ack
Regards,
Werner
So I still think that having the miscdevice just for enumeration and
use_leds_uapi purposes is overkill.
Regards,
Hans
