[PATCH 1/2] Add Clickpad support
Takashi Iwai
tiwai at suse.de
Wed Apr 21 00:28:40 PDT 2010
At Wed, 21 Apr 2010 16:31:29 +1000,
Peter Hutterer wrote:
>
> On Wed, Apr 21, 2010 at 08:16:40AM +0200, Takashi Iwai wrote:
> > At Wed, 21 Apr 2010 14:16:41 +1000,
> > Peter Hutterer wrote:
> > >
> > > Thank you for the patch. Just as a heads-up, I'd like to wait with this
> > > until there's at least one kernel released with this support.
> > > My comments to the patch are inline.
> >
> > Thanks for review!
> >
> > > On Thu, Apr 15, 2010 at 06:12:10PM +0200, Takashi Iwai wrote:
> > > > This patch adds the support for Synaptics Clickpad devices.
> > > > It requires the change in Linux kernel synaptics input driver, found in
> > > > https://patchwork.kernel.org/patch/92435/
> > > >
> > > > When the kernel driver sets only the left-button bit evbit, Clickpad
> > > > mode is activated. In this mode, the bottom touch area is used as
> > > > button emulations. Clicking at the bottom-left, bottom-center and
> > > > bottom-right zone corresponds to a left, center and right click.
> > > >
> > > > There are two new parameters added. One is a parameter to specify
> > > > the size of the bottom button-area, and another is to toggle the touch-
> > > > sensing in the button area.
> > > >
> > > > Signed-off-by: Takashi Iwai <tiwai at suse.de>
> > > > ---
> > > > include/synaptics-properties.h | 6 ++++
> > > > man/synaptics.man | 21 +++++++++++++++
> > > > src/eventcomm.c | 15 ++++++++++
> > > > src/properties.c | 19 +++++++++++++
> > > > src/synaptics.c | 56 ++++++++++++++++++++++++++++++++++++++++
> > > > src/synapticsstr.h | 4 +++
> > > > tools/synclient.c | 2 +
> > > > 7 files changed, 123 insertions(+), 0 deletions(-)
> > > >
> > > > diff --git a/include/synaptics-properties.h b/include/synaptics-properties.h
> > > > index cf330d8..c77afd3 100644
> > > > --- a/include/synaptics-properties.h
> > > > +++ b/include/synaptics-properties.h
> > > > @@ -155,4 +155,10 @@
> > > > /* 32 bit, 4 values, left, right, top, bottom */
> > > > #define SYNAPTICS_PROP_AREA "Synaptics Area"
> > > >
> > > > +/* 8 bit (0-100) */
> > > > +#define SYNAPTICS_PROP_TOUCH_BUTTON_AREA "Synaptics Touch Button Area"
> > >
> > > > +
> > > > +/* 8 bit (BOOL) */
> > > > +#define SYNAPTICS_PROP_TOUCH_BUTTON_SENSE "Synaptics Touch Button Sense"
> > > > +
> > > > #endif /* _SYNAPTICS_PROPERTIES_H_ */
> > > > diff --git a/man/synaptics.man b/man/synaptics.man
> > > > index 59fbaac..f92ea0c 100644
> > > > --- a/man/synaptics.man
> > > > +++ b/man/synaptics.man
> > > > @@ -509,6 +509,27 @@ Ignore movements, scrolling and tapping which take place below this edge.
> > > > The option is disabled by default and can be enabled by setting the
> > > > AreaBottomEdge option to any integer value other than zero. Property: "Synaptics Area"
> > > > .
> > > > +.TP
> > > > +.BI "Option \*qTouchButtonArea\*q \*q" integer \*q
> > > > +The percentage of touch-button area of Clickpad device.
> > > > +.
> > > > +The synaptics driver supports "ClickZone" mode, which emulates the buttons
> > > > +to click at the bottom of touchpad area. This option specifies how large
> > > > +is the area to be used as the button area. Passing zero to this disables
> > > > +the Clickpad behavior. The default value is 20.
> > >
> > > where does the 0-100 come from? what unit is this? it's answered in the
> > > header file but this should be in the man page as well.
> >
> > Right.
> >
> > > also, I'd rather have this stored in device units and leave things like
> > > "this is 20% of the area" to higher-level tools.
> >
> > I don't mind in any way. Maybe this parameter is rarely changed.
> >
> > > > +Property: "Synaptics Touch Button Area"
> > >
> > > please make this so that the Synaptics Area is re-used instead of
> > > introducing another area property. This should remove the need for this
> > > property altogether and just need the enable/disable value in the other one.
> >
> > Do you mean to extend "Synaptics Area" property?
> >
> > I find it might be better to split from Synaptics Area, because this
> > is pretty specific to Clickpad mode while Synaptics Area is a more
> > generic one. But, mixing them up is technically no problem, of
> > course.
>
> right now, the area detection code won't move the cursor if it's outside of
> the applicable area. this was essentially added for the touchpads the dell
> minis had. so as long as you're inside that area the pointer moves and
> outside nothing happens.
What about scrolling? Is the vert/horiz scroll sensed outside the
synaptics area?
> with this feature, you could modulate this option with a clickpad feature,
> i.e. if ClickPad is on, then the area outside is used as the button
> calculation area (or the bottom area anyway).
> I think this makes sense enough, given that the hardware is so different I
> don't think there's the need for having an inactive area and a separate
> clickpad area.
>
> or is this too confusing for the user - I'm not sure myself here.
I'll check this possibility.
> > > how do you get the default of 20? isn't this something that should be
> > > auto-scaled from the device in case future devices have a different
> > > resolution?
> >
> > It looks same regardless of the device size. I checked several
> > clickpad devices in different sizes, and all of them show mostly same
> > button area, ca 20% of the touchpad size at the bottom.
>
> is this area hardware-dependent though or can you click anywhere in the
> touchpad?
You can click anywhere. But the click at the bottom is better because
the device is supported on the top side, i.e. the touchpad sinks
deeper in the bottom side.
The 20% area is determined from the line drawn on the touchpad. It's
not necessarily inside this area; it should be rather a software
configuration. It might be that even windows driver accepts the click
outside the drawn area.
> from the option I gather that it is flexible,, which would suggest
> that we could reconfigure the whole thing to be much more flexible than just
> the bottom button approach.
Yes. That's the reason it being a parameter :)
> think of left edge is left button, right edge is right button.
The whole area detection can be more configurable, yes.
For example, we can define a complete area (origin and width/height)
for each LMR button, too.
> > > > if (pars->top_edge > pars->bottom_edge) {
> > > > @@ -2052,6 +2054,56 @@ HandleClickWithFingers(SynapticsParameters *para, struct SynapticsHwState *hw)
> > > > }
> > > > }
> > > >
> > > > +/* clickpad event handling */
> > > > +static void
> > > > +HandleClickpad(LocalDevicePtr local, struct SynapticsHwState *hw)
> > > > +{
> > > > + SynapticsPrivate *priv = (SynapticsPrivate *) (local->private);
> > > > + SynapticsParameters *para = &priv->synpara;
> > > > + int width = priv->maxx - priv->minx;
> > > > + int height = priv->maxy - priv->miny;
> > > > + int button_area_y, x_vscroll;
> > > > + int left_button_x, right_button_x;
> > > > +
> > > > + /* left and right clickpad button ranges;
> > > > + * the gap between them is interpreted as a middle-button click
> > > > + */
> > > > + left_button_x = width * 2/ 5 + priv->minx;
> > > ^ space missing
> >
> > Good catch. (Don't we have a tool like checkpatch.pl for kernel..? :)
>
> unfortunately not, because the coding style is nowhere near standardized.
Hehe. Living in legacy.
> > > > + right_button_x = width * 3 / 5 + priv->minx;
> > > > + /* button area */
> > > > + button_area_y = priv->maxy - height * para->touch_button_area / 100;
> > > > + /* allow position change for the v-scroll area exceptionally */
> > > > + x_vscroll = priv->maxx - width / 15;
> > >
> > > huh? what does this do? why don't you just use the right edge for this?
> >
> > The problem is that the current synaptics driver is way too sensitive
> > for the initial move. On Windows, the pointer doesn't move quickly at
> > first when you slide your finger. It sticks at the position for a
> > while, then starts moving. On Linux, it reacts very fast from the
> > very first moment.
> >
> > This is basically fine, and I like better. But, for clickpad, this is
> > a disadvantage because the pointer often moves together at clicking.
> > The parameter to disable touch-sensing at button area was introduced
> > to avoid it. But, at the same time, you do want sensing for the
> > scrolling area even in the button area. The above is to achieve such
> > an exception.
>
> ah, now I get it. should still be handled through the right edge setting
> (and documented in the code so the next person knows what's going on).
> though combined with the TouchpadArea that might be possible to fix this
> otherwise - haven't looked at that in detail though.
>
> can you also rename TouchButtonSense to TouchButtonDetect?
> that seems to be more self-explanatory (to me anyway :)
Yeah, OK.
BTW, Dmitry accepted the modified kernel patch, at least, for this
Clickpad device support. It's likely merged in 2.6.34 later, or at
most in 2.6.35-rc1.
The LED support part is still in discussion. Dmitry suggested to use
the led class driver instead of the input LED event.
I'm going to rewrite the patch and repost later.
thanks,
Takashi
More information about the xorg-devel
mailing list