[V2,10/10] input: touchscreen: ili210x: Add ILI251X support
diff mbox series

Message ID 20181220204305.28807-11-marex@denx.de
State Superseded
Headers show
Series
  • input: touchscreen: ili210x: Add ILI2511 support
Related show

Commit Message

Marek Vasut Dec. 20, 2018, 8:43 p.m. UTC
Add support for ILI251x touch controller. This controller is similar
to the ILI210x, except for the following differences:
- Does not support I2C R-W transfer, Read must be followed by an
  obscenely long delay, and then followed by Write
- Does support 10 simultaneous touch inputs.
- Touch data format is slightly different, pressure reporting does not
  work although the touch data contain such information.

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>
To: linux-input@vger.kernel.org
---
V2: - Implement delayed work for ILI251x
    - Fix operation with >6 fingers in ili210x_work
---
 drivers/input/touchscreen/ili210x.c | 128 ++++++++++++++++++++++++----
 1 file changed, 113 insertions(+), 15 deletions(-)

Comments

Dmitry Torokhov Dec. 21, 2018, 1:55 a.m. UTC | #1
On Thu, Dec 20, 2018 at 09:43:05PM +0100, Marek Vasut wrote:
> Add support for ILI251x touch controller. This controller is similar
> to the ILI210x, except for the following differences:
> - Does not support I2C R-W transfer, Read must be followed by an
>   obscenely long delay, and then followed by Write
> - Does support 10 simultaneous touch inputs.
> - Touch data format is slightly different, pressure reporting does not
>   work although the touch data contain such information.
> 
> 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>
> To: linux-input@vger.kernel.org
> ---
> V2: - Implement delayed work for ILI251x
>     - Fix operation with >6 fingers in ili210x_work
> ---
>  drivers/input/touchscreen/ili210x.c | 128 ++++++++++++++++++++++++----
>  1 file changed, 113 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
> index bb77d37aaaba..5099653dfb88 100644
> --- a/drivers/input/touchscreen/ili210x.c
> +++ b/drivers/input/touchscreen/ili210x.c
> @@ -6,8 +6,10 @@
>  #include <linux/input/mt.h>
>  #include <linux/delay.h>
>  #include <linux/gpio/consumer.h>
> +#include <linux/of_device.h>
>  
> -#define MAX_TOUCHES		2
> +#define ILI210X_TOUCHES		2
> +#define ILI251X_TOUCHES		10
>  #define DEFAULT_POLL_PERIOD	20
>  
>  /* Touchscreen commands */
> @@ -31,17 +33,25 @@ struct firmware_version {
>  	u8 minor;
>  } __packed;
>  
> +enum ili2xxx_model {
> +	MODEL_ILI210X,
> +	MODEL_ILI251X,
> +};
> +
>  struct ili210x {
>  	struct i2c_client *client;
>  	struct input_dev *input;
>  	unsigned int poll_period;
>  	struct delayed_work dwork;
>  	struct gpio_desc *reset_gpio;
> +	enum ili2xxx_model model;
> +	unsigned int max_touches;
>  };
>  
>  static int ili210x_read_reg(struct i2c_client *client, u8 reg, void *buf,
>  			    size_t len)
>  {
> +	struct ili210x *priv = i2c_get_clientdata(client);
>  	struct i2c_msg msg[2] = {
>  		{
>  			.addr	= client->addr,
> @@ -57,7 +67,38 @@ static int ili210x_read_reg(struct i2c_client *client, u8 reg, void *buf,
>  		}
>  	};
>  
> -	if (i2c_transfer(client->adapter, msg, 2) != 2) {
> +	if (priv->model == MODEL_ILI251X) {

Instead of doing conditional maybe define "ops" structure and tie it to
i2c and of table entries and call via pointers here and in coordinate
processing?

Thanks.
Marek Vasut Dec. 21, 2018, 2:27 a.m. UTC | #2
On 12/21/2018 02:55 AM, Dmitry Torokhov wrote:
> On Thu, Dec 20, 2018 at 09:43:05PM +0100, Marek Vasut wrote:
>> Add support for ILI251x touch controller. This controller is similar
>> to the ILI210x, except for the following differences:
>> - Does not support I2C R-W transfer, Read must be followed by an
>>   obscenely long delay, and then followed by Write
>> - Does support 10 simultaneous touch inputs.
>> - Touch data format is slightly different, pressure reporting does not
>>   work although the touch data contain such information.
>>
>> 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>
>> To: linux-input@vger.kernel.org
>> ---
>> V2: - Implement delayed work for ILI251x
>>     - Fix operation with >6 fingers in ili210x_work
>> ---
>>  drivers/input/touchscreen/ili210x.c | 128 ++++++++++++++++++++++++----
>>  1 file changed, 113 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
>> index bb77d37aaaba..5099653dfb88 100644
>> --- a/drivers/input/touchscreen/ili210x.c
>> +++ b/drivers/input/touchscreen/ili210x.c
>> @@ -6,8 +6,10 @@
>>  #include <linux/input/mt.h>
>>  #include <linux/delay.h>
>>  #include <linux/gpio/consumer.h>
>> +#include <linux/of_device.h>
>>  
>> -#define MAX_TOUCHES		2
>> +#define ILI210X_TOUCHES		2
>> +#define ILI251X_TOUCHES		10
>>  #define DEFAULT_POLL_PERIOD	20
>>  
>>  /* Touchscreen commands */
>> @@ -31,17 +33,25 @@ struct firmware_version {
>>  	u8 minor;
>>  } __packed;
>>  
>> +enum ili2xxx_model {
>> +	MODEL_ILI210X,
>> +	MODEL_ILI251X,
>> +};
>> +
>>  struct ili210x {
>>  	struct i2c_client *client;
>>  	struct input_dev *input;
>>  	unsigned int poll_period;
>>  	struct delayed_work dwork;
>>  	struct gpio_desc *reset_gpio;
>> +	enum ili2xxx_model model;
>> +	unsigned int max_touches;
>>  };
>>  
>>  static int ili210x_read_reg(struct i2c_client *client, u8 reg, void *buf,
>>  			    size_t len)
>>  {
>> +	struct ili210x *priv = i2c_get_clientdata(client);
>>  	struct i2c_msg msg[2] = {
>>  		{
>>  			.addr	= client->addr,
>> @@ -57,7 +67,38 @@ static int ili210x_read_reg(struct i2c_client *client, u8 reg, void *buf,
>>  		}
>>  	};
>>  
>> -	if (i2c_transfer(client->adapter, msg, 2) != 2) {
>> +	if (priv->model == MODEL_ILI251X) {
> 
> Instead of doing conditional maybe define "ops" structure and tie it to
> i2c and of table entries and call via pointers here and in coordinate
> processing?

Only for the read function or others as well ?
Dmitry Torokhov Dec. 21, 2018, 8:22 a.m. UTC | #3
On Fri, Dec 21, 2018 at 03:27:08AM +0100, Marek Vasut wrote:
> On 12/21/2018 02:55 AM, Dmitry Torokhov wrote:
> > On Thu, Dec 20, 2018 at 09:43:05PM +0100, Marek Vasut wrote:
> >> Add support for ILI251x touch controller. This controller is similar
> >> to the ILI210x, except for the following differences:
> >> - Does not support I2C R-W transfer, Read must be followed by an
> >>   obscenely long delay, and then followed by Write
> >> - Does support 10 simultaneous touch inputs.
> >> - Touch data format is slightly different, pressure reporting does not
> >>   work although the touch data contain such information.
> >>
> >> 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>
> >> To: linux-input@vger.kernel.org
> >> ---
> >> V2: - Implement delayed work for ILI251x
> >>     - Fix operation with >6 fingers in ili210x_work
> >> ---
> >>  drivers/input/touchscreen/ili210x.c | 128 ++++++++++++++++++++++++----
> >>  1 file changed, 113 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
> >> index bb77d37aaaba..5099653dfb88 100644
> >> --- a/drivers/input/touchscreen/ili210x.c
> >> +++ b/drivers/input/touchscreen/ili210x.c
> >> @@ -6,8 +6,10 @@
> >>  #include <linux/input/mt.h>
> >>  #include <linux/delay.h>
> >>  #include <linux/gpio/consumer.h>
> >> +#include <linux/of_device.h>
> >>  
> >> -#define MAX_TOUCHES		2
> >> +#define ILI210X_TOUCHES		2
> >> +#define ILI251X_TOUCHES		10
> >>  #define DEFAULT_POLL_PERIOD	20
> >>  
> >>  /* Touchscreen commands */
> >> @@ -31,17 +33,25 @@ struct firmware_version {
> >>  	u8 minor;
> >>  } __packed;
> >>  
> >> +enum ili2xxx_model {
> >> +	MODEL_ILI210X,
> >> +	MODEL_ILI251X,
> >> +};
> >> +
> >>  struct ili210x {
> >>  	struct i2c_client *client;
> >>  	struct input_dev *input;
> >>  	unsigned int poll_period;
> >>  	struct delayed_work dwork;
> >>  	struct gpio_desc *reset_gpio;
> >> +	enum ili2xxx_model model;
> >> +	unsigned int max_touches;
> >>  };
> >>  
> >>  static int ili210x_read_reg(struct i2c_client *client, u8 reg, void *buf,
> >>  			    size_t len)
> >>  {
> >> +	struct ili210x *priv = i2c_get_clientdata(client);
> >>  	struct i2c_msg msg[2] = {
> >>  		{
> >>  			.addr	= client->addr,
> >> @@ -57,7 +67,38 @@ static int ili210x_read_reg(struct i2c_client *client, u8 reg, void *buf,
> >>  		}
> >>  	};
> >>  
> >> -	if (i2c_transfer(client->adapter, msg, 2) != 2) {
> >> +	if (priv->model == MODEL_ILI251X) {
> > 
> > Instead of doing conditional maybe define "ops" structure and tie it to
> > i2c and of table entries and call via pointers here and in coordinate
> > processing?
> 
> Only for the read function or others as well ?

I think all where you currently branch depending on the model.

Thanks.
Marek Vasut Dec. 21, 2018, 8:47 p.m. UTC | #4
On 12/21/2018 09:22 AM, Dmitry Torokhov wrote:
> On Fri, Dec 21, 2018 at 03:27:08AM +0100, Marek Vasut wrote:
>> On 12/21/2018 02:55 AM, Dmitry Torokhov wrote:
>>> On Thu, Dec 20, 2018 at 09:43:05PM +0100, Marek Vasut wrote:
>>>> Add support for ILI251x touch controller. This controller is similar
>>>> to the ILI210x, except for the following differences:
>>>> - Does not support I2C R-W transfer, Read must be followed by an
>>>>   obscenely long delay, and then followed by Write
>>>> - Does support 10 simultaneous touch inputs.
>>>> - Touch data format is slightly different, pressure reporting does not
>>>>   work although the touch data contain such information.
>>>>
>>>> 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>
>>>> To: linux-input@vger.kernel.org
>>>> ---
>>>> V2: - Implement delayed work for ILI251x
>>>>     - Fix operation with >6 fingers in ili210x_work
>>>> ---
>>>>  drivers/input/touchscreen/ili210x.c | 128 ++++++++++++++++++++++++----
>>>>  1 file changed, 113 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
>>>> index bb77d37aaaba..5099653dfb88 100644
>>>> --- a/drivers/input/touchscreen/ili210x.c
>>>> +++ b/drivers/input/touchscreen/ili210x.c
>>>> @@ -6,8 +6,10 @@
>>>>  #include <linux/input/mt.h>
>>>>  #include <linux/delay.h>
>>>>  #include <linux/gpio/consumer.h>
>>>> +#include <linux/of_device.h>
>>>>  
>>>> -#define MAX_TOUCHES		2
>>>> +#define ILI210X_TOUCHES		2
>>>> +#define ILI251X_TOUCHES		10
>>>>  #define DEFAULT_POLL_PERIOD	20
>>>>  
>>>>  /* Touchscreen commands */
>>>> @@ -31,17 +33,25 @@ struct firmware_version {
>>>>  	u8 minor;
>>>>  } __packed;
>>>>  
>>>> +enum ili2xxx_model {
>>>> +	MODEL_ILI210X,
>>>> +	MODEL_ILI251X,
>>>> +};
>>>> +
>>>>  struct ili210x {
>>>>  	struct i2c_client *client;
>>>>  	struct input_dev *input;
>>>>  	unsigned int poll_period;
>>>>  	struct delayed_work dwork;
>>>>  	struct gpio_desc *reset_gpio;
>>>> +	enum ili2xxx_model model;
>>>> +	unsigned int max_touches;
>>>>  };
>>>>  
>>>>  static int ili210x_read_reg(struct i2c_client *client, u8 reg, void *buf,
>>>>  			    size_t len)
>>>>  {
>>>> +	struct ili210x *priv = i2c_get_clientdata(client);
>>>>  	struct i2c_msg msg[2] = {
>>>>  		{
>>>>  			.addr	= client->addr,
>>>> @@ -57,7 +67,38 @@ static int ili210x_read_reg(struct i2c_client *client, u8 reg, void *buf,
>>>>  		}
>>>>  	};
>>>>  
>>>> -	if (i2c_transfer(client->adapter, msg, 2) != 2) {
>>>> +	if (priv->model == MODEL_ILI251X) {
>>>
>>> Instead of doing conditional maybe define "ops" structure and tie it to
>>> i2c and of table entries and call via pointers here and in coordinate
>>> processing?
>>
>> Only for the read function or others as well ?
> 
> I think all where you currently branch depending on the model.

I had a discussion with netdev people and they mentioned these indirect
function calls now have a lot of overhead due to spectre/meltdown
mitigations. Do we care here or shall I just use those indirect calls ?
Dmitry Torokhov Dec. 22, 2018, 1:06 a.m. UTC | #5
On Fri, Dec 21, 2018 at 09:47:34PM +0100, Marek Vasut wrote:
> On 12/21/2018 09:22 AM, Dmitry Torokhov wrote:
> > On Fri, Dec 21, 2018 at 03:27:08AM +0100, Marek Vasut wrote:
> >> On 12/21/2018 02:55 AM, Dmitry Torokhov wrote:
> >>> On Thu, Dec 20, 2018 at 09:43:05PM +0100, Marek Vasut wrote:
> >>>> Add support for ILI251x touch controller. This controller is similar
> >>>> to the ILI210x, except for the following differences:
> >>>> - Does not support I2C R-W transfer, Read must be followed by an
> >>>>   obscenely long delay, and then followed by Write
> >>>> - Does support 10 simultaneous touch inputs.
> >>>> - Touch data format is slightly different, pressure reporting does not
> >>>>   work although the touch data contain such information.
> >>>>
> >>>> 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>
> >>>> To: linux-input@vger.kernel.org
> >>>> ---
> >>>> V2: - Implement delayed work for ILI251x
> >>>>     - Fix operation with >6 fingers in ili210x_work
> >>>> ---
> >>>>  drivers/input/touchscreen/ili210x.c | 128 ++++++++++++++++++++++++----
> >>>>  1 file changed, 113 insertions(+), 15 deletions(-)
> >>>>
> >>>> diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
> >>>> index bb77d37aaaba..5099653dfb88 100644
> >>>> --- a/drivers/input/touchscreen/ili210x.c
> >>>> +++ b/drivers/input/touchscreen/ili210x.c
> >>>> @@ -6,8 +6,10 @@
> >>>>  #include <linux/input/mt.h>
> >>>>  #include <linux/delay.h>
> >>>>  #include <linux/gpio/consumer.h>
> >>>> +#include <linux/of_device.h>
> >>>>  
> >>>> -#define MAX_TOUCHES		2
> >>>> +#define ILI210X_TOUCHES		2
> >>>> +#define ILI251X_TOUCHES		10
> >>>>  #define DEFAULT_POLL_PERIOD	20
> >>>>  
> >>>>  /* Touchscreen commands */
> >>>> @@ -31,17 +33,25 @@ struct firmware_version {
> >>>>  	u8 minor;
> >>>>  } __packed;
> >>>>  
> >>>> +enum ili2xxx_model {
> >>>> +	MODEL_ILI210X,
> >>>> +	MODEL_ILI251X,
> >>>> +};
> >>>> +
> >>>>  struct ili210x {
> >>>>  	struct i2c_client *client;
> >>>>  	struct input_dev *input;
> >>>>  	unsigned int poll_period;
> >>>>  	struct delayed_work dwork;
> >>>>  	struct gpio_desc *reset_gpio;
> >>>> +	enum ili2xxx_model model;
> >>>> +	unsigned int max_touches;
> >>>>  };
> >>>>  
> >>>>  static int ili210x_read_reg(struct i2c_client *client, u8 reg, void *buf,
> >>>>  			    size_t len)
> >>>>  {
> >>>> +	struct ili210x *priv = i2c_get_clientdata(client);
> >>>>  	struct i2c_msg msg[2] = {
> >>>>  		{
> >>>>  			.addr	= client->addr,
> >>>> @@ -57,7 +67,38 @@ static int ili210x_read_reg(struct i2c_client *client, u8 reg, void *buf,
> >>>>  		}
> >>>>  	};
> >>>>  
> >>>> -	if (i2c_transfer(client->adapter, msg, 2) != 2) {
> >>>> +	if (priv->model == MODEL_ILI251X) {
> >>>
> >>> Instead of doing conditional maybe define "ops" structure and tie it to
> >>> i2c and of table entries and call via pointers here and in coordinate
> >>> processing?
> >>
> >> Only for the read function or others as well ?
> > 
> > I think all where you currently branch depending on the model.
> 
> I had a discussion with netdev people and they mentioned these indirect
> function calls now have a lot of overhead due to spectre/meltdown
> mitigations. Do we care here or shall I just use those indirect calls ?

This really depends on the CPU and I am sure we will not have to live
with sucky CPUs for the rest of our lives. C++ crowd will want their
performance back. We should produce the most readable code and then rely
on the crazy architecture/compiler folks to fix up our mess as needed ;)

So indirect calls via a const ops structure please.

Thanks.
Marek Vasut Dec. 22, 2018, 2:03 a.m. UTC | #6
On 12/22/2018 02:06 AM, Dmitry Torokhov wrote:
> On Fri, Dec 21, 2018 at 09:47:34PM +0100, Marek Vasut wrote:
>> On 12/21/2018 09:22 AM, Dmitry Torokhov wrote:
>>> On Fri, Dec 21, 2018 at 03:27:08AM +0100, Marek Vasut wrote:
>>>> On 12/21/2018 02:55 AM, Dmitry Torokhov wrote:
>>>>> On Thu, Dec 20, 2018 at 09:43:05PM +0100, Marek Vasut wrote:
>>>>>> Add support for ILI251x touch controller. This controller is similar
>>>>>> to the ILI210x, except for the following differences:
>>>>>> - Does not support I2C R-W transfer, Read must be followed by an
>>>>>>   obscenely long delay, and then followed by Write
>>>>>> - Does support 10 simultaneous touch inputs.
>>>>>> - Touch data format is slightly different, pressure reporting does not
>>>>>>   work although the touch data contain such information.
>>>>>>
>>>>>> 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>
>>>>>> To: linux-input@vger.kernel.org
>>>>>> ---
>>>>>> V2: - Implement delayed work for ILI251x
>>>>>>     - Fix operation with >6 fingers in ili210x_work
>>>>>> ---
>>>>>>  drivers/input/touchscreen/ili210x.c | 128 ++++++++++++++++++++++++----
>>>>>>  1 file changed, 113 insertions(+), 15 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
>>>>>> index bb77d37aaaba..5099653dfb88 100644
>>>>>> --- a/drivers/input/touchscreen/ili210x.c
>>>>>> +++ b/drivers/input/touchscreen/ili210x.c
>>>>>> @@ -6,8 +6,10 @@
>>>>>>  #include <linux/input/mt.h>
>>>>>>  #include <linux/delay.h>
>>>>>>  #include <linux/gpio/consumer.h>
>>>>>> +#include <linux/of_device.h>
>>>>>>  
>>>>>> -#define MAX_TOUCHES		2
>>>>>> +#define ILI210X_TOUCHES		2
>>>>>> +#define ILI251X_TOUCHES		10
>>>>>>  #define DEFAULT_POLL_PERIOD	20
>>>>>>  
>>>>>>  /* Touchscreen commands */
>>>>>> @@ -31,17 +33,25 @@ struct firmware_version {
>>>>>>  	u8 minor;
>>>>>>  } __packed;
>>>>>>  
>>>>>> +enum ili2xxx_model {
>>>>>> +	MODEL_ILI210X,
>>>>>> +	MODEL_ILI251X,
>>>>>> +};
>>>>>> +
>>>>>>  struct ili210x {
>>>>>>  	struct i2c_client *client;
>>>>>>  	struct input_dev *input;
>>>>>>  	unsigned int poll_period;
>>>>>>  	struct delayed_work dwork;
>>>>>>  	struct gpio_desc *reset_gpio;
>>>>>> +	enum ili2xxx_model model;
>>>>>> +	unsigned int max_touches;
>>>>>>  };
>>>>>>  
>>>>>>  static int ili210x_read_reg(struct i2c_client *client, u8 reg, void *buf,
>>>>>>  			    size_t len)
>>>>>>  {
>>>>>> +	struct ili210x *priv = i2c_get_clientdata(client);
>>>>>>  	struct i2c_msg msg[2] = {
>>>>>>  		{
>>>>>>  			.addr	= client->addr,
>>>>>> @@ -57,7 +67,38 @@ static int ili210x_read_reg(struct i2c_client *client, u8 reg, void *buf,
>>>>>>  		}
>>>>>>  	};
>>>>>>  
>>>>>> -	if (i2c_transfer(client->adapter, msg, 2) != 2) {
>>>>>> +	if (priv->model == MODEL_ILI251X) {
>>>>>
>>>>> Instead of doing conditional maybe define "ops" structure and tie it to
>>>>> i2c and of table entries and call via pointers here and in coordinate
>>>>> processing?
>>>>
>>>> Only for the read function or others as well ?
>>>
>>> I think all where you currently branch depending on the model.
>>
>> I had a discussion with netdev people and they mentioned these indirect
>> function calls now have a lot of overhead due to spectre/meltdown
>> mitigations. Do we care here or shall I just use those indirect calls ?
> 
> This really depends on the CPU and I am sure we will not have to live
> with sucky CPUs for the rest of our lives. C++ crowd will want their
> performance back. We should produce the most readable code and then rely
> on the crazy architecture/compiler folks to fix up our mess as needed ;)
> 
> So indirect calls via a const ops structure please.

I'll have to live with that on the CortexA9 system I use that
touchscreen at though. And this is a register IO, so it's a critical
path IMO. I am a bit opposed to this ...
Marek Vasut Jan. 3, 2019, 1:24 a.m. UTC | #7
On 12/22/18 3:03 AM, Marek Vasut wrote:
> On 12/22/2018 02:06 AM, Dmitry Torokhov wrote:
>> On Fri, Dec 21, 2018 at 09:47:34PM +0100, Marek Vasut wrote:
>>> On 12/21/2018 09:22 AM, Dmitry Torokhov wrote:
>>>> On Fri, Dec 21, 2018 at 03:27:08AM +0100, Marek Vasut wrote:
>>>>> On 12/21/2018 02:55 AM, Dmitry Torokhov wrote:
>>>>>> On Thu, Dec 20, 2018 at 09:43:05PM +0100, Marek Vasut wrote:
>>>>>>> Add support for ILI251x touch controller. This controller is similar
>>>>>>> to the ILI210x, except for the following differences:
>>>>>>> - Does not support I2C R-W transfer, Read must be followed by an
>>>>>>>   obscenely long delay, and then followed by Write
>>>>>>> - Does support 10 simultaneous touch inputs.
>>>>>>> - Touch data format is slightly different, pressure reporting does not
>>>>>>>   work although the touch data contain such information.
>>>>>>>
>>>>>>> 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>
>>>>>>> To: linux-input@vger.kernel.org
>>>>>>> ---
>>>>>>> V2: - Implement delayed work for ILI251x
>>>>>>>     - Fix operation with >6 fingers in ili210x_work
>>>>>>> ---
>>>>>>>  drivers/input/touchscreen/ili210x.c | 128 ++++++++++++++++++++++++----
>>>>>>>  1 file changed, 113 insertions(+), 15 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
>>>>>>> index bb77d37aaaba..5099653dfb88 100644
>>>>>>> --- a/drivers/input/touchscreen/ili210x.c
>>>>>>> +++ b/drivers/input/touchscreen/ili210x.c
>>>>>>> @@ -6,8 +6,10 @@
>>>>>>>  #include <linux/input/mt.h>
>>>>>>>  #include <linux/delay.h>
>>>>>>>  #include <linux/gpio/consumer.h>
>>>>>>> +#include <linux/of_device.h>
>>>>>>>  
>>>>>>> -#define MAX_TOUCHES		2
>>>>>>> +#define ILI210X_TOUCHES		2
>>>>>>> +#define ILI251X_TOUCHES		10
>>>>>>>  #define DEFAULT_POLL_PERIOD	20
>>>>>>>  
>>>>>>>  /* Touchscreen commands */
>>>>>>> @@ -31,17 +33,25 @@ struct firmware_version {
>>>>>>>  	u8 minor;
>>>>>>>  } __packed;
>>>>>>>  
>>>>>>> +enum ili2xxx_model {
>>>>>>> +	MODEL_ILI210X,
>>>>>>> +	MODEL_ILI251X,
>>>>>>> +};
>>>>>>> +
>>>>>>>  struct ili210x {
>>>>>>>  	struct i2c_client *client;
>>>>>>>  	struct input_dev *input;
>>>>>>>  	unsigned int poll_period;
>>>>>>>  	struct delayed_work dwork;
>>>>>>>  	struct gpio_desc *reset_gpio;
>>>>>>> +	enum ili2xxx_model model;
>>>>>>> +	unsigned int max_touches;
>>>>>>>  };
>>>>>>>  
>>>>>>>  static int ili210x_read_reg(struct i2c_client *client, u8 reg, void *buf,
>>>>>>>  			    size_t len)
>>>>>>>  {
>>>>>>> +	struct ili210x *priv = i2c_get_clientdata(client);
>>>>>>>  	struct i2c_msg msg[2] = {
>>>>>>>  		{
>>>>>>>  			.addr	= client->addr,
>>>>>>> @@ -57,7 +67,38 @@ static int ili210x_read_reg(struct i2c_client *client, u8 reg, void *buf,
>>>>>>>  		}
>>>>>>>  	};
>>>>>>>  
>>>>>>> -	if (i2c_transfer(client->adapter, msg, 2) != 2) {
>>>>>>> +	if (priv->model == MODEL_ILI251X) {
>>>>>>
>>>>>> Instead of doing conditional maybe define "ops" structure and tie it to
>>>>>> i2c and of table entries and call via pointers here and in coordinate
>>>>>> processing?
>>>>>
>>>>> Only for the read function or others as well ?
>>>>
>>>> I think all where you currently branch depending on the model.
>>>
>>> I had a discussion with netdev people and they mentioned these indirect
>>> function calls now have a lot of overhead due to spectre/meltdown
>>> mitigations. Do we care here or shall I just use those indirect calls ?
>>
>> This really depends on the CPU and I am sure we will not have to live
>> with sucky CPUs for the rest of our lives. C++ crowd will want their
>> performance back. We should produce the most readable code and then rely
>> on the crazy architecture/compiler folks to fix up our mess as needed ;)
>>
>> So indirect calls via a const ops structure please.
> 
> I'll have to live with that on the CortexA9 system I use that
> touchscreen at though. And this is a register IO, so it's a critical
> path IMO. I am a bit opposed to this ...

I tried to add the callback, but the resulting code looks horrible, so
I'll skip this part.

Patch
diff mbox series

diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
index bb77d37aaaba..5099653dfb88 100644
--- a/drivers/input/touchscreen/ili210x.c
+++ b/drivers/input/touchscreen/ili210x.c
@@ -6,8 +6,10 @@ 
 #include <linux/input/mt.h>
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
+#include <linux/of_device.h>
 
-#define MAX_TOUCHES		2
+#define ILI210X_TOUCHES		2
+#define ILI251X_TOUCHES		10
 #define DEFAULT_POLL_PERIOD	20
 
 /* Touchscreen commands */
@@ -31,17 +33,25 @@  struct firmware_version {
 	u8 minor;
 } __packed;
 
+enum ili2xxx_model {
+	MODEL_ILI210X,
+	MODEL_ILI251X,
+};
+
 struct ili210x {
 	struct i2c_client *client;
 	struct input_dev *input;
 	unsigned int poll_period;
 	struct delayed_work dwork;
 	struct gpio_desc *reset_gpio;
+	enum ili2xxx_model model;
+	unsigned int max_touches;
 };
 
 static int ili210x_read_reg(struct i2c_client *client, u8 reg, void *buf,
 			    size_t len)
 {
+	struct ili210x *priv = i2c_get_clientdata(client);
 	struct i2c_msg msg[2] = {
 		{
 			.addr	= client->addr,
@@ -57,7 +67,38 @@  static int ili210x_read_reg(struct i2c_client *client, u8 reg, void *buf,
 		}
 	};
 
-	if (i2c_transfer(client->adapter, msg, 2) != 2) {
+	if (priv->model == MODEL_ILI251X) {
+		if (i2c_transfer(client->adapter, msg, 1) != 1) {
+			dev_err(&client->dev, "i2c transfer failed\n");
+			return -EIO;
+		}
+
+		mdelay(5);
+
+		if (i2c_transfer(client->adapter, msg + 1, 1) != 1) {
+			dev_err(&client->dev, "i2c transfer failed\n");
+			return -EIO;
+		}
+	} else {
+		if (i2c_transfer(client->adapter, msg, 2) != 2) {
+			dev_err(&client->dev, "i2c transfer failed\n");
+			return -EIO;
+		}
+	}
+
+	return 0;
+}
+
+static int ili210x_read(struct i2c_client *client, void *buf, size_t len)
+{
+	struct i2c_msg msg = {
+		.addr	= client->addr,
+		.flags	= I2C_M_RD,
+		.len	= len,
+		.buf	= buf,
+	};
+
+	if (i2c_transfer(client->adapter, &msg, 1) != 1) {
 		dev_err(&client->dev, "i2c transfer failed\n");
 		return -EIO;
 	}
@@ -71,7 +112,7 @@  static bool ili210x_touchdata_to_coords(struct ili210x *priv, u8 *touchdata,
 {
 	u8 x_low, x_high, y_low, y_high;
 
-	if (finger >= MAX_TOUCHES)
+	if (finger >= ILI210X_TOUCHES)
 		return false;
 
 	if (touchdata[0] & BIT(finger))
@@ -87,16 +128,50 @@  static bool ili210x_touchdata_to_coords(struct ili210x *priv, u8 *touchdata,
 	return true;
 }
 
+static bool ili251x_touchdata_to_coords(struct ili210x *priv, u8 *touchdata,
+					unsigned int finger,
+					unsigned int *x, unsigned int *y)
+{
+	u8 x_low, x_high, y_low, y_high;
+
+	if (finger >= ILI251X_TOUCHES)
+		return false;
+
+	x_high = touchdata[1 + (finger * 5) + 0];
+	if (!(x_high & BIT(7)))	/* Touch indication */
+		return false;
+
+	x_high &= 0x3f;
+	x_low = touchdata[1 + (finger * 5) + 1];
+	y_high = touchdata[1 + (finger * 5) + 2];
+	y_low = touchdata[1 + (finger * 5) + 3];
+
+	*x = x_low | (x_high << 8);
+	*y = y_low | (y_high << 8);
+
+	return true;
+}
+
 static bool ili210x_report_events(struct ili210x *priv, u8 *touchdata)
 {
+	struct input_dev *input = priv->input;
 	int i;
-	bool touch;
-	unsigned int x, y;
+	bool contact = false, touch = false;
+	unsigned int x = 0, y = 0;
 
-	for (i = 0; i < MAX_TOUCHES; i++) {
+	for (i = 0; i < priv->max_touches; i++) {
 		input_mt_slot(input, i);
 
-		touch = ili210x_touchdata_to_coords(priv, touchdata, i, &x, &y);
+		if (priv->model == MODEL_ILI210X) {
+			touch = ili210x_touchdata_to_coords(priv, touchdata,
+							    i, &x, &y);
+		} else if (priv->model == MODEL_ILI251X) {
+			touch = ili251x_touchdata_to_coords(priv, touchdata,
+							    i, &x, &y);
+			if (touch)
+				contact = true;
+		}
+
 		input_mt_report_slot_state(input, MT_TOOL_FINGER, touch);
 		if (touch) {
 			input_report_abs(input, ABS_MT_POSITION_X, x);
@@ -107,7 +182,10 @@  static bool ili210x_report_events(struct ili210x *priv, u8 *touchdata)
 	input_mt_report_pointer_emulation(input, false);
 	input_sync(input);
 
-	return touchdata[0] & 0xf3;
+	if (priv->model == MODEL_ILI210X)
+		contact = touchdata[0] & 0xf3;
+
+	return contact;
 }
 
 static void ili210x_work(struct work_struct *work)
@@ -115,12 +193,20 @@  static void ili210x_work(struct work_struct *work)
 	struct ili210x *priv = container_of(work, struct ili210x,
 					    dwork.work);
 	struct i2c_client *client = priv->client;
-	u8 touchdata[1 + 4 * MAX_TOUCHES];
+	u8 touchdata[64] = { 0 };
 	bool touch;
-	int error;
+	int error = -EINVAL;
+
+	if (priv->model == MODEL_ILI210X) {
+		error = ili210x_read_reg(client, REG_TOUCHDATA,
+					 touchdata, sizeof(touchdata));
+	} else if (priv->model == MODEL_ILI251X) {
+		error = ili210x_read_reg(client, REG_TOUCHDATA,
+					 touchdata, 31);
+		if (!error && touchdata[0] == 2)
+			error = ili210x_read(client, &touchdata[31], 20);
+	}
 
-	error = ili210x_read_reg(client, REG_TOUCHDATA,
-				 touchdata, sizeof(touchdata));
 	if (error) {
 		dev_err(&client->dev,
 			"Unable to get touchdata, err = %d\n", error);
@@ -178,6 +264,8 @@  static const struct attribute_group ili210x_attr_group = {
 	.attrs = ili210x_attributes,
 };
 
+static const struct i2c_device_id ili210x_i2c_id[];
+
 static int ili210x_i2c_probe(struct i2c_client *client,
 				       const struct i2c_device_id *id)
 {
@@ -187,9 +275,12 @@  static int ili210x_i2c_probe(struct i2c_client *client,
 	struct input_dev *input;
 	struct panel_info panel;
 	struct firmware_version firmware;
+	enum ili2xxx_model model;
 	int xmax, ymax;
 	int error;
 
+	model = (enum ili2xxx_model)id->driver_data;
+
 	dev_dbg(dev, "Probing for ILI210X I2C Touschreen driver");
 
 	if (client->irq <= 0) {
@@ -218,6 +309,11 @@  static int ili210x_i2c_probe(struct i2c_client *client,
 	priv->poll_period = DEFAULT_POLL_PERIOD;
 	INIT_DELAYED_WORK(&priv->dwork, ili210x_work);
 	priv->reset_gpio = reset_gpio;
+	priv->model = model;
+	if (model == MODEL_ILI210X)
+		priv->max_touches = ILI210X_TOUCHES;
+	if (model == MODEL_ILI251X)
+		priv->max_touches = ILI251X_TOUCHES;
 
 	i2c_set_clientdata(client, priv);
 
@@ -256,7 +352,7 @@  static int ili210x_i2c_probe(struct i2c_client *client,
 	input_set_abs_params(input, ABS_Y, 0, ymax, 0, 0);
 
 	/* Multi touch */
-	input_mt_init_slots(input, MAX_TOUCHES, 0);
+	input_mt_init_slots(input, priv->max_touches, 0);
 	input_set_abs_params(input, ABS_MT_POSITION_X, 0, xmax, 0, 0);
 	input_set_abs_params(input, ABS_MT_POSITION_Y, 0, ymax, 0, 0);
 
@@ -331,13 +427,15 @@  static SIMPLE_DEV_PM_OPS(ili210x_i2c_pm,
 			 ili210x_i2c_suspend, ili210x_i2c_resume);
 
 static const struct i2c_device_id ili210x_i2c_id[] = {
-	{ "ili210x", 0 },
+	{ "ili210x", MODEL_ILI210X },
+	{ "ili251x", MODEL_ILI251X },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, ili210x_i2c_id);
 
 static const struct of_device_id ili210x_dt_ids[] = {
-	{ .compatible = "ilitek,ili210x", },
+	{ .compatible = "ilitek,ili210x", .data = (void *)MODEL_ILI210X },
+	{ .compatible = "ilitek,ili251x", .data = (void *)MODEL_ILI251X },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, ili210x_dt_ids);