RE: [PATCH 3/3] gpio: sch: Enable IRQ support for Quark X1000

["Chang, Rebecca Swee Fun" <rebecca.swee.fun.chang@xxxxxxxxx>]

> -----Original Message-----
> From: 'Mika Westerberg' [mailto:mika.westerberg@xxxxxxxxxxxxxxx]
> Sent: 26 September, 2014 5:18 PM
> To: Chang, Rebecca Swee Fun
> Cc: Linus Walleij; linux-gpio@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> 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-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html