mfd wm8350: allocate irq descs dynamically
diff mbox

Message ID 1305878365-827-3-git-send-email-s.hauer@pengutronix.de
State New, archived
Headers show

Commit Message

Sascha Hauer May 20, 2011, 7:59 a.m. UTC
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(-)

Comments

Mark Brown May 23, 2011, 10:41 p.m. UTC | #1
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.
Sascha Hauer May 24, 2011, 7:28 a.m. UTC | #2
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
Mark Brown May 24, 2011, 9:46 a.m. UTC | #3
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.
Sascha Hauer May 24, 2011, 11:52 a.m. UTC | #4
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
Mark Brown May 24, 2011, 3:35 p.m. UTC | #5
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.
Sascha Hauer May 25, 2011, 8:13 a.m. UTC | #6
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
Mark Brown May 25, 2011, 9:23 a.m. UTC | #7
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.
Sascha Hauer May 25, 2011, 7:10 p.m. UTC | #8
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

Patch
diff mbox

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;