2019-05-29 08:33:19

by Chuanhua Han

[permalink] [raw]
Subject: [PATCH 2/3] gpio: mpc8xxx: Use IRQF_SHARED mode to request IRQ

GPIO3 and GPIO4 controllers share one irq number on Layerscape
platform. In the current implementation, only one GPIO controller
can register successfully.

This patch is to allow two controllers to share a single interrupt
number.

Signed-off-by: Zhang Ying-22455 <[email protected]>
Signed-off-by: Chuanhua Han <[email protected]>
---
drivers/gpio/gpio-mpc8xxx.c | 43 ++++++++++++++++++++++++++-----------
1 file changed, 30 insertions(+), 13 deletions(-)

diff --git a/drivers/gpio/gpio-mpc8xxx.c b/drivers/gpio/gpio-mpc8xxx.c
index 555e0e7957d9..63c8586fe5c8 100644
--- a/drivers/gpio/gpio-mpc8xxx.c
+++ b/drivers/gpio/gpio-mpc8xxx.c
@@ -11,6 +11,7 @@

#include <linux/kernel.h>
#include <linux/init.h>
+#include <linux/interrupt.h>
#include <linux/spinlock.h>
#include <linux/io.h>
#include <linux/of.h>
@@ -105,10 +106,9 @@ static int mpc8xxx_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
return -ENXIO;
}

-static void mpc8xxx_gpio_irq_cascade(struct irq_desc *desc)
+static irqreturn_t mpc8xxx_gpio_irq_cascade(int irq, void *dev_id)
{
- struct mpc8xxx_gpio_chip *mpc8xxx_gc = irq_desc_get_handler_data(desc);
- struct irq_chip *chip = irq_desc_get_chip(desc);
+ struct mpc8xxx_gpio_chip *mpc8xxx_gc = dev_id;
struct gpio_chip *gc = &mpc8xxx_gc->gc;
unsigned int mask;

@@ -117,8 +117,7 @@ static void mpc8xxx_gpio_irq_cascade(struct irq_desc *desc)
if (mask)
generic_handle_irq(irq_linear_revmap(mpc8xxx_gc->irq,
32 - ffs(mask)));
- if (chip->irq_eoi)
- chip->irq_eoi(&desc->irq_data);
+ return IRQ_HANDLED;
}

static void mpc8xxx_irq_unmask(struct irq_data *d)
@@ -129,6 +128,9 @@ static void mpc8xxx_irq_unmask(struct irq_data *d)

raw_spin_lock_irqsave(&mpc8xxx_gc->lock, flags);

+ gc->write_reg(mpc8xxx_gc->regs + GPIO_IER,
+ gc->pin2mask(gc, irqd_to_hwirq(d)));
+
gc->write_reg(mpc8xxx_gc->regs + GPIO_IMR,
gc->read_reg(mpc8xxx_gc->regs + GPIO_IMR)
| mpc_pin2mask(irqd_to_hwirq(d)));
@@ -302,21 +304,31 @@ static int mpc8xxx_probe(struct platform_device *pdev)
struct gpio_chip *gc;
const struct mpc8xxx_gpio_devtype *devtype =
of_device_get_match_data(&pdev->dev);
- int ret;
+ int ret, irq;

mpc8xxx_gc = devm_kzalloc(&pdev->dev, sizeof(*mpc8xxx_gc), GFP_KERNEL);
if (!mpc8xxx_gc)
return -ENOMEM;

- platform_set_drvdata(pdev, mpc8xxx_gc);
-
- raw_spin_lock_init(&mpc8xxx_gc->lock);
-
mpc8xxx_gc->regs = of_iomap(np, 0);
if (!mpc8xxx_gc->regs)
return -ENOMEM;

gc = &mpc8xxx_gc->gc;
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0) {
+ dev_err(&pdev->dev, "can't get irq number\n");
+ return irq;
+ }
+
+ mpc8xxx_gc->gc.label = pdev->name;
+ mpc8xxx_gc->gc.owner = THIS_MODULE;
+ mpc8xxx_gc->gc.base = -1;
+ mpc8xxx_gc->gc.ngpio = MPC8XXX_GPIO_PINS;
+
+ platform_set_drvdata(pdev, mpc8xxx_gc);
+
+ raw_spin_lock_init(&mpc8xxx_gc->lock);

