Message-ID: <73c36418-34d6-46cf-9f10-6ca5e569274f@tuxedocomputers.com>
Date: Wed, 23 Oct 2024 19:54:39 +0200
From: Werner Sembach <wse@...edocomputers.com>
To: Armin Wolf <W_Armin@....de>, Pavel Machek <pavel@....cz>,
 Benjamin Tissoires <bentiss@...nel.org>
Cc: Hans de Goede <hdegoede@...hat.com>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 dri-devel@...ts.freedesktop.org, jelle@...aa.nl, jikos@...nel.org,
 lee@...nel.org, linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-leds@...r.kernel.org, miguel.ojeda.sandonis@...il.com,
 ojeda@...nel.org, onitake@...il.com, platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH 1/1] platform/x86/tuxedo: Add virtual LampArray for TUXEDO
 NB04 devices

Hi,

Am 22.10.24 um 17:02 schrieb Armin Wolf:
> Am 22.10.24 um 11:37 schrieb Pavel Machek:
>
>> Hi!
>>
>>>> Personally I really like the idea to just emulate a HID LampArray device
>>>> for this instead or rolling our own API.  I believe there need to be
>>>> strong arguments to go with some alternative NIH API and I have not
>>>> heard such arguments yet.
>
> Using a virtual HID LampArray already creates two issues:
>
> 1. We have to supply device size data (length, width, height), but the driver
> cannot know this.
>
> 2. It is very difficult to extend the HID LampArray interface, for example
> there is no way to read the current LED color from the hardware or switch
> between different modes.
>
> A sysfs- and/or ioctl-based interface would allow us to:
>
> 1. Threat some data as optional.
>
> 2. Extend the interface later should the need arise.
>
> Looking at the tuxedo-drivers code, it seems that the WMI interface also reports:
>
> - preset color
> - device type (touchpad, keyboard, ...)
> - keyboard type (US/UK)
>
> Making this information available through the HID LampArray protocol would be
> difficult (except for the device type information).
>
>>> Agreed on everything Hans said.
>>>
>>> I'll personnaly fight against any new "illumination" API as long as we
>>> don't have committed users. This is the same policy the DRM folks
>>>> are
>> Well, and I'll personally fight against user<->kernel protocol as
>> crazy as HID LampArray is.
>>
>> OpenRGB is not suitable hardware driver.
>>                                 Pavel
>
> I agree.
>
> The point is that we need to design a userspace API since we cannot just allow
> userspace to access the raw device like with HID devices.
>
> And since we are already forced to come up with a userspace API, then maybe it 
> would
> make sense to build a extendable userspace API or else we might end up in the 
> exact
> same situation later should another similar driver appear.
>
> Since the HID LampArray is a hardware interface standard, we AFAIK cannot 
> easily extend it.
>
> Also i like to point out that OpenRGB seems to be willing to use this new 
> "illumination" API
> as long as the underlying hardware interface is properly documented so that 
> they can implement
> support for it under Windows.
>
> I would even volunteer to write the necessary OpenRGB backend since i already 
> contributed to
> the project in the past.

Just wanting to leave my 2 cents here: I'm in theory fine with both approaches 
(hidraw LampArray or wrapping it in some kind of new UAPI which at least has the 
LampArray feature set).

I also don't think that OpenRGB has a problem with a new Linux exclusive API as 
long as someone is doing the implementation work. After all the reason why 
OpenRGB was started is to unify all the different vendor APIs under one UI. So 
one more or less doesn't matter.

BUT: I already did work for the hidraw LampArray approach and OpenRGB already 
did work for that as well (albeit I didn't yet managed to get the draft running) 
and we already had a lengthy discussion about this in the last thread. (This one 
https://lore.kernel.org/all/20231011190017.1230898-1-wse@tuxedocomputers.com/) 
with all the same arguments.

e.g. Expansion of the API: How should that look like? It would have to be 
basically an own extension for every keyboard manufacturer because every one 
supports different built in modes with different values to tweak.

So I'm siding with Hans and Benjamin on this one.

My only plan for the current patch besides some more code beautification: Move 
the device-sku specific values (key map, and key positions) to a bpf driver.

The question in my mind currently is: Is the patch merge ready with just that? 
Or must the OpenRGB implemenation also be finished before the merge?

Best regards,

Werner

>
> Thanks,
> Armin Wolf
>

Powered by blists - more mailing lists