Re: Future handling of complex RGB devices on Linux v3

[Hans de Goede <hdegoede@redhat.com>]

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.



>>
>>>      - 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.

So I still think that having the miscdevice just for enumeration and
use_leds_uapi purposes is overkill.

Regards,

Hans