On Mon, Nov 4, 2019 at 12:37 PM Sven Van Asbroeck <thesven73@xxxxxxxxx> 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
