Re: [PATCH 1/2] pinctrl: bcm: nsp: use gpiolib infrastructure for interrupts

[Chris Packham <Chris.Packham@xxxxxxxxxxxxxxxxxxx>]

On Fri, 2019-11-01 at 19:55 -0700, Florian Fainelli wrote:
> 
> On 10/31/2019 6:56 PM, Chris Packham wrote:
> > Use more of the gpiolib infrastructure for handling interrupts. The
> > root interrupt still needs to be handled manually as it is shared with
> > other peripherals on the SoC.
> > 
> > This will allow multiple instances of this driver to be supported and
> > will clean up gracefully on failure thanks to the device managed APIs.
> > 
> > Signed-off-by: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>
> > ---
> 
> Just a couple of comments below:
> 
> [snip]
> 
> > +		irqc->name = dev_name(dev);
> 
> The irq_chip used to be named "gpio-a" now it most likely will contain
> the address.unit-name notation from Device Tree, since this is visible
> in /proc/interrupts one might consider this to be an ABI breakage.
> 

Oops my bad. I'd even been told as much from another patch. Fixed in
v3.

> > +		irqc->irq_ack = nsp_gpio_irq_ack;
> > +		irqc->irq_mask = nsp_gpio_irq_mask;
> > +		irqc->irq_unmask = nsp_gpio_irq_unmask;
> > +		irqc->irq_set_type = nsp_gpio_irq_set_type;
> >  
> > -			irq_set_chip_and_handler(irq, &nsp_gpio_irq_chip,
> > -						 handle_simple_irq);
> > -			irq_set_chip_data(irq, chip);
> > -		}
> > +		val = readl(chip->base + NSP_CHIP_A_INT_MASK);
> > +		val = val | NSP_CHIP_A_GPIO_INT_BIT;
> > +		writel(val, (chip->base + NSP_CHIP_A_INT_MASK));
> >  
> >  		/* Install ISR for this GPIO controller. */
> > -		ret = devm_request_irq(&pdev->dev, irq, nsp_gpio_irq_handler,
> > -				       IRQF_SHARED, "gpio-a", chip);
> > +		ret = devm_request_irq(dev, irq, nsp_gpio_irq_handler,
> > +				       IRQF_SHARED, "gpio-a", &chip->gc);
> >  		if (ret) {
> >  			dev_err(&pdev->dev, "Unable to request IRQ%d: %d\n",
> >  				irq, ret);
> > -			goto err_rm_gpiochip;
> > +			return ret;
> >  		}
> >  
> > -		val = readl(chip->base + NSP_CHIP_A_INT_MASK);
> > -		val = val | NSP_CHIP_A_GPIO_INT_BIT;
> > -		writel(val, (chip->base + NSP_CHIP_A_INT_MASK));
> > +		girq = &chip->gc.irq;
> > +		girq->chip = irqc;
> > +		/* This will let us handle the parent IRQ in the driver */
> > +		girq->parent_handler = NULL;
> > +		girq->num_parents = 0;
> > +		girq->parents = NULL;
> > +		girq->default_type = IRQ_TYPE_NONE;
> > +		girq->handler = handle_simple_irq;
> 
> It might be worth creating a helper that can be called to initialize all
> relevant members to the values that indicate: let me manage the
> interrupt. This would make us more future proof with respect to
> assumptions being made in gpiolib as well as if new fields are added in
> the future. This would be a separate patch obviously.

For now I'll leave this as-is. Linus W was thinking about what an API
for devices requiring shared irqs would look like. If that happens soon
then then this driver could switch to use this API. If it's going to be
a while I'll look at adding a helper as suggested and updating the 3
drivers that I know of that could benefit from it.

> 
> Other than that:
> 
> Reviewed-by: Florian Fainelli <f.fainelli@xxxxxxxxx>