Re: [PATCH v5 1/3] Input: cyttsp - Cypress TTSP capacitive multi-touch screen support

Hi Javier,

some more comments on the code. Presumable Dmitry will have additional
ones later on. Thank you for your patience.

+/* Slots management */
+#define CY_MAX_FINGER               4
+#define CY_MAX_ID                   15

I realize the firmware reports ids in the range [1..14], but it is not
visible from the code. Allowing all 16 values makes correctness
obvious.

+static int cyttsp_load_bl_regs(struct cyttsp *ts)
+{
+	int retval;
+
+	memset(&(ts->bl_data), 0, sizeof(struct cyttsp_bootloader_data));
+
+	ts->bl_data.bl_status = 0x10;
+
+	retval =  ttsp_read_block_data(ts, CY_REG_BASE,
+		sizeof(ts->bl_data), &(ts->bl_data));
+
+	return retval;
+}

Looks like you missed the removal of retval here.

+
+static int cyttsp_bl_app_valid(struct cyttsp *ts)
+{
+	int retval;
+
+	retval = cyttsp_load_bl_regs(ts);
+
+	if (retval < 0) {
+		retval = -ENODEV;
+		goto done;
+	}
+
+	if (GET_BOOTLOADERMODE(ts->bl_data.bl_status)) {
+		if (IS_VALID_APP(ts->bl_data.bl_status)) {
+			dev_dbg(ts->dev, "%s: App found; normal boot\n",
+				__func__);
+			return 0;
+		} else {
+			dev_dbg(ts->dev, "%s: NO APP; load firmware!!\n",
+				__func__);
+			return -ENODEV;
+		}
+	}
+
+	if (GET_HSTMODE(ts->bl_data.bl_file) == CY_OPERATE_MODE) {
+		if (!(IS_OPERATIONAL_ERR(ts->bl_data.bl_status))) {
+			dev_dbg(ts->dev, "%s: Operational\n",
+				__func__);
+			return 1;
+		} else {
+			dev_dbg(ts->dev, "%s: Operational failure\n",
+				__func__);
+			return -ENODEV;
+		}
+	}
+
+	dev_dbg(ts->dev, "%s: Non-Operational failure\n",
+		__func__);
+	retval = -ENODEV;
+done:
+	return retval;
+
+}

Are the debug paths still necessary? There are a lot of them.

+static int cyttsp_exit_bl_mode(struct cyttsp *ts)
+{
+	int retval;
+	int tries;
+	u8 bl_cmd[sizeof(bl_command)];
+
+	memcpy(bl_cmd, bl_command, sizeof(bl_command));
+	if (ts->platform_data->bl_keys)
+		memcpy(&bl_cmd[sizeof(bl_command) - CY_NUM_BL_KEYS],
+			ts->platform_data->bl_keys, sizeof(bl_command));
+
+	dev_dbg(ts->dev,
+		"%s: bl_cmd= "
+		"%02X %02X %02X %02X %02X %02X %02X %02X %02X %02X %02X\n",
+		__func__, bl_cmd[0], bl_cmd[1], bl_cmd[2],
+		bl_cmd[3], bl_cmd[4], bl_cmd[5], bl_cmd[6],
+		bl_cmd[7], bl_cmd[8], bl_cmd[9], bl_cmd[10]);
+
+	retval = ttsp_write_block_data(ts, CY_REG_BASE,
+		sizeof(bl_cmd), (void *)bl_cmd);
+	if (retval < 0)
+		return retval;
+
+	/* wait for TTSP Device to complete switch to Operational mode */
+	tries = 0;
+	do {
+		msleep(CY_DELAY_DFLT);
+		retval = cyttsp_load_bl_regs(ts);
+	} while (!((retval == 0) &&
+		!GET_BOOTLOADERMODE(ts->bl_data.bl_status)) &&
+		(tries++ < CY_DELAY_MAX));
+
+	dev_dbg(ts->dev, "%s: check bl ready tries=%d ret=%d stat=%02X\n",
+		__func__, tries, retval, ts->bl_data.bl_status);
+
+	if (retval < 0)
+		return retval;
+	else if (GET_BOOTLOADERMODE(ts->bl_data.bl_status))
+		return -ENODEV;
+	else
+		return 0;
+}

Regardless of if it can ever happen, programatically the (retval > 0)
&& GET_BOOTLOADERMODE(ts->bl_data.bl_status) case is slipping through here.

