Convert the AB3100 driver to use threaded IRH
in [Kernel]
|
Prev: [RFC] [PATCH 1/2] cgroups: modular subsystems support for use_id
Next: [PATCH v2 3/3] generic sys_ipc wrapper
From: Samuel Ortiz on 8 Jan 2010 04:40 Hi Linus, On Fri, Jan 08, 2010 at 12:08:45AM +0100, Linus Walleij wrote: > This converts the AB3100 core MFD driver to use a threaded > interrupt handler instead of the explicit top/bottom-half > construction with a workqueue. This saves some code and make it > more similar to other modern MFD drivers. patch applied, many thanks. Cheers, Samuel. > Signed-off-by: Linus Walleij <linus.walleij(a)stericsson.com> > --- > drivers/mfd/ab3100-core.c | 43 +++++++++++++------------------------------ > include/linux/mfd/ab3100.h | 3 --- > 2 files changed, 13 insertions(+), 33 deletions(-) > > diff --git a/drivers/mfd/ab3100-core.c b/drivers/mfd/ab3100-core.c > index fd42a80..93b2c14 100644 > --- a/drivers/mfd/ab3100-core.c > +++ b/drivers/mfd/ab3100-core.c > @@ -365,10 +365,13 @@ int ab3100_event_registers_startup_state_get(struct ab3100 *ab3100, > } > EXPORT_SYMBOL(ab3100_event_registers_startup_state_get); > > -/* Interrupt handling worker */ > -static void ab3100_work(struct work_struct *work) > +/* > + * This is a threaded interrupt handler so we can make some > + * I2C calls etc. > + */ > +static irqreturn_t ab3100_irq_handler(int irq, void *data) > { > - struct ab3100 *ab3100 = container_of(work, struct ab3100, work); > + struct ab3100 *ab3100 = data; > u8 event_regs[3]; > u32 fatevent; > int err; > @@ -376,7 +379,7 @@ static void ab3100_work(struct work_struct *work) > err = ab3100_get_register_page_interruptible(ab3100, AB3100_EVENTA1, > event_regs, 3); > if (err) > - goto err_event_wq; > + goto err_event; > > fatevent = (event_regs[0] << 16) | > (event_regs[1] << 8) | > @@ -398,29 +401,11 @@ static void ab3100_work(struct work_struct *work) > dev_dbg(ab3100->dev, > "IRQ Event: 0x%08x\n", fatevent); > > - /* By now the IRQ should be acked and deasserted so enable it again */ > - enable_irq(ab3100->i2c_client->irq); > - return; > + return IRQ_HANDLED; > > - err_event_wq: > + err_event: > dev_dbg(ab3100->dev, > - "error in event workqueue\n"); > - /* Enable the IRQ anyway, what choice do we have? */ > - enable_irq(ab3100->i2c_client->irq); > - return; > -} > - > -static irqreturn_t ab3100_irq_handler(int irq, void *data) > -{ > - struct ab3100 *ab3100 = data; > - /* > - * Disable the IRQ and dispatch a worker to handle the > - * event. Since the chip resides on I2C this is slow > - * stuff and we will re-enable the interrupts once th > - * worker has finished. > - */ > - disable_irq_nosync(irq); > - schedule_work(&ab3100->work); > + "error reading event status\n"); > return IRQ_HANDLED; > } > > @@ -904,12 +889,10 @@ static int __init ab3100_probe(struct i2c_client *client, > if (err) > goto exit_no_setup; > > - INIT_WORK(&ab3100->work, ab3100_work); > - > /* This real unpredictable IRQ is of course sampled for entropy */ > - err = request_irq(client->irq, ab3100_irq_handler, > - IRQF_DISABLED | IRQF_SAMPLE_RANDOM, > - "AB3100 IRQ", ab3100); > + err = request_threaded_irq(client->irq, NULL, ab3100_irq_handler, > + IRQF_ONESHOT | IRQF_SAMPLE_RANDOM, > + "ab3100-core", ab3100); > if (err) > goto exit_no_irq; > > diff --git a/include/linux/mfd/ab3100.h b/include/linux/mfd/ab3100.h > index e9aa4c9..9a881c3 100644 > --- a/include/linux/mfd/ab3100.h > +++ b/include/linux/mfd/ab3100.h > @@ -6,7 +6,6 @@ > */ > > #include <linux/device.h> > -#include <linux/workqueue.h> > #include <linux/regulator/machine.h> > > #ifndef MFD_AB3100_H > @@ -74,7 +73,6 @@ > * @testreg_client: secondary client for test registers > * @chip_name: name of this chip variant > * @chip_id: 8 bit chip ID for this chip variant > - * @work: an event handling worker > * @event_subscribers: event subscribers are listed here > * @startup_events: a copy of the first reading of the event registers > * @startup_events_read: whether the first events have been read > @@ -90,7 +88,6 @@ struct ab3100 { > struct i2c_client *testreg_client; > char chip_name[32]; > u8 chip_id; > - struct work_struct work; > struct blocking_notifier_head event_subscribers; > u32 startup_events; > bool startup_events_read; > -- > 1.6.3.3 > -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Samuel Ortiz on 8 Jan 2010 04:50 On Fri, Jan 08, 2010 at 10:33:03AM +0100, Samuel Ortiz wrote: > Hi Linus, > > On Fri, Jan 08, 2010 at 12:08:45AM +0100, Linus Walleij wrote: > > This converts the AB3100 core MFD driver to use a threaded > > interrupt handler instead of the explicit top/bottom-half > > construction with a workqueue. This saves some code and make it > > more similar to other modern MFD drivers. > patch applied, many thanks. Linus, I fogot to mention that I removed IRQF_SAMPLE_RANDOM from your request_threaded_irq() arguments, as it is scheduled for removal. See Documentation/feature-removal-schedule.txt. Please let me know if that's a problem for you. Cheers, Samuel. > Cheers, > Samuel. > > > > Signed-off-by: Linus Walleij <linus.walleij(a)stericsson.com> > > --- > > drivers/mfd/ab3100-core.c | 43 +++++++++++++------------------------------ > > include/linux/mfd/ab3100.h | 3 --- > > 2 files changed, 13 insertions(+), 33 deletions(-) > > > > diff --git a/drivers/mfd/ab3100-core.c b/drivers/mfd/ab3100-core.c > > index fd42a80..93b2c14 100644 > > --- a/drivers/mfd/ab3100-core.c > > +++ b/drivers/mfd/ab3100-core.c > > @@ -365,10 +365,13 @@ int ab3100_event_registers_startup_state_get(struct ab3100 *ab3100, > > } > > EXPORT_SYMBOL(ab3100_event_registers_startup_state_get); > > > > -/* Interrupt handling worker */ > > -static void ab3100_work(struct work_struct *work) > > +/* > > + * This is a threaded interrupt handler so we can make some > > + * I2C calls etc. > > + */ > > +static irqreturn_t ab3100_irq_handler(int irq, void *data) > > { > > - struct ab3100 *ab3100 = container_of(work, struct ab3100, work); > > + struct ab3100 *ab3100 = data; > > u8 event_regs[3]; > > u32 fatevent; > > int err; > > @@ -376,7 +379,7 @@ static void ab3100_work(struct work_struct *work) > > err = ab3100_get_register_page_interruptible(ab3100, AB3100_EVENTA1, > > event_regs, 3); > > if (err) > > - goto err_event_wq; > > + goto err_event; > > > > fatevent = (event_regs[0] << 16) | > > (event_regs[1] << 8) | > > @@ -398,29 +401,11 @@ static void ab3100_work(struct work_struct *work) > > dev_dbg(ab3100->dev, > > "IRQ Event: 0x%08x\n", fatevent); > > > > - /* By now the IRQ should be acked and deasserted so enable it again */ > > - enable_irq(ab3100->i2c_client->irq); > > - return; > > + return IRQ_HANDLED; > > > > - err_event_wq: > > + err_event: > > dev_dbg(ab3100->dev, > > - "error in event workqueue\n"); > > - /* Enable the IRQ anyway, what choice do we have? */ > > - enable_irq(ab3100->i2c_client->irq); > > - return; > > -} > > - > > -static irqreturn_t ab3100_irq_handler(int irq, void *data) > > -{ > > - struct ab3100 *ab3100 = data; > > - /* > > - * Disable the IRQ and dispatch a worker to handle the > > - * event. Since the chip resides on I2C this is slow > > - * stuff and we will re-enable the interrupts once th > > - * worker has finished. > > - */ > > - disable_irq_nosync(irq); > > - schedule_work(&ab3100->work); > > + "error reading event status\n"); > > return IRQ_HANDLED; > > } > > > > @@ -904,12 +889,10 @@ static int __init ab3100_probe(struct i2c_client *client, > > if (err) > > goto exit_no_setup; > > > > - INIT_WORK(&ab3100->work, ab3100_work); > > - > > /* This real unpredictable IRQ is of course sampled for entropy */ > > - err = request_irq(client->irq, ab3100_irq_handler, > > - IRQF_DISABLED | IRQF_SAMPLE_RANDOM, > > - "AB3100 IRQ", ab3100); > > + err = request_threaded_irq(client->irq, NULL, ab3100_irq_handler, > > + IRQF_ONESHOT | IRQF_SAMPLE_RANDOM, > > + "ab3100-core", ab3100); > > if (err) > > goto exit_no_irq; > > > > diff --git a/include/linux/mfd/ab3100.h b/include/linux/mfd/ab3100.h > > index e9aa4c9..9a881c3 100644 > > --- a/include/linux/mfd/ab3100.h > > +++ b/include/linux/mfd/ab3100.h > > @@ -6,7 +6,6 @@ > > */ > > > > #include <linux/device.h> > > -#include <linux/workqueue.h> > > #include <linux/regulator/machine.h> > > > > #ifndef MFD_AB3100_H > > @@ -74,7 +73,6 @@ > > * @testreg_client: secondary client for test registers > > * @chip_name: name of this chip variant > > * @chip_id: 8 bit chip ID for this chip variant > > - * @work: an event handling worker > > * @event_subscribers: event subscribers are listed here > > * @startup_events: a copy of the first reading of the event registers > > * @startup_events_read: whether the first events have been read > > @@ -90,7 +88,6 @@ struct ab3100 { > > struct i2c_client *testreg_client; > > char chip_name[32]; > > u8 chip_id; > > - struct work_struct work; > > struct blocking_notifier_head event_subscribers; > > u32 startup_events; > > bool startup_events_read; > > -- > > 1.6.3.3 > > > > -- > Intel Open Source Technology Centre > http://oss.intel.com/ -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Linus Walleij on 8 Jan 2010 05:10 2010/1/8 Samuel Ortiz <sameo(a)linux.intel.com>: > Linus, I fogot to mention that I removed IRQF_SAMPLE_RANDOM from your > request_threaded_irq() arguments, as it is scheduled for removal. See > Documentation/feature-removal-schedule.txt. > Please let me know if that's a problem for you. I got the warning from checkpatch as well, and since this is an interrupt source which is truly random and we really *want* to sample for some IRQ randomness I do want that functionality in there. However the fucntions to be used instead, rand_initialize_irq() + add_interrupt_randomness() aren't used anywhere else in the kernel than in the interrupt driver core. But I don't mind being first ... if you upload your git tree to git.kernel.org I'll make a patch to reenable this the right way. (Or I can rewrite the previous patch, your choice.) Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Samuel Ortiz on 8 Jan 2010 06:10
On Fri, Jan 08, 2010 at 11:09:31AM +0100, Linus Walleij wrote: > 2010/1/8 Samuel Ortiz <sameo(a)linux.intel.com>: > > > Linus, I fogot to mention that I removed IRQF_SAMPLE_RANDOM from your > > request_threaded_irq() arguments, as it is scheduled for removal. See > > Documentation/feature-removal-schedule.txt. > > Please let me know if that's a problem for you. > > I got the warning from checkpatch as well, and since this is an > interrupt source which is truly random and we really *want* to sample > for some IRQ randomness I do want that functionality in there. > > However the fucntions to be used instead, > rand_initialize_irq() + add_interrupt_randomness() aren't used > anywhere else in the kernel than in the interrupt driver core. > > But I don't mind being first ... if you upload your git tree to > git.kernel.org I'll make a patch to reenable this the right way. > (Or I can rewrite the previous patch, your choice.) I'll push your patch without IRQF_SAMPLE_RANDOM to my for-next branch. Feel free to send me a patch against it. Cheers, Samuel. > Linus -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |