Re: [PATCH 1/2] Input: Add driver for Cypress Generation 5 touchscreen

[Mylene Josserand]

Hello Maxime,

Thank you for the review!

On 30/05/2017 10:02, Maxime Ripard wrote:
> Hi Mylene,
>
> On Mon, May 29, 2017 at 04:45:37PM +0200, Mylène Josserand wrote:
> > +static int cyttsp5_read(struct cyttsp5 *ts, u8 *buf, u32 max)
> > +{
> > +	int rc;
> > +	u32 size;
> > +
> > +	if (!buf)
> > +		return -EINVAL;
> > +
> > +	/* Read the frame to retrieve the size */
> > +	rc = regmap_bulk_read(ts->regmap, 0, buf, 2);
> > +	if (rc < 0)
> > +		return rc;
>
> You should probably use a temporary buffer here, instead of the caller
> one. Otherwise, you might return an error (a bit below, in the (size >
> max) test), while you modified the caller buffer, which seems a bit
> odd.

Okay.

> > +	size = get_unaligned_le16(&buf[0]);
>
> isn't &buf[0] the same thing than buf here ?

Yep

> > +	if (!size || size == 2)
> > +		return 0;
> > +
> > +	if (size > max)
> > +		return -EINVAL;
> > +
> > +	/* Get the real value */
> > +	return regmap_bulk_read(ts->regmap, 0, buf, size);
> > +}
> > +
> > +static int cyttsp5_write(struct cyttsp5 *ts, unsigned int reg, u8 *data,
> > +			 size_t size)
> > +{
> > +	u8 cmd[size + 1];
> > +
> > +	/* High bytes of register address needed as first byte of cmd */
> > +	cmd[0] = HI_BYTE(reg);
> > +	/* Copy the rest of the data */
> > +	memcpy(&cmd[1], data, size);
> > +
> > +	return regmap_bulk_write(ts->regmap, LOW_BYTE(reg), cmd, size + 1);
>
> The way we need to access the buffers here is definitely not trivial,
> I guess some comment about how the hardware expects the commands to be
> sent would be welcome.

Yes, it is not trivial, I will add a comment.

> > +}
> > +
> > +static void cyttsp5_final_sync(struct input_dev *input, int max_slots,
> > +			       unsigned long *ids)
> > +{
> > +	int t;
> > +
> > +	for (t = 0; t < max_slots; t++) {
> > +		if (test_bit(t, ids))
> > +			continue;
> > +		input_mt_slot(input, t);
> > +		input_mt_report_slot_state(input, MT_TOOL_FINGER, false);
> > +	}
> > +
> > +	input_sync(input);
> > +}
> > +
> > +static void cyttsp5_report_slot_liftoff(struct cyttsp5 *ts, int max_slots)
> > +{
> > +	int t;
> > +
> > +	if (ts->num_prv_rec == 0)
> > +		return;
> > +
> > +	for (t = 0; t < max_slots; t++) {
> > +		input_mt_slot(ts->input, t);
> > +		input_mt_report_slot_state(ts->input, MT_TOOL_FINGER, false);
> > +	}
> > +}
> > +
> > +static void cyttsp5_mt_lift_all(struct cyttsp5 *ts)
> > +{
> > +	struct cyttsp5_sysinfo *si = &ts->sysinfo;
> > +	int max = si->tch_abs[CY_TCH_T].max;
> > +
> > +	if (ts->num_prv_rec != 0) {
> > +		cyttsp5_report_slot_liftoff(ts, max);
> > +		input_sync(ts->input);
> > +		ts->num_prv_rec = 0;
> > +	}
> > +}
> > +
> > +static void cyttsp5_get_touch_axis(int *axis, int size, int max, u8 *xy_data,
> > +				   int bofs)
> > +{
> > +	int nbyte;
> > +	int next;
> > +
> > +	for (nbyte = 0, *axis = 0, next = 0; nbyte < size; nbyte++) {
> > +		*axis = *axis + ((xy_data[next] >> bofs) << (nbyte * 8));
> > +		next++;
>
> Isn't nbyte and next the same thing here ?

Yes, thanks!

> > +	}
> > +
> > +	*axis &= max - 1;
> > +}
> > +
> > +static void cyttsp5_get_touch_record(struct cyttsp5 *ts,
> > +				     struct cyttsp5_touch *touch, u8 *xy_data)
> > +{
> > +	struct cyttsp5_sysinfo *si = &ts->sysinfo;
> > +	enum cyttsp5_tch_abs abs;
> > +
> > +	for (abs = CY_TCH_X; abs < CY_TCH_NUM_ABS; abs++) {
> > +		cyttsp5_get_touch_axis(&touch->abs[abs],
> > +				       si->tch_abs[abs].size,
> > +				       si->tch_abs[abs].max,
> > +				       xy_data + si->tch_abs[abs].ofs,
> > +				       si->tch_abs[abs].bofs);
> > +	}
> > +}
> > +
> > +static void cyttsp5_mt_process_touch(struct cyttsp5 *ts,
> > +				     struct cyttsp5_touch *touch)
> > +{
> > +	struct cyttsp5_sysinfo *si = &ts->sysinfo;
> > +	int tmp;
> > +
> > +	tmp = touch->abs[CY_TCH_X];
> > +	touch->abs[CY_TCH_X] = touch->abs[CY_TCH_Y];
> > +	touch->abs[CY_TCH_Y] = tmp;
>
> Why do you need to swap the values here?

The X/Y axis must be swapped otherwise, touchscreen's events are not 
correct (inversion between X and Y axis).
I noticed that there is a "touchscreen-swapped-x-y" property that exists 
in the touchscreen's device tree documentation.

http://elixir.free-electrons.com/linux/v4.12-rc4/source/Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt#L22

I will update the driver to use this property instead of hard-coding the 
swap.

> Instead of rolling your own implementation, you can use swap()
>
> > +	/* Convert MAJOR/MINOR from mm to resolution */
> > +	tmp = touch->abs[CY_TCH_MAJ] * 100 * si->sensing_conf_data.res_x;
> > +	touch->abs[CY_TCH_MAJ] = tmp / si->sensing_conf_data.len_x;
> > +	tmp = touch->abs[CY_TCH_MIN] * 100 * si->sensing_conf_data.res_x;
> > +	touch->abs[CY_TCH_MIN] = tmp / si->sensing_conf_data.len_x;
> > +}
> > +
> > +static void cyttsp5_get_mt_touches(struct cyttsp5 *ts,
> > +				   struct cyttsp5_touch *tch, int num_cur_tch)
> > +{
> > +	struct cyttsp5_sysinfo *si = &ts->sysinfo;
> > +	int i, t = 0;
> > +	DECLARE_BITMAP(ids, si->tch_abs[CY_TCH_T].max);
> > +	u8 *tch_addr;
> > +
> > +	bitmap_zero(ids, si->tch_abs[CY_TCH_T].max);
> > +	memset(tch->abs, 0, sizeof(tch->abs));
> > +
> > +	for (i = 0; i < num_cur_tch; i++) {
> > +		tch_addr = si->xy_data + (i * TOUCH_REPORT_SIZE);
> > +		cyttsp5_get_touch_record(ts, tch, tch_addr);
> > +
> > +		/* Process touch */
> > +		cyttsp5_mt_process_touch(ts, tch);
>
> Maybe you should just make that function return a structure, or fill
> one directly passed as an argument. Filling only one field of a
> particular structure passed as an argument to exchange data doesn't
> seem very natural. Especially since you don't really use tch in that
> function.

Okay, I will rework it or, even, remove it because this function is 
small now.

> > +
> > +		t = tch->abs[CY_TCH_T];
> > +		input_mt_slot(ts->input, t);
> > +		input_mt_report_slot_state(ts->input, MT_TOOL_FINGER, true);
> > +		__set_bit(t, ids);
> > +
> > +		/* position and pressure fields */
> > +		input_report_abs(ts->input, ABS_MT_POSITION_X, tch->abs[CY_TCH_X]);
> > +		input_report_abs(ts->input, ABS_MT_POSITION_Y, tch->abs[CY_TCH_Y]);
> > +		input_report_abs(ts->input, ABS_MT_PRESSURE, tch->abs[CY_TCH_P]);
> > +
> > +		/* Get the extended touch fields */
> > +		input_report_abs(ts->input, ABS_MT_TOUCH_MAJOR, tch->abs[CY_TCH_MAJ]);
> > +		input_report_abs(ts->input, ABS_MT_TOUCH_MINOR, tch->abs[CY_TCH_MIN]);
> > +	}
> > +
> > +	cyttsp5_final_sync(ts->input, si->tch_abs[CY_TCH_T].max, ids);
> > +
> > +	ts->num_prv_rec = num_cur_tch;
> > +}
> > +
> > +/* read xy_data for all current touches */
> > +static int cyttsp5_xy_worker(struct cyttsp5 *ts)
> > +{
> > +	struct device *dev = ts->dev;
> > +	struct cyttsp5_sysinfo *si = &ts->sysinfo;
> > +	int max_tch = si->sensing_conf_data.max_tch;
> > +	struct cyttsp5_touch tch;
> > +	u8 num_cur_tch;
> > +
> > +	cyttsp5_get_touch_axis(&tch.hdr, si->tch_hdr.size,
> > +			       si->tch_hdr.max,
> > +			       si->xy_mode + 3 + si->tch_hdr.ofs,
> > +			       si->tch_hdr.bofs);
> > +
> > +	num_cur_tch = tch.hdr;
> > +	if (num_cur_tch > max_tch) {
> > +		dev_err(dev, "%s: Num touch err detected (n=%d)\n",
> > +			__func__, num_cur_tch);
> > +		num_cur_tch = max_tch;
> > +	}
> > +
> > +	if (num_cur_tch == 0 && ts->num_prv_rec == 0)
> > +		return 0;
> > +
> > +	/* extract xy_data for all currently reported touches */
> > +	if (num_cur_tch)
> > +		cyttsp5_get_mt_touches(ts, &tch, num_cur_tch);
> > +	else
> > +		cyttsp5_mt_lift_all(ts);
> > +
> > +	return 0;
> > +}
> > +
> > +static int cyttsp5_mt_attention(struct device *dev)
> > +{
> > +	struct cyttsp5 *ts = dev_get_drvdata(dev);
> > +	struct cyttsp5_sysinfo *si = &ts->sysinfo;
> > +	int rc;
> > +
> > +	if (si->xy_mode[2] != HID_TOUCH_REPORT_ID)
> > +		return 0;
> > +
> > +	/* core handles handshake */
> > +	mutex_lock(&ts->mt_lock);
> > +	rc = cyttsp5_xy_worker(ts);
> > +	mutex_unlock(&ts->mt_lock);
> > +	if (rc < 0)
> > +		dev_err(dev, "%s: xy_worker error r=%d\n", __func__, rc);
> > +
> > +	return rc;
> > +}
> > +
> > +static int cyttsp5_setup_input_device(struct device *dev)
> > +{
> > +	struct cyttsp5 *ts = dev_get_drvdata(dev);
> > +	struct cyttsp5_sysinfo *si = &ts->sysinfo;
> > +	int max_x, max_y, max_p;
> > +	int max_x_tmp, max_y_tmp;
> > +	int rc;
> > +
> > +	__set_bit(EV_ABS, ts->input->evbit);
> > +	__set_bit(EV_REL, ts->input->evbit);
> > +	__set_bit(EV_KEY, ts->input->evbit);
> > +
> > +	max_x_tmp = si->sensing_conf_data.res_x;
> > +	max_y_tmp = si->sensing_conf_data.res_y;
> > +	max_x = max_y_tmp - 1;
> > +	max_y = max_x_tmp - 1;
> > +	max_p = si->sensing_conf_data.max_z;
> > +
> > +	input_mt_init_slots(ts->input, si->tch_abs[CY_TCH_T].max, 0);
> > +
> > +	__set_bit(ABS_MT_POSITION_X, ts->input->absbit);
> > +	__set_bit(ABS_MT_POSITION_Y, ts->input->absbit);
> > +	__set_bit(ABS_MT_PRESSURE, ts->input->absbit);
> > +
> > +	input_set_abs_params(ts->input, ABS_MT_POSITION_X, 0, max_x, 0, 0);
> > +	input_set_abs_params(ts->input, ABS_MT_POSITION_Y, 0, max_y, 0, 0);
> > +	input_set_abs_params(ts->input, ABS_MT_PRESSURE, 0, max_p, 0, 0);
> > +
> > +	input_set_abs_params(ts->input, ABS_MT_TOUCH_MAJOR, 0, MAX_AREA, 0, 0);
> > +	input_set_abs_params(ts->input, ABS_MT_TOUCH_MINOR, 0, MAX_AREA, 0, 0);
> > +
> > +	rc = input_register_device(ts->input);
> > +	if (rc < 0)
> > +		dev_err(dev, "%s: Error, failed register input device r=%d\n",
> > +			__func__, rc);
> > +
> > +	return rc;
> > +}
> > +
> > +static int cyttsp5_parse_dt_key_code(struct device *dev)
> > +{
> > +	struct cyttsp5 *ts = dev_get_drvdata(dev);
> > +	struct cyttsp5_sysinfo *si = &ts->sysinfo;
> > +	struct device_node *np, *pp;
> > +	int rc, i;
> > +
> > +	np = dev->of_node;
> > +	if (!np)
> > +		return -EINVAL;
> > +
> > +	if (si->num_btns) {
> > +		si->btn = devm_kzalloc(dev,
> > +				       si->num_btns * sizeof(struct cyttsp5_btn),
> > +				       GFP_KERNEL);
> > +		if (!si->btn)
> > +			return -ENOMEM;
> > +
> > +		/* Initialized the button to RESERVED */
> > +		for (i = 0; i < si->num_btns; i++) {
> > +			struct cyttsp5_btn *btns = &si->btn[i];
> > +			btns->key_code = KEY_RESERVED;
> > +			btns->enabled = true;
> > +		}
> > +	}
> > +
> > +	i = 0;
> > +	for_each_child_of_node(np, pp) {
> > +		struct cyttsp5_btn *btns = &si->btn[i];
> > +
> > +		rc = of_property_read_u32(pp, "linux,code", &btns->key_code);
> > +		if (rc) {
> > +			dev_err(dev, "%s: Inval linux,code prop\n", pp->name);
> > +			return -EINVAL;
> > +		}
> > +		btns->enabled = true;
> > +
> > +		i++;
> > +	}
> > +
> > +	return i;
> > +}
> > +
> > +static int cyttsp5_btn_attention(struct device *dev)
> > +{
> > +	struct cyttsp5 *ts = dev_get_drvdata(dev);
> > +	struct cyttsp5_sysinfo *si = &ts->sysinfo;
> > +	int cur_btn;
> > +	int cur_btn_state;
> > +
> > +	if (si->xy_mode[2] != HID_BTN_REPORT_ID)
> > +		return 0;
> > +
> > +	/* core handles handshake */
> > +	mutex_lock(&ts->btn_lock);
> > +
> > +	/* extract button press/release touch information */
> > +	if (si->num_btns > 0) {
> > +		for (cur_btn = 0; cur_btn < si->num_btns; cur_btn++) {
> > +			/* Get current button state */
> > +			cur_btn_state = (si->xy_data[0] >> (cur_btn * CY_BITS_PER_BTN))
> > +				& CY_NUM_BTN_EVENT_ID;
> > +
> > +			if (!si->btn[cur_btn].enabled)
> > +				continue;
> > +
> > +			input_report_key(ts->input, si->btn[cur_btn].key_code,
> > +					 cur_btn_state);
> > +			input_sync(ts->input);
> > +		}
> > +	}
> > +
> > +	mutex_unlock(&ts->btn_lock);
> > +
> > +	return 0;
> > +}
> > +
> > +static const u16 crc_table[16] = {
> > +	0x0000, 0x1021, 0x2042, 0x3063,
> > +	0x4084, 0x50a5, 0x60c6, 0x70e7,
> > +	0x8108, 0x9129, 0xa14a, 0xb16b,
> > +	0xc18c, 0xd1ad, 0xe1ce, 0xf1ef,
> > +};
>
> This looks like a shorter version of crc_itu_t_table.

