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

[Mylene Josserand]

Hello Thomas,

On 30/05/2017 10:43, Thomas Petazzoni wrote:
> Hello,
>
> A few more comments :)

Thank you for the review!

> On Mon, 29 May 2017 16:45:37 +0200, MylÃne Josserand wrote:
> > +struct cyttsp5 {
> > +	struct device *dev;
> > +	struct mutex system_lock;
> > +	struct mutex btn_lock;
> > +	struct mutex mt_lock;
>
> Three mutexes for such a driver is probably excessive, especially when
> they are in fact used "sequentially".

Yes, I will have a look at these tree mutexes to see if there are 
necessary or not.

> > +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) {
>
> Invert the test, and remove one indentation level:

Thanks!

> 	if (!si->num_btns)
> 		return 0;
>
> > +		si->btn = devm_kzalloc(dev,
> > +				       si->num_btns * sizeof(struct cyttsp5_btn),
> > +				       GFP_KERNEL);
> > +		if (!si->btn)
> > +			return -ENOMEM;
> > +
> > +		/* Initialized the button to RESERVED */
>
> Initialized -> Initialize
>
> > +		for (i = 0; i < si->num_btns; i++) {
> > +			struct cyttsp5_btn *btns = &si->btn[i];
> > +			btns->key_code = KEY_RESERVED;
> > +			btns->enabled = true;
>
> Does it make sense to have the button as "enabled" when it in fact
> isn't really? Also setting btns->enabled = true is done again below.

No, it was used in the initial driver. I can remove it now.

> > +		}
> > +	}
> > +
> > +	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;
>
> Propagate "rc".
>
> > +		}
>
> Add blank line here.
>
> > +		btns->enabled = true;
> > +
> > +		i++;
> > +	}
>
> si->num_btns comes from the HW (calculated in function
> cyttsp5_si_get_btn_data), but then you are trusting the DT to have the
> exact same number of buttons.
>
> If the DT has more button that si->num_btns, then you have a buffer
> overflow here because you for_each_child_of_node() loop will loop after
> the end of si->btn[].

Oh, thanks for the catch!

> > +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) {
>
> Invert this test:
>
> 	if (!si->num_btns)
> 		return 0;
>
> and then you save one indentation level.
>
> Of course, move the test outside the mutex lock/unlock section.

ACK

> > +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))
>
> Replace (1 << i) by BIT(i)

ACK

> > +			num_btns++;
> > +	}
> > +	si->num_btns = num_btns;
>
> Does it really make sense to have a temporary variable, rather than
> using si->num_btns directly?

Not at all :)

> > +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);
>
> The point of devm_kzalloc() is to not need to do a devm_kfree(). Of
> course, check you're doing this only in the ->probe() but it seems to
> be the case.

Okay

> > +		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);
>
> This is called only from the _startup() function, itself called from
> the _probe() function. So taking a mutex is useless here, there is no
> concurrency.
>
> This hid_cmd_state thing is a bit strange. It's set to the following
> values:
>
> +	ts->hid_cmd_state = 1;
> +	ts->hid_cmd_state = HID_OUTPUT_GET_SYSINFO + 1;
> +	ts->hid_cmd_state = HID_OUTPUT_BL_LAUNCH_APP + 1;
>
> and when the interrupt occurs it's set back to 0.
>
> So: why those values? Why this "1" and then those magic values + 1 ? Do
> you need something else than BUSY/DONE ?
>
> What about:
>
> 	enum { HID_CMD_DONE, HID_CMD_BUSY } hid_cmd_state;

The initial driver was doing it like this, I missed this refactoring 
during my clean-up.
Thank you for the remark, it is much more understandable like that.

> > +
> > +	/* 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)) {
>
> I don't think this IS_TMO() macro makes sense. And you case use "rc" as
> the return value.

Yep, it is true.

> > +		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;
>
> It's really weird for the "normal" case to jump to an "exit" label. Why
> don't you handle the normal case here? Also, this code means that
> you're currently ignoring the return value of
> cyttsp5_validate_cmd_response().

Fixed for V2

> > +
> > +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);
> > +	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;
>
> Same comments as for the previous function.
>
> > +	/* Set HID descriptor register */
> > +	memcpy(cmd, &hid_desc_register, sizeof(hid_desc_register));
> > +
> > +	regmap_write(ts->regmap, HID_DESC_REG, cmd[0]);
>
> Return value?
>
> > +	rc = regmap_write(ts->regmap, HID_DESC_REG, cmd[1]);
>
> Why are you sending these through two regmap_write() calls and not a
> regmap_bulk_write() ?

You are right. I will even update the cyttysp5_write function to be more 
generic to use it in that case (ie send the register without any data).

> > +	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)) {
>
> Same comment as above: don't use IS_TMO(), use "rc" instead of "t".

ACK

> > +		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));
>
> I don't see why the u8* cast would be needed here.

ACK

> > +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);
>
> Blank line here.
>
> > +	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) {
>
> Same as usual: to remove one indentation level, invert the test, by
> doing:
>
> 	if (rc)
> 		return IRQ_HANDLED;

ACK

> > +static int cyttsp5_deassert_int(struct cyttsp5 *ts)
> > +{
> > +	u16 size;
> > +	u8 buf[2];
> > +	int rc;
> > +
> > +	rc = regmap_bulk_read(ts->regmap, 0, buf, 2);
>
> Which register are you reading here?

In fact, for the read part, it is using an Input Report Register and it 
does not care about the register address. The only frame to send to 
perform a read is a frame with the I2C device address. It is possible to 
send anything as register address, it will still work.
I guess that for some consistency, I will set it to the Input Register 
(0x3) but, for what I understood, it does not mean anything to the device.

I will also add a comment to explain this specificity in the code.

> > +	scnprintf(ts->phys, sizeof(ts->phys), "%s/input%d", dev_name(dev), 0);
>
> Why use %d if the value is always 0 ?

No explanation, it is a rest of the old driver :) I will update it.

Thanks!

Best regards,

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