Hello Dmitry,

Thank you for the quick response.
Understand and agreed on all your comments.

Little question about the name of prefix and this driver file,
could it be named according to different protocol version named by ILITEK internally
as ilitek_p6x.c (as protocol version 6 for Lego series, and ili210x.c should be protocol 2).

In addition, could we named it like how touchscreen/elants_i2c.c and eftf2127.c worked?
Is there a way that we could keep ilitek as prefix and driver file's name?


On Wed, Feb 24, 2021 at 22:23:33PM -0800, Dmitry Torokhov wrote:

Hi Joe,

quoted

quoted

quoted

+struct ilitek_protocol_info {

Let's use ili23xx instead of ilitek prefix throughout the file. Or
whatever the first generation of the product supported by this driver.

This driver is planned to support 213X/23XX/25XX.

OK, then I'd use ili213x as the prefix and would have source as
ili213x.c

quoted

[Question]
Was wondering why ilitek prefix is not feasible ?

Just because the driver does not support all Ilitek devices and we
already have drivers/input/touchscreen/ili210x.c

quoted

Is it fine that 23XX prefix that still support 25XX ICs ?

That is fine.

Understand, thank you very much.

quoted

quoted

quoted

+	s32				screen_min_y;
+	s32				max_tp;
+	s32				x_ch;
+	s32				y_ch;
+	s32				slave_count;

What is the importance of this?

slave_count is different among different ILITEK touch ICs.
It would be used during FW upgrade.
We needs FW upgrade touch IC's slave also.

I do not see it currently being used anywhere, that is why I question.
If it is needed for future enhancements, I would prefer it was added
with them, not ahead of time.

Agreed; would also remove other unused item in v4.

quoted

quoted

quoted

+/* ILITEK I2C R/W APIs */
+static int ilitek_i2c_transfer(struct i2c_client *client,
+			       struct i2c_msg *msgs, int cnt)
+{
+	int ret = 0, retry = 4;
+
+	while (retry--) {
+		ret = i2c_transfer(client->adapter, msgs, cnt);
+		if (ret >= 0)
+			break;
+		mdelay(20);
+	}

Why does it this device need retries?

I2C transfer may failed cause FW not ready for some reason.
It should be fine for basic probe/ISR flow, and risky for FW update.

[Question]
Should it be removed, and patched with FW update patch ?

I think I would prefer firmware update executing retries as needed, and
common paths not having them.

Understand; would also remove this API in v4.

quoted

quoted

quoted

+
+	if (ilitek_data->ilitek_repeat_start && delay == 0 &&

Where is this being set? As far as I understand, repeated start is
property of the i2c controller, not peripheral device.

Also, please see if you can switch to using regmap, as it should take
care of selecting right mode, depending on controller's capabilities.

Some ilitek touch ICs may not support "I2C repeat start".

OK, then we need to somehow activate this feature, because as it is it
is a dead code.

Agreed; would modified here in v4.
Current patch for Lego series ICs should all support repeated start.
Should remove it here.

quoted

And agreed; it should be removed here in v4.

quoted

quoted

+static void ilitek_key_down(struct ilitek_ts_data *ilitek_data,
+			    int x, int y)
+{
+	int j = 0;
+	int x_min;
+	int x_max;
+	int y_min;
+	int y_max;
+
+	for (j = 0; j < ilitek_data->keycount; j++) {
+		x_min = ilitek_data->keyinfo[j].x;
+		x_max = ilitek_data->keyinfo[j].x + ilitek_data->key_xlen;
+		y_min = ilitek_data->keyinfo[j].y;
+		y_max = ilitek_data->keyinfo[j].y + ilitek_data->key_ylen;
+
+		if (x >= x_min && x <= x_max && y >= y_min && y <= y_max) {
+			input_report_key(ilitek_data->input_dev,
+					 ilitek_data->keyinfo[j].id, 1);
+
+			ilitek_data->keyinfo[j].status = 1;
+			ilitek_data->touch_key_hold_press = true;
+			ilitek_data->is_touched = true;
+			break;
+		}
+	}
+}

This sounds like handling of "soft keys" which is better left for
userspace. OTOH the Linux keycode seems to be coming from controller
firmware. This is really weird.

It handles keys which key ID and region are get from touch device firmware.
Then check region of touch points to trigger key event.

[Question]
Is it not feasible for a touchscreen driver do key handling?

Typically this is better handled in userspace, as then it works across
various touch controllers, vs. needing this special one.

Agreed; should remove key-related flow in v4.

quoted

quoted

quoted

+	report_max_point = buf[REPORT_ADDRESS_COUNT];
+	if (report_max_point > ilitek_data->max_tp) {
+		dev_err(dev, "FW report max point:%d > panel info. max:%d\n",
+			report_max_point, ilitek_data->max_tp);
+		return -EINVAL;
+	}
+
+	count = CEIL(report_max_point, packet_max_point);
+	for (i = 1; i < count; i++) {
+		ret = ilitek_i2c_write_and_read(ilitek_data, NULL, 0, 0,
+			buf + i * 64, 64);
+		if (ret < 0) {
+			dev_err(dev, "get touch info. err, count=%d\n", count);
+			goto err_release_touch_and_key;
+		}
+	}

This means you have 1 + count transactions on i2c bus. Have you
considered reading max number of bytes in one transaction, and then soft
out how much of received data is valid?

Due to limitation on FW of touch device, data byte for each I2C transaction
is limited. It would lead to I2C communication issue when large count of bytes
are request.

Also, for only a few touch points,
It should be less efficient to query max number of bytes.

I was thinking about having one read transaction for up to <max bytes
your devices can transmit>, then look at the header and decide how many,
if any, more requests you would need to issue. This way for majority of
cases I believe you would need only one read per interrupt.

Understand; just have checked with device FW developer,
the max bytes devices when touch info. report is 64 bytes.
It should contain 10 touch point data.

More I2C read should be in the case for touch points > 10
and < 40, which touch device FW could support.

quoted

quoted

quoted

+static ssize_t firmware_version_show(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct ilitek_ts_data *ilitek_data = i2c_get_clientdata(client);
+	int ret = 0;
+	u8 outbuf[64] = {0};
+
+	ilitek_irq_disable(ilitek_data);
+	mutex_lock(&ilitek_data->ilitek_mutex);
+	ret = api_protocol_set_cmd(ilitek_data, GET_FW_VER, NULL, outbuf);

Do you need to query this all the time? Is it possible to collect this
data at boot or after firmware flash and store for subsequent use?

Agreed; just return the stored FW version (queried at boot or after FW update).
The same modification in showing product_id below.
Will update in v4.

[Question]
Is it a kind of consensus that don't do I2C transaction from show/read function via "cat"?

It just seems simpler to me to do this once at probe time, since it is
not going to change unless firmware is updated. If amount of data was
large, I'd consider querying it dynamically in show() as then it would
be more expensive memory-wise to store it on the off-chance userspace
might want to see it.

Understand; thank you for the explanation.
Will update in v4.

quoted

quoted

quoted

+
+static const struct i2c_device_id ilitek_ts_i2c_id[] = {
+	{ILITEK_TS_NAME, 0},
+	{ /* not omitted */ },

What does "not omitted" mean in this context?

It reminds developers that the empty brackets should not be removed.
Will remove the comment in v4.

I think typical notation is "{ /* sentinel */ }" for this.

Agreed; would remove it as other drivers do in v4.

Thank you.

-- 
Dmitry

Thank you very much

--
Joe Hung