[Quantal][PULL] Cypress PS/2 Trackpad driver

Seth Forshee seth.forshee at canonical.com
Tue Aug 14 17:03:07 UTC 2012


On Mon, Aug 13, 2012 at 11:34:32AM -0700, Kamal Mostafa wrote:
> This patch set provides a driver for the Cypress PS/2 Trackpad found in
> the Dell XPS 13, XPS 15, and other laptop models.
> 
> The driver has been tested extensively in the "Sputnik Project" ISO and
> kernel PPA.  I have verified that the driver doesn't adversely affect
> non-Cypress trackpads by testing various laptops with Synaptics and
> Elantech trackpads.
> 
> I will submit the driver to mainline on behalf of Cypress, but we'd like
> to see it land in Ubuntu sooner rather than later.  SRU for Precise to
> follow inclusion in Quantal.
> 
> Bug reference:
>         https://bugs.launchpad.net/ubuntu/+source/linux/+bug/978807
>         'Cypress Trackpad' incorrectly detected as 'ImPS/2 Generic Wheel
>         Mouse' in 'Dell XPS 13 Ultrabook'

The driver is mostly isolated to its own source file, so I don't see it
causing a lot of maintenance headaches. The code overall doesn't look
too bad, but there are several small problems (and one bigger one). Even
after your cleanup the driver still doesn't conform to kernel coding
style (it's using 4 spaces for indentation instead of tabs). Probably it
ought to be using the psmouse_dbg, psmouse_info, etc. for debug messages
as well.

One possible problem for us is that the way you've taken their original
patch and then applied fixes on top of it, we could end up with commits
that won't build under certain configurations (i.e.
CONFIG_MOUSE_PS2_CYPRESS=n up to "UBUNTU: SAUCE: input: Cypress PS/2
Trackpad fix no-config stubs"). We'd probably always have it enabled of
course, but I thought it was worth mentioning.

The most objectionable thing though is the addition of the
PSMOUSE_CMD_CYTP state. I suspect upstream won't respond favorably to
this, and I find the co-opting of ps2dev->wait troublesome (even though
I don't think it's going to cause problems). Honestly I'm really not
sure why they can't just use ps2_command(), but if there's a good reason
they can't I still don't see why they can't do all of this internally
from the protocol_handler callback.

On the one hand it would be nice to have these issues fixed, but on the
other hand I don't see any of them causing us major problems either. So
I won't personally object to carrying this temporarily as a sauce patch,
with the understanding that you'll be working to get it upstream. It
would be nice to get the remaining coding style issues fixed.

Seth





More information about the kernel-team mailing list