* WM97xx touchscreen updates @ 2008-04-14 17:38 Mark Brown 2008-04-14 17:39 ` [PATCH 1/3] wm97xx-core: Only schedule interrupt handler if not already scheduled Mark Brown 0 siblings, 1 reply; 12+ messages in thread From: Mark Brown @ 2008-04-14 17:38 UTC (permalink / raw) To: Andrew Morton, Dmitry Torokhov, Jiri Kosina; +Cc: linux-input, linux-kernel This patch series updates the WM97xx touchscreen drivers with a couple of small cleanups and the addition of support for use of the WM97xx touchpanels to wake up from suspend. It is also available in the git repository at: git://opensource.wolfsonmicro.com/linux-2.6-touch upstream Mark Brown (3): wm97xx-core: Only schedule interrupt handler if not already scheduled wm97xx-core: Use IRQF_SAMPLE_RANDOM wm97xx-core: Support use as a wakeup source drivers/input/touchscreen/wm97xx-core.c | 39 ++++++++++++++++++++++++------ include/linux/wm97xx.h | 3 ++ 2 files changed, 34 insertions(+), 8 deletions(-) ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/3] wm97xx-core: Only schedule interrupt handler if not already scheduled 2008-04-14 17:38 WM97xx touchscreen updates Mark Brown @ 2008-04-14 17:39 ` Mark Brown 2008-04-14 17:39 ` [PATCH 2/3] wm97xx-core: Use IRQF_SAMPLE_RANDOM Mark Brown 2008-04-14 18:10 ` [PATCH 1/3] wm97xx-core: Only schedule interrupt handler if not already scheduled Dmitry Torokhov 0 siblings, 2 replies; 12+ messages in thread From: Mark Brown @ 2008-04-14 17:39 UTC (permalink / raw) To: Andrew Morton, Dmitry Torokhov, Jiri Kosina Cc: linux-input, linux-kernel, Mark Brown Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com> --- drivers/input/touchscreen/wm97xx-core.c | 14 +++++++------- 1 files changed, 7 insertions(+), 7 deletions(-) diff --git a/drivers/input/touchscreen/wm97xx-core.c b/drivers/input/touchscreen/wm97xx-core.c index 2910999..e27b1e0 100644 --- a/drivers/input/touchscreen/wm97xx-core.c +++ b/drivers/input/touchscreen/wm97xx-core.c @@ -328,18 +328,18 @@ static void wm97xx_pen_irq_worker(struct work_struct *work) * * We have to disable the codec interrupt in the handler because it * can take upto 1ms to clear the interrupt source. We schedule a task - * in a work queue to do the actual interaction with the chip (it - * doesn't matter if we end up reenqueing it before it is executed - * since we don't touch the chip until it has run). The interrupt is - * then enabled again in the slow handler when the source has been - * cleared. + * in a work queue to do the actual interaction with the chip. The + * interrupt is then enabled again in the slow handler when the source + * has been cleared. */ static irqreturn_t wm97xx_pen_interrupt(int irq, void *dev_id) { struct wm97xx *wm = dev_id; - wm->mach_ops->irq_enable(wm, 0); - queue_work(wm->ts_workq, &wm->pen_event_work); + if (!work_pending(&wm->pen_event_work)) { + wm->mach_ops->irq_enable(wm, 0); + queue_work(wm->ts_workq, &wm->pen_event_work); + } return IRQ_HANDLED; } -- 1.5.5 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/3] wm97xx-core: Use IRQF_SAMPLE_RANDOM 2008-04-14 17:39 ` [PATCH 1/3] wm97xx-core: Only schedule interrupt handler if not already scheduled Mark Brown @ 2008-04-14 17:39 ` Mark Brown 2008-04-14 17:39 ` [PATCH 3/3] wm97xx-core: Support use as a wakeup source Mark Brown 2008-04-14 18:10 ` [PATCH 1/3] wm97xx-core: Only schedule interrupt handler if not already scheduled Dmitry Torokhov 1 sibling, 1 reply; 12+ messages in thread From: Mark Brown @ 2008-04-14 17:39 UTC (permalink / raw) To: Andrew Morton, Dmitry Torokhov, Jiri Kosina Cc: linux-input, linux-kernel, Mark Brown The touchscreen interrupt is driven by human input which can reasonably be used to provide entropy. Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com> --- drivers/input/touchscreen/wm97xx-core.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/drivers/input/touchscreen/wm97xx-core.c b/drivers/input/touchscreen/wm97xx-core.c index e27b1e0..fec07c2 100644 --- a/drivers/input/touchscreen/wm97xx-core.c +++ b/drivers/input/touchscreen/wm97xx-core.c @@ -355,7 +355,8 @@ static int wm97xx_init_pen_irq(struct wm97xx *wm) * provided. */ BUG_ON(!wm->mach_ops->irq_enable); - if (request_irq(wm->pen_irq, wm97xx_pen_interrupt, IRQF_SHARED, + if (request_irq(wm->pen_irq, wm97xx_pen_interrupt, + IRQF_SHARED | IRQF_SAMPLE_RANDOM, "wm97xx-pen", wm)) { dev_err(wm->dev, "Failed to register pen down interrupt, polling"); -- 1.5.5 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/3] wm97xx-core: Support use as a wakeup source 2008-04-14 17:39 ` [PATCH 2/3] wm97xx-core: Use IRQF_SAMPLE_RANDOM Mark Brown @ 2008-04-14 17:39 ` Mark Brown 2008-04-14 20:34 ` Dmitry Torokhov 0 siblings, 1 reply; 12+ messages in thread From: Mark Brown @ 2008-04-14 17:39 UTC (permalink / raw) To: Andrew Morton, Dmitry Torokhov, Jiri Kosina Cc: linux-input, linux-kernel, Mark Brown The PENDOWN signal from the WM97xx touch screen controllers can be used to generate a wakeup event when the system is suspended. Provide a new machine configuration option 'suspend_mode' allowing machine drivers to enable this. If no suspend_mode is provided then the touch panel will be powered down when the system is suspended. Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com> --- drivers/input/touchscreen/wm97xx-core.c | 22 ++++++++++++++++++++++ include/linux/wm97xx.h | 3 +++ 2 files changed, 25 insertions(+), 0 deletions(-) diff --git a/drivers/input/touchscreen/wm97xx-core.c b/drivers/input/touchscreen/wm97xx-core.c index fec07c2..e1a1ca0 100644 --- a/drivers/input/touchscreen/wm97xx-core.c +++ b/drivers/input/touchscreen/wm97xx-core.c @@ -689,10 +689,32 @@ static int wm97xx_remove(struct device *dev) static int wm97xx_suspend(struct device *dev, pm_message_t state) { struct wm97xx *wm = dev_get_drvdata(dev); + u16 reg; + int suspend_mode; + + if (wm->mach_ops) + suspend_mode = wm->mach_ops->suspend_mode; + else + suspend_mode = 0; if (wm->input_dev->users) cancel_delayed_work_sync(&wm->ts_reader); + /* Power down the digitiser (bypassing the cache for resume) */ + reg = wm97xx_reg_read(wm, AC97_WM97XX_DIGITISER2); + reg &= ~WM97XX_PRP_DET_DIG; + if (wm->input_dev->users && suspend_mode) + reg |= suspend_mode; + wm->ac97->bus->ops->write(wm->ac97, AC97_WM97XX_DIGITISER2, reg); + + /* WM9713 has an additional power bit - turn it off if there + * are no users or if suspend mode is zero. */ + if (wm->id == WM9713_ID2 && + (!wm->input_dev->users || !suspend_mode)) { + reg = wm97xx_reg_read(wm, AC97_EXTENDED_MID) | 0x8000; + wm97xx_reg_write(wm, AC97_EXTENDED_MID, reg); + } + return 0; } diff --git a/include/linux/wm97xx.h b/include/linux/wm97xx.h index ed01c7d..2082833 100644 --- a/include/linux/wm97xx.h +++ b/include/linux/wm97xx.h @@ -258,6 +258,9 @@ struct wm97xx_mach_ops { /* pre and post sample - can be used to minimise any analog noise */ void (*pre_sample) (int); /* function to run before sampling */ void (*post_sample) (int); /* function to run after sampling */ + + /* WM97XX_PRP value to configure while system is suspended */ + u16 suspend_mode; }; struct wm97xx { -- 1.5.5 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] wm97xx-core: Support use as a wakeup source 2008-04-14 17:39 ` [PATCH 3/3] wm97xx-core: Support use as a wakeup source Mark Brown @ 2008-04-14 20:34 ` Dmitry Torokhov 2008-04-15 0:29 ` David Brownell 0 siblings, 1 reply; 12+ messages in thread From: Dmitry Torokhov @ 2008-04-14 20:34 UTC (permalink / raw) To: Mark Brown Cc: Andrew Morton, Jiri Kosina, linux-input, linux-kernel, David Brownell Hi Mark, On Mon, Apr 14, 2008 at 06:39:35PM +0100, Mark Brown wrote: > The PENDOWN signal from the WM97xx touch screen controllers can be used > to generate a wakeup event when the system is suspended. Provide a new > machine configuration option 'suspend_mode' allowing machine drivers to > enable this. If no suspend_mode is provided then the touch panel will be > powered down when the system is suspended. > Hmm, another unique module option... How about using device_can_wakeup() infrastructure that we already have. I'm adding David Brownell to CC since I believe he's doing a lot of similar work. > Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com> > --- > drivers/input/touchscreen/wm97xx-core.c | 22 ++++++++++++++++++++++ > include/linux/wm97xx.h | 3 +++ > 2 files changed, 25 insertions(+), 0 deletions(-) > > diff --git a/drivers/input/touchscreen/wm97xx-core.c b/drivers/input/touchscreen/wm97xx-core.c > index fec07c2..e1a1ca0 100644 > --- a/drivers/input/touchscreen/wm97xx-core.c > +++ b/drivers/input/touchscreen/wm97xx-core.c > @@ -689,10 +689,32 @@ static int wm97xx_remove(struct device *dev) > static int wm97xx_suspend(struct device *dev, pm_message_t state) > { > struct wm97xx *wm = dev_get_drvdata(dev); > + u16 reg; > + int suspend_mode; > + > + if (wm->mach_ops) > + suspend_mode = wm->mach_ops->suspend_mode; > + else > + suspend_mode = 0; > > if (wm->input_dev->users) > cancel_delayed_work_sync(&wm->ts_reader); > > + /* Power down the digitiser (bypassing the cache for resume) */ > + reg = wm97xx_reg_read(wm, AC97_WM97XX_DIGITISER2); > + reg &= ~WM97XX_PRP_DET_DIG; > + if (wm->input_dev->users && suspend_mode) > + reg |= suspend_mode; > + wm->ac97->bus->ops->write(wm->ac97, AC97_WM97XX_DIGITISER2, reg); > + > + /* WM9713 has an additional power bit - turn it off if there > + * are no users or if suspend mode is zero. */ > + if (wm->id == WM9713_ID2 && > + (!wm->input_dev->users || !suspend_mode)) { > + reg = wm97xx_reg_read(wm, AC97_EXTENDED_MID) | 0x8000; > + wm97xx_reg_write(wm, AC97_EXTENDED_MID, reg); > + } > + > return 0; > } > > diff --git a/include/linux/wm97xx.h b/include/linux/wm97xx.h > index ed01c7d..2082833 100644 > --- a/include/linux/wm97xx.h > +++ b/include/linux/wm97xx.h > @@ -258,6 +258,9 @@ struct wm97xx_mach_ops { > /* pre and post sample - can be used to minimise any analog noise */ > void (*pre_sample) (int); /* function to run before sampling */ > void (*post_sample) (int); /* function to run after sampling */ > + > + /* WM97XX_PRP value to configure while system is suspended */ > + u16 suspend_mode; > }; > > struct wm97xx { > -- > 1.5.5 > -- Dmitry ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] wm97xx-core: Support use as a wakeup source 2008-04-14 20:34 ` Dmitry Torokhov @ 2008-04-15 0:29 ` David Brownell 2008-04-15 12:18 ` Mark Brown 0 siblings, 1 reply; 12+ messages in thread From: David Brownell @ 2008-04-15 0:29 UTC (permalink / raw) To: Dmitry Torokhov Cc: Mark Brown, Andrew Morton, Jiri Kosina, linux-input, linux-kernel On Monday 14 April 2008, Dmitry Torokhov wrote: > Hi Mark, > > On Mon, Apr 14, 2008 at 06:39:35PM +0100, Mark Brown wrote: > > The PENDOWN signal from the WM97xx touch screen controllers can be used > > to generate a wakeup event when the system is suspended. Provide a new > > machine configuration option 'suspend_mode' allowing machine drivers to > > enable this. If no suspend_mode is provided then the touch panel will be > > powered down when the system is suspended. > > > > Hmm, another unique module option... How about using device_can_wakeup() > infrastructure that we already have. I'm adding David Brownell to CC since > I believe he's doing a lot of similar work. I don't quite follow the technical content of this "suspend_mode" mask, but it sure looks as if it should use the driver model flags in addition to whatever that mask is doing. There are two aspects to this: (a) Whether on a given system the touchscreen *can* issue wakeup events ... call device_init_wakeup(dev, true) to say it can. In this case, presumably suspend_mode == 0 means it can't. Board setup, or at latest probe(), should device_init_wakeup(). (b) Whether it *should* do so ... e.g. the Nokia N810 has a mode where it doesn't, and usually when you're carrying one around in a pocket (or purse, or whatever) you would say "doesn't". Userspace manages that answer via a sysfs file, and drivers test the answer with device_may_wakeup(dev). So: yes, this should initialize and use those driver model flags. > > Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com> > > --- > > drivers/input/touchscreen/wm97xx-core.c | 22 ++++++++++++++++++++++ > > include/linux/wm97xx.h | 3 +++ > > 2 files changed, 25 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/input/touchscreen/wm97xx-core.c b/drivers/input/touchscreen/wm97xx-core.c > > index fec07c2..e1a1ca0 100644 > > --- a/drivers/input/touchscreen/wm97xx-core.c > > +++ b/drivers/input/touchscreen/wm97xx-core.c > > @@ -689,10 +689,32 @@ static int wm97xx_remove(struct device *dev) > > static int wm97xx_suspend(struct device *dev, pm_message_t state) > > { > > struct wm97xx *wm = dev_get_drvdata(dev); > > + u16 reg; > > + int suspend_mode; > > + > > + if (wm->mach_ops) > > + suspend_mode = wm->mach_ops->suspend_mode; > > + else > > + suspend_mode = 0; > > > > if (wm->input_dev->users) > > cancel_delayed_work_sync(&wm->ts_reader); > > > > + /* Power down the digitiser (bypassing the cache for resume) */ > > + reg = wm97xx_reg_read(wm, AC97_WM97XX_DIGITISER2); > > + reg &= ~WM97XX_PRP_DET_DIG; > > + if (wm->input_dev->users && suspend_mode) This should probably check device_may_wakeup(SOMEDEV), rather than testing suspend_mode. The wakeup mechanism here isn't clear to me. Does this assume that the touchscreen IRQ is wakeup-capable? I don't see any calls to enable_irq_wake() or disable_irq_wake() ... or for that matter any other calls that might affect system behavior. > > + reg |= suspend_mode; > > + wm->ac97->bus->ops->write(wm->ac97, AC97_WM97XX_DIGITISER2, reg); > > + > > + /* WM9713 has an additional power bit - turn it off if there > > + * are no users or if suspend mode is zero. */ > > + if (wm->id == WM9713_ID2 && > > + (!wm->input_dev->users || !suspend_mode)) { > > + reg = wm97xx_reg_read(wm, AC97_EXTENDED_MID) | 0x8000; > > + wm97xx_reg_write(wm, AC97_EXTENDED_MID, reg); > > + } > > + > > return 0; > > } > > > > diff --git a/include/linux/wm97xx.h b/include/linux/wm97xx.h > > index ed01c7d..2082833 100644 > > --- a/include/linux/wm97xx.h > > +++ b/include/linux/wm97xx.h > > @@ -258,6 +258,9 @@ struct wm97xx_mach_ops { > > /* pre and post sample - can be used to minimise any analog noise */ > > void (*pre_sample) (int); /* function to run before sampling */ > > void (*post_sample) (int); /* function to run after sampling */ > > + > > + /* WM97XX_PRP value to configure while system is suspended */ > > + u16 suspend_mode; I'm more accustomed to thinking that the driver will know "the" setup bit values to assign which will ensure that the chip issues wakeup events ... that stuff is usually not board-specific. > > }; > > > > struct wm97xx { > > -- > > 1.5.5 > > > > -- > Dmitry > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] wm97xx-core: Support use as a wakeup source 2008-04-15 0:29 ` David Brownell @ 2008-04-15 12:18 ` Mark Brown 0 siblings, 0 replies; 12+ messages in thread From: Mark Brown @ 2008-04-15 12:18 UTC (permalink / raw) To: David Brownell Cc: Dmitry Torokhov, Andrew Morton, Jiri Kosina, linux-input, linux-kernel On Mon, Apr 14, 2008 at 05:29:07PM -0700, David Brownell wrote: > On Monday 14 April 2008, Dmitry Torokhov wrote: > > Hmm, another unique module option... How about using device_can_wakeup() The reason there are so many configuration options here is that many of the features of the driver depend on exactly how the chip is connected to the rest of the system and can't be automatically discovered. In the embedded applications where these devices are usually deployed this is usually an OK user experience - if these hooks aren't provided what tends to happen is that users patch the driver directly to get the configurablilty they need. > I don't quite follow the technical content of this "suspend_mode" > mask, but it sure looks as if it should use the driver model flags > in addition to whatever that mask is doing. It's probably worth pointing out here that this part of the WM97xx driver is split into two portions. This part of the driver covers the WM97xx itself but in order to actually use functionality like wakeup a machine specific driver needs to be added. The WM97xx chips have a series of multi-function pins, some of which can be configured to provide a signal mirroring the pen down status with most of the chip powered down. It can also be configured to do other things like try to start the AC97 bus and/or start the touchscreen digitiser up if it detects a pen down while suspended. At the minute this is implemented by having the WM97xx register a child device which platform code can provide. When the child device probes it configures both the WM97xx and the rest of the system. The child device can also provide other machine-specific things like use of the pen down or interrupt signals from the chip to control when the touch screen is read, synchronisation of the touch screen with writes to the display to reduce interference from the video signals and accelerated reading of the touchscreen if the AC97 controller is able to support it. > There are two aspects to this: > (a) Whether on a given system the touchscreen *can* issue wakeup > events ... call device_init_wakeup(dev, true) to say it can. > In this case, presumably suspend_mode == 0 means it can't. > Board setup, or at latest probe(), should device_init_wakeup(). > So: yes, this should initialize and use those driver model flags. There is a bit of a question about which device this should be checked, though the WM97xx itself is probably more sensible. > > > + reg = wm97xx_reg_read(wm, AC97_WM97XX_DIGITISER2); > > > + reg &= ~WM97XX_PRP_DET_DIG; > > > + if (wm->input_dev->users && suspend_mode) > This should probably check device_may_wakeup(SOMEDEV), rather > than testing suspend_mode. That by itself isn't enough since if the system hasn't been configured appropriately then the WM97xx won't be able to wake the system - we need the suspend_mode configuration to know how to wake. I'll do a spin of the patch which pushes the configuration of suspend mode through an API to let the core keep track of this. > The wakeup mechanism here isn't clear to me. Does this assume > that the touchscreen IRQ is wakeup-capable? I don't see any No, it doesn't assume that. For example, the system may wish to have the chip initiate wakeup by bringing the AC97 bus up or have an output from the WM97xx wired directly to some other component such as a PMU or microcontroller rather than to an interrupt line visible to the CPU. In theory this could also be used for some purpose other than waking the system though I'm not aware of any such users so I don't think that's worth worrying about right now. In any case, the default is to enable wakeup so most users won't notice this addition anyway. > calls to enable_irq_wake() or disable_irq_wake() ... or for that > matter any other calls that might affect system behavior. Some combination of the machine-specific driver and other platform code is expected to do this where required. > > > + /* WM97XX_PRP value to configure while system is suspended */ > > > + u16 suspend_mode; > I'm more accustomed to thinking that the driver will > know "the" setup bit values to assign which will > ensure that the chip issues wakeup events ... that > stuff is usually not board-specific. Unfortunately the fact that there are many different ways for the chip to be used in a system (none of which can be automatically discovered) means that this does need to be board-specific. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] wm97xx-core: Only schedule interrupt handler if not already scheduled 2008-04-14 17:39 ` [PATCH 1/3] wm97xx-core: Only schedule interrupt handler if not already scheduled Mark Brown 2008-04-14 17:39 ` [PATCH 2/3] wm97xx-core: Use IRQF_SAMPLE_RANDOM Mark Brown @ 2008-04-14 18:10 ` Dmitry Torokhov 2008-04-15 8:48 ` Mark Brown 1 sibling, 1 reply; 12+ messages in thread From: Dmitry Torokhov @ 2008-04-14 18:10 UTC (permalink / raw) To: Mark Brown; +Cc: Andrew Morton, Jiri Kosina, linux-input, linux-kernel Hi Mark, On Mon, Apr 14, 2008 at 06:39:33PM +0100, Mark Brown wrote: > static irqreturn_t wm97xx_pen_interrupt(int irq, void *dev_id) > { > struct wm97xx *wm = dev_id; > > - wm->mach_ops->irq_enable(wm, 0); > - queue_work(wm->ts_workq, &wm->pen_event_work); > + if (!work_pending(&wm->pen_event_work)) { > + wm->mach_ops->irq_enable(wm, 0); > + queue_work(wm->ts_workq, &wm->pen_event_work); > + } Given the fact that work will not be queued if it is pending anyway why is this change needed? -- Dmitry ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] wm97xx-core: Only schedule interrupt handler if not already scheduled 2008-04-14 18:10 ` [PATCH 1/3] wm97xx-core: Only schedule interrupt handler if not already scheduled Dmitry Torokhov @ 2008-04-15 8:48 ` Mark Brown 2008-04-15 13:02 ` Dmitry Torokhov 0 siblings, 1 reply; 12+ messages in thread From: Mark Brown @ 2008-04-15 8:48 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Andrew Morton, Jiri Kosina, linux-input, linux-kernel On Mon, Apr 14, 2008 at 02:10:02PM -0400, Dmitry Torokhov wrote: > On Mon, Apr 14, 2008 at 06:39:33PM +0100, Mark Brown wrote: > > + if (!work_pending(&wm->pen_event_work)) { > > + wm->mach_ops->irq_enable(wm, 0); > > + queue_work(wm->ts_workq, &wm->pen_event_work); > > + } > Given the fact that work will not be queued if it is pending anyway > why is this change needed? As well as not queuing the work it ensures that the calls to irq_enable() are balanced which helps with implementing that operation. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] wm97xx-core: Only schedule interrupt handler if not already scheduled 2008-04-15 8:48 ` Mark Brown @ 2008-04-15 13:02 ` Dmitry Torokhov 2008-04-15 14:42 ` Mark Brown 0 siblings, 1 reply; 12+ messages in thread From: Dmitry Torokhov @ 2008-04-15 13:02 UTC (permalink / raw) To: Andrew Morton, Jiri Kosina, linux-input, linux-kernel On Tue, Apr 15, 2008 at 09:48:19AM +0100, Mark Brown wrote: > On Mon, Apr 14, 2008 at 02:10:02PM -0400, Dmitry Torokhov wrote: > > On Mon, Apr 14, 2008 at 06:39:33PM +0100, Mark Brown wrote: > > > > + if (!work_pending(&wm->pen_event_work)) { > > > + wm->mach_ops->irq_enable(wm, 0); > > > + queue_work(wm->ts_workq, &wm->pen_event_work); > > > + } > > > Given the fact that work will not be queued if it is pending anyway > > why is this change needed? > > As well as not queuing the work it ensures that the calls to > irq_enable() are balanced which helps with implementing that operation. Hmm... another question then - what would fire up wm97xx_pen_interrupt() again, before wm97xx_pen_irq_worker had a chance to run, if the very first thing we do after getting interrupt is irq_enable(wm, 0)? -- Dmitry ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] wm97xx-core: Only schedule interrupt handler if not already scheduled 2008-04-15 13:02 ` Dmitry Torokhov @ 2008-04-15 14:42 ` Mark Brown 0 siblings, 0 replies; 12+ messages in thread From: Mark Brown @ 2008-04-15 14:42 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Andrew Morton, Jiri Kosina, linux-input, linux-kernel On Tue, Apr 15, 2008 at 09:02:35AM -0400, Dmitry Torokhov wrote: > On Tue, Apr 15, 2008 at 09:48:19AM +0100, Mark Brown wrote: > > As well as not queuing the work it ensures that the calls to > > irq_enable() are balanced which helps with implementing that operation. > Hmm... another question then - what would fire up wm97xx_pen_interrupt() > again, before wm97xx_pen_irq_worker had a chance to run, if the very > first thing we do after getting interrupt is irq_enable(wm, 0)? Normally nothing - in most systems the interrupt will not be shared and/or the board irq_enable() operation will completely disable the IRQ line. The main use case would be if the interrupt is shared and the board irq_enable() operation doesn't affect other sources, though boards may also decide that they can do without disabling the interrupt. ^ permalink raw reply [flat|nested] 12+ messages in thread
* WM97xx touchscreen updates @ 2008-04-15 14:45 Mark Brown 2008-04-15 14:47 ` [PATCH 1/3] wm97xx-core: Only schedule interrupt handler if not already scheduled Mark Brown 0 siblings, 1 reply; 12+ messages in thread From: Mark Brown @ 2008-04-15 14:45 UTC (permalink / raw) To: Andrew Morton, David Brownell, Dmitry Torokhov, Jiri Kosina Cc: linux-input, linux-kernel This patch series updates the WM97xx touchscreen drivers with a couple of small cleanups and the addition of support for use of the WM97xx touchpanels to wake up from suspend. Since the last submission I have: - Updated the commit message for scheduling of the interrupt handler. - Hooked the touchscreen into the driver model wakeup infrastructure. It is also available in the git repository at: git://opensource.wolfsonmicro.com/linux-2.6-touch upstream Mark Brown (3): wm97xx-core: Only schedule interrupt handler if not already scheduled wm97xx-core: Use IRQF_SAMPLE_RANDOM wm97xx-core: Support use as a wakeup source drivers/input/touchscreen/wm97xx-core.c | 56 ++++++++++++++++++++++++++---- include/linux/wm97xx.h | 3 ++ 2 files changed, 51 insertions(+), 8 deletions(-) ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/3] wm97xx-core: Only schedule interrupt handler if not already scheduled 2008-04-15 14:45 WM97xx touchscreen updates Mark Brown @ 2008-04-15 14:47 ` Mark Brown 2008-04-15 14:47 ` [PATCH 2/3] wm97xx-core: Use IRQF_SAMPLE_RANDOM Mark Brown 0 siblings, 1 reply; 12+ messages in thread From: Mark Brown @ 2008-04-15 14:47 UTC (permalink / raw) To: Andrew Morton, David Brownell, Dmitry Torokhov, Jiri Kosina Cc: linux-input, linux-kernel, Mark Brown As well as clarifying the fact that the driver can cope if a second interrupt occurs before the IRQ work is scheduled this also ensures that calls to the machine irq_enable() are balanced, making that easier to implement. Normally this is redundant due to the interrupt disabling but some unusal board configurations can trigger it. Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com> --- drivers/input/touchscreen/wm97xx-core.c | 14 +++++++------- 1 files changed, 7 insertions(+), 7 deletions(-) diff --git a/drivers/input/touchscreen/wm97xx-core.c b/drivers/input/touchscreen/wm97xx-core.c index 2910999..e27b1e0 100644 --- a/drivers/input/touchscreen/wm97xx-core.c +++ b/drivers/input/touchscreen/wm97xx-core.c @@ -328,18 +328,18 @@ static void wm97xx_pen_irq_worker(struct work_struct *work) * * We have to disable the codec interrupt in the handler because it * can take upto 1ms to clear the interrupt source. We schedule a task - * in a work queue to do the actual interaction with the chip (it - * doesn't matter if we end up reenqueing it before it is executed - * since we don't touch the chip until it has run). The interrupt is - * then enabled again in the slow handler when the source has been - * cleared. + * in a work queue to do the actual interaction with the chip. The + * interrupt is then enabled again in the slow handler when the source + * has been cleared. */ static irqreturn_t wm97xx_pen_interrupt(int irq, void *dev_id) { struct wm97xx *wm = dev_id; - wm->mach_ops->irq_enable(wm, 0); - queue_work(wm->ts_workq, &wm->pen_event_work); + if (!work_pending(&wm->pen_event_work)) { + wm->mach_ops->irq_enable(wm, 0); + queue_work(wm->ts_workq, &wm->pen_event_work); + } return IRQ_HANDLED; } -- 1.5.5 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/3] wm97xx-core: Use IRQF_SAMPLE_RANDOM 2008-04-15 14:47 ` [PATCH 1/3] wm97xx-core: Only schedule interrupt handler if not already scheduled Mark Brown @ 2008-04-15 14:47 ` Mark Brown 0 siblings, 0 replies; 12+ messages in thread From: Mark Brown @ 2008-04-15 14:47 UTC (permalink / raw) To: Andrew Morton, David Brownell, Dmitry Torokhov, Jiri Kosina Cc: linux-input, linux-kernel, Mark Brown The touchscreen interrupt is driven by human input which can reasonably be used to provide entropy. Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com> --- drivers/input/touchscreen/wm97xx-core.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/drivers/input/touchscreen/wm97xx-core.c b/drivers/input/touchscreen/wm97xx-core.c index e27b1e0..fec07c2 100644 --- a/drivers/input/touchscreen/wm97xx-core.c +++ b/drivers/input/touchscreen/wm97xx-core.c @@ -355,7 +355,8 @@ static int wm97xx_init_pen_irq(struct wm97xx *wm) * provided. */ BUG_ON(!wm->mach_ops->irq_enable); - if (request_irq(wm->pen_irq, wm97xx_pen_interrupt, IRQF_SHARED, + if (request_irq(wm->pen_irq, wm97xx_pen_interrupt, + IRQF_SHARED | IRQF_SAMPLE_RANDOM, "wm97xx-pen", wm)) { dev_err(wm->dev, "Failed to register pen down interrupt, polling"); -- 1.5.5 ^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2008-04-15 14:47 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-04-14 17:38 WM97xx touchscreen updates Mark Brown 2008-04-14 17:39 ` [PATCH 1/3] wm97xx-core: Only schedule interrupt handler if not already scheduled Mark Brown 2008-04-14 17:39 ` [PATCH 2/3] wm97xx-core: Use IRQF_SAMPLE_RANDOM Mark Brown 2008-04-14 17:39 ` [PATCH 3/3] wm97xx-core: Support use as a wakeup source Mark Brown 2008-04-14 20:34 ` Dmitry Torokhov 2008-04-15 0:29 ` David Brownell 2008-04-15 12:18 ` Mark Brown 2008-04-14 18:10 ` [PATCH 1/3] wm97xx-core: Only schedule interrupt handler if not already scheduled Dmitry Torokhov 2008-04-15 8:48 ` Mark Brown 2008-04-15 13:02 ` Dmitry Torokhov 2008-04-15 14:42 ` Mark Brown -- strict thread matches above, loose matches on Subject: below -- 2008-04-15 14:45 WM97xx touchscreen updates Mark Brown 2008-04-15 14:47 ` [PATCH 1/3] wm97xx-core: Only schedule interrupt handler if not already scheduled Mark Brown 2008-04-15 14:47 ` [PATCH 2/3] wm97xx-core: Use IRQF_SAMPLE_RANDOM Mark Brown
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).