Yes, true

> > +static u16 cyttsp5_compute_crc(u8 *buf, u32 size)
> > +{
> > +	u16 remainder = 0xFFFF;
> > +	u16 xor_mask = 0x0000;
> > +	u32 index;
> > +	u32 byte_value;
> > +	u32 table_index;
> > +	u32 crc_bit_width = sizeof(u16) * 8;
> > +
> > +	/* Divide the message by polynomial, via the table. */
> > +	for (index = 0; index < size; index++) {
> > +		byte_value = buf[index];
> > +		table_index = ((byte_value >> 4) & 0x0F)
> > +			^ (remainder >> (crc_bit_width - 4));
> > +		remainder = crc_table[table_index] ^ (remainder << 4);
> > +		table_index = (byte_value & 0x0F)
> > +			^ (remainder >> (crc_bit_width - 4));
> > +		remainder = crc_table[table_index] ^ (remainder << 4);
> > +	}
> > +
> > +	/* Perform the final remainder CRC. */
> > +	return remainder ^ xor_mask;
> > +}
>
> Could it be that it's just using the CRC ITU-T algorithm?

The datasheet says it is using a CCITT algorithm with polynomial (x^16 + 
x^12 + x^5 + 1) whereas the ITU-T has a polynomial of (x^16 + x^12 + 
x^15 + 1). I guess I can just use the itu table, then.

