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

[Javier Martinez Canillas]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/