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��������+%������w��{.n�����{��)��^n�r������&��z�ޗ�zf���h���~����������_��+v���)ߣ�
