diff mbox series

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

Message ID 20191101015621.12451-2-chris.packham@alliedtelesis.co.nz (mailing list archive)
State New, archived
Headers show
Series pinctrl: bcm: nsp: gpio improvements | expand

Commit Message

Chris Packham Nov. 1, 2019, 1:56 a.m. UTC 
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@alliedtelesis.co.nz>
---

Notes:
    Changes in v2:
    - none

 drivers/pinctrl/bcm/pinctrl-nsp-gpio.c | 105 ++++++++++---------------
 1 file changed, 42 insertions(+), 63 deletions(-)

Comments

Florian Fainelli Nov. 2, 2019, 2:55 a.m. UTC | #1 
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@alliedtelesis.co.nz>
> ---

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.

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

Other than that:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Chris Packham Nov. 3, 2019, 9:17 p.m. UTC | #2 
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@alliedtelesis.co.nz>
> > ---
> 
> 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@gmail.com>
Linus Walleij Nov. 3, 2019, 10:23 p.m. UTC | #3 
On Sat, Nov 2, 2019 at 3:55 AM Florian Fainelli <f.fainelli@gmail.com> wrote:

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

I have some different plans for this, but first I want to pull all
struct gpiolib_irq_chip *girq setup over to the new API,
so I can get rid of the old helper functions.

First chained variants, when that is done, threaded variants,
when that is done abstract this type that is using its own
parent handler and then eventually delete the old helper
functions.

Then I can think about adding new helper functions :D

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c b/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c
index e67ae52023ad..cf77c6fe9f9c 100644
--- a/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c
+++ b/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c
@@ -64,17 +64,16 @@ 
  * @gc: GPIO chip
  * @pctl: pointer to pinctrl_dev
  * @pctldesc: pinctrl descriptor
- * @irq_domain: pointer to irq domain
  * @lock: lock to protect access to I/O registers
  */
 struct nsp_gpio {
 	struct device *dev;
 	void __iomem *base;
 	void __iomem *io_ctrl;
+	struct irq_chip irqchip;
 	struct gpio_chip gc;
 	struct pinctrl_dev *pctl;
 	struct pinctrl_desc pctldesc;
-	struct irq_domain *irq_domain;
 	raw_spinlock_t lock;
 };
 
