| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20101231115323.GA9709@polaris.bitmath.org>
Date: Fri, 31 Dec 2010 12:53:23 +0100
From: "Henrik Rydberg" <rydberg@...omail.se>
To: Kevin McNeely <kev@...ress.com>
Cc: Dmitry Torokhov <dmitry.torokhov@...il.com>,
David Brown <davidb@...eaurora.org>,
Trilok Soni <tsoni@...eaurora.org>,
Samuel Ortiz <sameo@...ux.intel.com>,
Eric Miao <eric.y.miao@...il.com>,
Mike Frysinger <vapier@...too.org>,
Alan Cox <alan@...ux.intel.com>, linux-input@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [v3 1/3] 1/3 Touchscreen: Cypress TTSP G3 MTDEV Core Driver
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists