| Message ID | 20181220204305.28807-11-marex@denx.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | input: touchscreen: ili210x: Add ILI2511 support | expand |
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.
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 ?
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.
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 ?
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.
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 ...
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.
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);
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(-)