> -----Original Message----- > From: 'Mika Westerberg' [mailto:mika.westerb...@linux.intel.com] > Sent: 26 September, 2014 5:18 PM > To: Chang, Rebecca Swee Fun > Cc: Linus Walleij; linux-g...@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH 3/3] gpio: sch: Enable IRQ support for Quark X1000 > > On Fri, Sep 26, 2014 at 09:14:48AM +0000, Chang, Rebecca Swee Fun wrote: > > > > @@ -203,6 +376,17 @@ static int sch_gpio_probe(struct > > > > platform_device > > > *pdev) > > > > pdev->name)) > > > > return -EBUSY; > > > > > > > > + irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > > > > + sch->irq_support = !!irq; > > > > + if (sch->irq_support) { > > > > + sch->irq_num = irq->start; > > > > + if (sch->irq_num < 0) { > > > > + dev_warn(&pdev->dev, > > > > + "failed to obtain irq number for > > > > device\n"); > > > > + sch->irq_support = 0; > > > > + } > > > > + } > > > > + > > > > spin_lock_init(&sch->lock); > > > > sch->iobase = res->start; > > > > sch->chip = sch_gpio_chip; > > > > @@ -251,17 +435,72 @@ static int sch_gpio_probe(struct > > > > platform_device > > > *pdev) > > > > return -ENODEV; > > > > } > > > > > > > > + err = gpiochip_add(&sch->chip); > > > > + if (err < 0) > > > > + goto err_sch_gpio; > > > > + > > > > + if (sch->irq_support) { > > > > + sch->irq_base = irq_alloc_descs(-1, 0, sch->chip.ngpio, > > > > + NUMA_NO_NODE); > > > > + if (sch->irq_base < 0) { > > > > + dev_err(&pdev->dev, > > > > + "failed to add GPIO IRQ descs\n"); > > > > + sch->irq_base = -1; > > > > + goto err_sch_intr_chip; > > > > + } > > > > > > Was there some reason why you can't use gpiochip_irqchip_* helpers here? > > > > I will look into the helpers function and see what I can change here. > > > > > > > > > + > > > > + /* disable interrupts */ > > > > + sch_gpio_irq_disable_all(sch, sch->chip.ngpio); > > > > + > > > > + err = request_irq(sch->irq_num, sch_gpio_irq_handler, > > > > + IRQF_SHARED, KBUILD_MODNAME, sch); > > > > > > This seems weird, typically irqchip drivers don't call request_irq() > > > directly but instead irq_set_chained_handler() or similar. With > > > gpiochip_irqchip_* stuff you don't need even that. > > > > > Regarding this, gpio-sch is actually using shared interrupts and the > > IRQ resources are from ACPI SCI. As per my understanding, resources > > from ACPI SCI might be shared for power management purposes. In this > > case, irq_set_chained_handler() might not be able to use here. What do > > you think? > > I think you are right. And then you can't use gpiochip_irqchip_* helpers > either :- > (
Then I shall submit the changes for the first patch in V2 series for further review. Thanks. Rebecca -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