if (of_property_read_bool(np, "little-endian")) {
ret = bgpio_init(gc, &pdev->dev, 4,
@@ -364,7 +376,7 @@ static int mpc8xxx_probe(struct platform_device *pdev)
goto err;
}

- mpc8xxx_gc->irqn = irq_of_parse_and_map(np, 0);
+ mpc8xxx_gc->irqn = platform_get_irq(pdev, 0);
if (!mpc8xxx_gc->irqn)
return 0;

@@ -378,8 +390,13 @@ static int mpc8xxx_probe(struct platform_device *pdev)
gc->write_reg(mpc8xxx_gc->regs + GPIO_IMR, 0xffffffff);
gc->write_reg(mpc8xxx_gc->regs + GPIO_ICR2, 0xffffffff);

- irq_set_chained_handler_and_data(mpc8xxx_gc->irqn,
- mpc8xxx_gpio_irq_cascade, mpc8xxx_gc);
+ /* Request IRQ */
+ ret = devm_request_irq(&pdev->dev, irq, mpc8xxx_gpio_irq_cascade,
+ IRQF_SHARED, pdev->name, mpc8xxx_gc);
+ if (ret) {
+ dev_err(&pdev->dev, "can't claim irq %d\n", mpc8xxx_gc->irqn);
+ goto err;
+ }
return 0;
err:
iounmap(mpc8xxx_gc->regs);
--
2.17.1


2019-05-29 22:17:27

by Leo Li

[permalink] [raw]
Subject: Re: [PATCH 2/3] gpio: mpc8xxx: Use IRQF_SHARED mode to request IRQ

On Wed, May 29, 2019 at 3:33 AM Chuanhua Han <[email protected]> wrote:
>
> GPIO3 and GPIO4 controllers share one irq number on Layerscape
> platform. In the current implementation, only one GPIO controller
> can register successfully.
>
> This patch is to allow two controllers to share a single interrupt
> number.

This patch definitely did more than setting the IRQF_SHARED flag for
interrupt. If the driver do need some cleanup please separate the
cleanup into another patch.

>
> Signed-off-by: Zhang Ying-22455 <[email protected]>
> Signed-off-by: Chuanhua Han <[email protected]>
> ---
> drivers/gpio/gpio-mpc8xxx.c | 43 ++++++++++++++++++++++++++-----------
> 1 file changed, 30 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpio/gpio-mpc8xxx.c b/drivers/gpio/gpio-mpc8xxx.c
> index 555e0e7957d9..63c8586fe5c8 100644
> --- a/drivers/gpio/gpio-mpc8xxx.c
> +++ b/drivers/gpio/gpio-mpc8xxx.c
> @@ -11,6 +11,7 @@
>
> #include <linux/kernel.h>
> #include <linux/init.h>
> +#include <linux/interrupt.h>
> #include <linux/spinlock.h>
> #include <linux/io.h>
> #include <linux/of.h>
> @@ -105,10 +106,9 @@ static int mpc8xxx_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
> return -ENXIO;
> }
>
> -static void mpc8xxx_gpio_irq_cascade(struct irq_desc *desc)
> +static irqreturn_t mpc8xxx_gpio_irq_cascade(int irq, void *dev_id)
> {
> - struct mpc8xxx_gpio_chip *mpc8xxx_gc = irq_desc_get_handler_data(desc);
> - struct irq_chip *chip = irq_desc_get_chip(desc);
> + struct mpc8xxx_gpio_chip *mpc8xxx_gc = dev_id;
> struct gpio_chip *gc = &mpc8xxx_gc->gc;
> unsigned int mask;
>
> @@ -117,8 +117,7 @@ static void mpc8xxx_gpio_irq_cascade(struct irq_desc *desc)
> if (mask)
> generic_handle_irq(irq_linear_revmap(mpc8xxx_gc->irq,
> 32 - ffs(mask)));
> - if (chip->irq_eoi)
> - chip->irq_eoi(&desc->irq_data);
> + return IRQ_HANDLED;
> }
>
> static void mpc8xxx_irq_unmask(struct irq_data *d)
> @@ -129,6 +128,9 @@ static void mpc8xxx_irq_unmask(struct irq_data *d)
>
> raw_spin_lock_irqsave(&mpc8xxx_gc->lock, flags);
>
> + gc->write_reg(mpc8xxx_gc->regs + GPIO_IER,
> + gc->pin2mask(gc, irqd_to_hwirq(d)));

