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

login
register
mail settings
Submitter Kamal Mostafa
Date Jan. 17, 2013, 7:04 p.m.
Message ID <1358449462.28710.139.camel@fourier>
Download mbox
Permalink /patch/213350/
State New
Headers show

Pull-request

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

Comments

Kamal Mostafa - Jan. 17, 2013, 7:04 p.m.
The Cypress PS/2 Trackpad driver (which we're already shipping in
Precise and Quantal, but not Raring) has been committed to the Input
subsystem maintainer's master branch[0] pending merge into mainline
Linux 3.9.

Please consider this pull for Raring
http://kernel.ubuntu.com/git?p=kamal/ubuntu-raring.git;a=shortlog;h=refs/heads/cypress-for-raring
... which includes the Cypress driver cherry-picked from the Input
subsystem maintainer's repo, plus my "simulated multitouch" Cypress
driver enhancement (denied by upstream, but required for Unity
multitouch functionality; is included in our P and Q version).

After the driver commits land in mainline 3.9, we will replace the
cherry-picks.

I believe the possibility of regression is low; the driver has been well
tested, and (aside from its detection routine) will only affect systems
with Cypress PS/2 trackpads.

Thanks,

 -Kamal

[0] http://git.kernel.org/?p=linux/kernel/git/dtor/input.git;a=commitdiff;h=80524f083e2c3e70057f5bb476db92baa14cb22b
    http://git.kernel.org/?p=linux/kernel/git/dtor/input.git;a=commitdiff;h=0799a924bc93ba46a23e8e7e6b1431ab585fd2ea

-- >8 -----------------------------

The following changes since commit 18cc3a6a1debcf3819c47b5226ff44f9f89b5105:

  UBUNTU: Ubuntu-3.8.0-0.4 (2013-01-15 13:26:26 -0700)

are available in the git repository at:

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

for you to fetch changes up to 0c6dabecd306d66e56674db2e67a084db5abeb50:

  UBUNTU: [Config] Add CONFIG_PS2_CYPRESS (2013-01-17 10:13:40 -0800)

----------------------------------------------------------------
Dudley Du (1):
      UBUNTU: SAUCE: Input: add support for Cypress PS/2 Trackpads

Kamal Mostafa (3):
      UBUNTU: SAUCE: Input: increase struct ps2dev cmdbuf[] to 8 bytes
      UBUNTU: SAUCE: Input: Cypress PS/2 Trackpad simulated multitouch
      UBUNTU: [Config] Add CONFIG_PS2_CYPRESS

 debian.master/config/config.common.ubuntu |    1 +
 drivers/input/mouse/Kconfig               |   10 +
 drivers/input/mouse/Makefile              |    1 +
 drivers/input/mouse/cypress_ps2.c         |  743 +++++++++++++++++++++++++++++
 drivers/input/mouse/cypress_ps2.h         |  202 ++++++++
 drivers/input/mouse/psmouse-base.c        |   32 ++
 drivers/input/mouse/psmouse.h             |    1 +
 include/linux/libps2.h                    |    2 +-
 8 files changed, 991 insertions(+), 1 deletion(-)
 create mode 100644 drivers/input/mouse/cypress_ps2.c
 create mode 100644 drivers/input/mouse/cypress_ps2.h
Tim Gardner - Jan. 17, 2013, 8:14 p.m.
I agree the risk of regression is low and any fallout will be quickly 
detectable, i.e., your pointing device will quit working.

rtg
Seth Forshee - Jan. 17, 2013, 8:38 p.m.
On Thu, Jan 17, 2013 at 11:04:22AM -0800, Kamal Mostafa wrote:
> The Cypress PS/2 Trackpad driver (which we're already shipping in
> Precise and Quantal, but not Raring) has been committed to the Input
> subsystem maintainer's master branch[0] pending merge into mainline
> Linux 3.9.
> 
> Please consider this pull for Raring
> http://kernel.ubuntu.com/git?p=kamal/ubuntu-raring.git;a=shortlog;h=refs/heads/cypress-for-raring
> ... which includes the Cypress driver cherry-picked from the Input
> subsystem maintainer's repo, plus my "simulated multitouch" Cypress
> driver enhancement (denied by upstream, but required for Unity
> multitouch functionality; is included in our P and Q version).

