Re: Logitech G-series drivers

[Bruno PrÃmont]

On Sat, 21 February 2015 Ciprian Ciubotariu <cheepeero@xxxxxxx> wrote:
> Hi. Only now I realized you wrote some instructions. Below are my (quite 
> lengthy) responses.
> 
> On Thursday 19 February 2015 10:48:27 Bruno PrÃmont wrote:
> > Hi Ciprian,
> > 
> > Adding linux-input and Jiri (HID maintainer) to CC.
> 
> Should I register myself on the linux-input mailing list as well?

That's up to you, though it would help catching possibly relevant threads
affecting input devices or (later) Logitech devices you care about.

> > On Sun, 15 Feb 2015 23:17:27 +0200 Ciprian Ciubotariu wrote:
> > Did you check for older work on Logitech keyboards that has been
> > proposed on linux-input list some time ago and what is currently
> > present in Linus' tree?
> 
> Yes, I have checked the tree. The mailing list archives recorded some related 
> activity in 2010, but no patches were applied to the kernel tree.

Reviewing the work that happened back then and at least considering the
comments made at that time will help you get the code into a better shape.

That's about the time when I wrote picolcd driver, using the proposed
Logitech driver as a starting point.

> > An overview of the features covered by the drivers would help
> > understand what the new drivers add (and the differences between all
> > the covered keyboard variants).
> 
> I. From the user perspective, all G-series keyboards have
> 
>     (1) A set of "macro" keys and a set of support keys (macro set selection, 
> menu navigation and such). 
> 
>     (2) Some devices present an LCD, which is either monochrome or color. 
> 
>     (3) All have LEDs that allow changing the (3a) keyboard illumination 
> intensity and color, (3b) the backlight of the macro set selection keys, as 
> well (3c) as the backlight of the LCD. 
> 
> II. Hardware
> 
> The hardware presents these extra keys on a separate USB device. G110 and G19 
> use a separate endpoint for some of the keys, while other models use just HID 
> reports. All seem to use a custom bit-flag format to report the key status, but 
> I am not a HID expert - maybe it is standard. However, all these keys are dead 
> without these drivers.
>
> The LEDs are controlled via hid reports, but the calculus of the fields differs 
> on some models (G110 needs some weird maths).
> 
> The LCD framebuffer can be written to via USB interrupt/bulk endpoints 
> depending on the model. hid-gfb.c implements the LCD matrix via the kernel's 
> FB API. The LCD backlight can only be controlled independently on G19, and is 
> mapped to LED devices by hid-g19.

This is rather important information for explaining your choice of splitting
up the code.
You may want to extend this, eventually putting it into a file under
Documentation/

Giving a clear overview of how the various features are presented on USB/HID
side for each driver definitely helps.

This should also help you decide if unification is reasonable or just requires
too much glue-code to translate different HW implementations.

> III. Driver code
> 
> <snip>
> 
> > If you would like to get the drivers merged please create patches
> > against upstream tree, eventually starting with support for the
> > keyboard you own, adding support for the other keyboards in separate
> > patches.
> > You could also split your patch based on feature support.
> 
> While I am typing on the G19 right now, I have only tested G110, G13, G15v2 
> and they seem stable. I lack the G15 model, and G510 is not yet working.
> 
> To summarize, this submission is about devices g110, g13, g15v2 and g19, all 
> tested by me and working.
> 
> I have prepared a patch for the kernel tree, rebased into a single commit. I 
> can split it per-device, but I don't know how to split the changes to Kconfig, 
> hid-ids.h and hid-core.c so that each patch can be applied independently 
> (because the changes would overlap).

Splitting per-device is one way. More interesting for review would be splitting
by feature:
- base input driver handling the "dead" keys
- adding LEDs
- adding LCD fb/backlight
- adding other goodies

Proposing the patches in a series where later patches depend on previous ones
is the normal way of operating.

> The Kconfig made me wonder if I should (a) activate the dependent code in the 
> kernel (for instance the FB or LEDS_CLASS etc), or (b) deactivate the feature 
> from the driver and advise users in the help text.

For those wanting to trip down their kernels in size having the option to
reduce feature set via Kconfig options is welcome.
If you select dependencies (you can only properly do so when selecting
config options that have no dependencies) or have your options show up/default
on them is up to you.

> > It might be worth exploring the option to organize the driver(s) as a
> > MFD (multi-function-device) device.
> 
> I have looked at the list of MFD devices with make menuconfig, but I have not 
> seen any input devices there. Perhaps a radio, and mostly PMICs of various 
> sorts.
> 
> These are rather HID input devices with some extra custom goodies (extra keys, 
> LEDs and LCDs). The implementation is also based on the HID bus.

If I had to redo picolcd driver now I would give MFD a try.
When I have enough time to do so I may very well convert it driver.

The big advantage I see in MFD drivers is that the separate functions can
live in their respective areas in kernel code tree so they don't get skipped
when core/class code get API changes.


Bruno
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/