Prev: [PATCH 03/22] Constify struct backlight_ops for 2.6.32-git-053fe57ac v2
Next: [PATCH 0/7] tracing: Fix bugs in string filters.
From: Takashi Iwai on 15 Dec 2009 02:50
At Mon, 14 Dec 2009 23:33:58 -0800,
Dmitry Torokhov wrote:
>
> On Tue, Dec 15, 2009 at 07:40:55AM +0100, Takashi Iwai wrote:
> > At Mon, 14 Dec 2009 22:26:28 -0800,
> > Dmitry Torokhov wrote:
> > >
> > > [1 <text/plain; us-ascii (7bit)>]
> > > Hi Alex,
> > >
> > > On Mon, Dec 14, 2009 at 08:41:27PM -0700, Alex Chiang wrote:
> > > >
> > > > Thanks, I did grab Takashi's patches and verify that they work
> > > > for me. I tested the separated patches, not the v2 combined
> > > > patch, although it doesn't make any difference based on visual
> > > > inspection of v2.
> > > >
> > > > If you want, you can add my:
> > > >
> > > > Tested-by: Alex Chiang <achiang(a)hp.com>
> > >
> > > Thank you very much for testing, however could you please try a slightly
> > > different patch below (I did not quite like that the original patch
> > > mangled device's capability field and how it was reusing 'middle' field
> > > for different things)? It should apply on top of patch that
> > > I am attaching. I hope I did not screw it up too much,
> >
> > I can't test the patch right now since I'm at home, but I'm afraid
> > it's a bit different behavior. Namely,
> >
> > > @@ -330,20 +339,52 @@ static void synaptics_parse_new_hw(unsigned char buf[],
> > > struct synaptics_data *priv,
> > > struct synaptics_hw_state *hw)
> > > {
> > > - hw->x = (((buf[3] & 0x10) << 8) | ((buf[1] & 0x0f) << 8) | buf[4]);
> > > - hw->y = (((buf[3] & 0x20) << 7) | ((buf[1] & 0xf0) << 4) | buf[5]);
> > > + int x = (((buf[3] & 0x10) << 8) | ((buf[1] & 0x0f) << 8) | buf[4]);
> > > + int y = (((buf[3] & 0x20) << 7) | ((buf[1] & 0xf0) << 4) | buf[5]);
> > >
> > > hw->z = buf[2];
> > > hw->w = (((buf[0] & 0x30) >> 2) |
> > > ((buf[0] & 0x04) >> 1) |
> > > ((buf[3] & 0x04) >> 2));
> > >
> > > - hw->left = buf[0] & 0x01;
> > > - hw->right = buf[0] & 0x02;
> > > + if (SYN_CAP_CLICKPAD(priv->ext_cap)) {
> > > + int click = (buf[0] ^ buf[3]) & 0x01;
> > > +
> > > + if (click && y < YMIN_NOMINAL) {
> > > + /*
> > > + * User pressed in ClickZone; report new button
> > > + * state but use old coordinates and don't report
> > > + * any pressure to prevent pointer movement.
> > > + */
> > > + hw->left = x < CLICKPAD_LEFT_BTN_X;
> > > + hw->right = x > CLICKPAD_RIGHT_BTN_X;
> > > + hw->middle = x >= CLICKPAD_LEFT_BTN_X &&
> > > + x <= CLICKPAD_RIGHT_BTN_X;
> > > + hw->z = 0;
> > > +
> > > + } else {
> > > + /*
> > > + * Finger is outside of the ClickZone - report
> > > + * current coordinates.
> > > + */
> > > + hw->x = x;
> > > + hw->y = y;
> > > +
> > > + if (!click)
> > > + hw->left = hw->right = hw->middle = 0;
> > > + }
> >
> > Here, when you touch outside the button area, left/right/middle are
> > always zero because hw was initialized. So the above code gives the
> > click "released" state outside the button area.
> >
>
> No, I got rid of resetting hw state to 0.

Ah, it's in the second patch.

But looking at that one, hw is a local variable and still doesn't
inherit from the previous state, no? If so, the button state will be
just a random value.


thanks,

Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Takashi Iwai on 15 Dec 2009 05:20
At Tue, 15 Dec 2009 00:25:18 -0800,
Dmitry Torokhov wrote:
>
> On Tue, Dec 15, 2009 at 08:41:05AM +0100, Takashi Iwai wrote:
> > At Mon, 14 Dec 2009 23:33:58 -0800,
> > Dmitry Torokhov wrote:
> > >
> > > On Tue, Dec 15, 2009 at 07:40:55AM +0100, Takashi Iwai wrote:
> > > > At Mon, 14 Dec 2009 22:26:28 -0800,
> > > > Dmitry Torokhov wrote:
> > > > >
> > > > > [1 <text/plain; us-ascii (7bit)>]
> > > > > Hi Alex,
> > > > >
> > > > > On Mon, Dec 14, 2009 at 08:41:27PM -0700, Alex Chiang wrote:
> > > > > >
> > > > > > Thanks, I did grab Takashi's patches and verify that they work
> > > > > > for me. I tested the separated patches, not the v2 combined
> > > > > > patch, although it doesn't make any difference based on visual
> > > > > > inspection of v2.
> > > > > >
> > > > > > If you want, you can add my:
> > > > > >
> > > > > > Tested-by: Alex Chiang <achiang(a)hp.com>
> > > > >
> > > > > Thank you very much for testing, however could you please try a slightly
> > > > > different patch below (I did not quite like that the original patch
> > > > > mangled device's capability field and how it was reusing 'middle' field
> > > > > for different things)? It should apply on top of patch that
> > > > > I am attaching. I hope I did not screw it up too much,
> > > >
> > > > I can't test the patch right now since I'm at home, but I'm afraid
> > > > it's a bit different behavior. Namely,
> > > >
> > > > > @@ -330,20 +339,52 @@ static void synaptics_parse_new_hw(unsigned char buf[],
> > > > > struct synaptics_data *priv,
> > > > > struct synaptics_hw_state *hw)
> > > > > {
> > > > > - hw->x = (((buf[3] & 0x10) << 8) | ((buf[1] & 0x0f) << 8) | buf[4]);
> > > > > - hw->y = (((buf[3] & 0x20) << 7) | ((buf[1] & 0xf0) << 4) | buf[5]);
> > > > > + int x = (((buf[3] & 0x10) << 8) | ((buf[1] & 0x0f) << 8) | buf[4]);
> > > > > + int y = (((buf[3] & 0x20) << 7) | ((buf[1] & 0xf0) << 4) | buf[5]);
> > > > >
> > > > > hw->z = buf[2];
> > > > > hw->w = (((buf[0] & 0x30) >> 2) |
> > > > > ((buf[0] & 0x04) >> 1) |
> > > > > ((buf[3] & 0x04) >> 2));
> > > > >
> > > > > - hw->left = buf[0] & 0x01;
> > > > > - hw->right = buf[0] & 0x02;
> > > > > + if (SYN_CAP_CLICKPAD(priv->ext_cap)) {
> > > > > + int click = (buf[0] ^ buf[3]) & 0x01;
> > > > > +
> > > > > + if (click && y < YMIN_NOMINAL) {
> > > > > + /*
> > > > > + * User pressed in ClickZone; report new button
> > > > > + * state but use old coordinates and don't report
> > > > > + * any pressure to prevent pointer movement.
> > > > > + */
> > > > > + hw->left = x < CLICKPAD_LEFT_BTN_X;
> > > > > + hw->right = x > CLICKPAD_RIGHT_BTN_X;
> > > > > + hw->middle = x >= CLICKPAD_LEFT_BTN_X &&
> > > > > + x <= CLICKPAD_RIGHT_BTN_X;
> > > > > + hw->z = 0;
> > > > > +
> > > > > + } else {
> > > > > + /*
> > > > > + * Finger is outside of the ClickZone - report
> > > > > + * current coordinates.
> > > > > + */
> > > > > + hw->x = x;
> > > > > + hw->y = y;
> > > > > +
> > > > > + if (!click)
> > > > > + hw->left = hw->right = hw->middle = 0;
> > > > > + }
> > > >
> > > > Here, when you touch outside the button area, left/right/middle are
> > > > always zero because hw was initialized. So the above code gives the
> > > > click "released" state outside the button area.
> > > >
> > >
> > > No, I got rid of resetting hw state to 0.
> >
> > Ah, it's in the second patch.
> >
> > But looking at that one, hw is a local variable and still doesn't
> > inherit from the previous state, no? If so, the button state will be
> > just a random value.
> >
>
> Indeed, we need to keep the state in synaptics now, thanks for noticing.
> The updated patch is below.

Setting BTN_MIDDLE bit in set_input_params() is missing for clickpad.

Otherwise it looks OK to me (and the patch description is much better
than mine ;) Will test the patch later.


thanks,

Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Takashi Iwai on 15 Dec 2009 05:50
At Tue, 15 Dec 2009 00:25:18 -0800,
Dmitry Torokhov wrote:
>
> Indeed, we need to keep the state in synaptics now, thanks for noticing.
> The updated patch is below.

Also, one minor thing I noticed:

> - if (SYN_CAP_MIDDLE_BUTTON(priv->capabilities)) {
> - hw->middle = (buf[0] ^ buf[3]) & 0x01;
> - hw->scroll = hw->w == 2 ? (signed char)buf[1] : 0;
> + if (click && y < YMIN_NOMINAL) {

In my original patch, the position reporting in the button area is
disabled no matter whether clicked or not. This was intentional
because I find it quite annoying that the mouse pointer moves slightly
when I click. I often missed the target when I pressed strongly,
because my finger slipped a millimeter before the click state got
active.

This is a matter of taste, though.


> + /*
> + * User pressed in ClickZone; report new button
> + * state but use :w

It's not in "w" field...


thanks,

Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Alex Chiang on 15 Dec 2009 20:10
* Dmitry Torokhov <dmitry.torokhov(a)gmail.com>:
>
> The updated patch is below.
>
> --
> Dmitry

Should I test this one or wait one more iteration to address
Takashi's last comments?

Thanks,
/ac

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Takashi Iwai on 16 Dec 2009 02:00
At Tue, 15 Dec 2009 18:59:34 -0800,
Dmitry Torokhov wrote:
>
> On Tue, Dec 15, 2009 at 06:05:06PM -0700, Alex Chiang wrote:
> > * Dmitry Torokhov <dmitry.torokhov(a)gmail.com>:
> > >
> > > The updated patch is below.
> > >
> > > --
> > > Dmitry
> >
> > Should I test this one or wait one more iteration to address
> > Takashi's last comments?
> >
>
> Actually I think we took the wrong direction with the original patch and
> we should do what other buttonless devices (bcm5974) do: report touchpad
> click as left button and have Synaptics X driver provide enhanced
> support. This way we can have both modes (ClickZones and ClickButtons)
> and users will get to chose (provided that someone takes time to add
> that support to Synaptics driver of course ;) ).

My concern is, still, how would you identify this device. Will you
extend also some ioctls to expose caps and extcaps? Otherwise it's
difficult to identify this device automatically from the user-space.

The user-space can know that it's button-less, yes. But, how can it
know whether the device should be emulated via ClickZone?
We can use a driver option to x11 synaptics driver for that, as I
already sent you another patch. However, the driver option is
nowadays not preferred because xorg.conf is being dead on new
systems...

Or maybe HAL (or whatever upcoming one) can check the vendor/product
id of the machine (not the device) to provide the information. OTOH
this will also need frequent updates.


Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
First  |  Prev  |  Next  |  Last
Pages: 1 2 3 4 5
Prev: [PATCH 03/22] Constify struct backlight_ops for 2.6.32-git-053fe57ac v2
Next: [PATCH 0/7] tracing: Fix bugs in string filters.