| Message ID | 1344882872.13390.27.camel@fourier |
|---|---|
| State | New |
| Headers | show |
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
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 >
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
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