On Wed, Mar 23, 2022 at 09:22:49PM +0000, Filipe Laíns wrote: > On Tue, 2022-03-08 at 16:50 -0700, Manuel Schönlaub wrote: > > The HID++ protocol allows to set multicolor (RGB) to a static color. > > Multiple of such LED zones per device are supported. > > This patch exports said LEDs so that they can be set from userspace. > > > > Signed-off-by: Manuel Schönlaub <manuel.schoenlaub@xxxxxxxxx> > > --- > > drivers/hid/hid-logitech-hidpp.c | 188 +++++++++++++++++++++++++++++++ > > 1 file changed, 188 insertions(+) > > *snip* > > Hi Manuel, > > Thanks for putting this forward, although I am not sure if this is the best way > to handle this. > > Before anything, could you elaborate a bit on what lead to you wanting this? > > There are a couple of reasons why merging this in the kernel might be > problematic. > > 1) I don't think we will ever support the full capabilities of the devices, so > configuration via userspace apps will always be required, and here we are > introducing a weird line between the two. > > 2) There is already an ecosystem of userspace configuration apps, with which > this would conflict. They might not be in the best maintenance state due to lack > of time from the maintainers, but moving this functionality to the kernel, which > is harder change, and harder to ship to users, will only make that worse. > > Cheers, > Filipe Laíns Hi Filipe, sure. While I realize that there is e.g. ratbagd which supports a great deal of the HIDPP features and should allow you to control LEDs, unfortunately for my G305 it does not support the LED (and as far as I remember my G403 does not work at all with it). Then I figured that actually having the LEDs in kernel would allow led triggers to work with them, so you could do fancy stuff like showing disk or CPU activity or free physical memory... and here we are now. As for supporting the full capabilities of these devices: The patch just adds RGB leds, which is something already quite standardized in the linux kernel for a variety of devices. Some roccat mice even have support for changing the actual DPI in their kernel driver, which arguably is a whole different story though and not scope of this patch. There are also other features (like on-board profiles) which I would definitely see being better off in user space, especially as long as there is no additional benefit in having them in the kernel. Regarding the conflict in userspace: ratbagd currently seems to always write LED state in RAM and the on-board profiles at the same time, so I would argue that the use case here is different: The user space tools want to set the LED color in a persistent way, while here we want to have interaction with LED triggers and a more transient way. E.g. the driver would overwrite only the transient LED color, not the onboard-profiles. If that is already too much, what about a module option that allows a user to deactivate the feature? Best Regards, Manuel
