[PATCH] Convert the AB3100 driver to use threaded IRH

[PATCH] Convert the AB3100 driver to use threaded IRH

[Linus Walleij]

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.

Signed-off-by: Linus Walleij <linus.walleij@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

Re: [PATCH] Convert the AB3100 driver to use threaded IRH

[Samuel Ortiz]

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@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/

Re: [PATCH] Convert the AB3100 driver to use threaded IRH

[Samuel Ortiz]

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@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/

Re: [PATCH] Convert the AB3100 driver to use threaded IRH

[Linus Walleij]

2010/1/8 Samuel Ortiz <sameo@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

Re: [PATCH] Convert the AB3100 driver to use threaded IRH

[Samuel Ortiz]

On Fri, Jan 08, 2010 at 11:09:31AM +0100, Linus Walleij wrote:
> 2010/1/8 Samuel Ortiz <sameo@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/