Re: [PATCH v4] pch_gpio: Support interrupt function

[Grant Likely]

On Tue, Jul 05, 2011 at 12:52:29PM +0900, Tomoya MORINAGA wrote:
> Hi Grant,
> 
> (2011/07/05 1:31), Grant Likely wrote:
> 
> >>+#define PCH_IRQ_BASE		23
> >
> >Yikes!  IRQ ranges should really be dynamically assigned.  Don't hard
> >code an irq base.
> 
> As we discussed before,
> without specifying PCH_IRQ_BASE,
> it seemed IRQ number collision issue occurred.
> Thus, I added PCH_IRQ_BASE.

Then fix the irq collision.  If the core code hands out an irq range
that is in-use by some other driver, then the other driver isn't
reserving it's irqs correctly and it needs to be fixed.  Hacking
around the problem doesn't help anything in the long run.

> >>@@ -202,8 +345,36 @@ static int __devinit pch_gpio_probe(struct pci_dev *pdev,
> >>  		goto err_gpiochip_add;
> >>  	}
> >>
> >>+	irq_base = irq_alloc_descs(-1, PCH_IRQ_BASE, GPIO_NUM_PINS, GFP_KERNEL);
> >>+	if (irq_base<  0) {
> >>+		dev_err(&pdev->dev, "PCH gpio: Failed to get IRQ base num\n");
> >>+		goto err_irq_alloc_descs;
> >>+	}
> >>+	chip->irq_base = irq_base;
> >
> >This looks like it will cause the driver probe to completely fail,
> >even if the GPIO portion of the chip was setup correctly.  I would
> >think that if GPIO works the driver should at least enable that bit
> >even if IRQs are broken.
> 
> Do you mean in case of failing irq_alloc_descs,
> probe() shouldn't return ERROR but SUCCESS. Right ?

That's what I'm suggesting.  I'll leave it up to you whether or not it
is worthwhile though.

g.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/