[resend,1/1] Input: add support for HiDeep touchscreen

Hi Anthony,

On Mon, Oct 16, 2017 at 11:10:06AM +0900, Anthony Kim wrote:
> The HiDeep touchscreen device is a capacitive multi-touch controller
> mainly for multi-touch supported devices use. It use I2C interface for
> communication to IC and provide axis X, Y, Z locations for ten finger
> touch through input event interface to userspace.
> 
> It support the Crimson and the Lime two type IC. They are different
> the number of channel supported and FW size. But the working protocol
> is same.

I was looking at the patch and I have a few concerns. The device seems
to speak big-endian when being programmed, so I think it would make
sense to have dwz_info and the firmware as bin-endian data as well, to
avoid unnecessary conversions.

...

> +static int hideep_pgm_r_mem(struct hideep_ts *ts, u32 addr,
> +	struct pgm_packet *packet, u32 len)
> +{
> +	int ret;
> +	int i;
> +	u8 *buff;
> +	struct i2c_msg msg[2];
> +
> +	if ((len % sizeof(u32)) != 0)
> +		return -EINVAL;
> +
> +	buff = kmalloc(len, GFP_KERNEL);
> +
> +	if (!buff)
> +		return -ENOMEM;
> +
> +	put_unaligned_be32((0x00 | (len / sizeof(u32) - 1)),
> +		&packet->header.w[0]);
> +	put_unaligned_be32(addr, &packet->header.w[1]);
> +
> +	msg[0].addr = ts->client->addr;
> +	msg[0].flags = 0;
> +	msg[0].len = 5;
> +	msg[0].buf = &packet->header.b[3];
> +
> +	msg[1].addr = ts->client->addr;
> +	msg[1].flags = I2C_M_RD;
> +	msg[1].len = len;
> +	msg[1].buf = buff;
> +
> +	ret = i2c_transfer(ts->client->adapter, msg, 2);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	for (i = 0; i < len / sizeof(u32); i++)
> +		packet->payload[i] = get_unaligned_be32(&buff[i * sizeof(u32)]);
> +
> +	return ret;
> +}

You are leaking memory here (buff is not freed).

...

> +
> +static int hideep_pgm_r_reg(struct hideep_ts *ts, u32 addr,
> +	u32 *val)
> +{
> +	int ret;
> +	struct pgm_packet packet;
> +
> +	put_unaligned_be32(0x00, &packet.header.w[0]);
> +	put_unaligned_be32(addr, &packet.header.w[1]);

This is going to be done again in hideep_pgm_r_mem(). BTW, the elements
are perfectly aligned here.

...

> +
> +static int hideep_load_dwz(struct hideep_ts *ts)
> +{
> +	int ret = 0;
> +	struct pgm_packet packet_r;
> +
> +	ret = hideep_enter_pgm(ts);
> +
> +	if (ret)
> +		return ret;
> +
> +	mdelay(50);
> +
> +	hideep_pgm_r_mem(ts, HIDEEP_DWZ_INFO, &packet_r,
> +		sizeof(struct dwz_info));
> +
> +	memcpy(&ts->dwz_info, packet_r.payload,
> +		sizeof(struct dwz_info));
> +
> +	SW_RESET_IN_PGM(10);
> +
> +	if (get_unaligned_le16(&ts->dwz_info.product_code) & 0x60) {
> +		/* Lime fw size */
> +		dev_dbg(&ts->client->dev, "used lime IC");
> +		ts->fw_size = 1024 * 64;
> +		ts->nvm_mask = 0x0030027B;
> +	} else if (get_unaligned_le16(&ts->dwz_info.product_code) & 0x40) {

Bits in 0x40 are subset of 0x60 so this branch will never fire. Did you
mean to write something like

	switch (product_code & SOME_MASK) {
	case 0x60:
		// handle lime
		break;

	case 040:
		// handle crimson
		break;

	default:
		// bad product
	}

...

> +
> +static int hideep_pwr_on(struct hideep_ts *ts)
> +{
> +	int ret = 0;
> +	u8 cmd = 0x01;
> +
> +	if (ts->vcc_vdd) {

You will never get a NULL regulator here.

...

> +
> +static int hideep_parse_event(struct hideep_ts *ts, u8 *data)
> +{
> +	int touch_count;
> +
> +	ts->tch_count = data[0];
> +	ts->key_count = data[1] & 0x0f;
> +	ts->lpm_count = data[1] & 0xf0;
> +
> +	/* get touch event count */
> +	dev_dbg(&ts->client->dev, "mt = %d, key = %d, lpm = %02x",
> +		ts->tch_count, ts->key_count, ts->lpm_count);
> +
> +	/* get touch event information */
> +	if (ts->tch_count < HIDEEP_MT_MAX)
> +		memcpy(ts->touch_event, &data[HIDEEP_TOUCH_EVENT_INDEX],
> +			HIDEEP_MT_MAX * sizeof(struct hideep_event));

Why do we need all this copying of the data?

...

> +
> +	ret = devm_add_action_or_reset(&ts->client->dev, hideep_pwr_off, ts);
> +	if (ret) {
> +		hideep_pwr_off(ts);

devm_add_action_or_reset() calls the "action" on failure, there is no
need to call hideep_pwr_off() again here.

> +	}
> +
> +	mdelay(30);
> +
> +	/* read info */
> +	ret = hideep_load_dwz(ts);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "fail to load dwz, ret = 0x%x", ret);
> +		return ret;
> +	}
> +
> +	/* init input device */
> +	ts->input_dev = devm_input_allocate_device(&client->dev);
> +	if (!ts->input_dev) {
> +		dev_err(&client->dev, "can't allocate memory for input_dev");
> +		return -ENOMEM;
> +	}
> +
> +	touchscreen_parse_properties(ts->input_dev, true, &ts->prop);

You need to tell touchscreen_parse_properties what axes to expect before
calling it.

I tried addressing all the comments above (and some more), below is the
incremental patch that should apply on top of the patch I am replying
to. I am also attaching full source of hideep.c in case you have trouble
applying this patch. Please give it a try and let me know if it still
works. If there are any additional changes needed, please do it in top
of this patch and when it is time to merge I'll squash it all together.

Thanks!

[resend,1/1] Input: add support for HiDeep touchscreen

Commit Message

Patch