From: Henrik Rydberg <rydberg@euromail.se>
To: Kevin McNeely <kev@cypress.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>,
David Brown <davidb@codeaurora.org>,
Trilok Soni <tsoni@codeaurora.org>,
Samuel Ortiz <sameo@linux.intel.com>,
Eric Miao <eric.y.miao@gmail.com>,
Simtec Linux Team <linux@simtec.co.uk>,
Luotao Fu <l.fu@pengutronix.de>,
linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] touchscreen: Cypress TTSP G3 MTDEV Core Driver
Date: Mon, 15 Nov 2010 17:46:04 +0100 [thread overview]
Message-ID: <4CE163CC.1060202@euromail.se> (raw)
In-Reply-To: <1289327120-2612-1-git-send-email-kev@cypress.com>
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
next prev parent reply other threads:[~2010-11-15 16:47 UTC|newest]
Thread overview: 70+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <Kevin McNeely <kev@cypress.com>
2010-07-12 20:56 ` [PATCH] i2c: cyttsp i2c touchscreen driver init submit Kevin McNeely
2010-07-13 2:34 ` Christoph Fritz
2010-08-04 16:30 ` Kevin McNeely
2010-07-13 6:48 ` Henrik Rydberg
2010-08-04 16:38 ` Kevin McNeely
2010-07-13 7:31 ` Trilok Soni
2010-07-13 7:55 ` Dmitry Torokhov
2010-07-13 8:42 ` Trilok Soni
2010-07-22 10:33 ` Trilok Soni
2010-07-27 15:20 ` Kevin McNeely
2010-08-04 17:27 ` Kevin McNeely
2010-07-19 9:28 ` Jean Delvare
2010-08-04 17:22 ` Kevin McNeely
2010-08-05 18:12 ` [PATCH] i2c: cyttsp i2c and spi " Kevin McNeely
2010-08-05 20:45 ` Trilok Soni
2010-08-05 21:07 ` Dmitry Torokhov
2010-08-07 0:39 ` Kevin McNeely
2010-08-07 0:52 ` Kevin McNeely
2010-08-05 23:06 ` Henrik Rydberg
2010-08-07 0:32 ` Kevin McNeely
2010-08-07 0:49 ` Henrik Rydberg
2010-08-10 0:51 ` Kevin McNeely
2010-08-06 9:06 ` Trilok Soni
2010-08-10 0:49 ` Kevin McNeely
2010-11-09 18:25 ` [PATCH] touchscreen: Cypress TTSP G3 MTDEV Core Driver Kevin McNeely
2010-11-15 16:46 ` Henrik Rydberg [this message]
2010-11-19 17:39 ` Kevin McNeely
2010-12-01 7:22 ` Trilok Soni
2010-12-01 14:38 ` Henrik Rydberg
2010-12-01 23:59 ` Kevin McNeely
2010-12-02 0:01 ` Henrik Rydberg
2010-12-02 0:34 ` Dmitry Torokhov
2010-11-09 18:25 ` [PATCH] i2c: Cypress TTSP G3 MTDEV I2C Device Driver Kevin McNeely
2010-11-09 18:25 ` [PATCH] spi: Cypress TTSP G3 MTDEV SPI " Kevin McNeely
2010-12-04 2:06 ` [v2] touchscreen Cypress TTSP G3 MTDEV Core Driver Kevin McNeely
2010-12-05 9:11 ` Henrik Rydberg
2010-12-04 2:06 ` [v2] 2/3 i2c: Cypress TTSP G3 MTDEV I2C Device Driver Kevin McNeely
2010-12-04 2:06 ` [v2] 3/3 spi: Cypress TTSP G3 MTDEV SPI " Kevin McNeely
2010-12-29 19:17 ` [v3 1/3] 1/3 Touchscreen: Cypress TTSP G3 MTDEV Core Driver Kevin McNeely
2010-12-30 6:04 ` Shubhrajyoti Datta
2011-01-05 0:45 ` Kevin McNeely
2010-12-31 11:53 ` Henrik Rydberg
2010-12-31 12:55 ` Trilok Soni
2010-12-31 13:58 ` Henrik Rydberg
2011-01-03 9:44 ` Trilok Soni
2011-01-03 17:03 ` Kevin McNeely
2011-01-03 18:45 ` Henrik Rydberg
2011-01-03 20:50 ` Kevin McNeely
2011-01-04 1:50 ` Hong Liu
2011-01-05 0:38 ` Kevin McNeely
2010-12-29 19:17 ` [v3 2/3] 2/3 i2c: Cypress TTSP G3 MTDEV I2C Device Driver Kevin McNeely
2011-01-04 1:45 ` Hong Liu
2011-01-05 0:37 ` Kevin McNeely
2010-12-29 19:17 ` [v3 3/3] 3/3 spi: Cypress TTSP G3 MTDEV SPI " Kevin McNeely
2011-01-05 0:54 ` [v4 1/3] 1/3 Touchscreen: Cypress TTSP G3 Core Driver Kevin McNeely
2011-01-05 8:59 ` Henrik Rydberg
2011-01-05 17:07 ` Kevin McNeely
2011-01-05 17:34 ` Henrik Rydberg
2011-01-10 19:27 ` Kevin McNeely
2011-01-10 21:11 ` Dmitry Torokhov
2011-01-10 21:17 ` Kevin McNeely
2011-02-24 18:31 ` Kevin McNeely
2011-02-27 12:34 ` Henrik Rydberg
2011-01-05 0:54 ` [v4 2/3] 2/3 i2c: Cypress TTSP G3 I2C Device Driver Kevin McNeely
2011-01-05 0:54 ` [v4 3/3] 3/3 spi: Cypress TTSP G3 SPI " Kevin McNeely
2011-01-12 18:45 ` Dmitry Torokhov
2011-01-12 19:02 ` Kevin McNeely
2011-01-20 11:10 ` Trilok Soni
2011-01-21 9:27 ` Dmitry Torokhov
2011-01-21 22:14 ` Kevin McNeely
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4CE163CC.1060202@euromail.se \
--to=rydberg@euromail.se \
--cc=davidb@codeaurora.org \
--cc=dmitry.torokhov@gmail.com \
--cc=eric.y.miao@gmail.com \
--cc=kev@cypress.com \
--cc=l.fu@pengutronix.de \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@simtec.co.uk \
--cc=sameo@linux.intel.com \
--cc=tsoni@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).