@@ -136,8 +135,8 @@  static inline bool nsp_get_bit(struct nsp_gpio *chip, enum base_type address,
 
 static irqreturn_t nsp_gpio_irq_handler(int irq, void *data)
 {
-	struct nsp_gpio *chip = (struct nsp_gpio *)data;
-	struct gpio_chip gc = chip->gc;
+	struct gpio_chip *gc = (struct gpio_chip *)data;
+	struct nsp_gpio *chip = gpiochip_get_data(gc);
 	int bit;
 	unsigned long int_bits = 0;
 	u32 int_status;
@@ -155,14 +154,14 @@  static irqreturn_t nsp_gpio_irq_handler(int irq, void *data)
 		level &= readl(chip->base + NSP_GPIO_INT_MASK);
 		int_bits = level | event;
 
-		for_each_set_bit(bit, &int_bits, gc.ngpio) {
+		for_each_set_bit(bit, &int_bits, gc->ngpio) {
 			/*
 			 * Clear the interrupt before invoking the
 			 * handler, so we do not leave any window
 			 */
 			writel(BIT(bit), chip->base + NSP_GPIO_EVENT);
 			generic_handle_irq(
-				irq_linear_revmap(chip->irq_domain, bit));
+				irq_linear_revmap(gc->irq.domain, bit));
 		}
 	}
 
@@ -171,7 +170,8 @@  static irqreturn_t nsp_gpio_irq_handler(int irq, void *data)
 
 static void nsp_gpio_irq_ack(struct irq_data *d)
 {
-	struct nsp_gpio *chip = irq_data_get_irq_chip_data(d);
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct nsp_gpio *chip = gpiochip_get_data(gc);
 	unsigned gpio = d->hwirq;
 	u32 val = BIT(gpio);
 	u32 trigger_type;
@@ -189,7 +189,8 @@  static void nsp_gpio_irq_ack(struct irq_data *d)
  */
 static void nsp_gpio_irq_set_mask(struct irq_data *d, bool unmask)
 {
-	struct nsp_gpio *chip = irq_data_get_irq_chip_data(d);
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct nsp_gpio *chip = gpiochip_get_data(gc);
 	unsigned gpio = d->hwirq;
 	u32 trigger_type;
 
@@ -202,7 +203,8 @@  static void nsp_gpio_irq_set_mask(struct irq_data *d, bool unmask)
 
 static void nsp_gpio_irq_mask(struct irq_data *d)
 {
-	struct nsp_gpio *chip = irq_data_get_irq_chip_data(d);
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct nsp_gpio *chip = gpiochip_get_data(gc);
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&chip->lock, flags);
@@ -212,7 +214,8 @@  static void nsp_gpio_irq_mask(struct irq_data *d)
 
 static void nsp_gpio_irq_unmask(struct irq_data *d)
 {
-	struct nsp_gpio *chip = irq_data_get_irq_chip_data(d);
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct nsp_gpio *chip = gpiochip_get_data(gc);
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&chip->lock, flags);
@@ -222,7 +225,8 @@  static void nsp_gpio_irq_unmask(struct irq_data *d)
 
 static int nsp_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 {
-	struct nsp_gpio *chip = irq_data_get_irq_chip_data(d);
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct nsp_gpio *chip = gpiochip_get_data(gc);
 	unsigned gpio = d->hwirq;
 	bool level_low;
 	bool falling;
@@ -265,16 +269,6 @@  static int nsp_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 	return 0;
 }
 
-static struct irq_chip nsp_gpio_irq_chip = {
-	.name = "gpio-a",
-	.irq_enable = nsp_gpio_irq_unmask,
-	.irq_disable = nsp_gpio_irq_mask,
-	.irq_ack = nsp_gpio_irq_ack,
-	.irq_mask = nsp_gpio_irq_mask,
-	.irq_unmask = nsp_gpio_irq_unmask,
-	.irq_set_type = nsp_gpio_irq_set_type,
-};
-
 static int nsp_gpio_direction_input(struct gpio_chip *gc, unsigned gpio)
 {
 	struct nsp_gpio *chip = gpiochip_get_data(gc);
@@ -322,13 +316,6 @@  static int nsp_gpio_get(struct gpio_chip *gc, unsigned gpio)
 	return !!(readl(chip->base + NSP_GPIO_DATA_IN) & BIT(gpio));
 }
 
-static int nsp_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
-{
-	struct nsp_gpio *chip = gpiochip_get_data(gc);
-
-	return irq_linear_revmap(chip->irq_domain, offset);
-}
-
 static int nsp_get_groups_count(struct pinctrl_dev *pctldev)
 {
 	return 1;
@@ -613,10 +600,9 @@  static const struct of_device_id nsp_gpio_of_match[] = {
 static int nsp_gpio_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct resource *res;
 	struct nsp_gpio *chip;
 	struct gpio_chip *gc;
-	u32 val, count;
+	u32 val;
 	int irq, ret;
 
 	if (of_property_read_u32(pdev->dev.of_node, "ngpios", &val)) {
@@ -631,15 +617,13 @@  static int nsp_gpio_probe(struct platform_device *pdev)
 	chip->dev = dev;
 	platform_set_drvdata(pdev, chip);
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	chip->base = devm_ioremap_resource(dev, res);
+	chip->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(chip->base)) {
 		dev_err(dev, "unable to map I/O memory\n");
 		return PTR_ERR(chip->base);
 	}
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-	chip->io_ctrl = devm_ioremap_resource(dev, res);
+	chip->io_ctrl = devm_platform_ioremap_resource(pdev, 1);
 	if (IS_ERR(chip->io_ctrl)) {
 		dev_err(dev, "unable to map I/O memory\n");
 		return PTR_ERR(chip->io_ctrl);
@@ -659,44 +643,44 @@  static int nsp_gpio_probe(struct platform_device *pdev)
 	gc->direction_output = nsp_gpio_direction_output;
 	gc->set = nsp_gpio_set;
 	gc->get = nsp_gpio_get;
-	gc->to_irq = nsp_gpio_to_irq;
 
 	/* optional GPIO interrupt support */
 	irq = platform_get_irq(pdev, 0);
 	if (irq > 0) {
-		/* Create irq domain so that each pin can be assigned an IRQ.*/
-		chip->irq_domain = irq_domain_add_linear(gc->of_node, gc->ngpio,
-							 &irq_domain_simple_ops,
-							 chip);
-		if (!chip->irq_domain) {
-			dev_err(&pdev->dev, "Couldn't allocate IRQ domain\n");
-			return -ENXIO;
-		}
+		struct gpio_irq_chip *girq;
+		struct irq_chip *irqc;
 
-		/* Map each gpio to an IRQ and set the handler for gpiolib. */
-		for (count = 0; count < gc->ngpio; count++) {
-			int irq = irq_create_mapping(chip->irq_domain, count);
+		irqc = &chip->irqchip;
+		irqc->name = dev_name(dev);
+		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;
 	}
 
-	ret = gpiochip_add_data(gc, chip);
+	ret = devm_gpiochip_add_data(dev, gc, chip);
 	if (ret < 0) {
 		dev_err(dev, "unable to add GPIO chip\n");
 		return ret;
@@ -705,15 +689,10 @@  static int nsp_gpio_probe(struct platform_device *pdev)
 	ret = nsp_gpio_register_pinconf(chip);
 	if (ret) {
 		dev_err(dev, "unable to register pinconf\n");
-		goto err_rm_gpiochip;
+		return ret;
 	}
 
 	return 0;
-
-err_rm_gpiochip:
-	gpiochip_remove(gc);
-
-	return ret;
 }
 
 static struct platform_driver nsp_gpio_driver = {