Re: [PATCH 2/2] Input: ili210x - add ILI2117 support

Re: [PATCH 2/2] Input: ili210x - add ILI2117 support

[Sven Van Asbroeck]

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

Re: [PATCH 2/2] Input: ili210x - add ILI2117 support

[Adam Ford]

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

Re: [PATCH 2/2] Input: ili210x - add ILI2117 support

[Marek Vasut]

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.

Re: [PATCH 2/2] Input: ili210x - add ILI2117 support

[Dmitry Torokhov]

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.

-- 
Dmitry

Re: [PATCH 2/2] Input: ili210x - add ILI2117 support

[Dmitry Torokhov]

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.

-- 
Dmitry

Re: [PATCH 2/2] Input: ili210x - add ILI2117 support

[Marek Vasut]

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 ?

Re: [PATCH 2/2] Input: ili210x - add ILI2117 support

[Sven Van Asbroeck]

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.

Re: [PATCH 2/2] Input: ili210x - add ILI2117 support

[Adam Ford]

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.

Re: [PATCH 2/2] Input: ili210x - add ILI2117 support

[Dmitry Torokhov]

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.

-- 
Dmitry

Re: [PATCH 2/2] Input: ili210x - add ILI2117 support

[Sven Van Asbroeck]

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);
-- 
2.17.1

Re: [PATCH 2/2] Input: ili210x - add ILI2117 support

[Adam Ford]

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

Re: [PATCH 2/2] Input: ili210x - add ILI2117 support

[Sven Van Asbroeck]

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?

Re: [PATCH 2/2] Input: ili210x - add ILI2117 support

[Adam Ford]

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

Re: [PATCH 2/2] Input: ili210x - add ILI2117 support

[Dmitry Torokhov]

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.

-- 
Dmitry

Re: [PATCH 2/2] Input: ili210x - add ILI2117 support

[Adam Ford]

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

Re: [PATCH 2/2] Input: ili210x - add ILI2117 support

[Sven Van Asbroeck]

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.

Re: [PATCH 2/2] Input: ili210x - add ILI2117 support

[Adam Ford]

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.

Re: [PATCH 2/2] Input: ili210x - add ILI2117 support

[Sven Van Asbroeck]

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

Re: [PATCH 2/2] Input: ili210x - add ILI2117 support

[Sven Van Asbroeck]

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);

Re: [PATCH 2/2] Input: ili210x - add ILI2117 support

[Sven Van Asbroeck]

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.

Re: [PATCH 2/2] Input: ili210x - add ILI2117 support

[Dmitry Torokhov]

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.

-- 
Dmitry

Re: [PATCH 2/2] Input: ili210x - add ILI2117 support

[Rob Herring]

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

Re: [PATCH 2/2] Input: ili210x - add ILI2117 support

[Dmitry Torokhov]

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.

-- 
Dmitry