From: Ciprian Ciubotariu <cheepeero@gmx.net>
To: linux-kernel@vger.kernel.org, linux-input@vger.kernel.org
Cc: Jiri Kosina <jkosina@suse.cz>,
        Bruno =?ISO-8859-1?Q?Pr=E9mont?= <bonbons@linux-vserver.org>
Subject: Re: Logitech G-series drivers
Date: Sat, 21 Feb 2015 17:46:47 +0200
Message-ID: <2205053.ll7fhHWnZQ@pink>
User-Agent: KMail/4.14.3 (Linux/3.17.8-gentoo-r1; KDE/4.14.3; x86_64; ; )
In-Reply-To: <20150219104827.7f5d4e38@pluto.restena.lu>
References: <2384594.RyRtmbhCrj@pink> <20150219104827.7f5d4e38@pluto.restena.lu>
MIME-Version: 1.0
Content-Transfer-Encoding: 8BIT
Content-Type: text/plain; charset="iso-8859-1"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 7409
Lines: 173

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?

> 
> On Sun, 15 Feb 2015 23:17:27 +0200 Ciprian Ciubotariu wrote:
> > I would like to submit to your attention for inclusion in the mainline
> > kernel a series of drivers for a set of Logitech keybord devices. I
> > forked the sources under a GPL/GPLv2 license and performed maintenance
> > and stabilization work on them.
> > 
> > The repository I am working on is at
> > 
> >     https://github.com/CMoH/lg4l
> > 
> > Short description of the modules and files:
> >     - hid-g110 - Logitech G110 (tested)
> >     - hid-g13 - Logitech G13 (tested)
> >     - hid-g15v2 - Logitech G15, version 2 (tested)
> >     - hid-g19 - Logitech G19 (tested)
> >     
> >     - hid-g15 - Logitech G15 (not tested)
> >     - hid-g510 - Logitech G510 - not ready
> >     
> >     - hid-gcore - common functions for other modules
> >     - hid-gfb - framebuffer implementation for on-device displays
> >     - hid-ids.h - product IDs
> > 
> > I would like the opinion of a kernel developer on the possibility of
> > including these drivers in the kernel. If the answer is favorable, I will
> > prepare a series of patches against the kernel's master branch and work
> > towards them being accepted.
> 
> From a quick look at your github tree the drivers are prepared for
> building out-of-tree.
> 
> 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.

> 
> 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.

III. Driver code

The driver code can be understood starting from the probe functions for each 
model. The probe functions allocate a common structure via hid-gcore.c, and 
use the gdata member for model-custom information. Each probe function adds 
the appropriate LED-class devices, using functions in hid-gcore.c. Same goes 
for input devices, sysfs attributes and possibly framebuffer devices (via hid-
gfb.c).

Device initialization goes through a series of stages, which can be read via a 
bitfield in some reports. After this is completed, the gXX_raw_event function 
defers to gXX_raw_event_process_input, which reports input events. For G110 
and G19 we also have gXX_ep1_read and gXX_ep1_urb_completion, which handle the 
extra keys. gXX_led_.... keys handle LED brightness via sysfs.

The gfb_probe function in hid-gfb.c  is started by the main driver in 
monochrome or color mode, depending on model. It allocates a backbuffer for 
user interaction, and writes it via an bulk/interrupt USB endpoint to the 
device. The USB writes are performed using a certain frequency using 
FB_DEFERRED_IO (see gfb_fb_deferred_io, gfb_update and gfb_fb_send). The rest 
manages framebuffer operations and userspace access.


> There seem to be individual drivers for each keyboard type.
> Are the features so different that distinct drivers are needed or can
> the drivers be unified?

Some of my work was to start refactoring the set of copy-pasted drivers into a 
common functions module (hid-gcore). The LCDs use a common protocol, which was 
already isolated in hid-gfb. 

Perhaps the LEDs API could be unified, but the hardware protocol would still 
differ between models. The probe functions also seem similar, but I'll have to 
check for subtle differences before attempting to unify them. More work could 
be done in this direction.

However, I think that at this stage attempting to completely unify the drivers 
would make the code more unreadable. Maybe after some more refactoring.

Also, I am unsure if such an unification is compatible with Logitech's newer G-
series models (G710 etc), since they seem to change things in hardware at will 
between models.

> 
> 
> 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).

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.

I have followed the Documentation/SubmitChecklist guide to check the code for 
style problems, checked it with sparse and cross-compiled it for ARM and 
PPC64. The incremental changes can be reviewed at https://github.com/CMoH/lg4l

> 
> 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.

I don't have any preference though.

> 
> Bruno

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