| Message ID | 20090504145700.GA875@avionic-design.de (mailing list archive) |
|---|---|
| State | Awaiting Upstream, archived |
| Headers | show |
Hi Therry, On Mon, May 4, 2009 at 8:27 PM, Thierry Reding <thierry.reding@avionic-design.de> wrote: > * Trilok Soni wrote: >> Hi Thierry, >> >> On Mon, May 4, 2009 at 5:07 PM, Thierry Reding >> <thierry.reding@avionic-design.de> wrote: >> > * Kwangwoo Lee wrote: >> >> Hi Thierry and Trilok, >> >> >> >> On Wed, Apr 29, 2009 at 10:23 PM, Trilok Soni <soni.trilok@gmail.com> wrote: >> >> > Hi Thierry, >> >> > >> >> > I have added linux-omap community. How different is this chip from >> >> > tsc2007. It looks to me that this chip is not much different from >> >> > tsc2007 (this is just quick look at the driver). If they >> >> > are similar please consider using i2c_device_id feature in tsc2007 to >> >> > accommodate this chip. >> >> >> >> I agree with the Trilok's opinion. >> > [snip] >> > >> > I only noticed the tsc2007 driver some time ago, when the tsc2003 was already >> > finished (it's actually pretty old, I just never got around to submitting >> > it). However I never got the tsc2007 to work on my platform because it uses >> > sleeping functions inside the timer handler, which results in an oops right >> > after the first touchscreen interrupt. >> > >> > I guess I could try and fix the tsc2007 properly instead of having a second, >> > pretty similar driver in the tree. >> > >> >> This could be because of improper locking? If you share a crash we can >> have look at it. > > Attached is a patch that fixes things for me. The problem was that the I2C > transfers were done in interrupt context which fails for the controller I use > (PXA270). The attached patch uses a struct work_struct to schedule the I2C > transfers so they are executed in non-interrupt context. > > I've tested the patch on a minimal system with tslib and it work fine with > ts_calibrate and friends. Thanks for the patch. It looks good. Let's wait for Kwangwoo Lee to verify it on his platform. Also please add "tsc2003" entry into i2c_device_ids so that i2c_board_info can specify the .name as "tsc2003".
Hi Thierry, On Mon, May 4, 2009 at 11:57 PM, Thierry Reding <thierry.reding@avionic-design.de> wrote: > * Trilok Soni wrote: >> Hi Thierry, >> >> On Mon, May 4, 2009 at 5:07 PM, Thierry Reding >> <thierry.reding@avionic-design.de> wrote: >> > * Kwangwoo Lee wrote: >> >> Hi Thierry and Trilok, >> >> >> >> On Wed, Apr 29, 2009 at 10:23 PM, Trilok Soni <soni.trilok@gmail.com> wrote: >> >> > Hi Thierry, >> >> > >> >> > I have added linux-omap community. How different is this chip from >> >> > tsc2007. It looks to me that this chip is not much different from >> >> > tsc2007 (this is just quick look at the driver). If they >> >> > are similar please consider using i2c_device_id feature in tsc2007 to >> >> > accommodate this chip. >> >> >> >> I agree with the Trilok's opinion. >> > [snip] >> > >> > I only noticed the tsc2007 driver some time ago, when the tsc2003 was already >> > finished (it's actually pretty old, I just never got around to submitting >> > it). However I never got the tsc2007 to work on my platform because it uses >> > sleeping functions inside the timer handler, which results in an oops right >> > after the first touchscreen interrupt. >> > >> > I guess I could try and fix the tsc2007 properly instead of having a second, >> > pretty similar driver in the tree. >> > >> >> This could be because of improper locking? If you share a crash we can >> have look at it. > > Attached is a patch that fixes things for me. The problem was that the I2C > transfers were done in interrupt context which fails for the controller I use > (PXA270). The attached patch uses a struct work_struct to schedule the I2C > transfers so they are executed in non-interrupt context. Thanks for the patch. It looks good. :) The code in the patch is already merged in the main kernel tree. @@ -235,7 +245,7 @@ static irqreturn_t tsc2007_irq(int irq, void *handle) spin_lock_irqsave(&ts->lock, flags); if (likely(ts->get_pendown_state())) { - disable_irq(ts->irq); + disable_irq_nosync(ts->irq); hrtimer_start(&ts->timer, ktime_set(0, TS_POLL_DELAY), HRTIMER_MODE_REL); } Thanks. > I've tested the patch on a minimal system with tslib and it work fine with > ts_calibrate and friends. > > Thierry
Hi, On Tue, May 5, 2009 at 2:21 AM, Trilok Soni <soni.trilok@gmail.com> wrote: > Hi Therry, > > On Mon, May 4, 2009 at 8:27 PM, Thierry Reding > <thierry.reding@avionic-design.de> wrote: >> * Trilok Soni wrote: >>> Hi Thierry, >>> >>> On Mon, May 4, 2009 at 5:07 PM, Thierry Reding >>> <thierry.reding@avionic-design.de> wrote: >>> > * Kwangwoo Lee wrote: >>> >> Hi Thierry and Trilok, >>> >> >>> >> On Wed, Apr 29, 2009 at 10:23 PM, Trilok Soni <soni.trilok@gmail.com> wrote: >>> >> > Hi Thierry, >>> >> > >>> >> > I have added linux-omap community. How different is this chip from >>> >> > tsc2007. It looks to me that this chip is not much different from >>> >> > tsc2007 (this is just quick look at the driver). If they >>> >> > are similar please consider using i2c_device_id feature in tsc2007 to >>> >> > accommodate this chip. >>> >> >>> >> I agree with the Trilok's opinion. >>> > [snip] >>> > >>> > I only noticed the tsc2007 driver some time ago, when the tsc2003 was already >>> > finished (it's actually pretty old, I just never got around to submitting >>> > it). However I never got the tsc2007 to work on my platform because it uses >>> > sleeping functions inside the timer handler, which results in an oops right >>> > after the first touchscreen interrupt. >>> > >>> > I guess I could try and fix the tsc2007 properly instead of having a second, >>> > pretty similar driver in the tree. >>> > >>> >>> This could be because of improper locking? If you share a crash we can >>> have look at it. >> >> Attached is a patch that fixes things for me. The problem was that the I2C >> transfers were done in interrupt context which fails for the controller I use >> (PXA270). The attached patch uses a struct work_struct to schedule the I2C >> transfers so they are executed in non-interrupt context. >> >> I've tested the patch on a minimal system with tslib and it work fine with >> ts_calibrate and friends. > > Thanks for the patch. It looks good. Let's wait for Kwangwoo Lee to > verify it on his platform. Also please add "tsc2003" entry into > i2c_device_ids so that i2c_board_info can specify the .name as > "tsc2003". I tested the patch in my platform with tslib. And it looks good. Thanks,
* Kwangwoo Lee wrote: [...] > Thanks for the patch. It looks good. :) > The code in the patch is already merged in the main kernel tree. > > @@ -235,7 +245,7 @@ static irqreturn_t tsc2007_irq(int irq, void *handle) > spin_lock_irqsave(&ts->lock, flags); > > if (likely(ts->get_pendown_state())) { > - disable_irq(ts->irq); > + disable_irq_nosync(ts->irq); > hrtimer_start(&ts->timer, ktime_set(0, TS_POLL_DELAY), > HRTIMER_MODE_REL); > } Oh, I hadn't noticed. I diffed against 2.6.30-rc4. Do you want me to send a new patch or will you just rip that piece out? Thierry -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 6, 2009 at 3:45 PM, Thierry Reding <thierry.reding@avionic-design.de> wrote: > * Kwangwoo Lee wrote: > [...] >> Thanks for the patch. It looks good. :) >> The code in the patch is already merged in the main kernel tree. >> >> @@ -235,7 +245,7 @@ static irqreturn_t tsc2007_irq(int irq, void *handle) >> Â Â Â spin_lock_irqsave(&ts->lock, flags); >> >> Â Â Â if (likely(ts->get_pendown_state())) { >> - Â Â Â Â Â Â disable_irq(ts->irq); >> + Â Â Â Â Â Â disable_irq_nosync(ts->irq); >> Â Â Â Â Â Â Â hrtimer_start(&ts->timer, ktime_set(0, TS_POLL_DELAY), >> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â HRTIMER_MODE_REL); >> Â Â Â } > > Oh, I hadn't noticed. I diffed against 2.6.30-rc4. Do you want me to send a > new patch or will you just rip that piece out? Send fixed patch with your Signed-off-by line.
From: Thierry Reding <thierry.reding@avionic-design.de> Subject: [PATCH] tsc2007: Fix for I2C controllers that sleep during transfers. This patch fixes the tsc2007 driver with I2C controllers that sleep during transfers. By moving the critical code to a workqueue, I2C transfers can be scheduled to run in non-interrupt context. Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de> --- drivers/input/touchscreen/tsc2007.c | 25 ++++++++++++++++++------- 1 files changed, 18 insertions(+), 7 deletions(-) diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c index 4ab0702..6efb54d 100644 --- a/drivers/input/touchscreen/tsc2007.c +++ b/drivers/input/touchscreen/tsc2007.c @@ -70,6 +70,7 @@ struct ts_event { struct tsc2007 { struct input_dev *input; char phys[32]; + struct work_struct work; struct hrtimer timer; struct ts_event tc; @@ -173,6 +174,9 @@ static void tsc2007_send_event(void *tsc) dev_dbg(&ts->client->dev, "point(%4d,%4d), pressure (%4u)\n", x, y, rt); + } else { + if (!ts->pendown) + ts->pendown = 1; } hrtimer_start(&ts->timer, ktime_set(0, TS_POLL_PERIOD), @@ -197,11 +201,19 @@ static int tsc2007_read_values(struct tsc2007 *tsc) return 0; } +static void tsc2007_work(struct work_struct *work) +{ + struct tsc2007 *ts = container_of(work, struct tsc2007, work); + tsc2007_read_values(ts); + tsc2007_send_event(ts); +} + static enum hrtimer_restart tsc2007_timer(struct hrtimer *handle) { struct tsc2007 *ts = container_of(handle, struct tsc2007, timer); + unsigned long flags = 0; - spin_lock_irq(&ts->lock); + spin_lock_irqsave(&ts->lock, flags); if (unlikely(!ts->get_pendown_state() && ts->pendown)) { struct input_dev *input = ts->input; @@ -217,12 +229,10 @@ static enum hrtimer_restart tsc2007_timer(struct hrtimer *handle) } else { /* pen is still down, continue with the measurement */ dev_dbg(&ts->client->dev, "pen is still down\n"); - - tsc2007_read_values(ts); - tsc2007_send_event(ts); + schedule_work(&ts->work); } - spin_unlock_irq(&ts->lock); + spin_unlock_irqrestore(&ts->lock, flags); return HRTIMER_NORESTART; } @@ -235,7 +245,7 @@ static irqreturn_t tsc2007_irq(int irq, void *handle) spin_lock_irqsave(&ts->lock, flags); if (likely(ts->get_pendown_state())) { - disable_irq(ts->irq); + disable_irq_nosync(ts->irq); hrtimer_start(&ts->timer, ktime_set(0, TS_POLL_DELAY), HRTIMER_MODE_REL); } @@ -252,7 +262,7 @@ static int tsc2007_probe(struct i2c_client *client, const struct i2c_device_id *id) { struct tsc2007 *ts; - struct tsc2007_platform_data *pdata = pdata = client->dev.platform_data; + struct tsc2007_platform_data *pdata = client->dev.platform_data; struct input_dev *input_dev; int err; @@ -279,6 +289,7 @@ static int tsc2007_probe(struct i2c_client *client, hrtimer_init(&ts->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); ts->timer.function = tsc2007_timer; + INIT_WORK(&ts->work, tsc2007_work); spin_lock_init(&ts->lock); -- tg: (091438d..) adx/input/tsc2007 (depends on: adx/master)