[PATCH] mfd wm8350: allocate irq descs dynamically

[PATCH] mfd wm8350: allocate irq descs dynamically

[Sascha Hauer]

This allows boards to leave the irq_base field unitialized and
prevents them having to reserve irqs in the platform.
pdata can be optional for irq support now. Without pdata the
driver allocates some free irq range. With pdata and irq_base > 0
the driver allocates exactly the specified irq.
Without pdata the irq defaults to IRQF_TRIGGER_LOW.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mfd/wm8350-irq.c |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/mfd/wm8350-irq.c b/drivers/mfd/wm8350-irq.c
index ed4b22a..8a1fafd 100644
--- a/drivers/mfd/wm8350-irq.c
+++ b/drivers/mfd/wm8350-irq.c
@@ -473,17 +473,13 @@ int wm8350_irq_init(struct wm8350 *wm8350, int irq,
 {
 	int ret, cur_irq, i;
 	int flags = IRQF_ONESHOT;
+	int irq_base = -1;
 
 	if (!irq) {
 		dev_warn(wm8350->dev, "No interrupt support, no core IRQ\n");
 		return 0;
 	}
 
-	if (!pdata || !pdata->irq_base) {
-		dev_warn(wm8350->dev, "No interrupt support, no IRQ base\n");
-		return 0;
-	}
-
 	/* Mask top level interrupts */
 	wm8350_reg_write(wm8350, WM8350_SYSTEM_INTERRUPTS_MASK, 0xFFFF);
 
@@ -502,7 +498,17 @@ int wm8350_irq_init(struct wm8350 *wm8350, int irq,
 	wm8350->chip_irq = irq;
 	wm8350->irq_base = pdata->irq_base;
 
-	if (pdata->irq_high) {
+	if (pdata && pdata->irq_base > 0)
+		irq_base = pdata->irq_base;
+
+	wm8350->irq_base = irq_alloc_descs(irq_base, 0, ARRAY_SIZE(wm8350_irqs), 0);
+	if (wm8350->irq_base < 0) {
+		dev_warn(wm8350->dev, "Allocating irqs failed with %d\n",
+			wm8350->irq_base);
+		return 0;
+	}
+
+	if (pdata && pdata->irq_high) {
 		flags |= IRQF_TRIGGER_HIGH;
 
 		wm8350_set_bits(wm8350, WM8350_SYSTEM_CONTROL_1,
-- 
1.7.5.3

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] mfd wm8350: allocate irq descs dynamically

[Mark Brown]

On Thu, Jun 02, 2011 at 01:45:02PM +0200, Sascha Hauer wrote:
> 
> This allows boards to leave the irq_base field unitialized and
> prevents them having to reserve irqs in the platform.
> pdata can be optional for irq support now. Without pdata the
> driver allocates some free irq range. With pdata and irq_base > 0
> the driver allocates exactly the specified irq.
> Without pdata the irq defaults to IRQF_TRIGGER_LOW.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Re: [PATCH] mfd wm8350: allocate irq descs dynamically

[Mark Brown]

On Thu, Jun 02, 2011 at 12:53:49PM +0100, Mark Brown wrote:
> On Thu, Jun 02, 2011 at 01:45:02PM +0200, Sascha Hauer wrote:
> > 
> > This allows boards to leave the irq_base field unitialized and
> > prevents them having to reserve irqs in the platform.
> > pdata can be optional for irq support now. Without pdata the
> > driver allocates some free irq range. With pdata and irq_base > 0
> > the driver allocates exactly the specified irq.
> > Without pdata the irq defaults to IRQF_TRIGGER_LOW.

> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

> Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Actually I take that back, it looks like the wm831x equivalent patch is
causing some sort of semi-intermittent issue with the IRQ infrastructure
on non-sparse systems so I suspect this patch is also problematic.  The
major symptom is lockups in softirq handling.

Re: [PATCH] mfd wm8350: allocate irq descs dynamically

[Mark Brown]

On Thu, Jun 02, 2011 at 02:37:44PM +0100, Mark Brown wrote:

> Actually I take that back, it looks like the wm831x equivalent patch is
> causing some sort of semi-intermittent issue with the IRQ infrastructure
> on non-sparse systems so I suspect this patch is also problematic.  The
> major symptom is lockups in softirq handling.

I figured this out - it's not an issue with this code so I reinstate my
ack, sorry for the noise!

i.MX: switch to sparse irqs

[Sascha Hauer]

The following series switches the i.MX architecture to sparse
irqs. It allows us to remove some ugly dependencies on irq
number defines in both the boards and the architecture.

Sascha Hauer (9):
      ARM i.MX tzic: do not depend on MXC_INTERNAL_IRQS
      ARM i.MX avic: do not depend on MXC_INTERNAL_IRQS
      ARM i.MX: get rid of wrong MXC_INTERNAL_IRQ usage
      mfd wm8350: allocate irq descs dynamically
      ARM i.MX mx31ads: allocate irqs for expio dynamically
      ARM i.MX 3ds debugboard: allocate irqs dynamically
      ARM i.MX: use sparse irqs
      dma IPU: rework irq handling
      ARM i.MX3: remove now useless ipu platform data from boards

 arch/arm/Kconfig                               |    1 +
 arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c |    6 +-
 arch/arm/mach-imx/mach-armadillo5x0.c          |    6 +-
 arch/arm/mach-imx/mach-mx27_3ds.c              |    2 +-
 arch/arm/mach-imx/mach-mx31_3ds.c              |    6 +-
 arch/arm/mach-imx/mach-mx31ads.c               |   45 ++--
 arch/arm/mach-imx/mach-mx31moboard.c           |    6 +-
 arch/arm/mach-imx/mach-mx35_3ds.c              |    2 +-
 arch/arm/mach-imx/mach-pcm037.c                |    6 +-
 arch/arm/mach-imx/mach-pcm043.c                |    6 +-
 arch/arm/mach-imx/mach-vpr200.c                |    6 +-
 arch/arm/mach-imx/mm-imx31.c                   |    2 +-
 arch/arm/mach-imx/mm-imx35.c                   |    1 +
 arch/arm/mach-imx/mx31lilly-db.c               |    6 +-
 arch/arm/mach-mx5/board-cpuimx51.c             |   12 +-
 arch/arm/mach-mx5/board-mx51_3ds.c             |    2 +-
 arch/arm/mach-mx5/eukrea_mbimx51-baseboard.c   |    3 +-
 arch/arm/plat-mxc/3ds_debugboard.c             |   53 +++--
 arch/arm/plat-mxc/avic.c                       |   18 +-
 arch/arm/plat-mxc/gpio.c                       |    6 +-
 arch/arm/plat-mxc/include/mach/iomux-v1.h      |    3 -
 arch/arm/plat-mxc/include/mach/ipu.h           |    1 -
 arch/arm/plat-mxc/include/mach/irqs.h          |   53 +----
 arch/arm/plat-mxc/tzic.c                       |    4 +-
 drivers/dma/Kconfig                            |   10 -
 drivers/dma/ipu/ipu_idmac.c                    |   41 +---
 drivers/dma/ipu/ipu_intern.h                   |   14 +-
 drivers/dma/ipu/ipu_irq.c                      |  300 +++---------------------
 drivers/mfd/wm8350-irq.c                       |   16 +-
 29 files changed, 166 insertions(+), 471 deletions(-)

[PATCH] mfd wm8350: allocate irq descs dynamically

[Sascha Hauer]

This allows boards to leave the irq_base field unitialized and
prevents them having to reserve irqs in the platform.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mfd/wm8350-irq.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/mfd/wm8350-irq.c b/drivers/mfd/wm8350-irq.c
index ed4b22a..04408a5 100644
--- a/drivers/mfd/wm8350-irq.c
+++ b/drivers/mfd/wm8350-irq.c
@@ -479,8 +479,8 @@ int wm8350_irq_init(struct wm8350 *wm8350, int irq,
 		return 0;
 	}
 
-	if (!pdata || !pdata->irq_base) {
-		dev_warn(wm8350->dev, "No interrupt support, no IRQ base\n");
+	if (!pdata) {
+		dev_warn(wm8350->dev, "No interrupt support, no platform data\n");
 		return 0;
 	}
 
@@ -500,7 +500,17 @@ int wm8350_irq_init(struct wm8350 *wm8350, int irq,
 
 	mutex_init(&wm8350->irq_lock);
 	wm8350->chip_irq = irq;
-	wm8350->irq_base = pdata->irq_base;
+
+	if (!pdata->irq_base) {
+		wm8350->irq_base = irq_alloc_descs(-1, 0, ARRAY_SIZE(wm8350_irqs), 0);
+		if (wm8350->irq_base < 0) {
+			dev_warn(wm8350->dev, "Allocating irqs failed with %d\n",
+				wm8350->irq_base);
+			return 0;
+		}
+	} else {
+		wm8350->irq_base = pdata->irq_base;
+	}
 
 	if (pdata->irq_high) {
 		flags |= IRQF_TRIGGER_HIGH;
-- 
1.7.4.1

[PATCH] mfd wm8350: allocate irq descs dynamically

[Thomas Gleixner]

On Fri, 20 May 2011, Sascha Hauer wrote:
> @@ -500,7 +500,17 @@ int wm8350_irq_init(struct wm8350 *wm8350, int irq,
>  
>  	mutex_init(&wm8350->irq_lock);
>  	wm8350->chip_irq = irq;
> -	wm8350->irq_base = pdata->irq_base;

Keeping this line would save you the else path :)

> +
> +	if (!pdata->irq_base) {
> +		wm8350->irq_base = irq_alloc_descs(-1, 0, ARRAY_SIZE(wm8350_irqs), 0);
> +		if (wm8350->irq_base < 0) {
> +			dev_warn(wm8350->dev, "Allocating irqs failed with %d\n",
> +				wm8350->irq_base);
> +			return 0;

  Hmm, return 0 on failure ?

> +		}
> +	} else {
> +		wm8350->irq_base = pdata->irq_base;
> +	}
>  
>  	if (pdata->irq_high) {
>  		flags |= IRQF_TRIGGER_HIGH;
> -- 
> 1.7.4.1
> 
> 

[PATCH] mfd wm8350: allocate irq descs dynamically

[Sascha Hauer]

On Fri, May 20, 2011 at 02:52:28PM +0200, Thomas Gleixner wrote:
> On Fri, 20 May 2011, Sascha Hauer wrote:
> > @@ -500,7 +500,17 @@ int wm8350_irq_init(struct wm8350 *wm8350, int irq,
> >  
> >  	mutex_init(&wm8350->irq_lock);
> >  	wm8350->chip_irq = irq;
> > -	wm8350->irq_base = pdata->irq_base;
> 
> Keeping this line would save you the else path :)

Right.

> 
> > +
> > +	if (!pdata->irq_base) {
> > +		wm8350->irq_base = irq_alloc_descs(-1, 0, ARRAY_SIZE(wm8350_irqs), 0);
> > +		if (wm8350->irq_base < 0) {
> > +			dev_warn(wm8350->dev, "Allocating irqs failed with %d\n",
> > +				wm8350->irq_base);
> > +			return 0;
> 
>   Hmm, return 0 on failure ?

This function does this elsewhere aswell. The driver continues without
interrupt support in this case.

Looking at the driver again I wonder that it does not call
irq_set_chained_handler but request_threaded_irq for its chained
handler. Is it ok to do so or even prefered?

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] mfd wm8350: allocate irq descs dynamically

[Thomas Gleixner]

On Fri, 20 May 2011, Sascha Hauer wrote:

> On Fri, May 20, 2011 at 02:52:28PM +0200, Thomas Gleixner wrote:
> > On Fri, 20 May 2011, Sascha Hauer wrote:
> > > @@ -500,7 +500,17 @@ int wm8350_irq_init(struct wm8350 *wm8350, int irq,
> > >  
> > >  	mutex_init(&wm8350->irq_lock);
> > >  	wm8350->chip_irq = irq;
> > > -	wm8350->irq_base = pdata->irq_base;
> > 
> > Keeping this line would save you the else path :)
> 
> Right.
> 
> > 
> > > +
> > > +	if (!pdata->irq_base) {
> > > +		wm8350->irq_base = irq_alloc_descs(-1, 0, ARRAY_SIZE(wm8350_irqs), 0);
> > > +		if (wm8350->irq_base < 0) {
> > > +			dev_warn(wm8350->dev, "Allocating irqs failed with %d\n",
> > > +				wm8350->irq_base);
> > > +			return 0;
> > 
> >   Hmm, return 0 on failure ?
> 
> This function does this elsewhere aswell. The driver continues without
> interrupt support in this case.

Fair enough.
 
> Looking at the driver again I wonder that it does not call
> irq_set_chained_handler but request_threaded_irq for its chained
> handler. Is it ok to do so or even prefered?

It's ok and done on purpose. See the comment above the interrupt
handler function:

* This is a threaded IRQ handler so can access I2C/SPI. ...

Thanks,

	tglx

[PATCH] mfd wm8350: allocate irq descs dynamically

[Mark Brown]

On Fri, May 20, 2011 at 03:19:44PM +0200, Thomas Gleixner wrote:
> On Fri, 20 May 2011, Sascha Hauer wrote:
> 
> > On Fri, May 20, 2011 at 02:52:28PM +0200, Thomas Gleixner wrote:
> > > On Fri, 20 May 2011, Sascha Hauer wrote:
> > > > @@ -500,7 +500,17 @@ int wm8350_irq_init(struct wm8350 *wm8350, int irq,

Sascha, you should know to *always* CC maintainers on patches.  Don't
apply this patch, and please engage with the feedback I posted when you
sent an earlier version of this patch.

[PATCH] mfd wm8350: allocate irq descs dynamically

[Sascha Hauer]

Mark,

On Sat, May 21, 2011 at 12:29:47PM +0100, Mark Brown wrote:
> On Fri, May 20, 2011 at 03:19:44PM +0200, Thomas Gleixner wrote:
> > On Fri, 20 May 2011, Sascha Hauer wrote:
> > 
> > > On Fri, May 20, 2011 at 02:52:28PM +0200, Thomas Gleixner wrote:
> > > > On Fri, 20 May 2011, Sascha Hauer wrote:
> > > > > @@ -500,7 +500,17 @@ int wm8350_irq_init(struct wm8350 *wm8350, int irq,
> 
> Sascha, you should know to *always* CC maintainers on patches.  Don't
> apply this patch, and please engage with the feedback I posted when you
> sent an earlier version of this patch.

Sorry for this, should have added you to the cc list of that indivual
patch. I can't find your reply in my mail, could you send it again?

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] mfd wm8350: allocate irq descs dynamically

[Mark Brown]

On Mon, May 23, 2011 at 08:25:01AM +0200, Sascha Hauer wrote:

> Sorry for this, should have added you to the cc list of that indivual
> patch. I can't find your reply in my mail, could you send it again?

Attached.
-------------- next part --------------
An embedded message was scrubbed...
From: Mark Brown <broonie@opensource.wolfsonmicro.com>
Subject: Re: [PATCH] mfd wm8350: allocate irq descs dynamically
Date: Thu, 19 May 2011 14:04:41 -0700
Size: 3293
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110523/ba098eff/attachment-0002.mht>
-------------- next part --------------
An embedded message was scrubbed...
From: Mark Brown <broonie@opensource.wolfsonmicro.com>
Subject: Re: [PATCH] mfd wm8350: allocate irq descs dynamically
Date: Thu, 19 May 2011 23:04:03 +0100
Size: 3470
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110523/ba098eff/attachment-0003.mht>

[PATCH] mfd wm8350: allocate irq descs dynamically

[Sascha Hauer]

> 
> > -	if (!pdata || !pdata->irq_base) {
> > -		dev_warn(wm8350->dev, "No interrupt support, no IRQ base\n");
> > +	if (!pdata) {
> > +		dev_warn(wm8350->dev, "No interrupt support, no platform data\n");
> 
> This isn't terribly good, the only reason we're checking pdata here is
> because we can't dereference it to look up irq_base if it's not there.

I'm not sure what you mean here. Are you suggesting that we should
default to dynamically requested irqs without platform data?
I did it this way because without platform data pdata->irq_high is
unknown.

> 
> > +	if (!pdata->irq_base) {
> > +		wm8350->irq_base = irq_alloc_descs(-1, 0, ARRAY_SIZE(wm8350_irqs), 0);
> > +		if (wm8350->irq_base < 0) {
> 
> One other thing - it doesn't seem to be 100% desirable to making the
> allocation of the IRQ descriptors depend on not specifying a base - for
> many situations we're likely to want to know what the numbers we end up
> with are (eg, for passing to another device) but if we don't call
> irq_alloc_decs() the platform still has to arrange for the descriptors
> to be there in advance.  It feels like the code should always use
> irq_alloc_descs(), though obviously that's not going to work right now.

If that's the case the platform still can provide irq_base and then it
does know in advance. Also, shouldn't this other device be instantiated
from the wm8350 driver?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] mfd wm8350: allocate irq descs dynamically

[Mark Brown]

On Mon, May 23, 2011 at 04:41:26PM +0200, Sascha Hauer wrote:

> > This isn't terribly good, the only reason we're checking pdata here is
> > because we can't dereference it to look up irq_base if it's not there.

> I'm not sure what you mean here. Are you suggesting that we should
> default to dynamically requested irqs without platform data?

Yes, exactly.

> I did it this way because without platform data pdata->irq_high is
> unknown.

With platform data it has a default value, and indeed when the chip
powers on it has a default.

> > One other thing - it doesn't seem to be 100% desirable to making the
> > allocation of the IRQ descriptors depend on not specifying a base - for
> > many situations we're likely to want to know what the numbers we end up
> > with are (eg, for passing to another device) but if we don't call
> > irq_alloc_decs() the platform still has to arrange for the descriptors
> > to be there in advance.  It feels like the code should always use
> > irq_alloc_descs(), though obviously that's not going to work right now.

> If that's the case the platform still can provide irq_base and then it
> does know in advance. Also, shouldn't this other device be instantiated

You're missing my point here.  The platform not only has to allocate the
base number, it also has to do the allocation of the descriptors.  That
seems less than ideal as it means that any platform using the driver
has to replicate the code for allocating the IRQ range that was
assigned.

> from the wm8350 driver?

We normally instantiate drivers following the control bus heirachy, not
the interrupt controller heirachy...

[PATCH] mfd wm8350: allocate irq descs dynamically

[Sascha Hauer]

On Mon, May 23, 2011 at 04:22:48PM +0100, Mark Brown wrote:
> On Mon, May 23, 2011 at 04:41:26PM +0200, Sascha Hauer wrote:
> 
> > > This isn't terribly good, the only reason we're checking pdata here is
> > > because we can't dereference it to look up irq_base if it's not there.
> 
> > I'm not sure what you mean here. Are you suggesting that we should
> > default to dynamically requested irqs without platform data?
> 
> Yes, exactly.
> 
> > I did it this way because without platform data pdata->irq_high is
> > unknown.
> 
> With platform data it has a default value, and indeed when the chip
> powers on it has a default.

Ok, what should be the default for irq_high then if we do not have
platform data?

> 
> > > One other thing - it doesn't seem to be 100% desirable to making the
> > > allocation of the IRQ descriptors depend on not specifying a base - for
> > > many situations we're likely to want to know what the numbers we end up
> > > with are (eg, for passing to another device) but if we don't call
> > > irq_alloc_decs() the platform still has to arrange for the descriptors
> > > to be there in advance.  It feels like the code should always use
> > > irq_alloc_descs(), though obviously that's not going to work right now.
> 
> > If that's the case the platform still can provide irq_base and then it
> > does know in advance. Also, shouldn't this other device be instantiated
> 
> You're missing my point here.  The platform not only has to allocate the
> base number, it also has to do the allocation of the descriptors.  That
> seems less than ideal as it means that any platform using the driver
> has to replicate the code for allocating the IRQ range that was
> assigned.

We can do the irq_alloc_descs unconditionally then. If irq_base is
not given, we are happy with any irq irq_alloc_descs returns. If
irq_base is given, we check the return value of irq_alloc_descs for
exactly irq_base.

> 
> > from the wm8350 driver?
> 
> We normally instantiate drivers following the control bus heirachy, not
> the interrupt controller heirachy...

I assumed that drivers using the irqs from the wm8350 are usually
children of the wm8350, like the watchdog, rtc, regulator drivers already are.
This may not be true for the gpio interrupts, but you can calculate the
irq from the gpio number.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] mfd wm8350: allocate irq descs dynamically

[Mark Brown]

On Mon, May 23, 2011 at 06:46:01PM +0200, Sascha Hauer wrote:
> On Mon, May 23, 2011 at 04:22:48PM +0100, Mark Brown wrote:

> > With platform data it has a default value, and indeed when the chip
> > powers on it has a default.

> Ok, what should be the default for irq_high then if we do not have
> platform data?

Off - the default value for platform data is all zeros.

> > You're missing my point here.  The platform not only has to allocate the
> > base number, it also has to do the allocation of the descriptors.  That
> > seems less than ideal as it means that any platform using the driver
> > has to replicate the code for allocating the IRQ range that was
> > assigned.

> We can do the irq_alloc_descs unconditionally then. If irq_base is
> not given, we are happy with any irq irq_alloc_descs returns. If
> irq_base is given, we check the return value of irq_alloc_descs for
> exactly irq_base.

That's what I'm suggesting, but like I say I'm not convinced that's
actually going to do the right thing currently on non-sparse systems or
on systems that do actually have the IRQ range allocated for some other
reason.

> > We normally instantiate drivers following the control bus heirachy, not
> > the interrupt controller heirachy...

> I assumed that drivers using the irqs from the wm8350 are usually
> children of the wm8350, like the watchdog, rtc, regulator drivers already are.
> This may not be true for the gpio interrupts, but you can calculate the
> irq from the gpio number.

You can only do that if the device is using the GPIO as a GPIO as well
as using it as an interrupt.  If the device is just using the GPIO on
the PMIC as an interrupt then it doesn't help as the driver isn't using
the GPIO API at all.

[PATCH] mfd wm8350: allocate irq descs dynamically

[Sascha Hauer]

On Tue, May 24, 2011 at 06:41:00AM +0800, Mark Brown wrote:
> On Mon, May 23, 2011 at 06:46:01PM +0200, Sascha Hauer wrote:
> > On Mon, May 23, 2011 at 04:22:48PM +0100, Mark Brown wrote:
> 
> > > With platform data it has a default value, and indeed when the chip
> > > powers on it has a default.
> 
> > Ok, what should be the default for irq_high then if we do not have
> > platform data?
> 
> Off - the default value for platform data is all zeros.

Ok.

> 
> > > You're missing my point here.  The platform not only has to allocate the
> > > base number, it also has to do the allocation of the descriptors.  That
> > > seems less than ideal as it means that any platform using the driver
> > > has to replicate the code for allocating the IRQ range that was
> > > assigned.
> 
> > We can do the irq_alloc_descs unconditionally then. If irq_base is
> > not given, we are happy with any irq irq_alloc_descs returns. If
> > irq_base is given, we check the return value of irq_alloc_descs for
> > exactly irq_base.
> 
> That's what I'm suggesting, but like I say I'm not convinced that's
> actually going to do the right thing currently on non-sparse systems or

non-sparse systems handle this correctly.

> on systems that do actually have the IRQ range allocated for some other
> reason.

By 'some other reason' you mean an implementation bug? If the range is
already allocated the wm8350 driver should better not use it.

> 
> > > We normally instantiate drivers following the control bus heirachy, not
> > > the interrupt controller heirachy...
> 
> > I assumed that drivers using the irqs from the wm8350 are usually
> > children of the wm8350, like the watchdog, rtc, regulator drivers already are.
> > This may not be true for the gpio interrupts, but you can calculate the
> > irq from the gpio number.
> 
> You can only do that if the device is using the GPIO as a GPIO as well
> as using it as an interrupt.  If the device is just using the GPIO on
> the PMIC as an interrupt then it doesn't help as the driver isn't using
> the GPIO API at all.

You are right. As we agreed on allocating the irq descs unconditionally
and preserve the possibility for a fixed range passed in from platform
data this shouldn't be a problem. A platform can still use fixed irq
numbers if it wishes to.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] mfd wm8350: allocate irq descs dynamically

[Mark Brown]

On Tue, May 24, 2011 at 09:28:14AM +0200, Sascha Hauer wrote:
> On Tue, May 24, 2011 at 06:41:00AM +0800, Mark Brown wrote:

> > on systems that do actually have the IRQ range allocated for some other
> > reason.

> By 'some other reason' you mean an implementation bug? If the range is
> already allocated the wm8350 driver should better not use it.

Well, one obvious example is if someone's using the device with a sparse
system already - they'll have had to arrange to allocate the IRQ range
already.

[PATCH] mfd wm8350: allocate irq descs dynamically

[Sascha Hauer]

On Tue, May 24, 2011 at 05:46:59PM +0800, Mark Brown wrote:
> On Tue, May 24, 2011 at 09:28:14AM +0200, Sascha Hauer wrote:
> > On Tue, May 24, 2011 at 06:41:00AM +0800, Mark Brown wrote:
> 
> > > on systems that do actually have the IRQ range allocated for some other
> > > reason.
> 
> > By 'some other reason' you mean an implementation bug? If the range is
> > already allocated the wm8350 driver should better not use it.
> 
> Well, one obvious example is if someone's using the device with a sparse
> system already - they'll have had to arrange to allocate the IRQ range
> already.

There seems to be no user in the kernel. Anyway, sooner or later we'll
have the same problem with other mfd drivers. So what do yuo suggest?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] mfd wm8350: allocate irq descs dynamically

[Mark Brown]

On Tue, May 24, 2011 at 01:52:15PM +0200, Sascha Hauer wrote:
> On Tue, May 24, 2011 at 05:46:59PM +0800, Mark Brown wrote:

> > Well, one obvious example is if someone's using the device with a sparse
> > system already - they'll have had to arrange to allocate the IRQ range
> > already.

> There seems to be no user in the kernel. Anyway, sooner or later we'll
> have the same problem with other mfd drivers. So what do yuo suggest?

The other MFD drivers are exactly my issue here, obviously all these
devices have pretty much the same code and we should be doing the same
thing with all of them.  I've got a good idea about users for drivers I
maintain but less so for others.

[PATCH] mfd wm8350: allocate irq descs dynamically

[Sascha Hauer]

On Tue, May 24, 2011 at 11:35:46PM +0800, Mark Brown wrote:
> On Tue, May 24, 2011 at 01:52:15PM +0200, Sascha Hauer wrote:
> > On Tue, May 24, 2011 at 05:46:59PM +0800, Mark Brown wrote:
> 
> > > Well, one obvious example is if someone's using the device with a sparse
> > > system already - they'll have had to arrange to allocate the IRQ range
> > > already.
> 
> > There seems to be no user in the kernel. Anyway, sooner or later we'll
> > have the same problem with other mfd drivers. So what do yuo suggest?
> 
> The other MFD drivers are exactly my issue here, obviously all these
> devices have pretty much the same code and we should be doing the same
> thing with all of them.  I've got a good idea about users for drivers I
> maintain but less so for others.

Ok, how about this?

- Add a ALLOC_DESCS flag to platform data
- Without platform data are descs are dynamically allocated
- With platform data and ALLOC_DESCS not set: no dynamically
  allocated descs, no change at all
- With ALLOC_DESCS set descs are dynamically allocated with:
  - if irq_base is 0, the some range is allocated
  - with irq_base != 0, exactly irq_base + num is allocated

I also wonder if we should spill out a warning when ALLOC_DESCS is not
set. It basically means that we may use some otherwise used resources.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] mfd wm8350: allocate irq descs dynamically

[Mark Brown]

On Wed, May 25, 2011 at 10:13:26AM +0200, Sascha Hauer wrote:

> Ok, how about this?

> - Add a ALLOC_DESCS flag to platform data
> - Without platform data are descs are dynamically allocated
> - With platform data and ALLOC_DESCS not set: no dynamically
>   allocated descs, no change at all
> - With ALLOC_DESCS set descs are dynamically allocated with:
>   - if irq_base is 0, the some range is allocated
>   - with irq_base != 0, exactly irq_base + num is allocated

> I also wonder if we should spill out a warning when ALLOC_DESCS is not
> set. It basically means that we may use some otherwise used resources.

I dunno, that seems painful.  Perhaps we just accept that some boards
will be broken if things go wrong - the whole ALLOC_DESCS thing seems
more pain than it's worth.

[PATCH] mfd wm8350: allocate irq descs dynamically

[Sascha Hauer]

On Wed, May 25, 2011 at 05:23:17PM +0800, Mark Brown wrote:
> On Wed, May 25, 2011 at 10:13:26AM +0200, Sascha Hauer wrote:
> 
> > Ok, how about this?
> 
> > - Add a ALLOC_DESCS flag to platform data
> > - Without platform data are descs are dynamically allocated
> > - With platform data and ALLOC_DESCS not set: no dynamically
> >   allocated descs, no change at all
> > - With ALLOC_DESCS set descs are dynamically allocated with:
> >   - if irq_base is 0, the some range is allocated
> >   - with irq_base != 0, exactly irq_base + num is allocated
> 
> > I also wonder if we should spill out a warning when ALLOC_DESCS is not
> > set. It basically means that we may use some otherwise used resources.
> 
> I dunno, that seems painful.  Perhaps we just accept that some boards
> will be broken if things go wrong - the whole ALLOC_DESCS thing seems
> more pain than it's worth.

Great :)

I'll send an updated patch soon.

Thanks
 Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] mfd wm8350: allocate irq descs dynamically

[Sascha Hauer]

This allows boards to leave the irq_base field unitialized and
prevents them having to reserve irqs in the platform.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mfd/wm8350-irq.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

As this seems to be the first user of irq_alloc_descs outside the core I
added Thomas to cc to check if this is the correct usage of this
function.
Unfortunately I have no hardware to test this. I created this patch
since one of the wm8350 users is the mx31ads board which prevents me
from getting rid of the irq ifdeffery in i.MX land, so it would be nice
if someone could give it a test run.

diff --git a/drivers/mfd/wm8350-irq.c b/drivers/mfd/wm8350-irq.c
index ed4b22a..04408a5 100644
--- a/drivers/mfd/wm8350-irq.c
+++ b/drivers/mfd/wm8350-irq.c
@@ -479,8 +479,8 @@ int wm8350_irq_init(struct wm8350 *wm8350, int irq,
 		return 0;
 	}
 
-	if (!pdata || !pdata->irq_base) {
-		dev_warn(wm8350->dev, "No interrupt support, no IRQ base\n");
+	if (!pdata) {
+		dev_warn(wm8350->dev, "No interrupt support, no platform data\n");
 		return 0;
 	}
 
@@ -500,7 +500,17 @@ int wm8350_irq_init(struct wm8350 *wm8350, int irq,
 
 	mutex_init(&wm8350->irq_lock);
 	wm8350->chip_irq = irq;
-	wm8350->irq_base = pdata->irq_base;
+
+	if (!pdata->irq_base) {
+		wm8350->irq_base = irq_alloc_descs(-1, 0, ARRAY_SIZE(wm8350_irqs), 0);
+		if (wm8350->irq_base < 0) {
+			dev_warn(wm8350->dev, "Allocating irqs failed with %d\n",
+				wm8350->irq_base);
+			return 0;
+		}
+	} else {
+		wm8350->irq_base = pdata->irq_base;
+	}
 
 	if (pdata->irq_high) {
 		flags |= IRQF_TRIGGER_HIGH;
-- 
1.7.4.1

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] mfd wm8350: allocate irq descs dynamically

[Mark Brown]

On Thu, May 19, 2011 at 08:56:48PM +0200, Sascha Hauer wrote:

> -	if (!pdata || !pdata->irq_base) {
> -		dev_warn(wm8350->dev, "No interrupt support, no IRQ base\n");
> +	if (!pdata) {
> +		dev_warn(wm8350->dev, "No interrupt support, no platform data\n");

This isn't terribly good, the only reason we're checking pdata here is
because we can't dereference it to look up irq_base if it's not there.

Re: [PATCH] mfd wm8350: allocate irq descs dynamically

[Mark Brown]

On Thu, May 19, 2011 at 08:56:48PM +0200, Sascha Hauer wrote:

> +	if (!pdata->irq_base) {
> +		wm8350->irq_base = irq_alloc_descs(-1, 0, ARRAY_SIZE(wm8350_irqs), 0);
> +		if (wm8350->irq_base < 0) {

One other thing - it doesn't seem to be 100% desirable to making the
allocation of the IRQ descriptors depend on not specifying a base - for
many situations we're likely to want to know what the numbers we end up
with are (eg, for passing to another device) but if we don't call
irq_alloc_decs() the platform still has to arrange for the descriptors
to be there in advance.  It feels like the code should always use
irq_alloc_descs(), though obviously that's not going to work right now.