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,
> > > > +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
>
> >
> > [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
>
> > Is it fine that 23XX prefix that still support 25XX ICs ?
>
> That is fine.
Understand, thank you very much.
>
> > > > + 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.
>
> > > > +/* 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.
> >
> > > > +
> > > > + 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.
> > And agreed; it should be removed here in v4.
> >
> > > > +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.
> > > > + 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.
> > >
> > > > +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.
> >
> > > > +
> > > > +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
