From: Dixit Parmar <[email protected]>
For Sitronix st1633 multi-touch controller driver the co-ordinates reported
for multiple fingers were wrong.
So the below mentioned bug was filed,
Bugzilla Bug ID: 204561
While reading co-ordinates from specified I2C registers, the X & Y
co-ordinates should be read from proper I2C address for particular finger as
specified in chip specific datasheet.
for single touch this logic is working fine. However, for multi-touch
scenario the logic of reading data from data buffer has issues.
This patch fixes the reading logic from data buffer.
Previous logic:
* Offset of X & Y Lower byte coordinate is increased by i no. only(by 1 Byte)
for each finger.
New logic:
* The logic of reading X & Y Lower Byte coordinate needs to be increased
by i+y for each time/finger.
Signed-off-by: Dixit Parmar <[email protected]>
---
drivers/input/touchscreen/st1232.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/input/touchscreen/st1232.c b/drivers/input/touchscreen/st1232.c
index 3492339..1139714 100644
--- a/drivers/input/touchscreen/st1232.c
+++ b/drivers/input/touchscreen/st1232.c
@@ -81,8 +81,10 @@ static int st1232_ts_read_data(struct st1232_ts_data *ts)
for (i = 0, y = 0; i < ts->chip_info->max_fingers; i++, y += 3) {
finger[i].is_valid = buf[i + y] >> 7;
if (finger[i].is_valid) {
- finger[i].x = ((buf[i + y] & 0x0070) << 4) | buf[i + 1];
- finger[i].y = ((buf[i + y] & 0x0007) << 8) | buf[i + 2];
+ finger[i].x = ((buf[i + y] & 0x0070) << 4) |
+ buf[i + y + 1];
+ finger[i].y = ((buf[i + y] & 0x0007) << 8) |
+ buf[i + y + 2];
/* st1232 includes a z-axis / touch strength */
if (ts->chip_info->have_z)
--
2.7.4
Am 19.08.2019 12:08 schrieb Dixit Parmar:
> From: Dixit Parmar <[email protected]>
>
> For Sitronix st1633 multi-touch controller driver the co-ordinates
> reported
> for multiple fingers were wrong.
>
> So the below mentioned bug was filed,
> Bugzilla Bug ID: 204561
>
> While reading co-ordinates from specified I2C registers, the X & Y
> co-ordinates should be read from proper I2C address for particular
> finger as
> specified in chip specific datasheet.
>
> for single touch this logic is working fine. However, for multi-touch
> scenario the logic of reading data from data buffer has issues.
>
> This patch fixes the reading logic from data buffer.
>
> Previous logic:
> * Offset of X & Y Lower byte coordinate is increased by i no. only(by 1
> Byte)
> for each finger.
>
> New logic:
> * The logic of reading X & Y Lower Byte coordinate needs to be
> increased
> by i+y for each time/finger.
>
> Signed-off-by: Dixit Parmar <[email protected]>
> ---
> drivers/input/touchscreen/st1232.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/touchscreen/st1232.c
> b/drivers/input/touchscreen/st1232.c
> index 3492339..1139714 100644
> --- a/drivers/input/touchscreen/st1232.c
> +++ b/drivers/input/touchscreen/st1232.c
> @@ -81,8 +81,10 @@ static int st1232_ts_read_data(struct st1232_ts_data
> *ts)
> for (i = 0, y = 0; i < ts->chip_info->max_fingers; i++, y += 3) {
> finger[i].is_valid = buf[i + y] >> 7;
> if (finger[i].is_valid) {
> - finger[i].x = ((buf[i + y] & 0x0070) << 4) | buf[i + 1];
> - finger[i].y = ((buf[i + y] & 0x0007) << 8) | buf[i + 2];
> + finger[i].x = ((buf[i + y] & 0x0070) << 4) |
> + buf[i + y + 1];
> + finger[i].y = ((buf[i + y] & 0x0007) << 8) |
> + buf[i + y + 2];
Seems like you're right. It's simply +1 (for x) and +2 (for y) from the
high-byte locations.
Not sure how that went wrong.
Thank you,
Reviewed-by: Martin Kepplinger <[email protected]>
>
> /* st1232 includes a z-axis / touch strength */
> if (ts->chip_info->have_z)
Any review comments for this?
Or it should be merged?
Thanks.
On Thu, Aug 22, 2019 at 02:08:14PM +0200, Martin Kepplinger wrote:
> Am 19.08.2019 12:08 schrieb Dixit Parmar:
> > From: Dixit Parmar <[email protected]>
> >
> > For Sitronix st1633 multi-touch controller driver the co-ordinates
> > reported
> > for multiple fingers were wrong.
> >
> > So the below mentioned bug was filed,
> > Bugzilla Bug ID: 204561
> >
> > While reading co-ordinates from specified I2C registers, the X & Y
> > co-ordinates should be read from proper I2C address for particular
> > finger as
> > specified in chip specific datasheet.
> >
> > for single touch this logic is working fine. However, for multi-touch
> > scenario the logic of reading data from data buffer has issues.
> >
> > This patch fixes the reading logic from data buffer.
> >
> > Previous logic:
> > * Offset of X & Y Lower byte coordinate is increased by i no. only(by 1
> > Byte)
> > for each finger.
> >
> > New logic:
> > * The logic of reading X & Y Lower Byte coordinate needs to be increased
> > by i+y for each time/finger.
> >
> > Signed-off-by: Dixit Parmar <[email protected]>
> > ---
> > drivers/input/touchscreen/st1232.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/input/touchscreen/st1232.c
> > b/drivers/input/touchscreen/st1232.c
> > index 3492339..1139714 100644
> > --- a/drivers/input/touchscreen/st1232.c
> > +++ b/drivers/input/touchscreen/st1232.c
> > @@ -81,8 +81,10 @@ static int st1232_ts_read_data(struct st1232_ts_data
> > *ts)
> > for (i = 0, y = 0; i < ts->chip_info->max_fingers; i++, y += 3) {
> > finger[i].is_valid = buf[i + y] >> 7;
> > if (finger[i].is_valid) {
> > - finger[i].x = ((buf[i + y] & 0x0070) << 4) | buf[i + 1];
> > - finger[i].y = ((buf[i + y] & 0x0007) << 8) | buf[i + 2];
> > + finger[i].x = ((buf[i + y] & 0x0070) << 4) |
> > + buf[i + y + 1];
> > + finger[i].y = ((buf[i + y] & 0x0007) << 8) |
> > + buf[i + y + 2];
>
> Seems like you're right. It's simply +1 (for x) and +2 (for y) from the
> high-byte locations.
> Not sure how that went wrong.
>
> Thank you,
>
> Reviewed-by: Martin Kepplinger <[email protected]>
>
>
> >
> > /* st1232 includes a z-axis / touch strength */
> > if (ts->chip_info->have_z)
>
On 20.10.19 10:29, Dixit Parmar wrote:
> Any review comments for this?
> Or it should be merged?
>
> Thanks.
My comment and tag is there. This fixes multitouch and should be merged.
martin
>
>
> On Thu, Aug 22, 2019 at 02:08:14PM +0200, Martin Kepplinger wrote:
>> Am 19.08.2019 12:08 schrieb Dixit Parmar:
>>> From: Dixit Parmar <[email protected]>
>>>
>>> For Sitronix st1633 multi-touch controller driver the co-ordinates
>>> reported
>>> for multiple fingers were wrong.
>>>
>>> So the below mentioned bug was filed,
>>> Bugzilla Bug ID: 204561
>>>
>>> While reading co-ordinates from specified I2C registers, the X & Y
>>> co-ordinates should be read from proper I2C address for particular
>>> finger as
>>> specified in chip specific datasheet.
>>>
>>> for single touch this logic is working fine. However, for multi-touch
>>> scenario the logic of reading data from data buffer has issues.
>>>
>>> This patch fixes the reading logic from data buffer.
>>>
>>> Previous logic:
>>> * Offset of X & Y Lower byte coordinate is increased by i no. only(by 1
>>> Byte)
>>> for each finger.
>>>
>>> New logic:
>>> * The logic of reading X & Y Lower Byte coordinate needs to be increased
>>> by i+y for each time/finger.
>>>
>>> Signed-off-by: Dixit Parmar <[email protected]>
>>> ---
>>> drivers/input/touchscreen/st1232.c | 6 ++++--
>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/input/touchscreen/st1232.c
>>> b/drivers/input/touchscreen/st1232.c
>>> index 3492339..1139714 100644
>>> --- a/drivers/input/touchscreen/st1232.c
>>> +++ b/drivers/input/touchscreen/st1232.c
>>> @@ -81,8 +81,10 @@ static int st1232_ts_read_data(struct st1232_ts_data
>>> *ts)
>>> for (i = 0, y = 0; i < ts->chip_info->max_fingers; i++, y += 3) {
>>> finger[i].is_valid = buf[i + y] >> 7;
>>> if (finger[i].is_valid) {
>>> - finger[i].x = ((buf[i + y] & 0x0070) << 4) | buf[i + 1];
>>> - finger[i].y = ((buf[i + y] & 0x0007) << 8) | buf[i + 2];
>>> + finger[i].x = ((buf[i + y] & 0x0070) << 4) |
>>> + buf[i + y + 1];
>>> + finger[i].y = ((buf[i + y] & 0x0007) << 8) |
>>> + buf[i + y + 2];
>>
>> Seems like you're right. It's simply +1 (for x) and +2 (for y) from the
>> high-byte locations.
>> Not sure how that went wrong.
>>
>> Thank you,
>>
>> Reviewed-by: Martin Kepplinger <[email protected]>
>>
>>
>>>
>>> /* st1232 includes a z-axis / touch strength */
>>> if (ts->chip_info->have_z)
>>
On Mon, Oct 21, 2019 at 09:10:23AM +0200, Martin Kepplinger wrote:
> On 20.10.19 10:29, Dixit Parmar wrote:
> > Any review comments for this?
> > Or it should be merged?
> >
> > Thanks.
>
> My comment and tag is there. This fixes multitouch and should be merged.
Missed it earlier, sorry. I am applying it, but I wonder if we shoudl
not do the patch below as I find the version with 2 loop variables quite
confusing.
Thanks.
--
Dmitry
Input: st1232 - simplify parsing of read buffer
From: Dmitry Torokhov <[email protected]>
Signed-off-by: Dmitry Torokhov <[email protected]>
---
drivers/input/touchscreen/st1232.c | 50 ++++++++++++++++++------------------
1 file changed, 25 insertions(+), 25 deletions(-)
diff --git a/drivers/input/touchscreen/st1232.c b/drivers/input/touchscreen/st1232.c
index 1139714e72e2..47033ef3749a 100644
--- a/drivers/input/touchscreen/st1232.c
+++ b/drivers/input/touchscreen/st1232.c
@@ -57,38 +57,38 @@ static int st1232_ts_read_data(struct st1232_ts_data *ts)
{
struct st1232_ts_finger *finger = ts->finger;
struct i2c_client *client = ts->client;
- struct i2c_msg msg[2];
- int error;
- int i, y;
u8 start_reg = ts->chip_info->start_reg;
- u8 *buf = ts->read_buf;
-
- /* read touchscreen data */
- msg[0].addr = client->addr;
- msg[0].flags = 0;
- msg[0].len = 1;
- msg[0].buf = &start_reg;
-
- msg[1].addr = ts->client->addr;
- msg[1].flags = I2C_M_RD;
- msg[1].len = ts->read_buf_len;
- msg[1].buf = buf;
+ struct i2c_msg msg[] = {
+ {
+ .addr = client->addr,
+ .len = sizeof(start_reg),
+ .buf = &start_reg,
+ },
+ {
+ .addr = client->addr,
+ .flags = I2C_M_RD,
+ .len = ts->read_buf_len,
+ .buf = ts->read_buf,
+ }
+ };
+ int ret;
+ int i;
+ u8 *buf;
- error = i2c_transfer(client->adapter, msg, 2);
- if (error < 0)
- return error;
+ ret = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
+ if (ret != ARRAY_SIZE(msg))
+ return ret < 0 ? ret : -EIO;
- for (i = 0, y = 0; i < ts->chip_info->max_fingers; i++, y += 3) {
- finger[i].is_valid = buf[i + y] >> 7;
+ for (i = 0; i < ts->chip_info->max_fingers; i++) {
+ buf = &ts->read_buf[i * 4];
+ finger[i].is_valid = buf[0] >> 7;
if (finger[i].is_valid) {
- finger[i].x = ((buf[i + y] & 0x0070) << 4) |
- buf[i + y + 1];
- finger[i].y = ((buf[i + y] & 0x0007) << 8) |
- buf[i + y + 2];
+ finger[i].x = ((buf[0] & 0x70) << 4) | buf[1];
+ finger[i].y = ((buf[0] & 0x07) << 8) | buf[2];
/* st1232 includes a z-axis / touch strength */
if (ts->chip_info->have_z)
- finger[i].t = buf[i + 6];
+ finger[i].t = ts->read_buf[i + 6];
}
}
On 21.10.19 19:13, Dmitry Torokhov wrote:
> On Mon, Oct 21, 2019 at 09:10:23AM +0200, Martin Kepplinger wrote:
>> On 20.10.19 10:29, Dixit Parmar wrote:
>>> Any review comments for this?
>>> Or it should be merged?
>>>
>>> Thanks.
>>
>> My comment and tag is there. This fixes multitouch and should be merged.
>
> Missed it earlier, sorry. I am applying it, but I wonder if we shoudl
> not do the patch below as I find the version with 2 loop variables quite
> confusing.
>
> Thanks.
>
Makes sense to me.
Acked-by: Martin Kepplinger <[email protected]>
thanks
martin