[PATCH 4/8] staging: mt7621-gpio: implement '.irq_[request|release]_resources' functions

Sergio Paracuellos sergio.paracuellos at gmail.com
Sun Jun 10 10:30:00 UTC 2018


On Sun, Jun 10, 2018 at 06:56:21PM +1000, NeilBrown wrote:
> On Sat, Jun 09 2018, Sergio Paracuellos wrote:
> 
> > When implementing custom irqchips it is important to also
> > implement .irq_request_resources() and .irq_release_resources()
> > and make sure these call gpiochip_[un]lock_as_irq().
> > Add those two for this driver. Also store struct device pointer
> > in global state structure to be able to use 'dev_err' with the
> > device from 'mediatek_request_resources' function.
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos at gmail.com>
> > ---
> >  drivers/staging/mt7621-gpio/gpio-mt7621.c | 42 +++++++++++++++++++++++++++----
> >  1 file changed, 37 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> > index 654c9eb..2a1c506 100644
> > --- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
> > +++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> > @@ -40,6 +40,7 @@ struct mtk_gc {
> >  };
> >  
> >  struct mtk_data {
> > +	struct device *dev;
> >  	void __iomem *gpio_membase;
> >  	int gpio_irq;
> >  	struct irq_domain *gpio_irq_domain;
> > @@ -229,12 +230,42 @@ mediatek_gpio_irq_type(struct irq_data *d, unsigned int type)
> >  	return 0;
> >  }
> >  
> > +static int mediatek_irq_reqres(struct irq_data *d)
> > +{
> > +	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
> 
> By the end of the patch set, irq_data_get_irq_chip_data returns a
> 'struct gpio_chip' here.
> You can use container_of to get the 'rg', and you don't need 'bank' at
> all.

Function 'container_of' was a little confused and only
was used in the deleted 'gpio_to_irq' so I prefer to delete it also.
Is a wrong approach to use bank instead?

> 
> The same applies in several places.
> It might not be this patch that introduces the problem, but this was the
> easiest place to point to it.
> 
> Thanks,
> NeilBrown

Best regards,
    Sergio Paracuellos
> 
> 
> > +	int bank = irqd_to_hwirq(d) / MTK_BANK_WIDTH;
> > +	struct mtk_gc *rg = &gpio_data->gc_map[bank];
> > +	struct gpio_chip *gc = &rg->chip;
> > +	int ret;
> > +
> > +	ret = gpiochip_lock_as_irq(gc, irqd_to_hwirq(d));
> > +	if (ret) {
> > +		dev_err(gpio_data->dev, "unable to lock HW IRQ %lu for IRQ\n",
> > +			irqd_to_hwirq(d));
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void mediatek_irq_relres(struct irq_data *d)
> > +{
> > +	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
> > +	int bank = irqd_to_hwirq(d) / MTK_BANK_WIDTH;
> > +	struct mtk_gc *rg = &gpio_data->gc_map[bank];
> > +	struct gpio_chip *gc = &rg->chip;
> > +
> > +	gpiochip_unlock_as_irq(gc, irqd_to_hwirq(d));
> > +}
> > +
> >  static struct irq_chip mediatek_gpio_irq_chip = {
> > -	.name		= "GPIO",
> > -	.irq_unmask	= mediatek_gpio_irq_unmask,
> > -	.irq_mask	= mediatek_gpio_irq_mask,
> > -	.irq_mask_ack	= mediatek_gpio_irq_mask,
> > -	.irq_set_type	= mediatek_gpio_irq_type,
> > +	.name			= "GPIO",
> > +	.irq_unmask		= mediatek_gpio_irq_unmask,
> > +	.irq_mask		= mediatek_gpio_irq_mask,
> > +	.irq_mask_ack		= mediatek_gpio_irq_mask,
> > +	.irq_set_type		= mediatek_gpio_irq_type,
> > +	.irq_request_resources	= mediatek_irq_reqres,
> > +	.irq_release_resources	= mediatek_irq_relres,
> >  };
> >  
> >  static int
> > @@ -282,6 +313,7 @@ mediatek_gpio_probe(struct platform_device *pdev)
> >  			dev_err(&pdev->dev, "irq_domain_add_linear failed\n");
> >  	}
> >  
> > +	gpio_data->dev = &pdev->dev;
> >  	platform_set_drvdata(pdev, gpio_data);
> >  
> >  	for_each_child_of_node(np, bank)
> > -- 
> > 2.7.4




More information about the devel mailing list