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

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

Sorry for the late reply, was working on something else.

> -----Original Message-----
> From: Westerberg, Mika
> Sent: 16 October, 2014 6:19 PM
> To: Chang, Rebecca Swee Fun
> Cc: Linus Walleij; GPIO Subsystem Mailing List; Linux Kernel Mailing List; Denis
> Turischev
> Subject: Re: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000
> 
> On Thu, Oct 16, 2014 at 11:51:15AM +0800, Chang Rebecca Swee Fun wrote:
> > Intel Quark X1000 GPIO controller supports interrupt handling for both
> > core power well and resume power well. This patch is to enable the IRQ
> > support and provide IRQ handling for Intel Quark X1000 GPIO-SCH device
> > driver.
> >
> > This piece of work is derived from Dan O'Donovan's initial work for
> > Quark X1000 enabling.
> >
> > Signed-off-by: Chang Rebecca Swee Fun
> > <rebecca.swee.fun.chang@xxxxxxxxx>
> 
> In addition to what Varka Bhadram pointed out, I have few comments. See
> below.
> 
> Overall, looks good.
> 
> > ---
> >  drivers/gpio/gpio-sch.c |  257
> > ++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 245 insertions(+), 12 deletions(-)
> >
(...)

> > +static void sch_gpio_irq_disable(struct irq_data *d) {
> > +	struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
> > +	u32 gpio_num;
> > +
> > +	gpio_num = d->irq - sch->irq_base;
> > +	sch_gpio_register_clear(sch, gpio_num, GGPE); }
> > +
> > +static void sch_gpio_irq_ack(struct irq_data *d) {
> > +	struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
> > +	u32 gpio_num;
> > +
> > +	gpio_num = d->irq - sch->irq_base;
> > +	sch_gpio_reg_set(&(sch->chip), gpio_num, GTS, 1);
> 
> Maybe use the new sch_gpio_register_set() here instead?

According to Alexandre's review feedback, sch_gpio_register_set() and  sch_gpio_register_clear() actually having similar block of codes with sch_gpio_reg_set(). He suggested to call sch_gpio_reg_set(gc, gpio, reg, 1) in place of sch_gpio_register_set() and sch_gpio_reg_set(gc, gpio, reg, 0) for sch_gpio_register_clear(). I'm now in the progress of reworking the patches in that direction.

> 
> > +}
> > +
> > +static int sch_gpio_irq_type(struct irq_data *d, unsigned type) {
> > +	struct sch_gpio *sch = container_of(d, struct sch_gpio, data);
> > +	unsigned long flags;
> > +	u32 gpio_num;
> > +
> > +	if (d == NULL)
> > +		return -EINVAL;
> 
> How can that happen?

This is just to ensure the irq_data struct contains data. If you think this is no needed, I will remove it in next submission.
(...)
> > +
> > +static void sch_gpio_irqs_init(struct sch_gpio *sch, unsigned int
> > +num) {
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < num; i++) {
> > +		irq_set_chip_data(i + sch->irq_base, sch);
> > +		irq_set_chip_and_handler_name(i + sch->irq_base,
> > +					      &sch_irq_chip,
> > +					      handle_simple_irq,
> > +					      "sch_gpio_irq_chip");
> 
> Hmm, I wonder if this
> 
> 		irq_set_chip_and_handler_name(i + sch->irq_base,
> &sch_irq_chip,
> 					handle_simple_irq,
> "sch_gpio_irq_chip");
> 
> looks better. Up to you.
> 
I think I will take what you've suggested. :)

(...)
> >  static int sch_gpio_probe(struct platform_device *pdev)  {
> >  	struct sch_gpio *sch;
> > -	struct resource *res;
> > +	struct resource *res, *irq;
> > +	int err;
> >
> >  	sch = devm_kzalloc(&pdev->dev, sizeof(*sch), GFP_KERNEL);
> >  	if (!sch)
> > @@ -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) {
> 
> Is this really possible? I mean can't you just detect the irq support by looking
> sch->irq_num? If it is > 0 then it has interrupt support. I think that irq 0 is not
> valid.

Yes, I can do that, but the irq_num was being passed from MFD LPC_SCH. This implementation is to avoid enabling irq support if LPC_SCH passed in a negative value.
As for irq 0, I think it is valid. Based on my observation in "cat /proc/interrupt" there is a 0 irq line available.

> 
> > +			dev_warn(&pdev->dev,
> > +				 "failed to obtain irq number for device\n");
> > +			sch->irq_support = false;
> > +		}
> > +	}
> > +
> >  	spin_lock_init(&sch->lock);
> >  	sch->iobase = res->start;
> >  	sch->chip = sch_gpio_chip;
> > @@ -251,15 +435,64 @@ 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 allocate GPIO IRQ descs\n");
> > +			goto err_sch_intr_chip;
> > +		}
> > +
> > +		/* 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);
> > +		if (err) {
> > +			dev_err(&pdev->dev,
> > +				"%s failed to request IRQ\n", __func__);
> > +			goto err_sch_request_irq;
> > +		}
> > +
> > +		sch_gpio_irqs_init(sch, sch->chip.ngpio);
> > +	}
> > +
> >  	platform_set_drvdata(pdev, sch);
> >
> > -	return gpiochip_add(&sch->chip);
> > +	return 0;
> > +
> > +err_sch_request_irq:
> > +	irq_free_descs(sch->irq_base, sch->chip.ngpio);
> > +
> > +err_sch_intr_chip:
> > +	if (gpiochip_remove(&sch->chip))
> > +		dev_err(&pdev->dev,
> > +			"%s gpiochip_remove() failed\n", __func__);
> > +
> > +err_sch_gpio:
> > +	release_region(res->start, resource_size(res));
> 
> This is not needed as devm_request_region() will clean it up for you
> automatically. Even if probe() fails.

Noted with 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