Message-ID: <281f9b71-a565-4ff3-8343-ca36d604584d@redhat.com>
Date: Mon, 18 Mar 2024 12:11:39 +0100
From: Hans de Goede <hdegoede@...hat.com>
To: Werner Sembach <wse@...edocomputers.com>
Cc: Lee Jones <lee@...nel.org>, jikos@...nel.org,
 linux-kernel@...r.kernel.org, Jelle van der Waa <jelle@...aa.nl>,
 Miguel Ojeda <miguel.ojeda.sandonis@...il.com>,
 "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
 linux-input@...r.kernel.org, ojeda@...nel.org, linux-leds@...r.kernel.org,
 Pavel Machek <pavel@....cz>, Gregor Riepl <onitake@...il.com>
Subject: Re: Future handling of complex RGB devices on Linux v3

Hi Werner,

Sorry for the late reply.

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.

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

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

> 
>     - The actual logic interacting with this low level UAPI is implemented by a userspace driver
> 
> 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?

See above, I don't think we want the misc device for the hidraw case, at which
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.

> 
> - Out of my head it would look something like this:
> 
> 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.

I think overall the plan sounds good, with my main suggested change
being to not use an unnecessary misc device for the hid-raw case.

Regards,

Hans

Powered by blists - more mailing lists