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

login
register
mail settings
Submitter Kamal Mostafa
Date Aug. 13, 2012, 6:34 p.m.
Message ID <1344882872.13390.27.camel@fourier>
Download mbox
Permalink /patch/177023/
State New
Headers show

Pull-request

git://kernel.ubuntu.com/kamal/ubuntu-quantal.git cypress-for-quantal

Comments

Kamal Mostafa - Aug. 13, 2012, 6:34 p.m.
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'

Thanks,

 -Kamal

----------

The following changes since commit 0937d042b97a9540b5488ab172aa14b53e80014b:

  UBUNTU: Ubuntu-3.5.0-10.10 (2012-08-12 13:18:25 -0700)

are available in the git repository at:

  git://kernel.ubuntu.com/kamal/ubuntu-quantal.git cypress-for-quantal

for you to fetch changes up to 91504b2622b00ee80752fbd4962671592e4bd3f6:

  UBUNTU: SAUCE: [Config] add MOUSE_PS2_CYPRESS=y (2012-08-13 10:09:02 -0700)

----------------------------------------------------------------
Cypress Semiconductor Corporation (2):
      UBUNTU: SAUCE: input: Cypress PS/2 Trackpad mouse driver
      UBUNTU: SAUCE: input: Cypress PS/2 Trackpad link driver into psmouse-base

Kamal Mostafa (5):
      UBUNTU: SAUCE: input: Cypress PS/2 Trackpad code style cleanup
      UBUNTU: SAUCE: input: Cypress PS/2 Trackpad eliminate dead code
      UBUNTU: SAUCE: input: Cypress PS/2 Trackpad fix no-config stubs
      UBUNTU: SAUCE: input: Cypress PS/2 Trackpad set default debug_level=0
      UBUNTU: SAUCE: [Config] add MOUSE_PS2_CYPRESS=y

 debian.master/config/config.common.ubuntu |    1 +
 drivers/input/mouse/Kconfig               |   10 +
 drivers/input/mouse/Makefile              |    1 +
 drivers/input/mouse/cypress_ps2.c         |  943 +++++++++++++++++++++++++++++
 drivers/input/mouse/cypress_ps2.h         |  220 +++++++
 drivers/input/mouse/psmouse-base.c        |   40 ++
 drivers/input/mouse/psmouse.h             |    2 +
 7 files changed, 1217 insertions(+)
 create mode 100644 drivers/input/mouse/cypress_ps2.c
 create mode 100644 drivers/input/mouse/cypress_ps2.h
Seth Forshee - Aug. 14, 2012, 5:03 p.m.
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
Kamal Mostafa - Aug. 14, 2012, 5:38 p.m.
In reference to ...
  git://kernel.ubuntu.com/kamal/ubuntu-quantal.git cypress-for-quantal
  http://kernel.ubuntu.com/git?p=kamal/ubuntu-quantal.git;a=shortlog;h=refs/heads/cypress-for-quantal

Thanks for reviewing this Seth!...

On Tue, 2012-08-14 at 12:03 -0500, Seth Forshee wrote:
> 
> 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).

I had fixed all the checkpatch.pl whitespace "errors", but didn't try to
address the "warnings" (the 4-space indent is a warning).  I'll do that,
and resubmit the pull request.

>  Probably it
> ought to be using the psmouse_dbg, psmouse_info, etc. for debug messages
> as well.

I'll add that to my to-do list, but I'm hoping we can accept it in
Quantal as it is for now.

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

Ah, but I don't think that's so.  I think it will build okay at every
commit in any configuration, because I don't actually add cypress_ps2.o
to the Makefile until "UBUNTU: SAUCE: input: Cypress PS/2 Trackpad link
driver into psmouse-base".  Before that point, cypress_ps2.[ch] won't
even try to be compiled so there's no problem there, and its nice that
we can keep commits of their original code as they delivered it.  Yes?

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

I'll feed that information back to Cypress and/or add it to my to-do
list to investigate further.  Again hoping its acceptable as is for
Quantal.

> 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
>
Seth Forshee - Aug. 14, 2012, 6:27 p.m.
On Tue, Aug 14, 2012 at 10:38:02AM -0700, Kamal Mostafa wrote:
> > 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.
> 
> Ah, but I don't think that's so.  I think it will build okay at every
> commit in any configuration, because I don't actually add cypress_ps2.o
> to the Makefile until "UBUNTU: SAUCE: input: Cypress PS/2 Trackpad link
> driver into psmouse-base".  Before that point, cypress_ps2.[ch] won't
> even try to be compiled so there's no problem there, and its nice that
> we can keep commits of their original code as they delivered it.  Yes?

Okay, I missed that. I don't expect any build failures in that case.

Seth