I'm not really so keen on this simulated multitouch patch.

First, I think you overstate the case for the simulated multitouch
functionality. Without that patch the driver still has semi-mt support,
which should be adequate to support most two-finger gestures: drag,
pinch, spread, and two-finger clicks at minimum. It looks like there's
enough information coming out of the driver to support 3, 4, and 5
finger clicks as well. My understanding is that our userspace doesn't
handle those events the same as with full mt devices, but I don't see
that as being a driver problem.

As for gestures involving 3+ fingers, the code just generates fake touch
points offset horizontally from the first one. That means every gesture
looks like a drag.

In fact, with knowing the number of contacts and the bounding box I'd
think that userspace should even be able to support a 3+ finger drag
gesture. Probably better than the simulated multitouch even, because if
the bounding box isn't staying approximately the same size the gesture
likely isn't a drag.

What's wrong with limiting it to semi-mt, in line with the actual
capabilities of the hardware?

One more comment, regarding the implementation. The patch lacks
protection for report_data->contact_cnt > CTYP_MAX_MT_SLOTS whereas the
rest of the driver seems to protect against it. I'm assuming that
because you've increased that max number of slots from 2 to 5 there's no
longer a concern of having more contacts than slots? Because if that
happens you're going to overrun report_data->contacts.

Seth
Kamal Mostafa - Jan. 17, 2013, 10:03 p.m.
On Thu, 2013-01-17 at 14:38 -0600, Seth Forshee wrote:
> On Thu, Jan 17, 2013 at 11:04:22AM -0800, Kamal Mostafa wrote:
> > The Cypress PS/2 Trackpad driver [...]
> > plus my "simulated multitouch" Cypress
> > driver enhancement (denied by upstream, but required for Unity
> > multitouch functionality; is included in our P and Q version).


> I'm not really so keen on this simulated multitouch patch.
> 

Hi Seth-

Thanks for reviewing this, and for all your help with this driver.  I'm
certainly open to discussion about the validity of the Cypress simulated
multitouch patch...


> First, I think you overstate the case for the simulated multitouch
> functionality.

The main case for it is that the shipping Precise and Quantal SAUCE
versions of this driver do include the simulated multitouch enhancement.
The original driver as supplied by Cypress included a variant of it. 

So for the Cypress trackpad, Unity multi-touch gestures (>2 fingers)
work now in P and Q, but if we omit my simul-mt patch then they won't
work anymore in Raring: a regression.


>  Without that patch the driver still has semi-mt support,
> which should be adequate to support most two-finger gestures: drag,
> pinch, spread, and two-finger clicks at minimum. It looks like there's
> enough information coming out of the driver to support 3, 4, and 5
> finger clicks as well.

Correct.  The Cypress driver (with or without the simul-mt patch) does
already emit the newer fingers-count API events along with the semi-mt
bounding box via a call to input_mt_report_finger_count(), but ...


>  My understanding is that our userspace doesn't
> handle those events the same as with full mt devices, but I don't see
> that as being a driver problem.

Correct.  Our userspace (Unity via x-x-input-synaptics) does not support
that newer fingers-count API at all for semi-mt devices.  If my reading
of x-x-i-s is correct, even the latest upstream version of x-x-i-s does
not.  I spent several frustrating hours trying to make x-x-i-s to
support it, but couldn't figure out how without a major overhaul.

Upstream input subsystem maintainers agree with you that this is "not a
driver problem", but userspace doesn't seem to want to take
responsibility for it either.

From the perspective of a user of the machine, I do not care whether the
code is in the driver or in userspace...  If the hardware is capable of
>2 finger support and we have working code to support it, then I would
like it to work on my Ubuntu system.


> 
> As for gestures involving 3+ fingers, the code [Kamal's simul-mt patch]
> just generates fake touch
> points offset horizontally from the first one. That means every gesture
> looks like a drag.

A drag or a tap.  As coded, it works very nicely for Unity's 3-finger
drag and 4-finger tap gestures.


> 
> In fact, with knowing the number of contacts and the bounding box I'd
> think that userspace should even be able to support a 3+ finger drag
> gesture. Probably better than the simulated multitouch even, because if
> the bounding box isn't staying approximately the same size the gesture
> likely isn't a drag.

Agreed.  That's exactly what userspace *should* be doing for semi-mt
devices.  ;-)


