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 [off-list ref] wrote:

Hi Javier,

quoted

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

quoted

+/* 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.

quoted

+#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.

quoted

+
+/* 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.

quoted

+     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.

quoted

+
+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.

quoted

+
+     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.

quoted

+
+             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.

quoted

+     }
+
+     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.

quoted

+
+     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