This API has been removed for a while, are you sure you compiled and
tested the change on latest kernel?

commit 24efd94bc38290dc1d9775a1e767ed4685d8a79b
Author: Linus Walleij <[email protected]>
Date: Fri Oct 20 16:31:27 2017 +0200

gpio: mmio: Make pin2mask() a private business

The vtable call pin2mask() was introducing a vtable function call
in every gpiochip callback for a generic MMIO GPIO chip. This was
not exactly efficient. (Maybe link-time optimization could get rid of
it, I don't know.)

After removing all external calls into this API we can make it a
boolean flag in the struct gpio_chip call and sink the function into
the gpio-mmio driver yielding encapsulation and potential speedups.

Cc: Anton Vorontsov <[email protected]>
Signed-off-by: Linus Walleij <[email protected]>

> +
> gc->write_reg(mpc8xxx_gc->regs + GPIO_IMR,
> gc->read_reg(mpc8xxx_gc->regs + GPIO_IMR)
> | mpc_pin2mask(irqd_to_hwirq(d)));
> @@ -302,21 +304,31 @@ static int mpc8xxx_probe(struct platform_device *pdev)
> struct gpio_chip *gc;
> const struct mpc8xxx_gpio_devtype *devtype =
> of_device_get_match_data(&pdev->dev);
> - int ret;
> + int ret, irq;
>
> mpc8xxx_gc = devm_kzalloc(&pdev->dev, sizeof(*mpc8xxx_gc), GFP_KERNEL);
> if (!mpc8xxx_gc)
> return -ENOMEM;
>
> - platform_set_drvdata(pdev, mpc8xxx_gc);
> -
> - raw_spin_lock_init(&mpc8xxx_gc->lock);
> -
> mpc8xxx_gc->regs = of_iomap(np, 0);
> if (!mpc8xxx_gc->regs)
> return -ENOMEM;
>
> gc = &mpc8xxx_gc->gc;
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + dev_err(&pdev->dev, "can't get irq number\n");
> + return irq;
> + }
> +
> + mpc8xxx_gc->gc.label = pdev->name;
> + mpc8xxx_gc->gc.owner = THIS_MODULE;
> + mpc8xxx_gc->gc.base = -1;
> + mpc8xxx_gc->gc.ngpio = MPC8XXX_GPIO_PINS;
> +
> + platform_set_drvdata(pdev, mpc8xxx_gc);
> +
> + raw_spin_lock_init(&mpc8xxx_gc->lock);
>
> if (of_property_read_bool(np, "little-endian")) {
> ret = bgpio_init(gc, &pdev->dev, 4,
> @@ -364,7 +376,7 @@ static int mpc8xxx_probe(struct platform_device *pdev)
> goto err;
> }
>
> - mpc8xxx_gc->irqn = irq_of_parse_and_map(np, 0);
> + mpc8xxx_gc->irqn = platform_get_irq(pdev, 0);
> if (!mpc8xxx_gc->irqn)
> return 0;
>
> @@ -378,8 +390,13 @@ static int mpc8xxx_probe(struct platform_device *pdev)
> gc->write_reg(mpc8xxx_gc->regs + GPIO_IMR, 0xffffffff);
> gc->write_reg(mpc8xxx_gc->regs + GPIO_ICR2, 0xffffffff);
>
> - irq_set_chained_handler_and_data(mpc8xxx_gc->irqn,
> - mpc8xxx_gpio_irq_cascade, mpc8xxx_gc);
> + /* Request IRQ */
> + ret = devm_request_irq(&pdev->dev, irq, mpc8xxx_gpio_irq_cascade,
> + IRQF_SHARED, pdev->name, mpc8xxx_gc);
> + if (ret) {
> + dev_err(&pdev->dev, "can't claim irq %d\n", mpc8xxx_gc->irqn);
> + goto err;
> + }
> return 0;
> err:
> iounmap(mpc8xxx_gc->regs);
> --
> 2.17.1
>