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;
Date:   Thu, 24 Mar 2022 10:10:02 -0600
From:   Manuel =?iso-8859-1?Q?Sch=F6nlaub?= <manuel.schoenlaub@gmail.com>
To:     Bastien Nocera <hadess@hadess.net>
Cc:     Filipe =?iso-8859-1?Q?La=EDns?= <lains@riseup.net>,
        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: <YjyX2q1IXeJQ2LGG@hermes>
References: <Yifr4etBFPu1a2Ct@hermes>
 <275245e8048fa124055d9ff3d10ce6562294483a.camel@riseup.net>
 <ce3adf7013ba01aad54fb65bf9c657dd9d0b7d23.camel@hadess.net>
 <YjvlQrvRS+ZKNbZ5@hermes>
 <5b842bd4f8d17e5ecb8e7972637dbe7ad50060ac.camel@hadess.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <5b842bd4f8d17e5ecb8e7972637dbe7ad50060ac.camel@hadess.net>
Precedence: bulk

On Thu, Mar 24, 2022 at 10:32:21AM +0100, Bastien Nocera wrote:
> On Wed, 2022-03-23 at 21:28 -0600, Manuel Sch?nlaub wrote:
> > On Wed, Mar 23, 2022 at 11:24:18PM +0100, Bastien Nocera wrote:
> > > On Wed, 2022-03-23 at 21:22 +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.
> > > 
> > > There's already an API for LEDs in the kernel, why shouldn't it be
> > > used
> > > to avoid user-space needing to know how to configure Logitech, and
> > > every other brand of keyboards?
> > > 
> > > systemd has code to save and restore LED status, as well as code to
> > > change the level of backlight. I can imagine that it wouldn't take
> > > much
> > > to make it aware of RGB LEDs so it handles them properly, whether
> > > it's
> > > for Logitech, or another brand of keyboards, or laptops.
> > 
> > Teaching systemd-backlight about mulicolor backlights might be a nice
> > project
> > too. But their use case seems to be more about screen backlights as
> > it seems.
> > Did I overlook something here?
> 
> From rules.d/99-systemd.rules.in:
> # Pull in backlight save/restore for all backlight devices and
> # keyboard backlights
> SUBSYSTEM=="backlight", TAG+="systemd", IMPORT{builtin}="path_id", ENV{SYSTEMD_WANTS}+="systemd-backlight@backlight:$name.service"
> SUBSYSTEM=="leds", KERNEL=="*kbd_backlight", TAG+="systemd", IMPORT{builtin}="path_id", ENV{SYSTEMD_WANTS}+="systemd-backlight@leds:$name.service"
> 
> And from the NEWS file for systemd 243:
>         * systemd-logind now exposes a per-session SetBrightness() bus call, 
>           which may be used to securely change the brightness of a kernel
>           brightness device, if it belongs to the session's seat. By using this
>           call unprivileged clients can make changes to "backlight" and "leds"
>           devices securely with strict requirements on session membership.
>           Desktop environments may use this to generically make brightness
>           changes to such devices without shipping private SUID binaries or
>           udev rules for that purpose.
> 
> It's clear that it's not just displays.
> 
> > 
> > Oh and yeah, IMHO another argument could be that obviously at some
> > point
> > the LED management could be removed from those user space tools, as
> > the
> > kernel would already know about them.
> > 
> > After all the LED class devices should be there for a reason ;-)
> > 
> > Cheers,
> > 
> > Manuel
> > 
>

Yeah the SetBrightness in systemd / logind should work out of box.

Though I just noticed something: For this to be useful, the default
multi_intensity for each component of the multicolor LED in the kernel should be set to
max_brightness, effectively producing white (on RGB LEDs at least).
If it is zero, like now, SetBrightness would IMHO not actually switch
the LED on because of how the calculation of color components works. 
That way systemd wouldn't need to care about whether the LED is
multicolor or not.

The save/restore stuff IMHO works only for backlights and kbd_backlight LEDs,
as seen from the udev rules. At least out of the box. It even seems to stop
working once you would have two kbd_backlights for the same device
(as the name would be kbd_backlight-1, kbd_backlight-2, ... as per the
documentation on LED classdevs.)

Now there are three solutions:

1) Naming the Logitech LEDs <device>:rgb:kbd_backlight-N even on mice
and at some point send a patch to systemd adapting the udev rules to
care for devices with nuemrical suffixes.

2) Naming the LEDs <device>:rgb:backlight-N and send a patch to systemd
adding a new udev rule to cater for backlight LEDs in general.

3) Like 2 but expect users to write their own udev rules if they want
save/restore for Logitech devices.

What's your opinion on the naming? IMHO mice are not exactly keyboards,
so it probably should be a "general" backlight.
(Instead of "indicator", as in version 1 of the patch)

Cheers