| Message ID | 1313380295-26226-1-git-send-email-jj_ding@emc.com.tw (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi JJ, On Mon, Aug 15, 2011 at 11:51 AM, JJ Ding <jj_ding@emc.com.tw> wrote: > > x, y values are actually 12-bit long. Also update protocol document to reflect > the change. > > Signed-off-by: JJ Ding <jj_ding@emc.com.tw> Reviewed-by: Daniel Kurtz <djkurtz@chromium.org> > --- > Hello List, > > We are working on adding support for newer elantech touchpad, which we will > submit shortly. Along the way I found this error in current code. > This is a simple patch to correct x, y value ranges in v2 hardware. > Please review, thank you. > > Documentation/input/elantech.txt | 8 ++++---- > drivers/input/mouse/elantech.c | 8 ++++---- > 2 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/Documentation/input/elantech.txt b/Documentation/input/elantech.txt > index db798af..bce9941 100644 > --- a/Documentation/input/elantech.txt > +++ b/Documentation/input/elantech.txt > @@ -389,14 +389,14 @@ byte 0: > byte 1: > > bit 7 6 5 4 3 2 1 0 > - p7 p6 p5 p4 . x10 x9 x8 > + p7 p6 p5 p4 x11 x10 x9 x8 > > byte 2: > > bit 7 6 5 4 3 2 1 0 > x7 x6 x5 x4 x3 x2 x1 x0 > > - x10..x0 = absolute x value (horizontal) > + x11..x0 = absolute x value (horizontal) > > byte 3: > > @@ -420,7 +420,7 @@ byte 3: > byte 4: > > bit 7 6 5 4 3 2 1 0 > - p3 p1 p2 p0 . . y9 y8 > + p3 p1 p2 p0 y11 y10 y9 y8 > > p7..p0 = pressure (not EF113) > > @@ -429,7 +429,7 @@ byte 5: > bit 7 6 5 4 3 2 1 0 > y7 y6 y5 y4 y3 y2 y1 y0 > > - y9..y0 = absolute y value (vertical) > + y11..y0 = absolute y value (vertical) > > > 4.2.2 Two finger touch > diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c > index 3250356..da161da 100644 > --- a/drivers/input/mouse/elantech.c > +++ b/drivers/input/mouse/elantech.c > @@ -290,15 +290,15 @@ static void elantech_report_absolute_v2(struct psmouse *psmouse) > /* pass through... */ > case 1: > /* > - * byte 1: . . . . . x10 x9 x8 > + * byte 1: . . . . x11 x10 x9 x8 > * byte 2: x7 x6 x5 x4 x4 x2 x1 x0 > */ > - x1 = ((packet[1] & 0x07) << 8) | packet[2]; > + x1 = ((packet[1] & 0x0f) << 8) | packet[2]; > /* > - * byte 4: . . . . . . y9 y8 > + * byte 4: . . . . y11 y10 y9 y8 > * byte 5: y7 y6 y5 y4 y3 y2 y1 y0 > */ > - y1 = ETP_YMAX_V2 - (((packet[4] & 0x03) << 8) | packet[5]); > + y1 = ETP_YMAX_V2 - (((packet[4] & 0x0f) << 8) | packet[5]); > > input_report_abs(dev, ABS_X, x1); > input_report_abs(dev, ABS_Y, y1); > -- > 1.7.4.1 > -- 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 15-08-11 05:51, JJ Ding wrote: > x, y values are actually 12-bit long. Also update protocol document to reflect > the change. > > Signed-off-by: JJ Ding<jj_ding@emc.com.tw> > --- > Hello List, > > We are working on adding support for newer elantech touchpad, which we will > submit shortly. Along the way I found this error in current code. > This is a simple patch to correct x, y value ranges in v2 hardware. > Please review, thank you. Hello, I'm glad to hear Elantech is planning to contribute directly to the Linux driver! In your patch, be careful to the Y axis. ETP_YMAX_V2 is actually set to 760. Compared to the 4095 that can be reached, there might be overflow. Do all the v2 hardwares have the same resolution (in which case we can leave 760, or 768)? Or is there a way to query their resolution? Otherwise, we could just put ETP_YMAX_V2 = 4095, and ETP_YMIN_V2 = (4095 - 768), to be sure we cannot have an overflow. I think that's what is done in the synaptics driver. Dmitry, would this seems the correct way to set min/max in input_set_abs_params()? Cheers, Éric > > Documentation/input/elantech.txt | 8 ++++---- > drivers/input/mouse/elantech.c | 8 ++++---- > 2 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/Documentation/input/elantech.txt b/Documentation/input/elantech.txt > index db798af..bce9941 100644 > --- a/Documentation/input/elantech.txt > +++ b/Documentation/input/elantech.txt > @@ -389,14 +389,14 @@ byte 0: > byte 1: > > bit 7 6 5 4 3 2 1 0 > - p7 p6 p5 p4 . x10 x9 x8 > + p7 p6 p5 p4 x11 x10 x9 x8 > > byte 2: > > bit 7 6 5 4 3 2 1 0 > x7 x6 x5 x4 x3 x2 x1 x0 > > - x10..x0 = absolute x value (horizontal) > + x11..x0 = absolute x value (horizontal) > > byte 3: > > @@ -420,7 +420,7 @@ byte 3: > byte 4: > > bit 7 6 5 4 3 2 1 0 > - p3 p1 p2 p0 . . y9 y8 > + p3 p1 p2 p0 y11 y10 y9 y8 > > p7..p0 = pressure (not EF113) > > @@ -429,7 +429,7 @@ byte 5: > bit 7 6 5 4 3 2 1 0 > y7 y6 y5 y4 y3 y2 y1 y0 > > - y9..y0 = absolute y value (vertical) > + y11..y0 = absolute y value (vertical) > > > 4.2.2 Two finger touch > diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c > index 3250356..da161da 100644 > --- a/drivers/input/mouse/elantech.c > +++ b/drivers/input/mouse/elantech.c > @@ -290,15 +290,15 @@ static void elantech_report_absolute_v2(struct psmouse *psmouse) > /* pass through... */ > case 1: > /* > - * byte 1: . . . . . x10 x9 x8 > + * byte 1: . . . . x11 x10 x9 x8 > * byte 2: x7 x6 x5 x4 x4 x2 x1 x0 > */ > - x1 = ((packet[1]& 0x07)<< 8) | packet[2]; > + x1 = ((packet[1]& 0x0f)<< 8) | packet[2]; > /* > - * byte 4: . . . . . . y9 y8 > + * byte 4: . . . . y11 y10 y9 y8 > * byte 5: y7 y6 y5 y4 y3 y2 y1 y0 > */ > - y1 = ETP_YMAX_V2 - (((packet[4]& 0x03)<< 8) | packet[5]); > + y1 = ETP_YMAX_V2 - (((packet[4]& 0x0f)<< 8) | packet[5]); > > input_report_abs(dev, ABS_X, x1); > input_report_abs(dev, ABS_Y, y1); -- 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 Éric, On Mon, 15 Aug 2011 14:45:24 +0200, Éric Piel <E.A.B.Piel@tudelft.nl> wrote: > On 15-08-11 05:51, JJ Ding wrote: > > x, y values are actually 12-bit long. Also update protocol document to reflect > > the change. > > > > Signed-off-by: JJ Ding<jj_ding@emc.com.tw> > > --- > > Hello List, > > > > We are working on adding support for newer elantech touchpad, which we will > > submit shortly. Along the way I found this error in current code. > > This is a simple patch to correct x, y value ranges in v2 hardware. > > Please review, thank you. > Hello, > > I'm glad to hear Elantech is planning to contribute directly to the > Linux driver! I am glad, too. :) > > In your patch, be careful to the Y axis. ETP_YMAX_V2 is actually set to > 760. Compared to the 4095 that can be reached, there might be overflow. > > Do all the v2 hardwares have the same resolution (in which case we can > leave 760, or 768)? Or is there a way to query their resolution? Newer hardware does have a way to query max x, y ranges. We are going to submit such a patch, along with other newer hardware support code. I personally hope to submit the patchset in a few days. > Otherwise, we could just put ETP_YMAX_V2 = 4095, and ETP_YMIN_V2 = (4095 > - 768), to be sure we cannot have an overflow. I think that's what is > done in the synaptics driver. > > Dmitry, would this seems the correct way to set min/max in > input_set_abs_params()? > > Cheers, > Éric > > > > > > Documentation/input/elantech.txt | 8 ++++---- > > drivers/input/mouse/elantech.c | 8 ++++---- > > 2 files changed, 8 insertions(+), 8 deletions(-) > > > > diff --git a/Documentation/input/elantech.txt b/Documentation/input/elantech.txt > > index db798af..bce9941 100644 > > --- a/Documentation/input/elantech.txt > > +++ b/Documentation/input/elantech.txt > > @@ -389,14 +389,14 @@ byte 0: > > byte 1: > > > > bit 7 6 5 4 3 2 1 0 > > - p7 p6 p5 p4 . x10 x9 x8 > > + p7 p6 p5 p4 x11 x10 x9 x8 > > > > byte 2: > > > > bit 7 6 5 4 3 2 1 0 > > x7 x6 x5 x4 x3 x2 x1 x0 > > > > - x10..x0 = absolute x value (horizontal) > > + x11..x0 = absolute x value (horizontal) > > > > byte 3: > > > > @@ -420,7 +420,7 @@ byte 3: > > byte 4: > > > > bit 7 6 5 4 3 2 1 0 > > - p3 p1 p2 p0 . . y9 y8 > > + p3 p1 p2 p0 y11 y10 y9 y8 > > > > p7..p0 = pressure (not EF113) > > > > @@ -429,7 +429,7 @@ byte 5: > > bit 7 6 5 4 3 2 1 0 > > y7 y6 y5 y4 y3 y2 y1 y0 > > > > - y9..y0 = absolute y value (vertical) > > + y11..y0 = absolute y value (vertical) > > > > > > 4.2.2 Two finger touch > > diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c > > index 3250356..da161da 100644 > > --- a/drivers/input/mouse/elantech.c > > +++ b/drivers/input/mouse/elantech.c > > @@ -290,15 +290,15 @@ static void elantech_report_absolute_v2(struct psmouse *psmouse) > > /* pass through... */ > > case 1: > > /* > > - * byte 1: . . . . . x10 x9 x8 > > + * byte 1: . . . . x11 x10 x9 x8 > > * byte 2: x7 x6 x5 x4 x4 x2 x1 x0 > > */ > > - x1 = ((packet[1]& 0x07)<< 8) | packet[2]; > > + x1 = ((packet[1]& 0x0f)<< 8) | packet[2]; > > /* > > - * byte 4: . . . . . . y9 y8 > > + * byte 4: . . . . y11 y10 y9 y8 > > * byte 5: y7 y6 y5 y4 y3 y2 y1 y0 > > */ > > - y1 = ETP_YMAX_V2 - (((packet[4]& 0x03)<< 8) | packet[5]); > > + y1 = ETP_YMAX_V2 - (((packet[4]& 0x0f)<< 8) | packet[5]); > > > > input_report_abs(dev, ABS_X, x1); > > input_report_abs(dev, ABS_Y, y1); > > -- > 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 -- 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 Dmitry, What do you think about this patch? This is really a simple correction, I am wondering if it's OK to push this to 3.1? On Mon, 15 Aug 2011 11:51:35 +0800, JJ Ding <jj_ding@emc.com.tw> wrote: > x, y values are actually 12-bit long. Also update protocol document to reflect > the change. > > Signed-off-by: JJ Ding <jj_ding@emc.com.tw> > --- > Hello List, > > We are working on adding support for newer elantech touchpad, which we will > submit shortly. Along the way I found this error in current code. > This is a simple patch to correct x, y value ranges in v2 hardware. > Please review, thank you. > > Documentation/input/elantech.txt | 8 ++++---- > drivers/input/mouse/elantech.c | 8 ++++---- > 2 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/Documentation/input/elantech.txt b/Documentation/input/elantech.txt > index db798af..bce9941 100644 > --- a/Documentation/input/elantech.txt > +++ b/Documentation/input/elantech.txt > @@ -389,14 +389,14 @@ byte 0: > byte 1: > > bit 7 6 5 4 3 2 1 0 > - p7 p6 p5 p4 . x10 x9 x8 > + p7 p6 p5 p4 x11 x10 x9 x8 > > byte 2: > > bit 7 6 5 4 3 2 1 0 > x7 x6 x5 x4 x3 x2 x1 x0 > > - x10..x0 = absolute x value (horizontal) > + x11..x0 = absolute x value (horizontal) > > byte 3: > > @@ -420,7 +420,7 @@ byte 3: > byte 4: > > bit 7 6 5 4 3 2 1 0 > - p3 p1 p2 p0 . . y9 y8 > + p3 p1 p2 p0 y11 y10 y9 y8 > > p7..p0 = pressure (not EF113) > > @@ -429,7 +429,7 @@ byte 5: > bit 7 6 5 4 3 2 1 0 > y7 y6 y5 y4 y3 y2 y1 y0 > > - y9..y0 = absolute y value (vertical) > + y11..y0 = absolute y value (vertical) > > > 4.2.2 Two finger touch > diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c > index 3250356..da161da 100644 > --- a/drivers/input/mouse/elantech.c > +++ b/drivers/input/mouse/elantech.c > @@ -290,15 +290,15 @@ static void elantech_report_absolute_v2(struct psmouse *psmouse) > /* pass through... */ > case 1: > /* > - * byte 1: . . . . . x10 x9 x8 > + * byte 1: . . . . x11 x10 x9 x8 > * byte 2: x7 x6 x5 x4 x4 x2 x1 x0 > */ > - x1 = ((packet[1] & 0x07) << 8) | packet[2]; > + x1 = ((packet[1] & 0x0f) << 8) | packet[2]; > /* > - * byte 4: . . . . . . y9 y8 > + * byte 4: . . . . y11 y10 y9 y8 > * byte 5: y7 y6 y5 y4 y3 y2 y1 y0 > */ > - y1 = ETP_YMAX_V2 - (((packet[4] & 0x03) << 8) | packet[5]); > + y1 = ETP_YMAX_V2 - (((packet[4] & 0x0f) << 8) | packet[5]); > > input_report_abs(dev, ABS_X, x1); > input_report_abs(dev, ABS_Y, y1); > -- > 1.7.4.1 > -- 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 16-08-11 13:11, JJ Ding wrote: > Hi Dmitry, > > What do you think about this patch? > This is really a simple correction, I am wondering if it's OK to push > this to 3.1? Hi JJ Ding, The merge window of 3.1 is already closed, so only patches which fix bugs are accepted. Is there any hardware already available for which this patch solves a bug? That would be a great incentive to push it :-) Éric -- 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 Éric, Thanks for the reply. On Tue, 16 Aug 2011 23:50:47 +0200, Éric Piel <E.A.B.Piel@tudelft.nl> wrote: > On 16-08-11 13:11, JJ Ding wrote: > > Hi Dmitry, > > > > What do you think about this patch? > > This is really a simple correction, I am wondering if it's OK to push > > this to 3.1? > Hi JJ Ding, > The merge window of 3.1 is already closed, so only patches which fix > bugs are accepted. Is there any hardware already available for which > this patch solves a bug? That would be a great incentive to push it > > :-) As far as I know, this doesn't fix a bug. I just want to make sure the driver and what the firmware sends are consistent. Thank you for telling me this. I will try this again with newer hardware support patches, aiming for 3.2 inclusion. jj > Éric > -- 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 JJ, On Wed, Aug 17, 2011 at 09:28:04AM +0800, JJ Ding wrote: > Hi Éric, > > Thanks for the reply. > > On Tue, 16 Aug 2011 23:50:47 +0200, Éric Piel <E.A.B.Piel@tudelft.nl> wrote: > > On 16-08-11 13:11, JJ Ding wrote: > > > Hi Dmitry, > > > > > > What do you think about this patch? > > > This is really a simple correction, I am wondering if it's OK to push > > > this to 3.1? > > Hi JJ Ding, > > The merge window of 3.1 is already closed, so only patches which fix > > bugs are accepted. Is there any hardware already available for which > > this patch solves a bug? That would be a great incentive to push it > > > :-) > As far as I know, this doesn't fix a bug. I just want to make sure the > driver and what the firmware sends are consistent. As Éric mentioned, simply extending range to 12 bits is dangerous because we might cause overflows. Blindly increasing ETP_YMAX_V2 is not a good idea either as userspace would expect much larger device and reports would only cover fraction of its surface. We need to separate ETP_YMAX_V2 as absolute maximum from reported coordinates range, like it is done in synaptics driver. > > Thank you for telling me this. I will try this again with newer hardware > support patches, aiming for 3.2 inclusion. Yes, if hardware currently supported by mainline version of the driver is not affected it is better to queue the patch[es] for 3.2. BTW, that means that you should not wait till 3.2 merge window opens as we'll need time to review the patches and get them into my 'next' branch. Thanks.
Op 17-08-11 08:43, Dmitry Torokhov schreef: > Hi JJ, > >> As far as I know, this doesn't fix a bug. I just want to make sure the >> driver and what the firmware sends are consistent. > > As Éric mentioned, simply extending range to 12 bits is dangerous > because we might cause overflows. Blindly increasing ETP_YMAX_V2 is not > a good idea either as userspace would expect much larger device and > reports would only cover fraction of its surface. We need to separate > ETP_YMAX_V2 as absolute maximum from reported coordinates range, like it > is done in synaptics driver. Hi Dmitry, Concerning this point, I was wondering if it'd be ok to only send negative values? Such as min/max are between -768 and 0. So that, if the hardware actually has a bigger resolution, it can go down to -4095 without any overflow? It sounds the simplest way to report the coordinate for a device which has an opposite axis. Or is it safer to stay in the positive numbers, with min/max between 3328 and 4095, and in case of bigger resolution the values go down to 0? Cheers, Éric -- 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, Aug 17, 2011 at 11:53:55AM +0200, Éric Piel wrote: > Op 17-08-11 08:43, Dmitry Torokhov schreef: > >Hi JJ, > > > >>As far as I know, this doesn't fix a bug. I just want to make sure the > >>driver and what the firmware sends are consistent. > > > >As Éric mentioned, simply extending range to 12 bits is dangerous > >because we might cause overflows. Blindly increasing ETP_YMAX_V2 is not > >a good idea either as userspace would expect much larger device and > >reports would only cover fraction of its surface. We need to separate > >ETP_YMAX_V2 as absolute maximum from reported coordinates range, like it > >is done in synaptics driver. > Hi Dmitry, > Concerning this point, I was wondering if it'd be ok to only send > negative values? Such as min/max are between -768 and 0. So that, if > the hardware actually has a bigger resolution, it can go down to > -4095 without any overflow? Theoretically it should be possible, at least nothing in evdev protocol says that absolute coordinates shoudl be positive. It is certainly not true for joystick-like devvices. But I am concerned about users (as in applications/drivers) getting negative coordinates for touchpad/touchscreen like devices. I believe first reaction is to expect non-negative values. > It sounds the simplest way to report the > coordinate for a device which has an opposite axis. Or is it safer > to stay in the positive numbers, with min/max between 3328 and 4095, > and in case of bigger resolution the values go down to 0? I think this is the best way. EVIOCGABS gives the "window" into the expected range of coordinates emitted by the device, so as long as we keep the scale (i.e. not reporting min/max as 0-4096 while actual coordinates go only 0-768) it should work fine.
diff --git a/Documentation/input/elantech.txt b/Documentation/input/elantech.txt index db798af..bce9941 100644 --- a/Documentation/input/elantech.txt +++ b/Documentation/input/elantech.txt @@ -389,14 +389,14 @@ byte 0: byte 1: bit 7 6 5 4 3 2 1 0 - p7 p6 p5 p4 . x10 x9 x8 + p7 p6 p5 p4 x11 x10 x9 x8 byte 2: bit 7 6 5 4 3 2 1 0 x7 x6 x5 x4 x3 x2 x1 x0 - x10..x0 = absolute x value (horizontal) + x11..x0 = absolute x value (horizontal) byte 3: @@ -420,7 +420,7 @@ byte 3: byte 4: bit 7 6 5 4 3 2 1 0 - p3 p1 p2 p0 . . y9 y8 + p3 p1 p2 p0 y11 y10 y9 y8 p7..p0 = pressure (not EF113) @@ -429,7 +429,7 @@ byte 5: bit 7 6 5 4 3 2 1 0 y7 y6 y5 y4 y3 y2 y1 y0 - y9..y0 = absolute y value (vertical) + y11..y0 = absolute y value (vertical) 4.2.2 Two finger touch diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c index 3250356..da161da 100644 --- a/drivers/input/mouse/elantech.c +++ b/drivers/input/mouse/elantech.c @@ -290,15 +290,15 @@ static void elantech_report_absolute_v2(struct psmouse *psmouse) /* pass through... */ case 1: /* - * byte 1: . . . . . x10 x9 x8 + * byte 1: . . . . x11 x10 x9 x8 * byte 2: x7 x6 x5 x4 x4 x2 x1 x0 */ - x1 = ((packet[1] & 0x07) << 8) | packet[2]; + x1 = ((packet[1] & 0x0f) << 8) | packet[2]; /* - * byte 4: . . . . . . y9 y8 + * byte 4: . . . . y11 y10 y9 y8 * byte 5: y7 y6 y5 y4 y3 y2 y1 y0 */ - y1 = ETP_YMAX_V2 - (((packet[4] & 0x03) << 8) | packet[5]); + y1 = ETP_YMAX_V2 - (((packet[4] & 0x0f) << 8) | packet[5]); input_report_abs(dev, ABS_X, x1); input_report_abs(dev, ABS_Y, y1);
x, y values are actually 12-bit long. Also update protocol document to reflect the change. Signed-off-by: JJ Ding <jj_ding@emc.com.tw> --- Hello List, We are working on adding support for newer elantech touchpad, which we will submit shortly. Along the way I found this error in current code. This is a simple patch to correct x, y value ranges in v2 hardware. Please review, thank you. Documentation/input/elantech.txt | 8 ++++---- drivers/input/mouse/elantech.c | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-)