> > +static int cyttsp5_validate_cmd_response(struct cyttsp5 *ts, u8 code)
> > +{
> > +	u16 size, crc;
> > +	u8 status, offset;
> > +	int command_code;
> > +
> > +	size = get_unaligned_le16(&ts->response_buf[0]);
> > +
> > +	if (!size)
> > +		return 0;
> > +
> > +	offset = ts->response_buf[HID_OUTPUT_RESPONSE_REPORT_OFFSET];
> > +
> > +	if (offset == HID_BL_RESPONSE_REPORT_ID) {
> > +		if (ts->response_buf[4] != HID_OUTPUT_BL_SOP) {
> > +			dev_err(ts->dev, "%s: HID output response, wrong SOP\n",
> > +				__func__);
> > +			return -EPROTO;
> > +		}
> > +
> > +		if (ts->response_buf[size - 1] != HID_OUTPUT_BL_EOP) {
> > +			dev_err(ts->dev, "%s: HID output response, wrong EOP\n",
> > +				__func__);
> > +			return -EPROTO;
> > +		}
> > +
> > +		crc = cyttsp5_compute_crc(&ts->response_buf[4], size - 7);
> > +		if (ts->response_buf[size - 3] != LOW_BYTE(crc)
> > +		    || ts->response_buf[size - 2] != HI_BYTE(crc)) {
> > +			dev_err(ts->dev, "%s: HID output response, wrong CRC 0x%X\n",
> > +				__func__, crc);
> > +			return -EPROTO;
> > +		}
> > +
> > +		status = ts->response_buf[5];
> > +		if (status) {
> > +			dev_err(ts->dev, "%s: HID output response, ERROR:%d\n",
> > +				__func__, status);
> > +			return -EPROTO;
> > +		}
> > +	}
> > +
> > +	if (offset == HID_APP_RESPONSE_REPORT_ID) {
> > +		command_code = ts->response_buf[HID_OUTPUT_RESPONSE_CMD_OFFSET]
> > +			& HID_OUTPUT_RESPONSE_CMD_MASK;
> > +		if (command_code != code) {
> > +			dev_err(ts->dev,
> > +				"%s: HID output response, wrong command_code:%X\n",
> > +				__func__, command_code);
> > +			return -EPROTO;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void cyttsp5_si_get_btn_data(struct cyttsp5 *ts)
> > +{
> > +	struct cyttsp5_sysinfo *si = &ts->sysinfo;
> > +	int i;
> > +	int num_btns = 0;
> > +	unsigned int btns = ts->response_buf[HID_SYSINFO_BTN_OFFSET]
> > +		& HID_SYSINFO_BTN_MASK;
> > +
> > +	for (i = 0; i < HID_SYSINFO_MAX_BTN; i++) {
> > +		if (btns & (1 << i))
> > +			num_btns++;
> > +	}
> > +	si->num_btns = num_btns;
> > +}
> > +
> > +static int cyttsp5_get_sysinfo_regs(struct cyttsp5 *ts)
> > +{
> > +	struct cyttsp5_sensing_conf_data *scd = &ts->sysinfo.sensing_conf_data;
> > +	struct cyttsp5_sensing_conf_data_dev *scd_dev =
> > +		(struct cyttsp5_sensing_conf_data_dev *)
> > +		&ts->response_buf[HID_SYSINFO_SENSING_OFFSET];
> > +	struct cyttsp5_sysinfo *si = &ts->sysinfo;
> > +
> > +	cyttsp5_si_get_btn_data(ts);
> > +
> > +	scd->max_tch = scd_dev->max_num_of_tch_per_refresh_cycle;
> > +	scd->res_x = get_unaligned_le16(&scd_dev->res_x);
> > +	scd->res_y = get_unaligned_le16(&scd_dev->res_y);
> > +	scd->max_z = get_unaligned_le16(&scd_dev->max_z);
> > +	scd->len_x = get_unaligned_le16(&scd_dev->len_x);
> > +	scd->len_y = get_unaligned_le16(&scd_dev->len_y);
> > +
> > +	si->xy_data = devm_kzalloc(ts->dev, scd->max_tch * TOUCH_REPORT_SIZE,
> > +				   GFP_KERNEL);
> > +	if (!si->xy_data)
> > +		return -ENOMEM;
> > +
> > +	si->xy_mode = devm_kzalloc(ts->dev, TOUCH_INPUT_HEADER_SIZE, GFP_KERNEL);
> > +	if (!si->xy_mode) {
> > +		devm_kfree(ts->dev, si->xy_data);
> > +		return -ENOMEM;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int cyttsp5_hid_output_get_sysinfo(struct cyttsp5 *ts)
> > +{
> > +	int rc, t;
> > +	u8 cmd[HID_OUTPUT_GET_SYSINFO_SIZE];
> > +
> > +	mutex_lock(&ts->system_lock);
> > +	ts->hid_cmd_state = HID_OUTPUT_GET_SYSINFO + 1;
> > +	mutex_unlock(&ts->system_lock);
> > +
> > +	/* HI bytes of Output register address */
> > +	cmd[0] = LOW_BYTE(HID_OUTPUT_GET_SYSINFO_SIZE);
> > +	cmd[1] = HI_BYTE(HID_OUTPUT_GET_SYSINFO_SIZE);
> > +	cmd[2] = HID_APP_OUTPUT_REPORT_ID;
> > +	cmd[3] = 0x0; /* Reserved */
> > +	cmd[4] = HID_OUTPUT_GET_SYSINFO;
> > +
> > +	rc = cyttsp5_write(ts, HID_OUTPUT_REG, cmd,
> > +			   HID_OUTPUT_GET_SYSINFO_SIZE);
> > +	if (rc) {
> > +		dev_err(ts->dev, "%s: Failed to write command %d",
> > +			__func__, rc);
> > +		goto error;
> > +	}
> > +
> > +	t = wait_event_timeout(ts->wait_q, (ts->hid_cmd_state == 0),
> > +			       msecs_to_jiffies(CY_HID_OUTPUT_GET_SYSINFO_TIMEOUT));
> > +	if (IS_TMO(t)) {
> > +		dev_err(ts->dev, "%s: HID output cmd execution timed out\n",
> > +			__func__);
> > +		rc = -ETIME;
> > +		goto error;
> > +	}
> > +
> > +	rc = cyttsp5_validate_cmd_response(ts, HID_OUTPUT_GET_SYSINFO);
> > +	goto exit;
> > +
> > +error:
> > +	mutex_lock(&ts->system_lock);
> > +	ts->hid_cmd_state = 0;
> > +	mutex_unlock(&ts->system_lock);
> > +	return rc;
> > +exit:
> > +	rc = cyttsp5_get_sysinfo_regs(ts);
> > +
> > +	return rc;
> > +}
> > +
> > +static int cyttsp5_hid_output_bl_launch_app(struct cyttsp5 *ts)
> > +{
> > +	int rc, t;
> > +	u8 cmd[HID_OUTPUT_BL_LAUNCH_APP];
> > +	u16 crc;
> > +
> > +	mutex_lock(&ts->system_lock);
> > +	ts->hid_cmd_state = HID_OUTPUT_BL_LAUNCH_APP + 1;
> > +	mutex_unlock(&ts->system_lock);
> > +
> > +	cmd[0] = LOW_BYTE(HID_OUTPUT_BL_LAUNCH_APP_SIZE);
> > +	cmd[1] = HI_BYTE(HID_OUTPUT_BL_LAUNCH_APP_SIZE);
> > +	cmd[2] = HID_BL_OUTPUT_REPORT_ID;
> > +	cmd[3] = 0x0; /* Reserved */
> > +	cmd[4] = HID_OUTPUT_BL_SOP;
> > +	cmd[5] = HID_OUTPUT_BL_LAUNCH_APP;
> > +	cmd[6] = 0x0; /* Low bytes of data */
> > +	cmd[7] = 0x0; /* Hi bytes of data */
> > +	crc = cyttsp5_compute_crc(&cmd[4], 4);
> > +	cmd[8] = LOW_BYTE(crc);
> > +	cmd[9] = HI_BYTE(crc);
> > +	cmd[10] = HID_OUTPUT_BL_EOP;
> > +
> > +	rc = cyttsp5_write(ts, HID_OUTPUT_REG, cmd,
> > +			   HID_OUTPUT_BL_LAUNCH_APP_SIZE);
>
> It seems like you'll send HID_OUTPUT_BL_LAUNCH_APP_SIZE twice here,
> once as setup in cyttsp5_write, and once in the buffer you want to
> send. Is that on purpose?

I am not sure to see the second time it is sent. What do you mean by "as 
setup in cyttsp5_write"? Anyway, the size is needed in the buffer of the 
frame.

> > +	if (rc) {
> > +		dev_err(ts->dev, "%s: Failed to write command %d",
> > +			__func__, rc);
> > +		goto error;
> > +	}
> > +
> > +	t = wait_event_timeout(ts->wait_q, (ts->hid_cmd_state == 0),
> > +			       msecs_to_jiffies(CY_HID_OUTPUT_TIMEOUT));
> > +	if (IS_TMO(t)) {
> > +		dev_err(ts->dev, "%s: HID output cmd execution timed out\n",
> > +			__func__);
> > +		rc = -ETIME;
> > +		goto error;
> > +	}
> > +
> > +	rc = cyttsp5_validate_cmd_response(ts, HID_OUTPUT_BL_LAUNCH_APP);
> > +	goto exit;
> > +
> > +error:
> > +	mutex_lock(&ts->system_lock);
> > +	ts->hid_cmd_state = 0;
> > +	mutex_unlock(&ts->system_lock);
> > +exit:
> > +	return rc;
> > +}
> > +
> > +static int cyttsp5_get_hid_descriptor(struct cyttsp5 *ts,
> > +				      struct cyttsp5_hid_desc *desc)
> > +{
> > +	struct device *dev = ts->dev;
> > +	__le16 hid_desc_register = HID_DESC_REG;
> > +	int rc;
> > +	int t;
> > +	u8 cmd[2];
> > +
> > +	/* Read HID descriptor length and version */
> > +	mutex_lock(&ts->system_lock);
> > +	ts->hid_cmd_state = 1;
> > +	mutex_unlock(&ts->system_lock);
> > +
> > +	/* Set HID descriptor register */
> > +	memcpy(cmd, &hid_desc_register, sizeof(hid_desc_register));
> > +
> > +	regmap_write(ts->regmap, HID_DESC_REG, cmd[0]);
> > +	rc = regmap_write(ts->regmap, HID_DESC_REG, cmd[1]);
> > +	if (rc < 0) {
> > +		dev_err(dev, "%s: failed to get HID descriptor, rc=%d\n",
> > +			__func__, rc);
> > +		goto error;
> > +	}
> > +
> > +	t = wait_event_timeout(ts->wait_q, (ts->hid_cmd_state == 0),
> > +			       msecs_to_jiffies(CY_HID_GET_HID_DESCRIPTOR_TIMEOUT));
> > +	if (IS_TMO(t)) {
> > +		dev_err(ts->dev, "%s: HID get descriptor timed out\n",
> > +			__func__);
> > +		rc = -ETIME;
> > +		goto error;
> > +	}
> > +
> > +	memcpy((u8 *)desc, ts->response_buf, sizeof(struct cyttsp5_hid_desc));
> > +
> > +	/* Check HID descriptor length and version */
> > +	if (le16_to_cpu(desc->hid_desc_len) != sizeof(*desc) ||
> > +	    le16_to_cpu(desc->bcd_version) != HID_VERSION) {
> > +		dev_err(dev, "%s: Unsupported HID version\n", __func__);
> > +		return -ENODEV;
> > +	}
> > +
> > +	goto exit;
> > +
> > +error:
> > +	mutex_lock(&ts->system_lock);
> > +	ts->hid_cmd_state = 0;
> > +	mutex_unlock(&ts->system_lock);
> > +exit:
> > +	return rc;
> > +}
> > +
> > +static int fill_tch_abs(struct cyttsp5_tch_abs_params *tch_abs, int report_size,
> > +			int offset)
> > +{
> > +	tch_abs->ofs = offset / 8;
> > +	tch_abs->size = report_size / 8;
> > +	if (report_size % 8)
> > +		tch_abs->size += 1;
> > +	tch_abs->min = 0;
> > +	tch_abs->max = 1 << report_size;
> > +	tch_abs->bofs = offset - (tch_abs->ofs << 3);
> > +
> > +	return 0;
> > +}
> > +
> > +static int move_button_data(struct cyttsp5 *ts, struct cyttsp5_sysinfo *si)
> > +{
> > +	memcpy(si->xy_mode, ts->input_buf, BTN_INPUT_HEADER_SIZE);
> > +	memcpy(si->xy_data, &ts->input_buf[BTN_INPUT_HEADER_SIZE],
> > +	       BTN_REPORT_SIZE);
> > +
> > +	return 0;
> > +}
> > +
> > +static int move_touch_data(struct cyttsp5 *ts, struct cyttsp5_sysinfo *si)
> > +{
> > +	int max_tch = si->sensing_conf_data.max_tch;
> > +	int num_cur_tch;
> > +	int length;
> > +	struct cyttsp5_tch_abs_params *tch = &si->tch_hdr;
> > +
> > +	memcpy(si->xy_mode, ts->input_buf, TOUCH_INPUT_HEADER_SIZE);
> > +
> > +	cyttsp5_get_touch_axis(&num_cur_tch, tch->size,
> > +			       tch->max, si->xy_mode + 3 + tch->ofs, tch->bofs);
> > +	if (unlikely(num_cur_tch > max_tch))
> > +		num_cur_tch = max_tch;
> > +
> > +	length = num_cur_tch * TOUCH_REPORT_SIZE;
> > +
> > +	memcpy(si->xy_data, &ts->input_buf[TOUCH_INPUT_HEADER_SIZE], length);
> > +	return 0;
> > +}
> > +
> > +static irqreturn_t cyttsp5_handle_irq(int irq, void *handle)
> > +{
> > +	struct cyttsp5 *ts = handle;
> > +	int report_id;
> > +	int size;
> > +	int rc;
> > +
> > +	rc = cyttsp5_read(ts, ts->input_buf, CY_MAX_INPUT);
> > +	if (!rc) {
> > +		size = get_unaligned_le16(&ts->input_buf[0]);
> > +
> > +		/* check reset */
> > +		if (size == 0) {
> > +			memcpy(ts->response_buf, ts->input_buf, 2);
> > +
> > +			ts->hid_cmd_state = 0;
> > +			wake_up(&ts->wait_q);
> > +			mutex_unlock(&ts->system_lock);
>
> It doesn't seem like you've taken that mutex anywhere in the
> interrupt. Where is it taken?

I think it is a removal from the clean-up, thanks.

> Note that taking a mutex in some code path, and releasing it in some
> other is pretty confusing, and locks are complicated enough to not
> introduce any confusion.

Sure, I will pay attention next time.

> > +			return IRQ_HANDLED;
> > +		}
> > +
> > +		report_id = ts->input_buf[2];
> > +
> > +		if (report_id == HID_TOUCH_REPORT_ID) {
> > +			move_touch_data(ts, &ts->sysinfo);
> > +			cyttsp5_mt_attention(ts->dev);
> > +		} else if (report_id == HID_BTN_REPORT_ID) {
> > +			move_button_data(ts, &ts->sysinfo);
> > +			cyttsp5_btn_attention(ts->dev);
> > +		} else {
> > +			/* It is not an input but a command response */
> > +			memcpy(ts->response_buf, ts->input_buf, size);
> > +
> > +			mutex_lock(&ts->system_lock);
> > +			ts->hid_cmd_state = 0;
> > +			mutex_unlock(&ts->system_lock);
>
> And you take and release that lock here, which is even weirder :)
>
> > +			wake_up(&ts->wait_q);
> > +		}
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int cyttsp5_deassert_int(struct cyttsp5 *ts)
> > +{
> > +	u16 size;
> > +	u8 buf[2];
> > +	int rc;
> > +
> > +	rc = regmap_bulk_read(ts->regmap, 0, buf, 2);
> > +	if (rc < 0)
> > +		return rc;
> > +
> > +	size = get_unaligned_le16(&buf[0]);
> > +	if (size == 2 || size == 0)
> > +		return 0;
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static int cyttsp5_fill_all_touch(struct cyttsp5 *ts)
> > +{
> > +	struct cyttsp5_sysinfo *si = &ts->sysinfo;
> > +
> > +	fill_tch_abs(&si->tch_abs[CY_TCH_X], REPORT_SIZE_16,
> > +		     TOUCH_REPORT_DESC_X);
> > +	fill_tch_abs(&si->tch_abs[CY_TCH_Y], REPORT_SIZE_16,
> > +		     TOUCH_REPORT_DESC_Y);
> > +	fill_tch_abs(&si->tch_abs[CY_TCH_P], REPORT_SIZE_8,
> > +		     TOUCH_REPORT_DESC_P);
> > +	fill_tch_abs(&si->tch_abs[CY_TCH_T], REPORT_SIZE_5,
> > +		     TOUCH_REPORT_DESC_CONTACTID);
> > +	fill_tch_abs(&si->tch_hdr, REPORT_SIZE_5,
> > +		     TOUCH_REPORT_DESC_HDR_CONTACTCOUNT);
> > +	fill_tch_abs(&si->tch_abs[CY_TCH_MAJ], REPORT_SIZE_8,
> > +		     TOUCH_REPORT_DESC_MAJ);
> > +	fill_tch_abs(&si->tch_abs[CY_TCH_MIN], REPORT_SIZE_8,
> > +		     TOUCH_REPORT_DESC_MIN);
> > +
> > +	return 0;
> > +}
> > +
> > +static int cyttsp5_startup(struct cyttsp5 *ts)
> > +{
> > +	int rc;
> > +
> > +	rc = cyttsp5_deassert_int(ts);
> > +	if (rc) {
> > +		dev_err(ts->dev, "%s: Error on deassert int r=%d\n",
> > +			__func__, rc);
> > +		return -ENODEV;
> > +	}
> > +
> > +	/*
> > +	 * Launch the application as the device starts in bootloader mode
> > +	 * because of a power-on-reset
> > +	 */
> > +	rc = cyttsp5_hid_output_bl_launch_app(ts);
> > +	if (rc < 0) {
> > +		dev_err(ts->dev, "%s: Error on launch app r=%d\n",
> > +			__func__, rc);
> > +		goto exit;
> > +	}
> > +
> > +	rc = cyttsp5_get_hid_descriptor(ts, &ts->hid_desc);
> > +	if (rc < 0) {
> > +		dev_err(ts->dev, "%s: Error on getting HID descriptor r=%d\n",
> > +			__func__, rc);
> > +		goto exit;
> > +	}
> > +
> > +	rc = cyttsp5_fill_all_touch(ts);
> > +	if (rc < 0) {
> > +		dev_err(ts->dev, "%s: Error on getting report descriptor r=%d\n",
> > +			__func__, rc);
> > +		goto exit;
> > +	}
> > +
> > +	rc = cyttsp5_hid_output_get_sysinfo(ts);
> > +	if (rc) {
> > +		dev_err(ts->dev, "%s: Error on getting sysinfo r=%d\n",
> > +			__func__, rc);
> > +		goto exit;
> > +	}
> > +
> > +exit:
> > +
> > +	return rc;
> > +}
> > +
> > +static const struct of_device_id cyttsp5_of_match[] = {
> > +	{ .compatible = "cypress,cyttsp5", },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, cyttsp5_of_match);
>
> You probably want to make that conditional to CONFIG_OF (and use
> of_match_ptr on the pointer to that table in your struct driver);

Yep!

> > +
> > +static const struct i2c_device_id cyttsp5_i2c_id[] = {
> > +	{ CYTTSP5_NAME, 0, },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, cyttsp5_i2c_id);
> > +
> > +static int cyttsp5_probe(struct device *dev, struct regmap *regmap, int irq,
> > +			 const char *name)
> > +{
> > +	struct cyttsp5 *ts;
> > +	struct cyttsp5_sysinfo *si;
> > +	int rc = 0, i;
> > +
> > +	ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
> > +	if (!ts) {
> > +		rc = -ENOMEM;
> > +		goto error_alloc_data;
> > +	}
> > +
> > +	/* Initialize device info */
> > +	ts->regmap = regmap;
> > +	ts->dev = dev;
> > +	si = &ts->sysinfo;
> > +	dev_set_drvdata(dev, ts);
> > +
> > +	/* Initialize mutexes and spinlocks */
> > +	mutex_init(&ts->system_lock);
> > +	mutex_init(&ts->mt_lock);
> > +	mutex_init(&ts->btn_lock);
> > +
> > +	/* Initialize wait queue */
> > +	init_waitqueue_head(&ts->wait_q);
> > +
> > +	/* Call platform init function */
> > +	ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > +	if (IS_ERR(ts->reset_gpio)) {
> > +		rc = PTR_ERR(ts->reset_gpio);
> > +		dev_err(dev, "Failed to request reset gpio, error %d\n", rc);
> > +		return rc;
> > +	}
> > +
> > +	/* Need a delay to have device up */
> > +	msleep(20);
>
> In the case where the device has already been out of reset, this means
> that this alone is not sufficient to reset it and bring it out of
> reset, which also means that you do not have any guarantee on the
> current state of the device. I'm not sure how it would impact the
> proper operations though.

Okay. A reset frame can be sent to perform a "software reset". Should I 
add it on startup to be in a "known behavior"?

> > +
> > +	ts->irq = irq;
>
> You don't seem to use ts->irq anywhere else, you can probably just
> keep it as a local variable.

Yes, removed for v2.

> > +	rc = devm_request_threaded_irq(dev, ts->irq, NULL, cyttsp5_handle_irq,
> > +				       IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> > +				       name, ts);
> > +	if (rc) {
> > +		dev_err(dev, "unable to request IRQ\n");
> > +		goto error_setup_irq;
> > +	}
> > +
> > +	rc = cyttsp5_startup(ts);
> > +	if (rc) {
> > +		dev_err(ts->dev, "%s: Fail initial startup r=%d\n",
> > +			__func__, rc);
> > +		goto error_startup;
> > +	}
> > +
> > +	rc = cyttsp5_parse_dt_key_code(dev);
> > +	if (rc < 0) {
> > +		dev_err(ts->dev, "%s: Error while parsing dts\n", __func__);
> > +		goto error_startup;
> > +	}
> > +
> > +	ts->input = input_allocate_device();
> > +	if (!ts->input) {
> > +		dev_err(dev, "%s: Error, failed to allocate input device\n",
> > +			__func__);
> > +		rc = -ENODEV;
> > +		goto error_startup;
> > +	}
>
> In error_startup, you never uninitialize the device. Given your
> comment in cyttsp5_startup, this means that you might end up in a
> situation where your driver probes again with the device initialized
> and no longer in a bootloader mode, which is likely to cause some
> error.
>
> Putting the call to cyttsp5_startup later will make you less likely to
> fail (especially because of the DT parsing), and putting the device
> back into reset will probably address the issue once and for all.

Hm, I am not sure to understand what you want to say, here.
Could you explain me more what you have in mind?

Notice that the DT parsing uses a sysinfo's variable (si->num_btns) 
which is retrieved in the startup function (thanks to get_sysinfo). So, 
currently, it is not possible to move the startup function after the DT 
parsing.

> > +	ts->input->name = "cyttsp5";
> > +	scnprintf(ts->phys, sizeof(ts->phys), "%s/input%d", dev_name(dev), 0);
> > +	ts->input->phys = ts->phys;
> > +	ts->input->dev.parent = ts->dev;
> > +	input_set_drvdata(ts->input, ts);
> > +
> > +	touchscreen_parse_properties(ts->input, true, NULL);
> > +
> > +	if (si) {
> > +		__set_bit(EV_KEY, ts->input->evbit);
> > +		for (i = 0; i < si->num_btns; i++)
> > +			__set_bit(si->btn[i].key_code, ts->input->keybit);
> > +
> > +		rc = cyttsp5_setup_input_device(dev);
> > +		if (rc)
> > +			goto error_init_input;
> > +	}
> > +
> > +	return 0;
> > +
> > +error_init_input:
> > +	input_free_device(ts->input);
> > +error_startup:
> > +error_setup_irq:
> > +	dev_set_drvdata(dev, NULL);
> > +error_alloc_data:
> > +	dev_err(dev, "%s failed.\n", __func__);
>
> I'm not sure using those __func__ everywhere is worth it, and given
> your code, this error log is redundant with the "real" error log you
> used before the goto.

Sure, the vendor's driver had these dev_err prints and, actually, I did 
not modify them.

> > +	if (dev->of_node)
> > +		of_node_put(dev->of_node);
>
> I haven't seen any of_node_get, or any function that would do it,
> called in your driver so far, why do you need it?

I forgot to remove it from the initial driver, thanks.

> > +
> > +	return rc;
> > +}
> > +
> > +static int cyttsp5_i2c_probe(struct i2c_client *client,
> > +			     const struct i2c_device_id *id)
> > +{
> > +	struct regmap *regmap;
> > +	static const struct regmap_config config = {
> > +		.reg_bits = 8,
> > +		.val_bits = 8,
> > +	};
> > +
> > +	regmap = devm_regmap_init_i2c(client, &config);
> > +	if (IS_ERR(regmap)) {
> > +		dev_err(&client->dev, "%s: regmap allocation failed: %ld\n",
> > +			__func__, PTR_ERR(regmap));
> > +		return PTR_ERR(regmap);
> > +	}
> > +
> > +	return cyttsp5_probe(&client->dev, regmap, client->irq, client->name);
> > +}
> > +
> > +static int cyttsp5_remove(struct device *dev)
> > +{
> > +	const struct of_device_id *match;
> > +	struct cyttsp5 *ts = dev_get_drvdata(dev);
> > +
> > +	input_unregister_device(ts->input);
> > +
> > +	dev_set_drvdata(dev, NULL);
> > +
> > +	match = of_match_device(of_match_ptr(cyttsp5_of_match), dev);
> > +	if (match && dev->of_node)
> > +		of_node_put(dev->of_node);
> > +
> > +	return 0;
> > +}
> > +
> > +static int cyttsp5_i2c_remove(struct i2c_client *client)
> > +{
> > +	struct device *dev = &client->dev;
> > +
> > +	return cyttsp5_remove(dev);
> > +}
> > +
> > +static struct i2c_driver cyttsp5_i2c_driver = {
> > +	.driver = {
> > +		.name = CYTTSP5_NAME,
> > +		.owner = THIS_MODULE,
> > +		.of_match_table = cyttsp5_of_match,
> > +	},
> > +	.probe = cyttsp5_i2c_probe,
> > +	.remove = cyttsp5_i2c_remove,
> > +	.id_table = cyttsp5_i2c_id,
> > +};
> > +
> > +module_i2c_driver(cyttsp5_i2c_driver);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_DESCRIPTION("Touchscreen driver for Cypress TrueTouch Gen 5 Product");
> > +MODULE_AUTHOR("Mylène Josserand <mylene.josserand@xxxxxxxxxxxxxxxxxx>");
>
> Good work cleaning it up!

Thank you!

Best regards,

--
Mylène Josserand, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com