RE: [PATCH V3 1/3] Input: cyttsp - Cypress TTSP capacitivemulti-touch screen support

[Kevin McNeely]

Hi Javier,

Yes, the hardware reports touches in the range 1 to 14.

Best regards,
Kevin


> -----Original Message-----
> From: Javier Martinez Canillas [mailto:martinez.javier@xxxxxxxxx]
> Sent: Wednesday, September 28, 2011 4:23 PM
> To: Henrik Rydberg
> Cc: Dmitry Torokhov; Greg Kroah-Hartman; linux-input@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; Kevin McNeely
> Subject: Re: [PATCH V3 1/3] Input: cyttsp - Cypress TTSP capacitive multi-
> touch screen support
>
> On Tue, Sep 27, 2011 at 1:52 PM, Henrik Rydberg <rydberg@xxxxxxxxxxx>
> wrote:
> > Hi Javier,
> >
> >> Cypress TrueTouch(tm) Standard Product controllers are found in a
> >> wide range of embedded devices. This driver add support for a variety
> >> of TTSP controllers.
> >
> > please find some comments below.
> >
>
> Hello Henrik,
>
> Thanks a lot for the review
>
> >> +/* Slots management */
> >> +#define CY_MAX_FINGER               4 #define CY_UNUSED
> >> +0 #define CY_USED                     1
> >
> > These two look like bool, please use as bool instead.
> >
> >> +#define CY_MAX_ID                   15
> >
> > Why not 16 here?
> >
>
> Because as far as I know there are only 14 possible tracking ids (1-14), not 16.
> 15 and 0 are not valid track ids.
>
> When a contact lifts off the hardware reports a magic number (15) for it.
> Also, the sequence always start with 1 which is the track id that gets the first
> contact reported by the touch.
>
> >> +
> >> +/* TrueTouch Standard Product Gen3 interface definition */ struct
> >> +cyttsp_xydata {
> >> +     u8 hst_mode;
> >> +     u8 tt_mode;
> >> +     u8 tt_stat;
> >> +     struct cyttsp_tch tch1;
> >> +     u8 touch12_id;
> >> +     struct cyttsp_tch tch2;
> >> +     u8 gest_cnt;
> >> +     u8 gest_id;
> >> +     struct cyttsp_tch tch3;
> >> +     u8 touch34_id;
> >> +     struct cyttsp_tch tch4;
> >> +     u8 tt_undef[3];
> >> +     u8 act_dist;
> >> +     u8 tt_reserved;
> >> +} __packed;
> >
> > Too bad the touches are not an array here...
> >
>
> Yes, the problem is that the offset between touches is not uniform.
> For tch1 and tch2 is only 8 bits but the offset between tch2 and tch3 is 16 (u8
> * 2) for example.
>
> >> +     int slot_curr[CY_MAX_ID];
> >> +     int slot_prev[CY_MAX_ID];
> >
> > These two are not needed; the input core will take care of duplicates,
> > so slot_prev can be removed. Also, the set of current slots can be
> > tracked using a temporary bitmask, so slot_curr can be removed as well.
>
> Perfect, I'll remove both. I didn't know that the input core took care of
> duplicates, sorry for not getting this from the docs.
>
> >> +
> >> +static void cyttsp_get_tch(struct cyttsp_xydata *xy_data, int idx,
> >> +                        struct cyttsp_tch **tch) {
> >> +     switch (idx) {
> >> +     case 0:
> >> +             *tch = &xy_data->tch1;
> >> +             break;
> >> +     case 1:
> >> +             *tch = &xy_data->tch2;
> >> +             break;
> >> +     case 2:
> >> +             *tch = &xy_data->tch3;
> >> +             break;
> >> +     case 3:
> >> +             *tch = &xy_data->tch4;
> >> +             break;
> >> +     }
> >> +}
> >
> > How about returning a const struct cyttsp_tch* here instead.
> >
>
> Ok, will return a struct pointer instead.
>
> >> +
> >> +     cyttsp_extract_track_ids(&xy_data, ids);
> >> +
> >> +     for (i = 0; i < num_cur_tch; i++) {
> >> +             ts->slot_curr[ids[i] - 1] = CY_USED;
> >
> > Why the -1 here? Since the values are provably between 0 and 15, there
> > is no need to change them further.
>
> As I told you before is a HW thing. I looked at the track ids reported by the
> touchscreen and their values are always between 1 and 14. So I did the -1 to
> avoid having and unused element in the array. (I don't have the HW
> datasheet so maybe I'm wrong with this)
>
> Kevin?
>
> >
> > Also, the above line could be replaced by "used |= 1 << ids[i]", for instance.
> >
> >> +
> >> +             cyttsp_get_tch(&xy_data, i, &tch);
> >> +
> >> +             x = be16_to_cpu(tch->x);
> >> +             y = be16_to_cpu(tch->y);
> >> +             z = tch->z;
> >> +
> >> +             cyttsp_report_slot(ts->input, ids[i] - 1, x, y, z);
> >
> > Ditto, -1.
> >
> >> +     }
> >> +
> >> +     for (i = 0; i < CY_MAX_ID; i++) {
> >> +             if (ts->slot_prev[i] == CY_USED &&
> >> +                 ts->slot_curr[i] == CY_UNUSED)
> >> +                     cyttsp_report_slot_empty(ts->input, i);
> >> +             ts->slot_prev[i] = ts->slot_curr[i];
> >> +             ts->slot_curr[i] = CY_UNUSED;
> >> +     }
> >
> > Input core handles duplicate calls, so the above could be simplified.
> >
>
> Perfect, will clean this code.
>
> >> +
> >> +     for (i = 0; i < CY_MAX_ID; i++) {
> >> +             ts->slot_prev[i] = CY_UNUSED;
> >> +             ts->slot_curr[i] = CY_UNUSED;
> >> +     }
> >
> > Could be removed.
> >
>
> Yes, I'll do that.
>
> >
> > Thanks,
> > Henrik
> >
>
> Thanks for your comments, I will work on these issues and resubmit a new
> version of the patch-set.
>
> Best regards,
>
> --
> Javier MartÃnez Canillas
> (+34) 682 39 81 69
> Barcelona, Spain

This message and any attachments may contain Cypress (or its subsidiaries) confidential information. If it has been received in error, please advise the sender and immediately delete this message.
èº{.nÇ+‰·Ÿ®‰­†+%ŠËlzwm…ébëæìr¸›zX§»®w¥Š{ayºÊÚë,j­¢f£¢·hš‹àz¹®w¥¢¸¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾«‘êçzZ+ƒùšŽŠÝj"ú!¶iO•æ¬z·švØ^¶m§ÿðÃnÆàþY&—