Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <Yifr4etBFPu1a2Ct@hermes> <275245e8048fa124055d9ff3d10ce6562294483a.camel@riseup.net>
 <Yjvm1F3ZrJnyAOE6@hermes>
In-Reply-To: <Yjvm1F3ZrJnyAOE6@hermes>
From:   Benjamin Tissoires <benjamin.tissoires@redhat.com>
Date:   Thu, 24 Mar 2022 20:54:29 +0100
Message-ID: <CAO-hwJLW=UT6APsKKZaRHBvKn5GOe5xg+bLQH7TGy-PH8N4yUQ@mail.gmail.com>
Subject: Re: [PATCH] HID: logitech-hidpp: support Color LED feature (8071).
To:     =?UTF-8?Q?Manuel_Sch=C3=B6nlaub?= <manuel.schoenlaub@gmail.com>
Cc:     =?UTF-8?Q?Filipe_La=C3=ADns?= <lains@riseup.net>,
        Jiri Kosina <jikos@kernel.org>,
        "open list:HID CORE LAYER" <linux-input@vger.kernel.org>,
        lkml <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Mar 24, 2022 at 4:34 AM Manuel Sch=C3=B6nlaub
<manuel.schoenlaub@gmail.com> wrote:
>
> On Wed, Mar 23, 2022 at 09:22:49PM +0000, Filipe La=C3=ADns wrote:
> > On Tue, 2022-03-08 at 16:50 -0700, Manuel Sch=C3=B6nlaub 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=C3=B6nlaub <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 devi=
ces, so
> > configuration via userspace apps will always be required, and here we a=
re
> > 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 du=
e to lack
> > of time from the maintainers, but moving this functionality to the kern=
el, which
> > is harder change, and harder to ship to users, will only make that wors=
e.
> >
> > Cheers,
> > Filipe La=C3=ADns
>
> 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 tr=
iggers
> to work with them, so you could do fancy stuff like showing disk or CPU a=
ctivity
> 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 kern=
el for
> a variety of devices.
> Some roccat mice even have support for changing the actual DPI in their k=
ernel
> 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 defi=
nitely
> see being better off in user space, especially as long as there is no add=
itional
> benefit in having them in the kernel.
>
> Regarding the conflict in userspace: ratbagd currently seems to always wr=
ite
> 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 interac=
tion 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 use=
r 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
>