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

[Maxime Ripard <maxime.ripard@free-electrons.com>]

[-- Attachment #1: Type: text/plain, Size: 8993 bytes --]

On Tue, Jun 06, 2017 at 10:03:48AM +0200, Mylene Josserand wrote:
> > > +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.

Nevermind, I confused the HID_OUTPUT_BL_LAUNCH_APP_SIZE with the
register, my bad.

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

I guess you'd be safer just getting the GPIO with the low level by
default, and then changing to high. That way you know that you went
through a reset state, before bringing up the device.
> > > +	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?

I meant to say that there was still an issue with the reset here, and
moving the DT parsing code further would ease the reset operation.

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

Can't that be made the other way around? You parse the number of
buttons in the DT, and check the consistency with the hardware?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]