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>,
Mike Frysinger <vapier@gentoo.org>,
Alan Cox <alan@linux.intel.com>,
linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [v3 1/3] 1/3 Touchscreen: Cypress TTSP G3 MTDEV Core Driver
Date: Fri, 31 Dec 2010 12:53:23 +0100 [thread overview]
Message-ID: <20101231115323.GA9709@polaris.bitmath.org> (raw)
In-Reply-To: <1293650268-1561-1-git-send-email-kev@cypress.com>
Hi Kevin,
Thanks for the changes, it looks much better now. Some comments on the MT
part inline.
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 06ea8da..7d886bc 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -124,6 +124,11 @@ config TOUCHSCREEN_CY8CTMG110
> To compile this driver as a module, choose M here: the
> module will be called cy8ctmg110_ts.
>
> +config TOUCHSCREEN_CYTTSP_CORE
> + bool "Cypress TTSP touchscreen core"
> + help
> + Always activated for Cypress TTSP touchscreen
> +
Tristate please.
[...]
> +
> +struct cyttsp_tch {
> + u16 x;
> + u16 y;
> + u8 z;
> +};
Does not appear to be used anywhere.
[...]
> +struct cyttsp {
> + struct device *dev;
> + int irq;
> + struct input_dev *input;
> + struct mutex mutex;
Is this really used?
> + char phys[32];
> + const struct bus_type *bus_type;
> + const struct cyttsp_platform_data *platform_data;
> + struct cyttsp_bus_ops *bus_ops;
> + struct cyttsp_xydata xy_data;
> + struct cyttsp_bootloader_data bl_data;
> + struct cyttsp_sysinfo_data sysinfo_data;
> + struct completion bl_ready;
> + enum cyttsp_powerstate power_state;
> +};
> +
> +static const u8 bl_command[] = {
> + CY_BL_FILE0, CY_BL_CMD, CY_BL_EXIT,
> + CY_BL_KEY0, CY_BL_KEY1, CY_BL_KEY2,
> + CY_BL_KEY3, CY_BL_KEY4, CY_BL_KEY5,
> + CY_BL_KEY6, CY_BL_KEY7
> +};
If the CY_BL* values are only used here, how about putting the numbers here directly?
[...]
> +static int ttsp_read_block_data(struct cyttsp *ts, u8 command,
> + u8 length, void *buf)
> +{
> + int retval;
> + int tries;
> +
> + if (!buf || !length)
> + return -EIO;
-EINVAL?
> +
> + for (tries = 0, retval = -1;
> + tries < CY_NUM_RETRY && (retval < 0);
> + tries++)
> + retval = ts->bus_ops->read(ts->bus_ops, command, length, buf);
> +
> + return retval;
> +}
[...]
> +/* process current touches */
> +static int cyttsp_xy_worker(struct cyttsp *ts)
> +{
> + u8 num_cur_tch = 0;
Does not appear to need initialization.
> + u16 x;
> + u16 y;
> + u8 z;
Perhaps x[2], y[2] etc here.
> +
> + /* Get touch 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;
> +
> + /* determine number of currently active touches */
> + num_cur_tch = GET_NUM_TOUCHES(ts->xy_data.tt_stat);
> +
> + /* check for any error conditions */
> + if (ts->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 */
> + num_cur_tch = 0;
> + dev_dbg(ts->dev, "%s: Large area detected\n", __func__);
> + } else if (num_cur_tch > 2) {
> + /* terminate all active tracks */
> + num_cur_tch = 0;
> + 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 */
> + num_cur_tch = 0;
> + dev_dbg(ts->dev, "%s: Invalid buffer detected\n", __func__);
> + }
> +
> + /* send touches */
> + if (!num_cur_tch)
> + /* terminate previous active touch */
> + input_mt_sync(ts->input);
> +
> + if (num_cur_tch) {
> + /* send touch 1 */
> + /*
> + * If there is only one current active touch,
> + * it will be reported in the touch 1 regardless
> + * if it was reported in the touch 2 previously
> + */
Perhaps "touch" should be "track" or "slot" here?
> + x = (ts->xy_data.tch1_xhi << 8) + ts->xy_data.tch1_xlo;
> + y = (ts->xy_data.tch1_yhi << 8) + ts->xy_data.tch1_ylo;
> + z = ts->xy_data.tch1_z;
Perhaps x[0] = ... here
> + input_report_abs(ts->input, ABS_MT_POSITION_X, x);
> + input_report_abs(ts->input, ABS_MT_POSITION_Y, y);
> + input_report_abs(ts->input, ABS_MT_TOUCH_MAJOR, z);
> + input_mt_sync(ts->input);
Then these could still be collected in a loop.
> + }
> +
> + if (num_cur_tch > 1) {
> + /* send touch 2 */
> + x = (ts->xy_data.tch2_xhi << 8) + ts->xy_data.tch2_xlo;
> + y = (ts->xy_data.tch2_yhi << 8) + ts->xy_data.tch2_ylo;
> + z = ts->xy_data.tch2_z;
And x[1] = ...
> + input_report_abs(ts->input, ABS_MT_POSITION_X, x);
> + input_report_abs(ts->input, ABS_MT_POSITION_Y, y);
> + input_report_abs(ts->input, ABS_MT_TOUCH_MAJOR, z);
> + input_mt_sync(ts->input);
> + }
Ditto.
> +
> + input_sync(ts->input);
> +
> + return 0;
> +}
If the in-kernel tracking module was in place, would you consider switching to the slots protocol?
Thanks,
Henrik
next prev parent reply other threads:[~2010-12-31 11:58 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
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 [this message]
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=20101231115323.GA9709@polaris.bitmath.org \
--to=rydberg@euromail.se \
--cc=alan@linux.intel.com \
--cc=davidb@codeaurora.org \
--cc=dmitry.torokhov@gmail.com \
--cc=eric.y.miao@gmail.com \
--cc=kev@cypress.com \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=sameo@linux.intel.com \
--cc=tsoni@codeaurora.org \
--cc=vapier@gentoo.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).