Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Wed, 23 Mar 2022 21:34:44 -0600
From:   Manuel =?iso-8859-1?Q?Sch=F6nlaub?= <manuel.schoenlaub@gmail.com>
To:     Filipe =?iso-8859-1?Q?La=EDns?= <lains@riseup.net>
Cc:     jikos@kernel.org, benjamin.tissoires@redhat.com,
        linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] HID: logitech-hidpp: support Color LED feature (8071).
Message-ID: <Yjvm1F3ZrJnyAOE6@hermes>
References: <Yifr4etBFPu1a2Ct@hermes>
 <275245e8048fa124055d9ff3d10ce6562294483a.camel@riseup.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <275245e8048fa124055d9ff3d10ce6562294483a.camel@riseup.net>
Precedence: bulk

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@gmail.com>
> > ---
> > ?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