lkml.org 
[lkml]   [2010]   [Nov]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] touchscreen: Cypress TTSP G3 MTDEV Core Driver
    On 11/09/2010 07:25 PM, Kevin McNeely wrote:

    > Initial release of Cypress TTSP Gen3 Core Driver.
    > Core Driver includes platform data definition file,
    > core driver definition file, and core touchscreen
    > touch handling of device data. Generates
    > multi-touch input events.
    >
    > Signed-off-by: Kevin McNeely <kev@cypress.com>

    > ---


    Thanks for your submission. Please find comments on MT inline. On a general
    note, it is strongly recommended that this driver be converted to the MT slots
    (type B) protocol.

    > +
    > +/* TrueTouch Standard Product Gen3 interface definition */
    > +struct cyttsp_xydata {
    > + u8 hst_mode;
    > + u8 tt_mode;
    > + u8 tt_stat;
    > + struct cyttsp_touch tch1;
    > + u8 touch12_id;
    > + struct cyttsp_touch tch2;
    > + u8 gest_cnt;
    > + u8 gest_id;
    > + struct cyttsp_touch tch3;
    > + u8 touch34_id;
    > + struct cyttsp_touch tch4;
    > + u8 tt_undef[3];
    > + u8 gest_set;


    I take it the gesture functionality is not in use in this driver?

    > + u8 tt_reserved;
    > +};
    > +
    > +
    > +struct cyttsp_tch {
    > + struct cyttsp_touch *tch;
    > + u8 *id;
    > +};


    Given how this mapping is used, it could possibly be dropped altogether. See
    further comment on cyttsp_init_tch_map().

    > +

    > +struct cyttsp_trk {
    > + u8 tch;
    > + u8 w;


    It seems the device does not report contact width, so it is better not reported
    at all.

    > + u16 x;
    > + u16 y;
    > + u8 z;
    > +};
    > +
    > +struct cyttsp {
    > + struct device *dev;
    > + int irq;
    > + struct input_dev *input;
    > + struct mutex mutex;
    > + char phys[32];
    > + struct bus_type *bus_type;
    > + struct cyttsp_platform_data *platform_data;
    > + struct cyttsp_xydata xy_data;
    > + struct cyttsp_bootloader_data bl_data;
    > + struct cyttsp_sysinfo_data sysinfo_data;
    > + struct cyttsp_trk prv_trk[CY_NUM_TRK_ID];


    Apart from the ids, what information is used from the previous frame?

    > +static int cyttsp_gesture_setup(struct cyttsp *ts)
    > +{
    > + int retval;
    > + u8 gesture_setup;
    > +
    > + /* Init gesture; active distance setup */
    > + gesture_setup = ts->platform_data->gest_set;
    > + retval = ttsp_write_block_data(ts, CY_REG_GEST_SET,
    > + sizeof(gesture_setup), &gesture_setup);
    > +
    > + return retval;
    > +}


    What does this initialization actually do?

    > +
    > +static void cyttsp_init_tch_map(struct cyttsp *ts)
    > +{
    > + ts->tch_map[0].tch = &ts->xy_data.tch1;
    > + ts->tch_map[0].id = &ts->xy_data.touch12_id;
    > + ts->tch_map[1].tch = &ts->xy_data.tch2;
    > + ts->tch_map[1].id = &ts->xy_data.touch12_id;
    > + ts->tch_map[2].tch = &ts->xy_data.tch3;
    > + ts->tch_map[2].id = &ts->xy_data.touch34_id;
    > + ts->tch_map[3].tch = &ts->xy_data.tch4;
    > + ts->tch_map[3].id = &ts->xy_data.touch34_id;
    > +}


    Calling cyttsp_get_xydata() four times with special arguments would make this
    function unnecessary.

    > +
    > +static void handle_multi_touch(struct cyttsp_track_data *t, struct cyttsp *ts)
    > +{
    > + u8 id;
    > + u8 cnt = 0;
    > +
    > + /* terminate any previous touch where the track
    > + * is missing from the current event
    > + */
    > + for (id = 0; id < CY_NUM_TRK_ID; id++) {
    > + if (t->cur_trk[id].tch) {
    > + /* put active current track data */
    > + input_report_abs(ts->input,
    > + ABS_MT_TRACKING_ID, id);

    > + input_report_abs(ts->input,
    > + ABS_MT_WIDTH_MAJOR, t->cur_trk[id].w);


    This value does not seem to be reported by the device and should be dropped.

    > + input_report_abs(ts->input,
    > + ABS_MT_POSITION_X, t->cur_trk[id].x);
    > + input_report_abs(ts->input,
    > + ABS_MT_POSITION_Y, t->cur_trk[id].y);
    > + input_report_abs(ts->input,
    > + ABS_MT_TOUCH_MAJOR, t->cur_trk[id].z);
    > + input_mt_sync(ts->input);
    > +
    > + dev_dbg(ts->dev, "%s: MT1: X=%d Y=%d Z=%d\n",
    > + __func__,
    > + t->cur_trk[id].x,
    > + t->cur_trk[id].y,
    > + t->cur_trk[id].z);
    > + /* save current track data into previous track data */
    > + ts->prv_trk[id] = t->cur_trk[id];
    > + cnt++;
    > + } else if (ts->prv_trk[id].tch) {
    > + /* put lift-off previous track data */
    > + input_report_abs(ts->input,
    > + ABS_MT_TRACKING_ID, id);


    Reporting tracking id here unfortunately does not work very well. With the type
    A protocol, ids not reported will automatically be removed, and

    > + input_report_abs(ts->input,
    > + ABS_MT_WIDTH_MAJOR, ts->prv_trk[id].w);
    > + input_report_abs(ts->input,
    > + ABS_MT_POSITION_X, ts->prv_trk[id].x);
    > + input_report_abs(ts->input,
    > + ABS_MT_POSITION_Y, ts->prv_trk[id].y);
    > + input_report_abs(ts->input,
    > + ABS_MT_TOUCH_MAJOR, CY_NTCH);


    checking for zero touch like this only applies for drivers not reporting
    tracking id.

    > + input_mt_sync(ts->input);
    > +
    > + dev_dbg(ts->dev, "%s: MT1: X=%d Y=%d Z=%d lift-off\n",
    > + __func__,
    > + ts->prv_trk[id].x,
    > + ts->prv_trk[id].y,
    > + CY_NTCH);
    > + ts->prv_trk[id].tch = CY_NTCH;
    > + cnt++;
    > + }
    > + }
    > +
    > + /* signal the view motion event */
    > + if (cnt)
    > + input_sync(ts->input);
    > +}


    Since the device does its own tracking, the driver would benefit greatly from
    using the type B protocol.

    > +static int cyttsp_xy_worker(struct cyttsp *ts)
    > +{
    > + u8 cur_tch = 0;
    > + u8 tch;
    > + struct cyttsp_track_data trk;
    > +
    > + /* get event data from CYTTSP device */
    > + if (ttsp_read_block_data(ts,
    > + CY_REG_BASE, sizeof(struct cyttsp_xydata), &ts->xy_data))
    > + return 0;
    > +
    > + /* touch extension handling */
    > + if (ttsp_tch_ext(ts, &ts->xy_data))
    > + return 0;
    > +
    > + /* provide flow control handshake */
    > + if (ts->platform_data->use_hndshk)
    > + if (cyttsp_hndshk(ts, ts->xy_data.hst_mode))
    > + return 0;
    > +
    > + cur_tch = GET_NUM_TOUCHES(ts->xy_data.tt_stat);
    > +
    > + if (ts->bus_ops->power_state == CY_IDLE_STATE)
    > + return 0;
    > + else if (GET_BOOTLOADERMODE(ts->xy_data.tt_mode)) {
    > + return -1;
    > + } else if (IS_LARGE_AREA(ts->xy_data.tt_stat) == 1) {
    > + /* terminate all active tracks */
    > + cur_tch = CY_NTCH;
    > + dev_dbg(ts->dev, "%s: Large area detected\n", __func__);
    > + } else if (cur_tch > CY_NUM_TCH_ID) {
    > + /* terminate all active tracks */
    > + cur_tch = CY_NTCH;
    > + dev_dbg(ts->dev, "%s: Num touch error detected\n", __func__);
    > + } else if (IS_BAD_PKT(ts->xy_data.tt_mode)) {
    > + /* terminate all active tracks */
    > + cur_tch = CY_NTCH;
    > + dev_dbg(ts->dev, "%s: Invalid buffer detected\n", __func__);
    > + }
    > +
    > + /* process the touches */
    > + cyttsp_init_cur_trks(&trk);
    > +
    > + for (tch = 0; tch < cur_tch; tch++) {
    > + cyttsp_get_xydata(ts, &trk,
    > + tch & 0x01 ?
    > + (*(ts->tch_map[tch].id) & 0x0F) :
    > + (*(ts->tch_map[tch].id) & 0xF0) >> 4,
    > + CY_SMALL_TOOL_WIDTH,

    > + be16_to_cpu((ts->tch_map[tch].tch)->x),
    > + be16_to_cpu((ts->tch_map[tch].tch)->y),
    > + (ts->tch_map[tch].tch)->z);
    > + }


    How about expanding this loop with special arguments instead?

    > +/*
    > + * Active distance in pixels for a gesture to be reported
    > + * if set to 0, then all gesture movements are reported
    > + * Valid range is 0 - 15
    > + */
    > +#define CY_ACT_DIST_DFLT 8
    > +#define CY_ACT_DIST CY_ACT_DIST_DFLT


    These do not seem to be used anywhere.

    > +
    > +enum cyttsp_gest {
    > + CY_GEST_GRP_NONE = 0,
    > + CY_GEST_GRP1 = 0x10,
    > + CY_GEST_GRP2 = 0x20,
    > + CY_GEST_GRP3 = 0x40,
    > + CY_GEST_GRP4 = 0x80,
    > +};


    Neither do these.

    Thanks,
    Henrik


    \
     
     \ /
      Last update: 2010-11-15 17:51    [W:0.039 / U:0.080 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site