RE: [v3 1/3] 1/3 Touchscreen: Cypress TTSP G3 MTDEV Core Driver

[Kevin McNeely]

Hi Henrik,

Thanks for your comments. Comments and questions inline below...

> -----Original Message-----
> From: Henrik Rydberg [mailto:rydberg@xxxxxxxxxxx]
> Sent: Friday, December 31, 2010 3:53 AM
> To: Kevin McNeely
> Cc: Dmitry Torokhov; David Brown; Trilok Soni; Samuel Ortiz; Eric
Miao;
> Mike Frysinger; Alan Cox; linux-input@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> 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.

This will be changed.

> 
> [...]
> > +
> > +struct cyttsp_tch {
> > +	u16 x;
> > +	u16 y;
> > +	u8 z;
> > +};
> 
> Does not appear to be used anywhere.

This will be removed.

> 
> [...]
> > +struct cyttsp {
> > +	struct device *dev;
> > +	int irq;
> > +	struct input_dev *input;
> > +	struct mutex mutex;
> 
> Is this really used?

No. This will be removed.

> 
> > +	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?

This will be done.

> 
> [...]
> > +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?

This will be changed.

> 
> > +
> > +	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.

This will be cleaned up.

> 
> > +	u16 x;
> > +	u16 y;
> > +	u8 z;
> 
> Perhaps x[2], y[2] etc here.

This will be implemented.

> 
> > +
> > +	/* 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?

I plan to keep the "touch" in order to match our part documentation.

> 
> > +		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

This will be implemented.

> 
> > +		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.

The loop will be added.

> 
> > +	}
> > +
> > +	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] = ...

The same, x[1] and the loop will be added.

> 
> > +		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.

The same.

> 
> > +
> > +	input_sync(ts->input);
> > +
> > +	return 0;
> > +}
> 
> If the in-kernel tracking module was in place, would you consider
> switching to the slots protocol?

Hi Henrik, can you please clarify?  Should I remove MTDEV from the patch
title?

Thanks and best regards,
Kevin

> 
> Thanks,
> Henrik

---------------------------------------------------------------
This message and any attachments may contain Cypress (or its
subsidiaries) confidential information. If it has been received
in error, please advise the sender and immediately delete this
message.
---------------------------------------------------------------

--
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/