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

Hi Javier,

Cypress TrueTouch(tm) Standard Product controllers are found in
a wide range of embedded devices. This driver add support for a
variety of TTSP controllers.

please find some comments below.

These two look like bool, please use as bool instead.

+#define CY_MAX_ID                   15

Why not 16 here?

+struct cyttsp_tch {
+	__be16 x, y;
+	u8 z;
+} __packed;
+
+/* TrueTouch Standard Product Gen3 interface definition */
+struct cyttsp_xydata {
+	u8 hst_mode;
+	u8 tt_mode;
+	u8 tt_stat;
+	struct cyttsp_tch tch1;
+	u8 touch12_id;
+	struct cyttsp_tch tch2;
+	u8 gest_cnt;
+	u8 gest_id;
+	struct cyttsp_tch tch3;
+	u8 touch34_id;
+	struct cyttsp_tch tch4;
+	u8 tt_undef[3];
+	u8 act_dist;
+	u8 tt_reserved;
+} __packed;

Too bad the touches are not an array here...

+
+/* TTSP System Information interface definition */
+struct cyttsp_sysinfo_data {
+	u8 hst_mode;
+	u8 mfg_cmd;
+	u8 mfg_stat;
+	u8 cid[3];
+	u8 tt_undef1;
+	u8 uid[8];
+	u8 bl_verh;
+	u8 bl_verl;
+	u8 tts_verh;
+	u8 tts_verl;
+	u8 app_idh;
+	u8 app_idl;
+	u8 app_verh;
+	u8 app_verl;
+	u8 tt_undef[5];
+	u8 scn_typ;
+	u8 act_intrvl;
+	u8 tch_tmout;
+	u8 lp_intrvl;
+};
+
+/* TTSP Bootloader Register Map interface definition */
+#define CY_BL_CHKSUM_OK 0x01
+struct cyttsp_bootloader_data {
+	u8 bl_file;
+	u8 bl_status;
+	u8 bl_error;
+	u8 blver_hi;
+	u8 blver_lo;
+	u8 bld_blver_hi;
+	u8 bld_blver_lo;
+	u8 ttspver_hi;
+	u8 ttspver_lo;
+	u8 appid_hi;
+	u8 appid_lo;
+	u8 appver_hi;
+	u8 appver_lo;
+	u8 cid_0;
+	u8 cid_1;
+	u8 cid_2;
+};
+
+struct cyttsp {
+	struct device *dev;
+	int irq;
+	struct input_dev *input;
+	char phys[32];
+	const struct cyttsp_platform_data *platform_data;
+	struct cyttsp_bus_ops *bus_ops;
+	struct cyttsp_bootloader_data bl_data;
+	struct cyttsp_sysinfo_data sysinfo_data;
+	struct completion bl_ready;
+	enum cyttsp_powerstate power_state;
+	int slot_curr[CY_MAX_ID];
+	int slot_prev[CY_MAX_ID];

These two are not needed; the input core will take care of duplicates,
so slot_prev can be removed. Also, the set of current slots can be
tracked using a temporary bitmask, so slot_curr can be removed as well.

<snip>

+static void cyttsp_report_slot(struct input_dev *dev, int slot,
+			       int x, int y, int z)
+{
+	input_mt_slot(dev, slot);
+	input_mt_report_slot_state(dev, MT_TOOL_FINGER, true);
+	input_report_abs(dev, ABS_MT_POSITION_X, x);
+	input_report_abs(dev, ABS_MT_POSITION_Y, y);
+	input_report_abs(dev, ABS_MT_TOUCH_MAJOR, z);
+}
+
+static void cyttsp_report_slot_empty(struct input_dev *dev, int slot)
+{
+	input_mt_slot(dev, slot);
+	input_mt_report_slot_state(dev, MT_TOOL_FINGER, false);
+}
+
+static void cyttsp_extract_track_ids(struct cyttsp_xydata *xy_data, int *ids)
+{
+	ids[0] = xy_data->touch12_id >> 4;
+	ids[1] = xy_data->touch12_id & 0xF;
+	ids[2] = xy_data->touch34_id >> 4;
+	ids[3] = xy_data->touch34_id & 0xF;
+}
+
+static void cyttsp_get_tch(struct cyttsp_xydata *xy_data, int idx,
+			   struct cyttsp_tch **tch)
+{
+	switch (idx) {
+	case 0:
+		*tch = &xy_data->tch1;
+		break;
+	case 1:
+		*tch = &xy_data->tch2;
+		break;
+	case 2:
+		*tch = &xy_data->tch3;
+		break;
+	case 3:
+		*tch = &xy_data->tch4;
+		break;
+	}
+}

How about returning a const struct cyttsp_tch* here instead.

+static int cyttsp_handle_tchdata(struct cyttsp *ts)
+{
+	struct cyttsp_xydata xy_data;
+	u8 num_cur_tch;
+	int i;
+	int ids[4];
+	struct cyttsp_tch *tch = NULL;
+	int x, y, z;
+
+	/* Get touch data from CYTTSP device */
+	if (ttsp_read_block_data(ts,
+		CY_REG_BASE, sizeof(struct cyttsp_xydata), &xy_data))
+		return 0;
+
+	/* touch extension handling */
+	if (ttsp_tch_ext(ts, &xy_data))
+		return 0;
+
+	/* provide flow control handshake */
+	if (ts->platform_data->use_hndshk)
+		if (cyttsp_hndshk(ts, xy_data.hst_mode))
+			return 0;
+
+	/* determine number of currently active touches */
+	num_cur_tch = GET_NUM_TOUCHES(xy_data.tt_stat);
+
+	/* check for any error conditions */
+	if (ts->power_state == CY_IDLE_STATE)
+		return 0;
+	else if (GET_BOOTLOADERMODE(xy_data.tt_mode)) {
+		return -1;
+	} else if (IS_LARGE_AREA(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 > CY_MAX_FINGER) {
+		/* terminate all active tracks */
+		num_cur_tch = 0;
+		dev_dbg(ts->dev, "%s: Num touch error detected\n", __func__);
+	} else if (IS_BAD_PKT(xy_data.tt_mode)) {
+		/* terminate all active tracks */
+		num_cur_tch = 0;
+		dev_dbg(ts->dev, "%s: Invalid buffer detected\n", __func__);
+	}
+
+	cyttsp_extract_track_ids(&xy_data, ids);
+
+	for (i = 0; i < num_cur_tch; i++) {
+		ts->slot_curr[ids[i] - 1] = CY_USED;

Why the -1 here? Since the values are provably between 0 and 15, there
is no need to change them further.

Also, the above line could be replaced by "used |= 1 << ids[i]", for instance.

+
+		cyttsp_get_tch(&xy_data, i, &tch);
+
+		x = be16_to_cpu(tch->x);
+		y = be16_to_cpu(tch->y);
+		z = tch->z;
+
+		cyttsp_report_slot(ts->input, ids[i] - 1, x, y, z);

Ditto, -1.

+	}
+
+	for (i = 0; i < CY_MAX_ID; i++) {
+		if (ts->slot_prev[i] == CY_USED &&
+		    ts->slot_curr[i] == CY_UNUSED)
+			cyttsp_report_slot_empty(ts->input, i);
+		ts->slot_prev[i] = ts->slot_curr[i];
+		ts->slot_curr[i] = CY_UNUSED;
+	}

Input core handles duplicate calls, so the above could be simplified.

+
+	input_sync(ts->input);
+
+	return 0;
+}

<snip>

+void *cyttsp_core_init(struct cyttsp_bus_ops *bus_ops, struct device *dev, int irq)
+{
+	struct input_dev *input_device;
+	int i;
+	int ret;
+
+	struct cyttsp *ts = kzalloc(sizeof(*ts), GFP_KERNEL);
+
+	if (!ts) {
+		pr_err("%s: Error, kzalloc\n", __func__);
+		goto error_alloc_data;
+	}
+
+	if (dev == NULL || bus_ops == NULL) {
+		kfree(ts);
+		goto error_alloc_data;
+	}
+
+	ts->dev = dev;
+	ts->platform_data = dev->platform_data;
+	ts->bus_ops = bus_ops;
+	init_completion(&ts->bl_ready);
+
+	if (ts->platform_data->init) {
+		if (ts->platform_data->init()) {
+			dev_dbg(ts->dev, "%s: Error, platform init failed!\n",
+				__func__);
+			goto error_init;
+		}
+	}
+
+	ts->irq = irq;
+	if (ts->irq <= 0) {
+		dev_dbg(ts->dev, "%s: Error, failed to allocate irq\n",
+			__func__);
+			goto error_init;
+	}
+
+	/* Create the input device and register it. */
+	input_device = input_allocate_device();
+	if (!input_device) {
+		dev_dbg(ts->dev, "%s: Error, failed to allocate input device\n",
+			__func__);
+		goto error_input_allocate_device;
+	}
+
+	ts->input = input_device;
+	input_device->name = ts->platform_data->name;
+	snprintf(ts->phys, sizeof(ts->phys), "%s", dev_name(dev));
+	input_device->phys = ts->phys;
+	input_device->dev.parent = ts->dev;
+	input_device->open = cyttsp_open;
+	input_device->close = cyttsp_close;
+	input_set_drvdata(input_device, ts);
+
+	__set_bit(EV_SYN, input_device->evbit);
+	__set_bit(EV_KEY, input_device->evbit);
+	__set_bit(EV_ABS, input_device->evbit);
+
+	input_set_abs_params(input_device, ABS_MT_POSITION_X,
+			     0, ts->platform_data->maxx, 0, 0);
+	input_set_abs_params(input_device, ABS_MT_POSITION_Y,
+			     0, ts->platform_data->maxy, 0, 0);
+	input_set_abs_params(input_device, ABS_MT_TOUCH_MAJOR,
+			     0, CY_MAXZ, 0, 0);
+
+	input_mt_init_slots(input_device, CY_MAX_ID);
+
+	for (i = 0; i < CY_MAX_ID; i++) {
+		ts->slot_prev[i] = CY_UNUSED;
+		ts->slot_curr[i] = CY_UNUSED;
+	}

Could be removed.

+	ret = input_register_device(input_device);
+	if (ret) {
+		dev_err(ts->dev, "%s: Error, failed to register input device: %d\n",
+			__func__, ret);
+		goto error_input_register_device;
+	}
+
+	goto no_error;
+
+error_input_register_device:
+	input_free_device(input_device);
+error_input_allocate_device:
+	if (ts->platform_data->exit)
+		ts->platform_data->exit();
+error_init:
+	kfree(ts);
+error_alloc_data:
+no_error:
+	return ts;
+}

Thanks,
Henrik