Message-ID: <2337211.N3hLRRcU3d@pink>
Date:	Tue, 24 Feb 2015 02:32:49 +0200
From:	Ciprian Ciubotariu <cheepeero@....net>
To:	linux-kernel@...r.kernel.org, linux-input@...r.kernel.org
Cc:	Jiri Kosina <jkosina@...e.cz>,
	Bruno Premont <bonbons@...ux-vserver.org>
Subject: Re: Logitech G-series drivers

On Saturday 21 February 2015 22:57:28 Bruno Prémont wrote:
> On Sat, 21 February 2015 Ciprian Ciubotariu <cheepeero@....net> wrote:
> > > 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.

I'll transcribe this to a file.

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

This would require simulating such a path to the current state of the code. In 
reality I inherited the code with all the features, fixed the bugs and 
refactored it for submission here.

However, see discussion below on the MFD issue. Performing this separation 
might make the code quite readable, even as a single patch.

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

I see. I can change the driver so that hid-gfb is in drivers/video/fbdev. The 
LEDs vs. input parts are harder to separate, but I can see a hid-gleds.c in 
drivers/leds. Both of them would be hidden though, and used as support code 
from the main driver(s).

However, I cannot follow the reasons to put the main HID driver into 
drivers/mfd. Wouldn't picolcd break kernel builds if it wouldn't be updated on 
core/class API changes? Also, the keyboard device presents itself fully 
functional, as opposed to a PMIC where hardware connectivity determines which 
features are actually usable.

I also got clarified on how to implement the LCD_BACKLIGHT part for the G19. 
Thanks for pointing me towards picolcd.

The only question is... do I do all this on my github repository as an out-of-
tree module? And next after a while I come back to LKML with a new version?

Or do I try to clean the patch for the drivers in this shape and continue on 
directly on the kernel tree?

How should I proceed?

Thanks,

Ciprian


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

Powered by blists - more mailing lists