Date:   Tue, 6 Jun 2017 10:03:48 +0200
From:   Mylene Josserand <mylene.josserand@...e-electrons.com>
To:     Maxime Ripard <maxime.ripard@...e-electrons.com>
Cc:     dmitry.torokhov@...il.com, fery@...ress.com, robh+dt@...nel.org,
        mark.rutland@....com, linux-kernel@...r.kernel.org,
        linux-input@...r.kernel.org, devicetree@...r.kernel.org,
        thomas.petazzoni@...e-electrons.com
Subject: Re: [PATCH 1/2] Input: Add driver for Cypress Generation 5
 touchscreen

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@...e-electrons.com>");
>
> Good work cleaning it up!

Thank you!

Best regards,

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

Powered by blists - more mailing lists