| Message ID | 20190302141704.32547-1-marex@denx.de (mailing list archive) |
|---|---|
| State | Under Review |
| Headers | show |
| Series | [1/2] Input: ili210x - Add DT binding for the Ilitek ILI2117 touch controller | expand |
On Sat, 2 Mar 2019 15:17:03 +0100, Marek Vasut wrote: > Add DT binding for the Ilitek ILI2117 touch controller, which is yet > again a slightly different device in the ILI21xx family. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Cc: Henrik Rydberg <rydberg@bitmath.org> > Cc: Olivier Sobrie <olivier@sobrie.be> > Cc: Philipp Puschmann <pp@emlix.com> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: devicetree@vger.kernel.org > To: linux-input@vger.kernel.org > --- > Documentation/devicetree/bindings/input/ilitek,ili2xxx.txt | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > Reviewed-by: Rob Herring <robh@kernel.org>
On 3/27/19 8:44 PM, Rob Herring wrote: > On Sat, 2 Mar 2019 15:17:03 +0100, Marek Vasut wrote: >> Add DT binding for the Ilitek ILI2117 touch controller, which is yet >> again a slightly different device in the ILI21xx family. >> >> Signed-off-by: Marek Vasut <marex@denx.de> >> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> >> Cc: Henrik Rydberg <rydberg@bitmath.org> >> Cc: Olivier Sobrie <olivier@sobrie.be> >> Cc: Philipp Puschmann <pp@emlix.com> >> Cc: Rob Herring <robh+dt@kernel.org> >> Cc: devicetree@vger.kernel.org >> To: linux-input@vger.kernel.org >> --- >> Documentation/devicetree/bindings/input/ilitek,ili2xxx.txt | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> > > Reviewed-by: Rob Herring <robh@kernel.org> Bump ? I noticed these patches were never applied.
Dmitry / Marek, There have been two attempts to add ILI2117 touch controller support. I was about to add a third, but luckily I checked the mailing list before writing any code :) Adding this support would clearly be beneficial for the common good. What can we do to get this in motion again? Last time I checked, Marek posted a patch which added the 2117, but Dmitry objected, because the code became too unwieldy. Dmitry then posted a cleanup patch, which did not work for Marek. So everything came to a halt. See: https://patchwork.kernel.org/patch/10836651/ https://www.spinics.net/lists/linux-input/msg62670.html Dmitry, would you perhaps be willing to accept Marek's patch, and perform the cleanup later? Marek, would you perhaps be willing to invest some time to debug Dmitry's cleanup patch? On my end, I've reviewed Dmitry's patch and it looks mostly ok. I saw one difference with ILI210X which could explain Marek's results, but I can't be sure - because I could not locate the 210X's register layout on the web. In Dmitry's patch, we see: touch = ili210x_report_events(priv, touchdata); if (touch || chip->continue_polling(touchdata)) schedule_delayed_work(&priv->dwork, msecs_to_jiffies(priv->poll_period)); but this is not exactly equivalent to the original. Because in the original, the 210X's decision to kick off delayed work is completely independent of the value of touch. Suggested replacement: touch = ili210x_report_events(priv, touchdata); if (chip->continue_polling) touch = chip->continue_polling(touchdata); Sven
On Fri, Nov 1, 2019 at 3:48 PM Sven Van Asbroeck <thesven73@gmail.com> wrote: > > Dmitry / Marek, > > There have been two attempts to add ILI2117 touch controller support. > I was about to add a third, but luckily I checked the mailing list > before writing any code :) > > Adding this support would clearly be beneficial for the common good. > What can we do to get this in motion again? > > Last time I checked, Marek posted a patch which added the 2117, but Dmitry > objected, because the code became too unwieldy. Dmitry then posted a cleanup > patch, which did not work for Marek. So everything came to a halt. > See: > https://patchwork.kernel.org/patch/10836651/ > https://www.spinics.net/lists/linux-input/msg62670.html > > Dmitry, would you perhaps be willing to accept Marek's patch, and perform the > cleanup later? > > Marek, would you perhaps be willing to invest some time to debug Dmitry's > cleanup patch? > > On my end, I've reviewed Dmitry's patch and it looks mostly ok. I saw one > difference with ILI210X which could explain Marek's results, but I can't be > sure - because I could not locate the 210X's register layout on the web. > > In Dmitry's patch, we see: > > touch = ili210x_report_events(priv, touchdata); > if (touch || chip->continue_polling(touchdata)) > schedule_delayed_work(&priv->dwork, > msecs_to_jiffies(priv->poll_period)); > > but this is not exactly equivalent to the original. Because in the original, > the 210X's decision to kick off delayed work is completely independent of > the value of touch. > If anyone is interested, I posted a patch to add ili2117A. https://patchwork.kernel.org/patch/10849877/ I am not sure if it's compatible with the non-A version. > Suggested replacement: > > touch = ili210x_report_events(priv, touchdata); > if (chip->continue_polling) > touch = chip->continue_polling(touchdata); > > Sven
On 11/4/19 12:55 AM, Adam Ford wrote: > On Fri, Nov 1, 2019 at 3:48 PM Sven Van Asbroeck wrote: >> >> Dmitry / Marek, >> >> There have been two attempts to add ILI2117 touch controller support. >> I was about to add a third, but luckily I checked the mailing list >> before writing any code :) >> >> Adding this support would clearly be beneficial for the common good. >> What can we do to get this in motion again? >> >> Last time I checked, Marek posted a patch which added the 2117, but Dmitry >> objected, because the code became too unwieldy. Dmitry then posted a cleanup >> patch, which did not work for Marek. So everything came to a halt. >> See: >> https://patchwork.kernel.org/patch/10836651/ >> https://www.spinics.net/lists/linux-input/msg62670.html >> >> Dmitry, would you perhaps be willing to accept Marek's patch, and perform the >> cleanup later? >> >> Marek, would you perhaps be willing to invest some time to debug Dmitry's >> cleanup patch? >> >> On my end, I've reviewed Dmitry's patch and it looks mostly ok. I saw one >> difference with ILI210X which could explain Marek's results, but I can't be >> sure - because I could not locate the 210X's register layout on the web. >> >> In Dmitry's patch, we see: >> >> touch = ili210x_report_events(priv, touchdata); >> if (touch || chip->continue_polling(touchdata)) >> schedule_delayed_work(&priv->dwork, >> msecs_to_jiffies(priv->poll_period)); >> >> but this is not exactly equivalent to the original. Because in the original, >> the 210X's decision to kick off delayed work is completely independent of >> the value of touch. >> > > If anyone is interested, I posted a patch to add ili2117A. > https://patchwork.kernel.org/patch/10849877/ > > I am not sure if it's compatible with the non-A version. This patch could've gone in as-is, the rework was not necessary (and indeed, didn't work). I don't know why this patch wasn't applied in the end, maybe it was just missed.
Hi Sven, On Fri, Nov 01, 2019 at 04:48:01PM -0400, Sven Van Asbroeck wrote: > Dmitry / Marek, > > There have been two attempts to add ILI2117 touch controller support. > I was about to add a third, but luckily I checked the mailing list > before writing any code :) > > Adding this support would clearly be beneficial for the common good. > What can we do to get this in motion again? > > Last time I checked, Marek posted a patch which added the 2117, but Dmitry > objected, because the code became too unwieldy. Dmitry then posted a cleanup > patch, which did not work for Marek. So everything came to a halt. > See: > https://patchwork.kernel.org/patch/10836651/ > https://www.spinics.net/lists/linux-input/msg62670.html > > Dmitry, would you perhaps be willing to accept Marek's patch, and perform the > cleanup later? > > Marek, would you perhaps be willing to invest some time to debug Dmitry's > cleanup patch? > > On my end, I've reviewed Dmitry's patch and it looks mostly ok. I saw one > difference with ILI210X which could explain Marek's results, but I can't be > sure - because I could not locate the 210X's register layout on the web. > > In Dmitry's patch, we see: > > touch = ili210x_report_events(priv, touchdata); > if (touch || chip->continue_polling(touchdata)) > schedule_delayed_work(&priv->dwork, > msecs_to_jiffies(priv->poll_period)); > > but this is not exactly equivalent to the original. Because in the original, > the 210X's decision to kick off delayed work is completely independent of > the value of touch. No, it is not independent really. Bits 0 and 1 in the first byte correspond to touches with first and 2nd finger, so checking for touch in addition to 0xf3 mask is not incorrect. Can you please tell me what device you have? Do the patches work for you? Marek, sorry for letting the patches linger. Can you please tell me what touch controller did you test with that failed for you? I think I see at least one issue in ili251x_read_touch_data() - the check whether we should read the second part of the packet should check if data[0] == 2, not 0. Thanks.
On Mon, Nov 04, 2019 at 01:16:21AM +0100, Marek Vasut wrote: > On 11/4/19 12:55 AM, Adam Ford wrote: > > On Fri, Nov 1, 2019 at 3:48 PM Sven Van Asbroeck wrote: > >> > >> Dmitry / Marek, > >> > >> There have been two attempts to add ILI2117 touch controller support. > >> I was about to add a third, but luckily I checked the mailing list > >> before writing any code :) > >> > >> Adding this support would clearly be beneficial for the common good. > >> What can we do to get this in motion again? > >> > >> Last time I checked, Marek posted a patch which added the 2117, but Dmitry > >> objected, because the code became too unwieldy. Dmitry then posted a cleanup > >> patch, which did not work for Marek. So everything came to a halt. > >> See: > >> https://patchwork.kernel.org/patch/10836651/ > >> https://www.spinics.net/lists/linux-input/msg62670.html > >> > >> Dmitry, would you perhaps be willing to accept Marek's patch, and perform the > >> cleanup later? > >> > >> Marek, would you perhaps be willing to invest some time to debug Dmitry's > >> cleanup patch? > >> > >> On my end, I've reviewed Dmitry's patch and it looks mostly ok. I saw one > >> difference with ILI210X which could explain Marek's results, but I can't be > >> sure - because I could not locate the 210X's register layout on the web. > >> > >> In Dmitry's patch, we see: > >> > >> touch = ili210x_report_events(priv, touchdata); > >> if (touch || chip->continue_polling(touchdata)) > >> schedule_delayed_work(&priv->dwork, > >> msecs_to_jiffies(priv->poll_period)); > >> > >> but this is not exactly equivalent to the original. Because in the original, > >> the 210X's decision to kick off delayed work is completely independent of > >> the value of touch. > >> > > > > If anyone is interested, I posted a patch to add ili2117A. > > https://patchwork.kernel.org/patch/10849877/ > > > > I am not sure if it's compatible with the non-A version. > > This patch could've gone in as-is, the rework was not necessary (and > indeed, didn't work). I don't know why this patch wasn't applied in the > end, maybe it was just missed. Do we really need a brand new driver here? It looks pretty similar to the other flavors... Thanks.
On 11/4/19 8:01 AM, Dmitry Torokhov wrote: > Hi Sven, > > On Fri, Nov 01, 2019 at 04:48:01PM -0400, Sven Van Asbroeck wrote: >> Dmitry / Marek, >> >> There have been two attempts to add ILI2117 touch controller support. >> I was about to add a third, but luckily I checked the mailing list >> before writing any code :) >> >> Adding this support would clearly be beneficial for the common good. >> What can we do to get this in motion again? >> >> Last time I checked, Marek posted a patch which added the 2117, but Dmitry >> objected, because the code became too unwieldy. Dmitry then posted a cleanup >> patch, which did not work for Marek. So everything came to a halt. >> See: >> https://patchwork.kernel.org/patch/10836651/ >> https://www.spinics.net/lists/linux-input/msg62670.html >> >> Dmitry, would you perhaps be willing to accept Marek's patch, and perform the >> cleanup later? >> >> Marek, would you perhaps be willing to invest some time to debug Dmitry's >> cleanup patch? >> >> On my end, I've reviewed Dmitry's patch and it looks mostly ok. I saw one >> difference with ILI210X which could explain Marek's results, but I can't be >> sure - because I could not locate the 210X's register layout on the web. >> >> In Dmitry's patch, we see: >> >> touch = ili210x_report_events(priv, touchdata); >> if (touch || chip->continue_polling(touchdata)) >> schedule_delayed_work(&priv->dwork, >> msecs_to_jiffies(priv->poll_period)); >> >> but this is not exactly equivalent to the original. Because in the original, >> the 210X's decision to kick off delayed work is completely independent of >> the value of touch. > > No, it is not independent really. Bits 0 and 1 in the first byte > correspond to touches with first and 2nd finger, so checking for touch > in addition to 0xf3 mask is not incorrect. > > Can you please tell me what device you have? Do the patches work for > you? > > Marek, sorry for letting the patches linger. Can you please tell me what > touch controller did you test with that failed for you? See Message-ID <20190917032842.GL237523@dtor-ws> . I tested the ILI2117 with these two patches and it works. With the additional two patches from you on top, it failed, unless I reverted: Input: ili210x - define and use chip operations structure > I think I see at > least one issue in ili251x_read_touch_data() - the check whether we > should read the second part of the packet should check if data[0] == 2, > not 0. But that's not a problem of this particular patch, so maybe this patch can finally be applied and then we can debug the subsequent patches ?
On Mon, Nov 4, 2019 at 2:01 AM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > Can you please tell me what device you have? Do the patches work for > you? I have an ILI2117A/ILI2118A. I'll try out the patches today. I'm stuck with a 4.1 vendor kernel, so I'll need to backport the (patched) mainline driver to 4.1 before I'm able to test. I wil try Marek's patch, and Dmitry's rebased patch from https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git/log/?h=ili2xxx-touchscreen and let you know the results. If the problem with Dmitry's patch is located in ili251x_read_touch_data, then using a ILI2117A should work fine.
On Mon, Nov 4, 2019 at 7:49 AM Sven Van Asbroeck <thesven73@gmail.com> wrote: > > On Mon, Nov 4, 2019 at 2:01 AM Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > > > Can you please tell me what device you have? Do the patches work for > > you? > > I have an ILI2117A/ILI2118A. > > I'll try out the patches today. I'm stuck with a 4.1 vendor kernel, so > I'll need to backport the (patched) mainline driver to 4.1 before > I'm able to test. > > I wil try Marek's patch, and Dmitry's rebased patch from > https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git/log/?h=ili2xxx-touchscreen > and let you know the results. For what it's worth, I tried the 3-patch series from that branch on 5.4-RC5, but the ts_calibrate and ts_test_mt do not appear to receive touch info. They execute when I 'export TSLIB_TSDEVICE=/dev/input/event0' but I don't get touch data. If I do a hex-dump of /dev/event0 and it dumps data if and only if I hold my finger on the touch screen for a while. I'll try just doing the 1st patch to see if it it's any better. adam > > If the problem with Dmitry's patch is located in ili251x_read_touch_data, > then using a ILI2117A should work fine.
On Mon, Nov 04, 2019 at 08:19:28AM -0600, Adam Ford wrote: > On Mon, Nov 4, 2019 at 7:49 AM Sven Van Asbroeck <thesven73@gmail.com> wrote: > > > > On Mon, Nov 4, 2019 at 2:01 AM Dmitry Torokhov > > <dmitry.torokhov@gmail.com> wrote: > > > > > > Can you please tell me what device you have? Do the patches work for > > > you? > > > > I have an ILI2117A/ILI2118A. > > > > I'll try out the patches today. I'm stuck with a 4.1 vendor kernel, so > > I'll need to backport the (patched) mainline driver to 4.1 before > > I'm able to test. > > > > I wil try Marek's patch, and Dmitry's rebased patch from > > https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git/log/?h=ili2xxx-touchscreen > > and let you know the results. > > For what it's worth, I tried the 3-patch series from that branch on > 5.4-RC5, but the ts_calibrate and ts_test_mt do not appear to receive > touch info. They execute when I 'export > TSLIB_TSDEVICE=/dev/input/event0' but I don't get touch data. > > If I do a hex-dump of /dev/event0 and it dumps data if and only if I > hold my finger on the touch screen for a while. > > I'll try just doing the 1st patch to see if it it's any better. OK, I see where I messed up ili211x_read_touch_data(), it should use i2c_master_recv() and not ili210x_read_reg(). I'll make a fix up patch later today.
Ok, so here are my test results on an ili211x :
Using Marek's patch:
https://patchwork.kernel.org/patch/10836651/#22811657
It works fine.
Using Dmitry's patch:
https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git/log/?h=ili2xxx-touchscreen
Does not work at all - the driver even enters an infinite loop.
I tracked this down to two separate issues:
1. the ili211x does not have a touchdata register; but the driver tries to
read from one.
2. the ili211x should never poll - otherwise it will read all zeros, which
passes the crc check (!), then results in ten finger touches all
at (x=0, y=0).
The patch at the end of this e-mail addresses these two issues. When it is
applied, the ili211x works fine.
Of course, this does not address the issue Marek saw with Dmitry's patch
on the ili251x.
Sven
diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
index 7a9c46b8a079..f51a3a19075f 100644
--- a/drivers/input/touchscreen/ili210x.c
+++ b/drivers/input/touchscreen/ili210x.c
@@ -36,7 +36,7 @@ struct ili2xxx_chip {
int (*get_touch_data)(struct i2c_client *client, u8 *data);
bool (*parse_touch_data)(const u8 *data, unsigned int finger,
unsigned int *x, unsigned int *y);
- bool (*continue_polling)(const u8 *data);
+ bool (*continue_polling)(const u8 *data, bool has_touch);
unsigned int max_touches;
};
@@ -96,7 +96,7 @@ static bool ili210x_touchdata_to_coords(const u8 *touchdata,
return true;
}
-static bool ili210x_check_continue_polling(const u8 *data)
+static bool ili210x_check_continue_polling(const u8 *data, bool has_touch)
{
return data[0] & 0xf3;
}
@@ -111,14 +111,19 @@ static const struct ili2xxx_chip ili210x_chip = {
static int ili211x_read_touch_data(struct i2c_client *client, u8 *data)
{
+ struct i2c_msg msg = {
+ .addr = client->addr,
+ .flags = I2C_M_RD,
+ .len = ILI211X_DATA_SIZE,
+ .buf = data,
+ };
s16 sum = 0;
int i;
- int error;
- error = ili210x_read_reg(client, REG_TOUCHDATA,
- data, ILI211X_DATA_SIZE);
- if (error)
- return error;
+ if (i2c_transfer(client->adapter, &msg, 1) != 1) {
+ dev_err(&client->dev, "i2c transfer failed\n");
+ return -EIO;
+ }
/* This chip uses custom checksum at the end of data */
for (i = 0; i <= ILI211X_DATA_SIZE - 2; i++)
@@ -152,7 +157,7 @@ static bool ili211x_touchdata_to_coords(const u8 *touchdata,
return true;
}
-static bool ili2xxx_decline_polling(const u8 *data)
+static bool ili2xxx_decline_polling(const u8 *data, bool has_touch)
{
return false;
}
@@ -216,11 +221,16 @@ static bool ili251x_touchdata_to_coords(const u8 *touchdata,
return true;
}
+static bool ili251x_check_continue_polling(const u8 *data, bool has_touch)
+{
+ return has_touch;
+}
+
static const struct ili2xxx_chip ili251x_chip = {
.read_reg = ili251x_read_reg,
.get_touch_data = ili251x_read_touch_data,
.parse_touch_data = ili251x_touchdata_to_coords,
- .continue_polling = ili2xxx_decline_polling,
+ .continue_polling = ili251x_check_continue_polling,
.max_touches = 10,
};
@@ -268,7 +278,7 @@ static irqreturn_t ili210x_irq(int irq, void *irq_data)
}
touch = ili210x_report_events(priv, touchdata);
- keep_polling = touch || chip->continue_polling(touchdata);
+ keep_polling = chip->continue_polling(touchdata, touch);
if (keep_polling)
msleep(ILI2XXX_POLL_PERIOD);
} while (!priv->stop && keep_polling);
On Mon, Nov 4, 2019 at 12:37 PM Sven Van Asbroeck <thesven73@gmail.com> wrote: > > Ok, so here are my test results on an ili211x : > > Using Marek's patch: > https://patchwork.kernel.org/patch/10836651/#22811657 > It works fine. I am using IRQ_TYPE_EDGE_RISING for the 2117A. Is that correct? For my touchscreen, the IRQ line is low until a touch is detected, so I assume we want to capure on the rising edge. I noticed the example uses IRQ_TYPE_EDGE_FALLING. If rising is correct, we should probably update the binding to show an example for the 2117. > > Using Dmitry's patch: > https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git/log/?h=ili2xxx-touchscreen > Does not work at all - the driver even enters an infinite loop. > Regarding Dmitry's patch, Is it a good idea to use msleep in an IRQ? It seems like using the schedule_delayed_work() call seems like it will get in and get out of the ISR faster. If we use msleep and scan again, isn't it possible to starve other processes? > I tracked this down to two separate issues: > 1. the ili211x does not have a touchdata register; but the driver tries to > read from one. > 2. the ili211x should never poll - otherwise it will read all zeros, which > passes the crc check (!), then results in ten finger touches all > at (x=0, y=0). > > The patch at the end of this e-mail addresses these two issues. When it is > applied, the ili211x works fine. > > Of course, this does not address the issue Marek saw with Dmitry's patch > on the ili251x. > Sven's patches appear to work for me when manually applied on top of Dmity' and Marek's patches. > Sven > > diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c > index 7a9c46b8a079..f51a3a19075f 100644 > --- a/drivers/input/touchscreen/ili210x.c > +++ b/drivers/input/touchscreen/ili210x.c > @@ -36,7 +36,7 @@ struct ili2xxx_chip { > int (*get_touch_data)(struct i2c_client *client, u8 *data); > bool (*parse_touch_data)(const u8 *data, unsigned int finger, > unsigned int *x, unsigned int *y); > - bool (*continue_polling)(const u8 *data); > + bool (*continue_polling)(const u8 *data, bool has_touch); > unsigned int max_touches; > }; > > @@ -96,7 +96,7 @@ static bool ili210x_touchdata_to_coords(const u8 *touchdata, > return true; > } > > -static bool ili210x_check_continue_polling(const u8 *data) > +static bool ili210x_check_continue_polling(const u8 *data, bool has_touch) > { > return data[0] & 0xf3; > } > @@ -111,14 +111,19 @@ static const struct ili2xxx_chip ili210x_chip = { > > static int ili211x_read_touch_data(struct i2c_client *client, u8 *data) > { > + struct i2c_msg msg = { > + .addr = client->addr, > + .flags = I2C_M_RD, > + .len = ILI211X_DATA_SIZE, > + .buf = data, > + }; > s16 sum = 0; > int i; > - int error; > > - error = ili210x_read_reg(client, REG_TOUCHDATA, > - data, ILI211X_DATA_SIZE); > - if (error) > - return error; > + if (i2c_transfer(client->adapter, &msg, 1) != 1) { > + dev_err(&client->dev, "i2c transfer failed\n"); > + return -EIO; > + } > > /* This chip uses custom checksum at the end of data */ > for (i = 0; i <= ILI211X_DATA_SIZE - 2; i++) > @@ -152,7 +157,7 @@ static bool ili211x_touchdata_to_coords(const u8 *touchdata, > return true; > } > > -static bool ili2xxx_decline_polling(const u8 *data) > +static bool ili2xxx_decline_polling(const u8 *data, bool has_touch) > { > return false; > } > @@ -216,11 +221,16 @@ static bool ili251x_touchdata_to_coords(const u8 *touchdata, > return true; > } > > +static bool ili251x_check_continue_polling(const u8 *data, bool has_touch) > +{ > + return has_touch; > +} > + > static const struct ili2xxx_chip ili251x_chip = { > .read_reg = ili251x_read_reg, > .get_touch_data = ili251x_read_touch_data, > .parse_touch_data = ili251x_touchdata_to_coords, > - .continue_polling = ili2xxx_decline_polling, > + .continue_polling = ili251x_check_continue_polling, > .max_touches = 10, > }; > > @@ -268,7 +278,7 @@ static irqreturn_t ili210x_irq(int irq, void *irq_data) > } > > touch = ili210x_report_events(priv, touchdata); > - keep_polling = touch || chip->continue_polling(touchdata); > + keep_polling = chip->continue_polling(touchdata, touch); > if (keep_polling) Why not just check the value of touch instead of invoking the function pointer which takes the value of touch in as a parameter? > msleep(ILI2XXX_POLL_PERIOD); > } while (!priv->stop && keep_polling); > -- > 2.17.1 adam
Hi Adam, On Mon, Nov 4, 2019 at 4:28 PM Adam Ford <aford173@gmail.com> wrote: > > I am using IRQ_TYPE_EDGE_RISING for the 2117A. Is that correct? For > my touchscreen, the IRQ line is low until a touch is detected, so I > assume we want to capure on the rising edge. That is correct for the 2117A, as far as I know. I am using the same setting. > > Regarding Dmitry's patch, > Is it a good idea to use msleep in an IRQ? It seems like using the > schedule_delayed_work() call seems like it will get in and get out of > the ISR faster. > > If we use msleep and scan again, isn't it possible to starve other processes? I believe using msleep() is ok because this is not a "real" interrupt handler, but a threaded one. It runs in a regular kernel thread, with its interrupt turned off (but all other interrupts remain enabled). Its interrupt is re-enabled automatically after the threaded handler returns. See https://elixir.bootlin.com/linux/latest/source/include/linux/interrupt.h#L50 > > @@ -268,7 +278,7 @@ static irqreturn_t ili210x_irq(int irq, void *irq_data) > > } > > > > touch = ili210x_report_events(priv, touchdata); > > - keep_polling = touch || chip->continue_polling(touchdata); > > + keep_polling = chip->continue_polling(touchdata, touch); > > if (keep_polling) > > Why not just check the value of touch instead of invoking the function > pointer which takes the value of touch in as a parameter? > The value of touch must be checked inside the callback, because some variants use it to decide if they should poll again, and some do not, such as the ili211x. If I have misinterpreted your suggestion, could you perhaps express it in C, so I can understand better?
On Mon, Nov 4, 2019 at 3:43 PM Sven Van Asbroeck <thesven73@gmail.com> wrote: > > Hi Adam, > > On Mon, Nov 4, 2019 at 4:28 PM Adam Ford <aford173@gmail.com> wrote: > > > > I am using IRQ_TYPE_EDGE_RISING for the 2117A. Is that correct? For > > my touchscreen, the IRQ line is low until a touch is detected, so I > > assume we want to capure on the rising edge. > > That is correct for the 2117A, as far as I know. I am using the same > setting. > > > > > Regarding Dmitry's patch, > > Is it a good idea to use msleep in an IRQ? It seems like using the > > schedule_delayed_work() call seems like it will get in and get out of > > the ISR faster. > > > > If we use msleep and scan again, isn't it possible to starve other processes? > > I believe using msleep() is ok because this is not a "real" interrupt handler, > but a threaded one. It runs in a regular kernel thread, with its interrupt > turned off (but all other interrupts remain enabled). Its interrupt is > re-enabled automatically after the threaded handler returns. > > See > https://elixir.bootlin.com/linux/latest/source/include/linux/interrupt.h#L50 > > > > @@ -268,7 +278,7 @@ static irqreturn_t ili210x_irq(int irq, void *irq_data) > > > } > > > > > > touch = ili210x_report_events(priv, touchdata); > > > - keep_polling = touch || chip->continue_polling(touchdata); > > > + keep_polling = chip->continue_polling(touchdata, touch); > > > if (keep_polling) > > > > Why not just check the value of touch instead of invoking the function > > pointer which takes the value of touch in as a parameter? > > > > The value of touch must be checked inside the callback, because > some variants use it to decide if they should poll again, and > some do not, such as the ili211x. That makes sense. > > If I have misinterpreted your suggestion, could you perhaps > express it in C, so I can understand better? You explained it. I'm good. adam
On Mon, Nov 04, 2019 at 05:25:23PM -0600, Adam Ford wrote: > On Mon, Nov 4, 2019 at 3:43 PM Sven Van Asbroeck <thesven73@gmail.com> wrote: > > > > Hi Adam, > > > > On Mon, Nov 4, 2019 at 4:28 PM Adam Ford <aford173@gmail.com> wrote: > > > > > > I am using IRQ_TYPE_EDGE_RISING for the 2117A. Is that correct? For > > > my touchscreen, the IRQ line is low until a touch is detected, so I > > > assume we want to capure on the rising edge. > > > > That is correct for the 2117A, as far as I know. I am using the same > > setting. > > > > > > > > Regarding Dmitry's patch, > > > Is it a good idea to use msleep in an IRQ? It seems like using the > > > schedule_delayed_work() call seems like it will get in and get out of > > > the ISR faster. > > > > > > If we use msleep and scan again, isn't it possible to starve other processes? > > > > I believe using msleep() is ok because this is not a "real" interrupt handler, > > but a threaded one. It runs in a regular kernel thread, with its interrupt > > turned off (but all other interrupts remain enabled). Its interrupt is > > re-enabled automatically after the threaded handler returns. > > > > See > > https://elixir.bootlin.com/linux/latest/source/include/linux/interrupt.h#L50 > > > > > > @@ -268,7 +278,7 @@ static irqreturn_t ili210x_irq(int irq, void *irq_data) > > > > } > > > > > > > > touch = ili210x_report_events(priv, touchdata); > > > > - keep_polling = touch || chip->continue_polling(touchdata); > > > > + keep_polling = chip->continue_polling(touchdata, touch); > > > > if (keep_polling) > > > > > > Why not just check the value of touch instead of invoking the function > > > pointer which takes the value of touch in as a parameter? > > > > > > > The value of touch must be checked inside the callback, because > > some variants use it to decide if they should poll again, and > > some do not, such as the ili211x. > > That makes sense. > > > > If I have misinterpreted your suggestion, could you perhaps > > express it in C, so I can understand better? > > You explained it. > I'm good. OK, I refreshed the branch with fixes and a couple of new patches. It is on top of 5.3 now. If this works for you guys I will be merging it for 5.5. Thanks.
On Mon, Nov 4, 2019 at 5:36 PM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > On Mon, Nov 04, 2019 at 05:25:23PM -0600, Adam Ford wrote: > > On Mon, Nov 4, 2019 at 3:43 PM Sven Van Asbroeck <thesven73@gmail.com> wrote: > > > > > > Hi Adam, > > > > > > On Mon, Nov 4, 2019 at 4:28 PM Adam Ford <aford173@gmail.com> wrote: > > > > > > > > I am using IRQ_TYPE_EDGE_RISING for the 2117A. Is that correct? For > > > > my touchscreen, the IRQ line is low until a touch is detected, so I > > > > assume we want to capure on the rising edge. > > > > > > That is correct for the 2117A, as far as I know. I am using the same > > > setting. > > > > > > > > > > > Regarding Dmitry's patch, > > > > Is it a good idea to use msleep in an IRQ? It seems like using the > > > > schedule_delayed_work() call seems like it will get in and get out of > > > > the ISR faster. > > > > > > > > If we use msleep and scan again, isn't it possible to starve other processes? > > > > > > I believe using msleep() is ok because this is not a "real" interrupt handler, > > > but a threaded one. It runs in a regular kernel thread, with its interrupt > > > turned off (but all other interrupts remain enabled). Its interrupt is > > > re-enabled automatically after the threaded handler returns. > > > > > > See > > > https://elixir.bootlin.com/linux/latest/source/include/linux/interrupt.h#L50 > > > > > > > > @@ -268,7 +278,7 @@ static irqreturn_t ili210x_irq(int irq, void *irq_data) > > > > > } > > > > > > > > > > touch = ili210x_report_events(priv, touchdata); > > > > > - keep_polling = touch || chip->continue_polling(touchdata); > > > > > + keep_polling = chip->continue_polling(touchdata, touch); > > > > > if (keep_polling) > > > > > > > > Why not just check the value of touch instead of invoking the function > > > > pointer which takes the value of touch in as a parameter? > > > > > > > > > > The value of touch must be checked inside the callback, because > > > some variants use it to decide if they should poll again, and > > > some do not, such as the ili211x. > > > > That makes sense. > > > > > > If I have misinterpreted your suggestion, could you perhaps > > > express it in C, so I can understand better? > > > > You explained it. > > I'm good. > > OK, I refreshed the branch with fixes and a couple of new patches. It is > on top of 5.3 now. If this works for you guys I will be merging it for > 5.5. I will test it tomorrow on a 2117a and reply with results. I am very excited to see this integrated. adam > > Thanks. > > -- > Dmitry
On Mon, Nov 4, 2019 at 6:40 PM Adam Ford <aford173@gmail.com> wrote: > > I will test it tomorrow on a 2117a and reply with results. I am very > excited to see this integrated. > I will do the same. That should give us confidence that 211x works ok. Dmitry, should someone retest 251x and 210x after such a significant change? Unfortunately I don't have access to these chips.
On Mon, Nov 4, 2019 at 8:04 PM Sven Van Asbroeck <thesven73@gmail.com> wrote: > > On Mon, Nov 4, 2019 at 6:40 PM Adam Ford <aford173@gmail.com> wrote: > > > > I will test it tomorrow on a 2117a and reply with results. I am very > > excited to see this integrated. For the series: Tested-by: Adam Ford <aford173@gmail.com> #imx6q-logicpd adam > > > > I will do the same. That should give us confidence that 211x works > ok. > > Dmitry, should someone retest 251x and 210x after such a significant > change? Unfortunately I don't have access to these chips.
On Mon, Nov 4, 2019 at 6:36 PM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > OK, I refreshed the branch with fixes and a couple of new patches. It is > on top of 5.3 now. If this works for you guys I will be merging it for > 5.5. For the series: Tested-by: Sven Van Asbroeck <TheSven73@gmail.com> # ILI2118A variant
On Mon, Nov 4, 2019 at 6:36 PM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > OK, I refreshed the branch with fixes and a couple of new patches. It is > on top of 5.3 now. If this works for you guys I will be merging it for > 5.5. > According to the ili2117a/2118a datasheet I have, there are still a few loose ends. Some of these might be too inconsequential to worry about. Dmitry, tell me which ones you think are important, if any, and I will spin a patch if you like. Or you can do it, just let me know. > { "ili210x", (long)&ili210x_chip }, > { "ili2117", (long)&ili211x_chip }, > { "ili251x", (long)&ili251x_chip }, > > { .compatible = "ilitek,ili210x", .data = &ili210x_chip }, > { .compatible = "ilitek,ili2117", .data = &ili211x_chip }, > { .compatible = "ilitek,ili251x", .data = &ili251x_chip }, My datasheet says ILI2117A/ILI2118A, so maybe the compatible string should really be "ilitek,ili211x", just like the other variants ? In addition, should we add ili2117/ili2118 in comments somewhere, so others can find this driver with a simple grep? > error = devm_device_add_group(dev, &ili210x_attr_group); > if (error) { > dev_err(dev, "Unable to create sysfs attributes, err: %d\n", > error); > return error; > } The ili2117/ili2118 does not have a calibrate register, so this sysfs group is unsupported and perhaps may even be harmful if touched (?). Perhaps add a flag to struct ili2xxx_chip ? > input_set_abs_params(input, ABS_MT_POSITION_X, 0, 0xffff, 0, 0); > input_set_abs_params(input, ABS_MT_POSITION_Y, 0, 0xffff, 0, 0); The max position on ili2117/8 is 0xfff. The OS I'm using (Android) likes to know the correct min and max. So it can map touch coords to pixel coords. Perhaps add this to struct ili2xxx_chip ? > /* Get firmware version */ > error = chip->read_reg(client, REG_FIRMWARE_VERSION, > &firmware, sizeof(firmware)); On ili2117/ili2118, the firmware version register is different (0x03), and the layout is different too: byte name 0 vendor id 1 reserved 2 firmware version upper 3 firmware version lower 4 reserved 5 reserved 6 reserved 7 reserved But, does it even make sense to retrieve the firmware version? All it's used for is a dev_dbg log print, which under normal circumstances is a noop: > dev_dbg(dev, > "ILI210x initialized (IRQ: %d), firmware version %d.%d.%d", > client->irq, firmware.id, firmware.major, firmware.minor);
On Tue, Nov 5, 2019 at 10:29 AM Sven Van Asbroeck <thesven73@gmail.com> wrote: > > The max position on ili2117/8 is 0xfff. Sorry, it's actually 2047.
On Tue, Nov 05, 2019 at 10:29:53AM -0500, Sven Van Asbroeck wrote: > On Mon, Nov 4, 2019 at 6:36 PM Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > > > OK, I refreshed the branch with fixes and a couple of new patches. It is > > on top of 5.3 now. If this works for you guys I will be merging it for > > 5.5. > > > > According to the ili2117a/2118a datasheet I have, there are still a > few loose ends. > Some of these might be too inconsequential to worry about. > Dmitry, tell me which ones you think are important, if any, > and I will spin a patch if you like. Or you can do it, just let me know. > > > { "ili210x", (long)&ili210x_chip }, > > { "ili2117", (long)&ili211x_chip }, > > { "ili251x", (long)&ili251x_chip }, > > > > { .compatible = "ilitek,ili210x", .data = &ili210x_chip }, > > { .compatible = "ilitek,ili2117", .data = &ili211x_chip }, > > { .compatible = "ilitek,ili251x", .data = &ili251x_chip }, > > My datasheet says ILI2117A/ILI2118A, so maybe the compatible string should > really be "ilitek,ili211x", just like the other variants ? We have not landed the DT for 2117, so we can either rename it as "ilitek,ili211x" or have 2 separate compatibles. Rob, do you have preference? > > In addition, should we add ili2117/ili2118 in comments somewhere, so others > can find this driver with a simple grep? > > > error = devm_device_add_group(dev, &ili210x_attr_group); > > if (error) { > > dev_err(dev, "Unable to create sysfs attributes, err: %d\n", > > error); > > return error; > > } > > The ili2117/ili2118 does not have a calibrate register, so this sysfs group > is unsupported and perhaps may even be harmful if touched (?). > > Perhaps add a flag to struct ili2xxx_chip ? I guess we need is_visible() implementation for the attributes here and yes, a flag to the chip structure. > > > input_set_abs_params(input, ABS_MT_POSITION_X, 0, 0xffff, 0, 0); > > input_set_abs_params(input, ABS_MT_POSITION_Y, 0, 0xffff, 0, 0); > > The max position on ili2117/8 is 0xfff. The OS I'm using (Android) likes to know > the correct min and max. So it can map touch coords to pixel coords. What about the others? I doubt any of them actually support 64K resolution and I expect everyone simply used device tree to specify correct size. Marek, you worked with other versions of this controller, what is your experience? > > Perhaps add this to struct ili2xxx_chip ? > > > /* Get firmware version */ > > error = chip->read_reg(client, REG_FIRMWARE_VERSION, > > &firmware, sizeof(firmware)); > > On ili2117/ili2118, the firmware version register is different (0x03), and > the layout is different too: > > byte name > 0 vendor id > 1 reserved > 2 firmware version upper > 3 firmware version lower > 4 reserved > 5 reserved > 6 reserved > 7 reserved > > But, does it even make sense to retrieve the firmware version? All it's used > for is a dev_dbg log print, which under normal circumstances is a noop: > > > dev_dbg(dev, > > "ILI210x initialized (IRQ: %d), firmware version %d.%d.%d", > > client->irq, firmware.id, firmware.major, firmware.minor); I'd be OK with simply dropping this. Thanks.
On Mon, Nov 11, 2019 at 12:17 PM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > On Tue, Nov 05, 2019 at 10:29:53AM -0500, Sven Van Asbroeck wrote: > > On Mon, Nov 4, 2019 at 6:36 PM Dmitry Torokhov > > <dmitry.torokhov@gmail.com> wrote: > > > > > > OK, I refreshed the branch with fixes and a couple of new patches. It is > > > on top of 5.3 now. If this works for you guys I will be merging it for > > > 5.5. > > > > > > > According to the ili2117a/2118a datasheet I have, there are still a > > few loose ends. > > Some of these might be too inconsequential to worry about. > > Dmitry, tell me which ones you think are important, if any, > > and I will spin a patch if you like. Or you can do it, just let me know. > > > > > { "ili210x", (long)&ili210x_chip }, > > > { "ili2117", (long)&ili211x_chip }, > > > { "ili251x", (long)&ili251x_chip }, > > > > > > { .compatible = "ilitek,ili210x", .data = &ili210x_chip }, > > > { .compatible = "ilitek,ili2117", .data = &ili211x_chip }, > > > { .compatible = "ilitek,ili251x", .data = &ili251x_chip }, > > > > My datasheet says ILI2117A/ILI2118A, so maybe the compatible string should > > really be "ilitek,ili211x", just like the other variants ? > > We have not landed the DT for 2117, so we can either rename it as > "ilitek,ili211x" or have 2 separate compatibles. > > Rob, do you have preference? The rule is we don't do wildcards for compatible strings. However, if there's not a visible difference to s/w or you can determine which is which by ID registers, then it is fine to have a single compatible. I couldn't find a datasheet, so can't give better answer. Rob
On Mon, Nov 11, 2019 at 12:43:19PM -0600, Rob Herring wrote: > On Mon, Nov 11, 2019 at 12:17 PM Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > > > On Tue, Nov 05, 2019 at 10:29:53AM -0500, Sven Van Asbroeck wrote: > > > On Mon, Nov 4, 2019 at 6:36 PM Dmitry Torokhov > > > <dmitry.torokhov@gmail.com> wrote: > > > > > > > > OK, I refreshed the branch with fixes and a couple of new patches. It is > > > > on top of 5.3 now. If this works for you guys I will be merging it for > > > > 5.5. > > > > > > > > > > According to the ili2117a/2118a datasheet I have, there are still a > > > few loose ends. > > > Some of these might be too inconsequential to worry about. > > > Dmitry, tell me which ones you think are important, if any, > > > and I will spin a patch if you like. Or you can do it, just let me know. > > > > > > > { "ili210x", (long)&ili210x_chip }, > > > > { "ili2117", (long)&ili211x_chip }, > > > > { "ili251x", (long)&ili251x_chip }, > > > > > > > > { .compatible = "ilitek,ili210x", .data = &ili210x_chip }, > > > > { .compatible = "ilitek,ili2117", .data = &ili211x_chip }, > > > > { .compatible = "ilitek,ili251x", .data = &ili251x_chip }, > > > > > > My datasheet says ILI2117A/ILI2118A, so maybe the compatible string should > > > really be "ilitek,ili211x", just like the other variants ? > > > > We have not landed the DT for 2117, so we can either rename it as > > "ilitek,ili211x" or have 2 separate compatibles. > > > > Rob, do you have preference? > > The rule is we don't do wildcards for compatible strings. However, if > there's not a visible difference to s/w or you can determine which is > which by ID registers, then it is fine to have a single compatible. I > couldn't find a datasheet, so can't give better answer. OK, so I merged the branch keeping ilitek,ili2117 compatible. Sven, please feel free to send the patches addressing issues you discovered and I will be glad to apply them. Thanks.
diff --git a/Documentation/devicetree/bindings/input/ilitek,ili2xxx.txt b/Documentation/devicetree/bindings/input/ilitek,ili2xxx.txt index b2a76301e632..dc194b2c151a 100644 --- a/Documentation/devicetree/bindings/input/ilitek,ili2xxx.txt +++ b/Documentation/devicetree/bindings/input/ilitek,ili2xxx.txt @@ -1,8 +1,9 @@ -Ilitek ILI210x/ILI251x touchscreen controller +Ilitek ILI210x/ILI2117/ILI251x touchscreen controller Required properties: - compatible: ilitek,ili210x for ILI210x + ilitek,ili2117 for ILI2117 ilitek,ili251x for ILI251x - reg: The I2C address of the device
Add DT binding for the Ilitek ILI2117 touch controller, which is yet again a slightly different device in the ILI21xx family. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> Cc: Henrik Rydberg <rydberg@bitmath.org> Cc: Olivier Sobrie <olivier@sobrie.be> Cc: Philipp Puschmann <pp@emlix.com> Cc: Rob Herring <robh+dt@kernel.org> Cc: devicetree@vger.kernel.org To: linux-input@vger.kernel.org --- Documentation/devicetree/bindings/input/ilitek,ili2xxx.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)