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-input" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html