| Message ID | 20190429152411.12835-1-daniel@zonque.org (mailing list archive) |
|---|---|
| State | Under Review |
| Headers | show |
| Series | [v4,1/2] input: touch: eeti: move ISR code to own function | expand |
Hi Dmitry, Is this one good to go in? WDYT? Thanks, Daniel On 29/4/2019 5:24 PM, Daniel Mack wrote: > Move the ISR handling code to its own function This allows us to call > it at resume time to read the hardware state. > > Signed-off-by: Daniel Mack <daniel@zonque.org> > --- > drivers/input/touchscreen/eeti_ts.c | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/drivers/input/touchscreen/eeti_ts.c b/drivers/input/touchscreen/eeti_ts.c > index 7fe41965c5d1..3e13cc12aaaf 100644 > --- a/drivers/input/touchscreen/eeti_ts.c > +++ b/drivers/input/touchscreen/eeti_ts.c > @@ -75,11 +75,9 @@ static void eeti_ts_report_event(struct eeti_ts *eeti, u8 *buf) > input_sync(eeti->input); > } > > -static irqreturn_t eeti_ts_isr(int irq, void *dev_id) > +static void eeti_ts_read(struct eeti_ts *eeti) > { > - struct eeti_ts *eeti = dev_id; > - int len; > - int error; > + int len, error; > char buf[6]; > > do { > @@ -92,12 +90,18 @@ static irqreturn_t eeti_ts_isr(int irq, void *dev_id) > break; > } > > - if (buf[0] & 0x80) { > - /* Motion packet */ > + /* Motion packet */ > + if (buf[0] & 0x80) > eeti_ts_report_event(eeti, buf); > - } > } while (eeti->running && > eeti->attn_gpio && gpiod_get_value_cansleep(eeti->attn_gpio)); > +} > + > +static irqreturn_t eeti_ts_isr(int irq, void *dev_id) > +{ > + struct eeti_ts *eeti = dev_id; > + > + eeti_ts_read(eeti); > > return IRQ_HANDLED; > } >
Hi Daniel, On Mon, May 06, 2019 at 04:52:19PM +0200, Daniel Mack wrote: > Hi Dmitry, > > Is this one good to go in? WDYT? I wonder if we should combine the 2 and have something like below. Thanks! Input: eeti_ts - read hardware state once after wakeup From: Daniel Mack <daniel@zonque.org> For systems in which the touch IRQ is acting as wakeup source, and that do not support level-driven interrupts, the interrupt controller might not latch the GPIO IRQ during sleep. In such cases, the interrupt will never occur again after resume, hence the touch screen appears dead. To fix this, check for the assertion of the attn gpio, and read form the controller once in the resume path to read the hardware status and to arm the IRQ again. Introduce a mutex to guard eeti_ts_read() against parallel invocations from different contexts. Signed-off-by: Daniel Mack <daniel@zonque.org> Reported-by: Sven Neumann <Sven.Neumann@teufel.de> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/input/touchscreen/eeti_ts.c | 71 ++++++++++++++++++++++++++++------- 1 file changed, 56 insertions(+), 15 deletions(-) diff --git a/drivers/input/touchscreen/eeti_ts.c b/drivers/input/touchscreen/eeti_ts.c index 7fe41965c5d1..e3aee95344fc 100644 --- a/drivers/input/touchscreen/eeti_ts.c +++ b/drivers/input/touchscreen/eeti_ts.c @@ -41,6 +41,7 @@ struct eeti_ts { struct input_dev *input; struct gpio_desc *attn_gpio; struct touchscreen_properties props; + struct mutex mutex; bool running; }; @@ -75,42 +76,80 @@ static void eeti_ts_report_event(struct eeti_ts *eeti, u8 *buf) input_sync(eeti->input); } +static int eeti_ts_read(struct eeti_ts *eeti) +{ + int len, error; + char buf[6]; + + len = i2c_master_recv(eeti->client, buf, sizeof(buf)); + if (len != sizeof(buf)) { + error = len < 0 ? len : -EIO; + dev_err(&eeti->client->dev, + "failed to read touchscreen data: %d\n", + error); + return error; + } + + /* Motion packet */ + if (buf[0] & 0x80) + eeti_ts_report_event(eeti, buf); + + return 0; +} + static irqreturn_t eeti_ts_isr(int irq, void *dev_id) { struct eeti_ts *eeti = dev_id; - int len; int error; - char buf[6]; + + mutex_lock(&eeti->mutex); do { - len = i2c_master_recv(eeti->client, buf, sizeof(buf)); - if (len != sizeof(buf)) { - error = len < 0 ? len : -EIO; - dev_err(&eeti->client->dev, - "failed to read touchscreen data: %d\n", - error); + /* + * If we have attention GPIO, trust it. Otherwise we'll read + * once and exit. We assume that in this case we are using + * level triggered interrupt and it will get raised again + * if/when there is more data. + */ + if (eeti->attn_gpio && + !gpiod_get_value_cansleep(eeti->attn_gpio)) { break; } - if (buf[0] & 0x80) { - /* Motion packet */ - eeti_ts_report_event(eeti, buf); - } - } while (eeti->running && - eeti->attn_gpio && gpiod_get_value_cansleep(eeti->attn_gpio)); + error = eeti_ts_read(eeti); + if (error) + break; + + } while (eeti->running && eeti->attn_gpio); + mutex_unlock(&eeti->mutex); return IRQ_HANDLED; } static void eeti_ts_start(struct eeti_ts *eeti) { + mutex_lock(&eeti->mutex); + eeti->running = true; - wmb(); enable_irq(eeti->client->irq); + + /* + * Kick the controller in case we are using edge interrupt and + * we missed our edge while interrupt was disabled. We expect + * the attention GPIO to be wired in this case. + */ + if (eeti->attn_gpio && gpiod_get_value_cansleep(eeti->attn_gpio)) + eeti_ts_read(eeti); + + mutex_lock(&eeti->mutex); } static void eeti_ts_stop(struct eeti_ts *eeti) { + /* + * Not locking here, just setting a flag and expect that the + * interrupt thread will notice the flag eventually. + */ eeti->running = false; wmb(); disable_irq(eeti->client->irq); @@ -153,6 +192,8 @@ static int eeti_ts_probe(struct i2c_client *client, return -ENOMEM; } + mutex_init(&eeti->mutex); + input = devm_input_allocate_device(dev); if (!input) { dev_err(dev, "Failed to allocate input device.\n");
On 14/5/2019 5:12 AM, Dmitry Torokhov wrote: > Hi Daniel, > > On Mon, May 06, 2019 at 04:52:19PM +0200, Daniel Mack wrote: >> Hi Dmitry, >> >> Is this one good to go in? WDYT? > > I wonder if we should combine the 2 and have something like below. Sure, looks fine to me, except for one small yet important thing - see below. > Input: eeti_ts - read hardware state once after wakeup > > From: Daniel Mack <daniel@zonque.org> > > For systems in which the touch IRQ is acting as wakeup source, and that do > not support level-driven interrupts, the interrupt controller might not > latch the GPIO IRQ during sleep. In such cases, the interrupt will never > occur again after resume, hence the touch screen appears dead. > > To fix this, check for the assertion of the attn gpio, and read form the > controller once in the resume path to read the hardware status and > to arm the IRQ again. > > Introduce a mutex to guard eeti_ts_read() against parallel invocations > from different contexts. > > Signed-off-by: Daniel Mack <daniel@zonque.org> > Reported-by: Sven Neumann <Sven.Neumann@teufel.de> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/input/touchscreen/eeti_ts.c | 71 ++++++++++++++++++++++++++++------- > 1 file changed, 56 insertions(+), 15 deletions(-) [...] > static void eeti_ts_start(struct eeti_ts *eeti) > { > + mutex_lock(&eeti->mutex); > + > eeti->running = true; > - wmb(); > enable_irq(eeti->client->irq); > + > + /* > + * Kick the controller in case we are using edge interrupt and > + * we missed our edge while interrupt was disabled. We expect > + * the attention GPIO to be wired in this case. > + */ > + if (eeti->attn_gpio && gpiod_get_value_cansleep(eeti->attn_gpio)) > + eeti_ts_read(eeti); > + > + mutex_lock(&eeti->mutex); If you turn the above into a mutex_unlock(), I'm happy :) Thanks, Daniel
On Wed, May 15, 2019 at 09:54:17PM +0200, Daniel Mack wrote: > On 14/5/2019 5:12 AM, Dmitry Torokhov wrote: > > Hi Daniel, > > > > On Mon, May 06, 2019 at 04:52:19PM +0200, Daniel Mack wrote: > >> Hi Dmitry, > >> > >> Is this one good to go in? WDYT? > > > > I wonder if we should combine the 2 and have something like below. > > Sure, looks fine to me, except for one small yet important thing - see > below. > > > > > Input: eeti_ts - read hardware state once after wakeup > > > > From: Daniel Mack <daniel@zonque.org> > > > > For systems in which the touch IRQ is acting as wakeup source, and that do > > not support level-driven interrupts, the interrupt controller might not > > latch the GPIO IRQ during sleep. In such cases, the interrupt will never > > occur again after resume, hence the touch screen appears dead. > > > > To fix this, check for the assertion of the attn gpio, and read form the > > controller once in the resume path to read the hardware status and > > to arm the IRQ again. > > > > Introduce a mutex to guard eeti_ts_read() against parallel invocations > > from different contexts. > > > > Signed-off-by: Daniel Mack <daniel@zonque.org> > > Reported-by: Sven Neumann <Sven.Neumann@teufel.de> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > --- > > drivers/input/touchscreen/eeti_ts.c | 71 ++++++++++++++++++++++++++++------- > > 1 file changed, 56 insertions(+), 15 deletions(-) > > [...] > > > static void eeti_ts_start(struct eeti_ts *eeti) > > { > > + mutex_lock(&eeti->mutex); > > + > > eeti->running = true; > > - wmb(); > > enable_irq(eeti->client->irq); > > + > > + /* > > + * Kick the controller in case we are using edge interrupt and > > + * we missed our edge while interrupt was disabled. We expect > > + * the attention GPIO to be wired in this case. > > + */ > > + if (eeti->attn_gpio && gpiod_get_value_cansleep(eeti->attn_gpio)) > > + eeti_ts_read(eeti); > > + > > + mutex_lock(&eeti->mutex); > > If you turn the above into a mutex_unlock(), I'm happy :) *sigh* I so wanted to save 2 bytes here ;) Thanks for spotting this.
diff --git a/drivers/input/touchscreen/eeti_ts.c b/drivers/input/touchscreen/eeti_ts.c index 7fe41965c5d1..3e13cc12aaaf 100644 --- a/drivers/input/touchscreen/eeti_ts.c +++ b/drivers/input/touchscreen/eeti_ts.c @@ -75,11 +75,9 @@ static void eeti_ts_report_event(struct eeti_ts *eeti, u8 *buf) input_sync(eeti->input); } -static irqreturn_t eeti_ts_isr(int irq, void *dev_id) +static void eeti_ts_read(struct eeti_ts *eeti) { - struct eeti_ts *eeti = dev_id; - int len; - int error; + int len, error; char buf[6]; do { @@ -92,12 +90,18 @@ static irqreturn_t eeti_ts_isr(int irq, void *dev_id) break; } - if (buf[0] & 0x80) { - /* Motion packet */ + /* Motion packet */ + if (buf[0] & 0x80) eeti_ts_report_event(eeti, buf); - } } while (eeti->running && eeti->attn_gpio && gpiod_get_value_cansleep(eeti->attn_gpio)); +} + +static irqreturn_t eeti_ts_isr(int irq, void *dev_id) +{ + struct eeti_ts *eeti = dev_id; + + eeti_ts_read(eeti); return IRQ_HANDLED; }
Move the ISR handling code to its own function This allows us to call it at resume time to read the hardware state. Signed-off-by: Daniel Mack <daniel@zonque.org> --- drivers/input/touchscreen/eeti_ts.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)