> 
> What's wrong with limiting it to semi-mt, in line with the actual
> capabilities of the hardware?

My feeling is that the addition of the simul-mt patch is useful and
harmless, so what's wrong with including it?


> One more comment, regarding the implementation. The patch lacks
> protection for report_data->contact_cnt > CTYP_MAX_MT_SLOTS whereas the
> rest of the driver seems to protect against it. I'm assuming that
> because you've increased that max number of slots from 2 to 5 there's no
> longer a concern of having more contacts than slots? Because if that
> happens you're going to overrun report_data->contacts.

Your assumption is correct.  contact_cnt cannot ever become > 5, and the
checks for contact_cnt > CTYP_MAX_MT_SLOTS in the rest of the driver are
actually necessary *only* to support the semi-mt case.  Those checks
aren't for overrun protection, they are what implements the regular
semi-mt "maximum-of-two-coordinate-pairs" behavior.

 -Kamal
Seth Forshee - Jan. 17, 2013, 10:46 p.m.
On Thu, Jan 17, 2013 at 02:03:16PM -0800, Kamal Mostafa wrote:
> On Thu, 2013-01-17 at 14:38 -0600, Seth Forshee wrote:
> > On Thu, Jan 17, 2013 at 11:04:22AM -0800, Kamal Mostafa wrote:
> > > The Cypress PS/2 Trackpad driver [...]
> > > plus my "simulated multitouch" Cypress
> > > driver enhancement (denied by upstream, but required for Unity
> > > multitouch functionality; is included in our P and Q version).
> 
> 
> > I'm not really so keen on this simulated multitouch patch.
> > 
> 
> Hi Seth-
> 
> Thanks for reviewing this, and for all your help with this driver.  I'm
> certainly open to discussion about the validity of the Cypress simulated
> multitouch patch...

I don't think there's much to discuss as far as validity goes. The
driver just makes stuff up. From a technical perspective I don't see how
that could be considered valid.

> > First, I think you overstate the case for the simulated multitouch
> > functionality.
> 
> The main case for it is that the shipping Precise and Quantal SAUCE
> versions of this driver do include the simulated multitouch enhancement.
> The original driver as supplied by Cypress included a variant of it. 
> 
> So for the Cypress trackpad, Unity multi-touch gestures (>2 fingers)
> work now in P and Q, but if we omit my simul-mt patch then they won't
> work anymore in Raring: a regression.

The fact that this would be a regression is the *only* legitimate
argument I see in favor of keeping the patch. But it's a strong
argument. Too bad we didn't notice this and get rid of it before putting
it into P and Q.

> > As for gestures involving 3+ fingers, the code [Kamal's simul-mt patch]
> > just generates fake touch
> > points offset horizontally from the first one. That means every gesture
> > looks like a drag.
> 
> A drag or a tap.  As coded, it works very nicely for Unity's 3-finger
> drag and 4-finger tap gestures.

Okay, sure. I wasn't really thinking of taps as being gesutres ;-)

> > What's wrong with limiting it to semi-mt, in line with the actual
> > capabilities of the hardware?
> 
> My feeling is that the addition of the simul-mt patch is useful and
> harmless, so what's wrong with including it?

One minor problem would be if any desktop environment running on top of
our kernel supports gestures with >2 fingers other than tap and drag.
Users may be confused to find that every gesture gets treated like a
drag.

> > One more comment, regarding the implementation. The patch lacks
> > protection for report_data->contact_cnt > CTYP_MAX_MT_SLOTS whereas the
> > rest of the driver seems to protect against it. I'm assuming that
> > because you've increased that max number of slots from 2 to 5 there's no
> > longer a concern of having more contacts than slots? Because if that
> > happens you're going to overrun report_data->contacts.
> 
> Your assumption is correct.  contact_cnt cannot ever become > 5, and the
> checks for contact_cnt > CTYP_MAX_MT_SLOTS in the rest of the driver are
> actually necessary *only* to support the semi-mt case.  Those checks
> aren't for overrun protection, they are what implements the regular
> semi-mt "maximum-of-two-coordinate-pairs" behavior.

Good, I just wanted to be sure. Thanks for confirming.

Seth