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. > > 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. 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. Cheers, Benjamin > > Best Regards, > > Manuel >
