| Message ID | 1309324042-22943-3-git-send-email-djkurtz@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Daniel, > Synaptics touchpads report increasing y from bottom to top. > This is inverted from normal userspace "top of screen is 0" coordinates. > Thus, the kernel driver reports inverted y coordinates to userspace. > > In some cases, however, y = 0 is sent by the touchpad. > In these cases, the kernel driver should not invert, and just report 0. > > This patch also refactors the inversion into a macro, and moves it > into packet processing instead of during position reporting. The patch seems to invert the current output? > > Signed-off-by: Daniel Kurtz <djkurtz@chromium.org> > --- > drivers/input/mouse/synaptics.c | 16 +++++++++------- > 1 files changed, 9 insertions(+), 7 deletions(-) > > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c > index e06e045..f6d0c04 100644 > --- a/drivers/input/mouse/synaptics.c > +++ b/drivers/input/mouse/synaptics.c > @@ -44,6 +44,8 @@ > #define YMIN_NOMINAL 1408 > #define YMAX_NOMINAL 4448 > > +#define INVERT_Y(y) (((y) != 0) ? (YMAX_NOMINAL + YMIN_NOMINAL - (y)) : 0) > + > > /***************************************************************************** > * Stuff we need even when we do not want native Synaptics support > @@ -409,9 +411,9 @@ static int synaptics_parse_hw_state(const unsigned char buf[], > hw->x = (((buf[3] & 0x10) << 8) | > ((buf[1] & 0x0f) << 8) | > buf[4]); > - hw->y = (((buf[3] & 0x20) << 7) | > + hw->y = INVERT_Y((((buf[3] & 0x20) << 7) | > ((buf[1] & 0xf0) << 4) | > - buf[5]); > + buf[5])); > > hw->z = buf[2]; > hw->w = (((buf[0] & 0x30) >> 2) | > @@ -421,7 +423,8 @@ static int synaptics_parse_hw_state(const unsigned char buf[], > if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c) && hw->w == 2) { > /* Gesture packet: (x, y, z) at half resolution */ > priv->mt.x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1; > - priv->mt.y = (((buf[4] & 0xf0) << 4) | buf[2]) << 1; > + priv->mt.y = INVERT_Y((((buf[4] & 0xf0) << 4) > + | buf[2]) << 1); > priv->mt.z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1; > return 1; > } > @@ -473,7 +476,7 @@ static int synaptics_parse_hw_state(const unsigned char buf[], > } > } else { > hw->x = (((buf[1] & 0x1f) << 8) | buf[2]); > - hw->y = (((buf[4] & 0x1f) << 8) | buf[5]); > + hw->y = INVERT_Y(((buf[4] & 0x1f) << 8) | buf[5]); > > hw->z = (((buf[0] & 0x30) << 2) | (buf[3] & 0x3F)); > hw->w = (((buf[1] & 0x80) >> 4) | ((buf[0] & 0x04) >> 1)); > @@ -491,8 +494,7 @@ static void set_slot(struct input_dev *dev, int slot, bool active, int x, int y) > input_mt_report_slot_state(dev, MT_TOOL_FINGER, active); > if (active) { > input_report_abs(dev, ABS_MT_POSITION_X, x); > - input_report_abs(dev, ABS_MT_POSITION_Y, > - YMAX_NOMINAL + YMIN_NOMINAL - y); > + input_report_abs(dev, ABS_MT_POSITION_Y, y); > } > } > > @@ -584,7 +586,7 @@ static void synaptics_process_packet(struct psmouse *psmouse) > > if (num_fingers > 0) { > input_report_abs(dev, ABS_X, hw.x); > - input_report_abs(dev, ABS_Y, YMAX_NOMINAL + YMIN_NOMINAL - hw.y); > + input_report_abs(dev, ABS_Y, hw.y); > } > input_report_abs(dev, ABS_PRESSURE, hw.z); > > -- > 1.7.3.1 > Thanks, Henrik -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Henrik Thanks for the review. On Tue, Jul 5, 2011 at 5:08 AM, Henrik Rydberg <rydberg@euromail.se> wrote: > > Hi Daniel, > > > Synaptics touchpads report increasing y from bottom to top. > > This is inverted from normal userspace "top of screen is 0" coordinates. > > Thus, the kernel driver reports inverted y coordinates to userspace. > > > > In some cases, however, y = 0 is sent by the touchpad. > > In these cases, the kernel driver should not invert, and just report 0. > > > > This patch also refactors the inversion into a macro, and moves it > > into packet processing instead of during position reporting. > > The patch seems to invert the current output? By 'current' do you mean referenced from the previous implementation? Or referenced from the raw input. It does indeed invert the raw input. This is the same as the previous implementation did. The difference is that it does not also invert the special 'y=0' into an arbitrarily large value. Is this your concern? -Dan > > > > > Signed-off-by: Daniel Kurtz <djkurtz@chromium.org> > > --- > > drivers/input/mouse/synaptics.c | 16 +++++++++------- > > 1 files changed, 9 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c > > index e06e045..f6d0c04 100644 > > --- a/drivers/input/mouse/synaptics.c > > +++ b/drivers/input/mouse/synaptics.c > > @@ -44,6 +44,8 @@ > > #define YMIN_NOMINAL 1408 > > #define YMAX_NOMINAL 4448 > > > > +#define INVERT_Y(y) (((y) != 0) ? (YMAX_NOMINAL + YMIN_NOMINAL - (y)) : 0) > > + > > > > /***************************************************************************** > > * Stuff we need even when we do not want native Synaptics support > > @@ -409,9 +411,9 @@ static int synaptics_parse_hw_state(const unsigned char buf[], > > hw->x = (((buf[3] & 0x10) << 8) | > > ((buf[1] & 0x0f) << 8) | > > buf[4]); > > - hw->y = (((buf[3] & 0x20) << 7) | > > + hw->y = INVERT_Y((((buf[3] & 0x20) << 7) | > > ((buf[1] & 0xf0) << 4) | > > - buf[5]); > > + buf[5])); > > > > hw->z = buf[2]; > > hw->w = (((buf[0] & 0x30) >> 2) | > > @@ -421,7 +423,8 @@ static int synaptics_parse_hw_state(const unsigned char buf[], > > if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c) && hw->w == 2) { > > /* Gesture packet: (x, y, z) at half resolution */ > > priv->mt.x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1; > > - priv->mt.y = (((buf[4] & 0xf0) << 4) | buf[2]) << 1; > > + priv->mt.y = INVERT_Y((((buf[4] & 0xf0) << 4) > > + | buf[2]) << 1); > > priv->mt.z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1; > > return 1; > > } > > @@ -473,7 +476,7 @@ static int synaptics_parse_hw_state(const unsigned char buf[], > > } > > } else { > > hw->x = (((buf[1] & 0x1f) << 8) | buf[2]); > > - hw->y = (((buf[4] & 0x1f) << 8) | buf[5]); > > + hw->y = INVERT_Y(((buf[4] & 0x1f) << 8) | buf[5]); > > > > hw->z = (((buf[0] & 0x30) << 2) | (buf[3] & 0x3F)); > > hw->w = (((buf[1] & 0x80) >> 4) | ((buf[0] & 0x04) >> 1)); > > @@ -491,8 +494,7 @@ static void set_slot(struct input_dev *dev, int slot, bool active, int x, int y) > > input_mt_report_slot_state(dev, MT_TOOL_FINGER, active); > > if (active) { > > input_report_abs(dev, ABS_MT_POSITION_X, x); > > - input_report_abs(dev, ABS_MT_POSITION_Y, > > - YMAX_NOMINAL + YMIN_NOMINAL - y); > > + input_report_abs(dev, ABS_MT_POSITION_Y, y); > > } > > } > > > > @@ -584,7 +586,7 @@ static void synaptics_process_packet(struct psmouse *psmouse) > > > > if (num_fingers > 0) { > > input_report_abs(dev, ABS_X, hw.x); > > - input_report_abs(dev, ABS_Y, YMAX_NOMINAL + YMIN_NOMINAL - hw.y); > > + input_report_abs(dev, ABS_Y, hw.y); > > } > > input_report_abs(dev, ABS_PRESSURE, hw.z); > > > > -- > > 1.7.3.1 > > > > Thanks, > Henrik -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 5, 2011 at 1:07 PM, Henrik Rydberg <rydberg@euromail.se> wrote: >> > > In some cases, however, y = 0 is sent by the touchpad. >> > > In these cases, the kernel driver should not invert, and just report 0. >> > > >> > > This patch also refactors the inversion into a macro, and moves it >> > > into packet processing instead of during position reporting. >> > >> > The patch seems to invert the current output? >> >> By 'current' do you mean referenced from the previous implementation? >> Or referenced from the raw input. >> It does indeed invert the raw input. >> This is the same as the previous implementation did. >> The difference is that it does not also invert the special 'y=0' into >> an arbitrarily large value. >> Is this your concern? > > It would be clearer to just change the argument of the > input_report_abs() instances, would it not? An explanation why zero, > outside the value range, should be output also needs a rationale. It > would seem such packets should be masked somehow. > When I saw this, thats what it looked like to me. We already know that hw.x == 1 is invalid and added to a list to filter out but maybe hw.y == 0 is invalid as well and it wasn't as obvious because of the inversion. Sound the following pre-existing line be expanded to its filtered? if (hw.z > 0 && hw.x > 1) { Chris -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 6, 2011 at 1:42 AM, Chase Douglas <chase.douglas@canonical.com> wrote: > > On 06/28/2011 10:07 PM, djkurtz@chromium.org wrote: > > From: Daniel Kurtz <djkurtz@chromium.org> > > > > Synaptics touchpads report increasing y from bottom to top. > > This is inverted from normal userspace "top of screen is 0" coordinates. > > Thus, the kernel driver reports inverted y coordinates to userspace. > > > > In some cases, however, y = 0 is sent by the touchpad. > > In these cases, the kernel driver should not invert, and just report 0. > > Under what cases is y sent as 0, and why do we want to report it as 0 to > userspace? I know of two such cases for the image sensor: (1) When all fingers, save the first finger are removed, an AGM packet with (x=0, y=0, z=0) is sent. (2) When all fingers are removed, an SGM packet with (x=0, y=0, z=0) is sent. After uploading this patch set, I played with the profile sensor again, and I also saw it sometimes sends y=1 packets. I don't know what those are. This is mostly useful for debugging the kernel driver. When observing the raw position values, the special 0 (and 1?) cases are more obvious when not inverted. I think I am misleading in my commit message. I don't believe these are actually ever passed through to userspace. > > > > > This patch also refactors the inversion into a macro, and moves it > > into packet processing instead of during position reporting. > > It's getting really hard to read the bit transformations with a macro. > I'd prefer an approach that splits the computation. Maybe: > > hw->y = bit_manipulations; > hw->y = INVERT_Y(hw->y); > > (could use a temporary variable too) > > Or maybe define a static inline function that takes the hw state as an arg: > > hw->y = bit_manipulations; > fix_y(hw); > > I'd prefer this implementation if every scenario involved manipulating > the y value of the hw state struct. Ok. This option sounds good to me. Both the sgm & agm cases manipulate the same struct. > > Also, it would be great if you could add your explanation of why Y is > inverted as a comment above INVERT_Y (or whatever function/macro it > eventually becomes). Ok. I agree a comment would help. Thanks for the review! -Dan > > Thanks! > > -- Chase > > > Signed-off-by: Daniel Kurtz <djkurtz@chromium.org> > > --- > > drivers/input/mouse/synaptics.c | 16 +++++++++------- > > 1 files changed, 9 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c > > index e06e045..f6d0c04 100644 > > --- a/drivers/input/mouse/synaptics.c > > +++ b/drivers/input/mouse/synaptics.c > > @@ -44,6 +44,8 @@ > > #define YMIN_NOMINAL 1408 > > #define YMAX_NOMINAL 4448 > > > > +#define INVERT_Y(y) (((y) != 0) ? (YMAX_NOMINAL + YMIN_NOMINAL - (y)) : 0) > > + > > > > /***************************************************************************** > > * Stuff we need even when we do not want native Synaptics support > > @@ -409,9 +411,9 @@ static int synaptics_parse_hw_state(const unsigned char buf[], > > hw->x = (((buf[3] & 0x10) << 8) | > > ((buf[1] & 0x0f) << 8) | > > buf[4]); > > - hw->y = (((buf[3] & 0x20) << 7) | > > + hw->y = INVERT_Y((((buf[3] & 0x20) << 7) | > > ((buf[1] & 0xf0) << 4) | > > - buf[5]); > > + buf[5])); > > > > hw->z = buf[2]; > > hw->w = (((buf[0] & 0x30) >> 2) | > > @@ -421,7 +423,8 @@ static int synaptics_parse_hw_state(const unsigned char buf[], > > if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c) && hw->w == 2) { > > /* Gesture packet: (x, y, z) at half resolution */ > > priv->mt.x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1; > > - priv->mt.y = (((buf[4] & 0xf0) << 4) | buf[2]) << 1; > > + priv->mt.y = INVERT_Y((((buf[4] & 0xf0) << 4) > > + | buf[2]) << 1); > > priv->mt.z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1; > > return 1; > > } > > @@ -473,7 +476,7 @@ static int synaptics_parse_hw_state(const unsigned char buf[], > > } > > } else { > > hw->x = (((buf[1] & 0x1f) << 8) | buf[2]); > > - hw->y = (((buf[4] & 0x1f) << 8) | buf[5]); > > + hw->y = INVERT_Y(((buf[4] & 0x1f) << 8) | buf[5]); > > > > hw->z = (((buf[0] & 0x30) << 2) | (buf[3] & 0x3F)); > > hw->w = (((buf[1] & 0x80) >> 4) | ((buf[0] & 0x04) >> 1)); > > @@ -491,8 +494,7 @@ static void set_slot(struct input_dev *dev, int slot, bool active, int x, int y) > > input_mt_report_slot_state(dev, MT_TOOL_FINGER, active); > > if (active) { > > input_report_abs(dev, ABS_MT_POSITION_X, x); > > - input_report_abs(dev, ABS_MT_POSITION_Y, > > - YMAX_NOMINAL + YMIN_NOMINAL - y); > > + input_report_abs(dev, ABS_MT_POSITION_Y, y); > > } > > } > > > > @@ -584,7 +586,7 @@ static void synaptics_process_packet(struct psmouse *psmouse) > > > > if (num_fingers > 0) { > > input_report_abs(dev, ABS_X, hw.x); > > - input_report_abs(dev, ABS_Y, YMAX_NOMINAL + YMIN_NOMINAL - hw.y); > > + input_report_abs(dev, ABS_Y, hw.y); > > } > > input_report_abs(dev, ABS_PRESSURE, hw.z); > > > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 6, 2011 at 2:07 AM, Henrik Rydberg <rydberg@euromail.se> wrote: >> > > In some cases, however, y = 0 is sent by the touchpad. >> > > In these cases, the kernel driver should not invert, and just report 0. >> > > >> > > This patch also refactors the inversion into a macro, and moves it >> > > into packet processing instead of during position reporting. >> > >> > The patch seems to invert the current output? >> >> By 'current' do you mean referenced from the previous implementation? >> Or referenced from the raw input. >> It does indeed invert the raw input. >> This is the same as the previous implementation did. >> The difference is that it does not also invert the special 'y=0' into >> an arbitrarily large value. >> Is this your concern? > > It would be clearer to just change the argument of the > input_report_abs() instances, would it not? An explanation why zero, > outside the value range, should be output also needs a rationale. It > would seem such packets should be masked somehow. Actually, I think it is clearer to change all the y's in one function, during parsing, rather than scattered about in various output functions. I'm sorry, I think I was misleading in my commit message. This change doesn't affect how packets get masked. The y=0 is not handled differently now that it is no longer inverted. If it wasn't sent to userspace before, it still won't be sent. The older inverted y=0 was also, by definition, out of range, so hopefully those packets are masked, and continue to be masked. Not inverting y=0 just makes it a bit easier to debug the driver, by slightly more accurately parsing the incoming raw data. > > Thanks, > Henrik > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/05/2011 03:50 PM, Daniel Kurtz wrote: > On Wed, Jul 6, 2011 at 1:42 AM, Chase Douglas > <chase.douglas@canonical.com> wrote: >> >> On 06/28/2011 10:07 PM, djkurtz@chromium.org wrote: >>> From: Daniel Kurtz <djkurtz@chromium.org> >>> >>> Synaptics touchpads report increasing y from bottom to top. >>> This is inverted from normal userspace "top of screen is 0" coordinates. >>> Thus, the kernel driver reports inverted y coordinates to userspace. >>> >>> In some cases, however, y = 0 is sent by the touchpad. >>> In these cases, the kernel driver should not invert, and just report 0. >> >> Under what cases is y sent as 0, and why do we want to report it as 0 to >> userspace? > > I know of two such cases for the image sensor: > (1) When all fingers, save the first finger are removed, an AGM > packet with (x=0, y=0, z=0) is sent. > (2) When all fingers are removed, an SGM packet with (x=0, y=0, z=0) is sent. > > After uploading this patch set, I played with the profile sensor > again, and I also saw it sometimes sends y=1 packets. I don't know > what those are. > > This is mostly useful for debugging the kernel driver. > When observing the raw position values, the special 0 (and 1?) cases > are more obvious when not inverted. > I think I am misleading in my commit message. I don't believe these > are actually ever passed through to userspace. If they are set, then they are passed through evdev, unless I'm missing something... Given the cases listed above, it seems like we should be special casing these scenarios. Maybe that means dropping the event or handling finger count transitions or something else. I'm happy with the patch as a cleanup of y inversion, but I don't see the utility of passing y = 0 through. With all the debugging techniques we have (printk, systemtap, perf, ftrace, etc.), I don't think we need this functionality, and it might lead a code reviewer to scratch their head wondering what the point was :). Thanks! -- Chase -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 6, 2011 at 7:06 AM, Chase Douglas <chase.douglas@canonical.com> wrote: > On 07/05/2011 03:50 PM, Daniel Kurtz wrote: >> On Wed, Jul 6, 2011 at 1:42 AM, Chase Douglas >> <chase.douglas@canonical.com> wrote: >>> >>> On 06/28/2011 10:07 PM, djkurtz@chromium.org wrote: >>>> From: Daniel Kurtz <djkurtz@chromium.org> >>>> >>>> Synaptics touchpads report increasing y from bottom to top. >>>> This is inverted from normal userspace "top of screen is 0" coordinates. >>>> Thus, the kernel driver reports inverted y coordinates to userspace. >>>> >>>> In some cases, however, y = 0 is sent by the touchpad. >>>> In these cases, the kernel driver should not invert, and just report 0. >>> >>> Under what cases is y sent as 0, and why do we want to report it as 0 to >>> userspace? >> >> I know of two such cases for the image sensor: >> (1) When all fingers, save the first finger are removed, an AGM >> packet with (x=0, y=0, z=0) is sent. >> (2) When all fingers are removed, an SGM packet with (x=0, y=0, z=0) is sent. >> >> After uploading this patch set, I played with the profile sensor >> again, and I also saw it sometimes sends y=1 packets. I don't know >> what those are. >> >> This is mostly useful for debugging the kernel driver. >> When observing the raw position values, the special 0 (and 1?) cases >> are more obvious when not inverted. >> I think I am misleading in my commit message. I don't believe these >> are actually ever passed through to userspace. > > If they are set, then they are passed through evdev, unless I'm missing > something... Given the cases listed above, it seems like we should be > special casing these scenarios. Maybe that means dropping the event or > handling finger count transitions or something else. > > I'm happy with the patch as a cleanup of y inversion, but I don't see > the utility of passing y = 0 through. With all the debugging techniques > we have (printk, systemtap, perf, ftrace, etc.), I don't think we need > this functionality, and it might lead a code reviewer to scratch their > head wondering what the point was :). I meant if you were to printk y, it is much more obvious to see y=0 than y=5321 or something like that. I don't think this reaches userspace because y=0 packets are usually filtered out since z and x are also 0. I guess this is the point, actually, that wasn't clear to me in Henrik's astute observation: We should do the conversion when reporting to userspace. Thus, only do the conversion for packets that aren't already masked! I will refactor to an inline function, but put the conversion back in the input_report_abs() calls. -Dan > > Thanks! > > -- Chase > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 06, 2011 at 07:15:07AM +0800, Daniel Kurtz wrote: > On Wed, Jul 6, 2011 at 7:06 AM, Chase Douglas > <chase.douglas@canonical.com> wrote: > > On 07/05/2011 03:50 PM, Daniel Kurtz wrote: > >> On Wed, Jul 6, 2011 at 1:42 AM, Chase Douglas > >> <chase.douglas@canonical.com> wrote: > >>> > >>> On 06/28/2011 10:07 PM, djkurtz@chromium.org wrote: > >>>> From: Daniel Kurtz <djkurtz@chromium.org> > >>>> > >>>> Synaptics touchpads report increasing y from bottom to top. > >>>> This is inverted from normal userspace "top of screen is 0" coordinates. > >>>> Thus, the kernel driver reports inverted y coordinates to userspace. > >>>> > >>>> In some cases, however, y = 0 is sent by the touchpad. > >>>> In these cases, the kernel driver should not invert, and just report 0. > >>> > >>> Under what cases is y sent as 0, and why do we want to report it as 0 to > >>> userspace? > >> > >> I know of two such cases for the image sensor: > >> (1) When all fingers, save the first finger are removed, an AGM > >> packet with (x=0, y=0, z=0) is sent. > >> (2) When all fingers are removed, an SGM packet with (x=0, y=0, z=0) is sent. > >> > >> After uploading this patch set, I played with the profile sensor > >> again, and I also saw it sometimes sends y=1 packets. I don't know > >> what those are. > >> > >> This is mostly useful for debugging the kernel driver. > >> When observing the raw position values, the special 0 (and 1?) cases > >> are more obvious when not inverted. > >> I think I am misleading in my commit message. I don't believe these > >> are actually ever passed through to userspace. > > > > If they are set, then they are passed through evdev, unless I'm missing > > something... Given the cases listed above, it seems like we should be > > special casing these scenarios. Maybe that means dropping the event or > > handling finger count transitions or something else. > > > > I'm happy with the patch as a cleanup of y inversion, but I don't see > > the utility of passing y = 0 through. With all the debugging techniques > > we have (printk, systemtap, perf, ftrace, etc.), I don't think we need > > this functionality, and it might lead a code reviewer to scratch their > > head wondering what the point was :). > > I meant if you were to printk y, it is much more obvious to see y=0 > than y=5321 or something like that. > I don't think this reaches userspace because y=0 packets are usually > filtered out since z and x are also 0. > > I guess this is the point, actually, that wasn't clear to me in > Henrik's astute observation: > We should do the conversion when reporting to userspace. Thus, only > do the conversion for packets that aren't already masked! > > I will refactor to an inline function, but put the conversion back in > the input_report_abs() calls. Better yet, drop this patch completely. If there is no (or low) pressure coordinate values are not interesting and shoudl be ignored by both humans and userspase consumers. Thanks.
diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c index e06e045..f6d0c04 100644 --- a/drivers/input/mouse/synaptics.c +++ b/drivers/input/mouse/synaptics.c @@ -44,6 +44,8 @@ #define YMIN_NOMINAL 1408 #define YMAX_NOMINAL 4448 +#define INVERT_Y(y) (((y) != 0) ? (YMAX_NOMINAL + YMIN_NOMINAL - (y)) : 0) + /***************************************************************************** * Stuff we need even when we do not want native Synaptics support @@ -409,9 +411,9 @@ static int synaptics_parse_hw_state(const unsigned char buf[], hw->x = (((buf[3] & 0x10) << 8) | ((buf[1] & 0x0f) << 8) | buf[4]); - hw->y = (((buf[3] & 0x20) << 7) | + hw->y = INVERT_Y((((buf[3] & 0x20) << 7) | ((buf[1] & 0xf0) << 4) | - buf[5]); + buf[5])); hw->z = buf[2]; hw->w = (((buf[0] & 0x30) >> 2) | @@ -421,7 +423,8 @@ static int synaptics_parse_hw_state(const unsigned char buf[], if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c) && hw->w == 2) { /* Gesture packet: (x, y, z) at half resolution */ priv->mt.x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1; - priv->mt.y = (((buf[4] & 0xf0) << 4) | buf[2]) << 1; + priv->mt.y = INVERT_Y((((buf[4] & 0xf0) << 4) + | buf[2]) << 1); priv->mt.z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1; return 1; } @@ -473,7 +476,7 @@ static int synaptics_parse_hw_state(const unsigned char buf[], } } else { hw->x = (((buf[1] & 0x1f) << 8) | buf[2]); - hw->y = (((buf[4] & 0x1f) << 8) | buf[5]); + hw->y = INVERT_Y(((buf[4] & 0x1f) << 8) | buf[5]); hw->z = (((buf[0] & 0x30) << 2) | (buf[3] & 0x3F)); hw->w = (((buf[1] & 0x80) >> 4) | ((buf[0] & 0x04) >> 1)); @@ -491,8 +494,7 @@ static void set_slot(struct input_dev *dev, int slot, bool active, int x, int y) input_mt_report_slot_state(dev, MT_TOOL_FINGER, active); if (active) { input_report_abs(dev, ABS_MT_POSITION_X, x); - input_report_abs(dev, ABS_MT_POSITION_Y, - YMAX_NOMINAL + YMIN_NOMINAL - y); + input_report_abs(dev, ABS_MT_POSITION_Y, y); } } @@ -584,7 +586,7 @@ static void synaptics_process_packet(struct psmouse *psmouse) if (num_fingers > 0) { input_report_abs(dev, ABS_X, hw.x); - input_report_abs(dev, ABS_Y, YMAX_NOMINAL + YMIN_NOMINAL - hw.y); + input_report_abs(dev, ABS_Y, hw.y); } input_report_abs(dev, ABS_PRESSURE, hw.z);