+static int cyttsp_set_operational_mode(struct cyttsp *ts)
+{
+	struct cyttsp_xydata xy_data;
+	int retval;
+	int tries;
+	u8 cmd = CY_OPERATE_MODE;
+
+	retval = ttsp_write_block_data(ts, CY_REG_BASE, sizeof(cmd), &cmd);
+
+	if (retval < 0)
+		return retval;
+
+	/* wait for TTSP Device to complete switch to Operational mode */
+	tries = 0;
+	do {
+		retval = ttsp_read_block_data(ts, CY_REG_BASE,
+			sizeof(xy_data), &(xy_data));
+	} while (!((retval == 0) &&
+		(xy_data.act_dist == CY_ACT_DIST_DFLT)) &&
+		(tries++ < CY_DELAY_MAX));
+
+	dev_dbg(ts->dev, "%s: check op ready tries=%d ret=%d dist=%02X\n",
+		__func__, tries, retval, xy_data.act_dist);
+
+	return retval;
+}

So (xy_data.act_dist == CY_ACT_DIST_DFL) here is not an error?

+static int cyttsp_set_sysinfo_mode(struct cyttsp *ts)
+{
+	int retval;
+	int tries;
+	u8 cmd = CY_SYSINFO_MODE;
+
+	memset(&(ts->sysinfo_data), 0, sizeof(struct cyttsp_sysinfo_data));
+
+	/* switch to sysinfo mode */
+	retval = ttsp_write_block_data(ts, CY_REG_BASE, sizeof(cmd), &cmd);
+	if (retval < 0)
+		return retval;
+
+	/* read sysinfo registers */
+	tries = 0;
+	do {
+		msleep(CY_DELAY_DFLT);
+		retval = ttsp_read_block_data(ts, CY_REG_BASE,
+			sizeof(ts->sysinfo_data), &(ts->sysinfo_data));
+	} while (!((retval == 0) &&
+		!((ts->sysinfo_data.tts_verh == 0) &&
+		(ts->sysinfo_data.tts_verl == 0))) &&
+		(tries++ < CY_DELAY_MAX));

Expands to (retval ||  !ts->sysinfo_data.tts_verh &&
!ts->sysinfo_data.tts_verl) && (tries++ < CY_DELAY_MAX), which seems a
bit simpler to me. Also, &ts->sysinfo_data could be a pointer here.

Coding verh+verl as a 16-bit value?

Are the timeouted (tries >= CY_DELAY_MAX) cases not errors here either?

<snip>

+static int cyttsp_soft_reset(struct cyttsp *ts)
+{
+	int retval;
+	u8 cmd = CY_SOFT_RESET_MODE;
+	long wait_jiffies = msecs_to_jiffies(CY_DELAY_DFLT * CY_DELAY_MAX);
+	/* wait for interrupt to set ready completion */
+	INIT_COMPLETION(ts->bl_ready);
+
+	retval = ttsp_write_block_data(ts, CY_REG_BASE, sizeof(cmd), &cmd);
+	if (retval < 0)
+		return retval;
+
+	retval = wait_for_completion_timeout(&ts->bl_ready, wait_jiffies);
+
+	if (retval > 0)
+		retval = 0;

Seems this one slipped.

+
+	return retval;
+}
+
+static int cyttsp_hndshk(struct cyttsp *ts, u8 hst_mode)
+{
+	int retval;
+	u8 cmd;
+
+	cmd = hst_mode & CY_HNDSHK_BIT ?
+		hst_mode ^ CY_HNDSHK_BIT :
+		hst_mode | CY_HNDSHK_BIT;

Whoa, please redo this one correctly.

+
+	retval = ttsp_write_block_data(ts, CY_REG_BASE,
+		sizeof(cmd), (u8 *)&cmd);
+
+	return retval;
+}

+int cyttsp_resume(void *handle)
+{
+	struct cyttsp *ts = handle;
+	int retval = 0;
+	struct cyttsp_xydata xydata;
+
+	if (ts) {

Neater if returning on (!ts) here instead.

+		if (ts->platform_data->use_sleep && (ts->power_state !=
+						     CY_ACTIVE_STATE)) {
+			if (ts->platform_data->wakeup) {
+				retval = ts->platform_data->wakeup();
+				if (retval < 0)
+					dev_dbg(ts->dev, "%s: Error, wakeup failed!\n",
+						__func__);
+			} else {
+				dev_dbg(ts->dev, "%s: Error, wakeup not implemented "
+					"(check board file).\n", __func__);
+				retval = -ENOSYS;
+			}
+			if (!(retval < 0)) {
+				retval = ttsp_read_block_data(ts, CY_REG_BASE,
+							      sizeof(xydata),
+							      &xydata);
+				if (!(retval < 0) &&
+				    !GET_HSTMODE(xydata.hst_mode))
+					ts->power_state = CY_ACTIVE_STATE;
+			}
+		}
+		dev_dbg(ts->dev, "%s: Wake Up %s\n", __func__,
+			(retval < 0) ? "FAIL" : "PASS");
+	}
+	return retval;
+}
+EXPORT_SYMBOL_GPL(cyttsp_resume);

Thanks.
Henrik