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

["Westerberg, Mika" <mika.westerberg@xxxxxxxxx>]

On Wed, Nov 05, 2014 at 11:33:01AM +0200, Chang, Rebecca Swee Fun wrote:
> 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.

Sounds good.

> > 
> > > +}
> > > +
> > > +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.

I think you can drop that check. Also if 'd' would be NULL you will
crash already in

	struct sch_gpio *sch = container_of(d, struct sch_gpio, data);

> (...)
> > > +
> > > +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.

I see.

In that case, how about following?

	irq = platform_get_irq(pdev, 0)
	if (irq >= 0) {
		/* Add irq support here */
	}

> 
> > 
> > > +			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