On Thu, Mar 24, 2022 at 08:54:29PM +0100, Benjamin Tissoires wrote: > On Thu, Mar 24, 2022 at 4:34 AM Manuel Schönlaub > <manuel.schoenlaub@xxxxxxxxx> wrote: > > > > 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. > > The one thing that concerns me with those gaming LEDs, is that there > is much more than just color/intensity. > Those LEDs have effects that you can enable (breathing, pulse, color > changing, etc...) and I am not sure how much you are going to be able > to sync with the simple LED class. > Sure. I actually had thought a bit about that and would say that the concept of breathing, pulse etc.. can be modeled quite well with hardware patterns. > > 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? > > Please no. I am tired of having way too many options that nobody uses > except for a couple of people and we can not remove/change them > because of those 2 persons. That's true. I would certainly hate that too. > > Either you manage to sync the LED class state somehow (in a sensible > manner), or I don't think having such LEDs in the kernel is a good > thing because we are going to fight against userspace. I'd like to give it a shot and come up with a follow-up patch series implementing e.g. breathing. Let's see how that turns out. > Cheers, > Benjamin > > > > > Best Regards, > > > > Manuel > > >
