Re: Re: [PATCH] HP: wmi: added support for 4 zone keyboard rgb

[Carlos Ferreira]

Hi, sorry for the (big) delay

> Hi Carlos,
> 
> On 3/24/24 7:05 PM, Carlos Ferreira wrote:
> > Added support for 4 zone keyboard rgb on omen laptops.
> > 
> > Signed-off-by: Carlos Ferreira <carlosmiguelferreira.2003@xxxxxxxxx>
> 
> Thank you for your patch and sorry for being slow with replying to this.
> 
> There actually already was a previous attemp to add support for
> the 4 zone keyboard to hp-wmi by Rishit Bansal:
> 
> https://lore.kernel.org/platform-driver-x86/20230131235027.36304-1-rishitbansal0@xxxxxxxxx/
> 
> As discussed there we really want to define a new standardized
> userspace API for the backlight functionality of these zoned
> RGB keyboards. Using driver specific sysfs attributes for this
> is undesirable, since that will never get wide support in userspace.
> 
> OTOH if we define and document a new standard userspace API for this
> then hopefully standard userspace stacks like KDE and GNOME will
> eventually get support for this and then for the next zoned rgb
> keyboard things will just work using the new standard API once
> kernel support is merged.
> 
> I realize that using a single LED class device with kbd_backlight
> in the name to tap into the existing userspace support to at least
> control the overall backlight brightness is useful and tempting but
> 
> IMHO this really is a case where we need a new userspace API and then
> emulating just a single brightness control for compatilbility with
> existing userspace UI code can be done in powerdevil (KDE) or
> upower (GNOME and others) in combination with offereing a more
> complete DBUS API to also allow controlling the zones separately.
> 

That makes sense. I should post my first implementation using the
multicolor led api soon.

> Recently another (laptop) driver for Casper Excalibur laptops has
> been posted and this also include support for a 4 zone rgb keyboard:
> https://lore.kernel.org/platform-driver-x86/20240324181201.87882-2-mustafa.eskieksi@xxxxxxxxx/
> 
> This driver actually already implements the userspace API proposed in
> the discussion surrounding the earlier "[PATCH V3] platform/x86: hp-wmi:
> Support omen backlight control wmi-acpi methods" patch.
> 
> This driver creates 4 LED class devices using the multi-color LED API
> for RGB. One LED class device per zone. These are named:
> 
> casper:rgb:kbd_zoned_backlight-right
> casper:rgb:kbd_zoned_backlight-middle
> casper:rgb:kbd_zoned_backlight-left
> casper:rgb:kbd_zoned_backlight-corners
> 
> Where as for the HP laptop case I believe the 4 multi-color LED
> class devices should have the following names since the zones
> are different:
> 
> hp:rgb:kbd_zoned_backlight-main
> hp:rgb:kbd_zoned_backlight-wasd
> hp:rgb:kbd_zoned_backlight-cursor
> hp:rgb:kbd_zoned_backlight-numpad
> 

For this driver i think it should be something more like this:

hp:rgb:kbd_zoned_backlight-right
hp:rgb:kbd_zoned_backlight-middle
hp:rgb:kbd_zoned_backlight-left
hp:rgb:kbd_zoned_backlight-wasd

> As I mentioned in my review of the Casper Excalibur laptop driver
> as part of adding support for these zoned rgb keyboards to various
> laptop vendor specific drivers we should also document how these
> devices are presented to userspace:
> 
> A separate patch needs to be written to add documentation about
> the use of these names for zoned RGB backlit kbds in a new paragraph /
> subsection of the "LED Device Naming" section of:
> 
> Documentation/leds/leds-class.rst 
> 
> And this should document at least the 2 currently known
> zone sets:
> 
> :rgb:kbd_zoned_backlight-right
> :rgb:kbd_zoned_backlight-middle
> :rgb:kbd_zoned_backlight-left
> :rgb:kbd_zoned_backlight-corners
> 
> :rgb:kbd_zoned_backlight-main
> :rgb:kbd_zoned_backlight-wasd
> :rgb:kbd_zoned_backlight-cursor
> :rgb:kbd_zoned_backlight-numpad
> 
> with a comment that in the future different zone names are possible
> if keyboards with a different zoning scheme show up.
> 
> Perhaps you can work together with Mustafa on writing a patch for:
> Documentation/leds/leds-class.rst ?
> 

I am open to it if it was not done yet. But could you please
be a bit more specific about what exactly
needs to be documented about my patch?
Zone names, brightness control, userspace interaction?

> for this and then hopefully Pavel can review + ack this patch
> and then we can move forward with both the hp and the casper
> laptop zoned rgb keyboard support.
> 
> Regards,
> 
> Hans