diff mbox series

[RFC,3/3] gpio: mb86s70: enable ACPI and irqchip support

Message ID 20190425102020.21533-4-ard.biesheuvel@linaro.org
State New
Headers show
Series synquacer: implement ACPI gpio/interrupt support | expand

Commit Message

Ard Biesheuvel April 25, 2019, 10:20 a.m. UTC 
In order to support this GPIO block in combination with an EXIU
interrupt controller on ACPI systems such as Socionext SynQuacer,
add the enumeration boilerplate, and add the EXIU irqchip handling
to the probe path. Also, make the clock handling conditonal on
non-ACPI enumeration, since ACPI handles this internally.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 drivers/gpio/Kconfig        |  4 ++
 drivers/gpio/gpio-mb86s7x.c | 58 +++++++++++++++++---
 2 files changed, 53 insertions(+), 9 deletions(-)

-- 
2.20.1

Comments

Linus Walleij April 25, 2019, 1:23 p.m. UTC | #1 
Hi Ard, thanks for your patch!

Including Mika here as well, he knows how ACPI is supposed to
be wired up with GPIOs.

On Thu, Apr 25, 2019 at 12:20 PM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:

> In order to support this GPIO block in combination with an EXIU

> interrupt controller on ACPI systems such as Socionext SynQuacer,

> add the enumeration boilerplate, and add the EXIU irqchip handling

> to the probe path. Also, make the clock handling conditonal on

> non-ACPI enumeration, since ACPI handles this internally.

>

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

(...)

> +config GPIO_MB86S7X_ACPI

> +       def_bool y

> +       depends on ACPI && ARCH_SYNQUACER


I would say

depends on ACPI
depends on (ARCH_SYNQUACER || COMPILE_TEST)

so we get some compiler coverage.

> +#include <linux/acpi.h>

>  #include <linux/io.h>

>  #include <linux/init.h>

>  #include <linux/clk.h>

> @@ -27,6 +28,8 @@

>  #include <linux/spinlock.h>

>  #include <linux/slab.h>

>

> +#include "gpiolib.h"


Normally not a good sign to dereference gpiolib internals,
but there are some few exceptions. Why do you do this?

> +int exiu_acpi_init(struct platform_device *pdev, struct gpio_chip *gc);


Just merge it all into one file instead of having these crisscrossy calls.
I prefer some minor #ifdef or straight C if (IS_ENABLED()) around
specific code.

> -       return ret;

> +       if (mb86s70_gpio_have_acpi(pdev))

> +               acpi_gpiochip_request_interrupts(&gchip->gc);

> +

> +       return 0;


I guess this is right...

>         struct mb86s70_gpio_chip *gchip = platform_get_drvdata(pdev);

>

> +       if (gchip->gc.irq.domain) {

> +               acpi_gpiochip_free_interrupts(&gchip->gc);

> +               irq_domain_remove(gchip->gc.irq.domain);

> +       }


Mika can possibly comment on the right way to do ACPI bring-up
and take-down.

Overall it looks pretty straightforward to me, but I'd prefer to just
merge the irqchip driver over into the gpio driver, I can't see why
not.

Or is the irqchip used without the GPIO driver sometimes?

Yours,
Linus Walleij
Ard Biesheuvel April 25, 2019, 1:35 p.m. UTC | #2 
On Thu, 25 Apr 2019 at 15:24, Linus Walleij <linus.walleij@linaro.org> wrote:
>

> Hi Ard, thanks for your patch!

>

> Including Mika here as well, he knows how ACPI is supposed to

> be wired up with GPIOs.

>

> On Thu, Apr 25, 2019 at 12:20 PM Ard Biesheuvel

> <ard.biesheuvel@linaro.org> wrote:

>

> > In order to support this GPIO block in combination with an EXIU

> > interrupt controller on ACPI systems such as Socionext SynQuacer,

> > add the enumeration boilerplate, and add the EXIU irqchip handling

> > to the probe path. Also, make the clock handling conditonal on

> > non-ACPI enumeration, since ACPI handles this internally.

> >

> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> (...)

>

> > +config GPIO_MB86S7X_ACPI

> > +       def_bool y

> > +       depends on ACPI && ARCH_SYNQUACER

>

> I would say

>

> depends on ACPI

> depends on (ARCH_SYNQUACER || COMPILE_TEST)

>

> so we get some compiler coverage.

>

> > +#include <linux/acpi.h>

> >  #include <linux/io.h>

> >  #include <linux/init.h>

> >  #include <linux/clk.h>

> > @@ -27,6 +28,8 @@

> >  #include <linux/spinlock.h>

> >  #include <linux/slab.h>

> >

> > +#include "gpiolib.h"

>

> Normally not a good sign to dereference gpiolib internals,

> but there are some few exceptions. Why do you do this?

>


I needed this for acpi_gpiochip_request_interrupts() et al

> > +int exiu_acpi_init(struct platform_device *pdev, struct gpio_chip *gc);

>

> Just merge it all into one file instead of having these crisscrossy calls.

> I prefer some minor #ifdef or straight C if (IS_ENABLED()) around

> specific code.

>


Fair enough, but please see below.

> > -       return ret;

> > +       if (mb86s70_gpio_have_acpi(pdev))

> > +               acpi_gpiochip_request_interrupts(&gchip->gc);

> > +

> > +       return 0;

>

> I guess this is right...

>

> >         struct mb86s70_gpio_chip *gchip = platform_get_drvdata(pdev);

> >

> > +       if (gchip->gc.irq.domain) {

> > +               acpi_gpiochip_free_interrupts(&gchip->gc);

> > +               irq_domain_remove(gchip->gc.irq.domain);

> > +       }

>

> Mika can possibly comment on the right way to do ACPI bring-up

> and take-down.

>

> Overall it looks pretty straightforward to me, but I'd prefer to just

> merge the irqchip driver over into the gpio driver, I can't see why

> not.

>

> Or is the irqchip used without the GPIO driver sometimes?

>


This is where it becomes a bit nasty: the GPIO controller and the EXIU
interrupt controller are two separate IP blocks, but they are
obviously complimentary. *However*, on SynQuacer, the first 4 physical
lines are not shared between the two, and so you could have 4
interrupt lines handled through the EXIU and 4 different GPIO lines
through the GPIO unit.

On DT, we don't care since we model them as two different units. On
ACPI, however, we cannot model interrupt controllers in the same way,
and so I decided to merge them.

Perhaps the solution is to model the EXIU as a pseudo-GPIO block that
only supports interrupts and not ordinary GPIO?
Mika Westerberg April 25, 2019, 2:27 p.m. UTC | #3 
On Thu, Apr 25, 2019 at 03:23:53PM +0200, Linus Walleij wrote:
> > -       return ret;

> > +       if (mb86s70_gpio_have_acpi(pdev))

> > +               acpi_gpiochip_request_interrupts(&gchip->gc);

> > +

> > +       return 0;

> 

> I guess this is right...

> 

> >         struct mb86s70_gpio_chip *gchip = platform_get_drvdata(pdev);

> >

> > +       if (gchip->gc.irq.domain) {

> > +               acpi_gpiochip_free_interrupts(&gchip->gc);

> > +               irq_domain_remove(gchip->gc.irq.domain);

> > +       }

> 

> Mika can possibly comment on the right way to do ACPI bring-up

> and take-down.


Only comment I have is that normally you don't need to call
acpi_gpiochip_request_interrupts() and acpi_gpiochip_free_interrupts()
in GPIO chip drivers itself. The core should take care of this if the
device ACPI description has an _AEI method.
Ard Biesheuvel April 26, 2019, 8:19 a.m. UTC | #4 
On Thu, 25 Apr 2019 at 16:27, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>

> On Thu, Apr 25, 2019 at 03:23:53PM +0200, Linus Walleij wrote:

> > > -       return ret;

> > > +       if (mb86s70_gpio_have_acpi(pdev))

> > > +               acpi_gpiochip_request_interrupts(&gchip->gc);

> > > +

> > > +       return 0;

> >

> > I guess this is right...

> >

> > >         struct mb86s70_gpio_chip *gchip = platform_get_drvdata(pdev);

> > >

> > > +       if (gchip->gc.irq.domain) {

> > > +               acpi_gpiochip_free_interrupts(&gchip->gc);

> > > +               irq_domain_remove(gchip->gc.irq.domain);

> > > +       }

> >

> > Mika can possibly comment on the right way to do ACPI bring-up

> > and take-down.

>

> Only comment I have is that normally you don't need to call

> acpi_gpiochip_request_interrupts() and acpi_gpiochip_free_interrupts()

> in GPIO chip drivers itself. The core should take care of this if the

> device ACPI description has an _AEI method.


Thanks Mika.

The core only takes care of this for nested/cascaded domains. In this
case, the calls are needed, or the ACPI event interrupt don't get
registered.
Mika Westerberg April 26, 2019, 9:15 a.m. UTC | #5 
On Fri, Apr 26, 2019 at 10:19:42AM +0200, Ard Biesheuvel wrote:
> On Thu, 25 Apr 2019 at 16:27, Mika Westerberg

> <mika.westerberg@linux.intel.com> wrote:

> >

> > On Thu, Apr 25, 2019 at 03:23:53PM +0200, Linus Walleij wrote:

> > > > -       return ret;

> > > > +       if (mb86s70_gpio_have_acpi(pdev))

> > > > +               acpi_gpiochip_request_interrupts(&gchip->gc);

> > > > +

> > > > +       return 0;

> > >

> > > I guess this is right...

> > >

> > > >         struct mb86s70_gpio_chip *gchip = platform_get_drvdata(pdev);

> > > >

> > > > +       if (gchip->gc.irq.domain) {

> > > > +               acpi_gpiochip_free_interrupts(&gchip->gc);

> > > > +               irq_domain_remove(gchip->gc.irq.domain);

> > > > +       }

> > >

> > > Mika can possibly comment on the right way to do ACPI bring-up

> > > and take-down.

> >

> > Only comment I have is that normally you don't need to call

> > acpi_gpiochip_request_interrupts() and acpi_gpiochip_free_interrupts()

> > in GPIO chip drivers itself. The core should take care of this if the

> > device ACPI description has an _AEI method.

> 

> Thanks Mika.

> 

> The core only takes care of this for nested/cascaded domains. In this

> case, the calls are needed, or the ACPI event interrupt don't get

> registered.


I see. Then I agree calling those directly in the driver is the right
thing to do.
diff mbox series

Patch

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 3f50526a771f..2c2773ea9627 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -315,6 +315,10 @@  config GPIO_MB86S7X
 	help
 	  Say yes here to support the GPIO controller in Fujitsu MB86S70 SoCs.
 
+config GPIO_MB86S7X_ACPI
+	def_bool y
+	depends on ACPI && ARCH_SYNQUACER
+
 config GPIO_MENZ127
 	tristate "MEN 16Z127 GPIO support"
 	depends on MCB
diff --git a/drivers/gpio/gpio-mb86s7x.c b/drivers/gpio/gpio-mb86s7x.c
index 3134c0d2bfe4..d254783f7e71 100644
--- a/drivers/gpio/gpio-mb86s7x.c
+++ b/drivers/gpio/gpio-mb86s7x.c
@@ -14,6 +14,7 @@ 
  *  GNU General Public License for more details.
  */
 
+#include <linux/acpi.h>
 #include <linux/io.h>
 #include <linux/init.h>
 #include <linux/clk.h>
@@ -27,6 +28,8 @@ 
 #include <linux/spinlock.h>
 #include <linux/slab.h>
 
+#include "gpiolib.h"
+
 /*
  * Only first 8bits of a register correspond to each pin,
  * so there are 4 registers for 32 pins.
@@ -44,6 +47,8 @@  struct mb86s70_gpio_chip {
 	spinlock_t lock;
 };
 
+int exiu_acpi_init(struct platform_device *pdev, struct gpio_chip *gc);
+
 static int mb86s70_gpio_request(struct gpio_chip *gc, unsigned gpio)
 {
 	struct mb86s70_gpio_chip *gchip = gpiochip_get_data(gc);
@@ -143,6 +148,12 @@  static void mb86s70_gpio_set(struct gpio_chip *gc, unsigned gpio, int value)
 	spin_unlock_irqrestore(&gchip->lock, flags);
 }
 
+static bool mb86s70_gpio_have_acpi(struct platform_device *pdev)
+{
+	return IS_ENABLED(CONFIG_GPIO_MB86S7X_ACPI) &&
+	       ACPI_COMPANION(&pdev->dev);
+}
+
 static int mb86s70_gpio_probe(struct platform_device *pdev)
 {
 	struct mb86s70_gpio_chip *gchip;
@@ -160,13 +171,15 @@  static int mb86s70_gpio_probe(struct platform_device *pdev)
 	if (IS_ERR(gchip->base))
 		return PTR_ERR(gchip->base);
 
-	gchip->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(gchip->clk))
-		return PTR_ERR(gchip->clk);
+	if (!mb86s70_gpio_have_acpi(pdev)) {
+		gchip->clk = devm_clk_get(&pdev->dev, NULL);
+		if (IS_ERR(gchip->clk))
+			return PTR_ERR(gchip->clk);
 
-	ret = clk_prepare_enable(gchip->clk);
-	if (ret)
-		return ret;
+		ret = clk_prepare_enable(gchip->clk);
+		if (ret)
+			return ret;
+	}
 
 	spin_lock_init(&gchip->lock);
 
@@ -182,21 +195,39 @@  static int mb86s70_gpio_probe(struct platform_device *pdev)
 	gchip->gc.parent = &pdev->dev;
 	gchip->gc.base = -1;
 
+	if (mb86s70_gpio_have_acpi(pdev)) {
+		ret = exiu_acpi_init(pdev, &gchip->gc);
+		if (ret) {
+			dev_err(&pdev->dev, "couldn't register gpio irqchip\n");
+			return ret;
+		}
+	}
+
 	ret = gpiochip_add_data(&gchip->gc, gchip);
 	if (ret) {
 		dev_err(&pdev->dev, "couldn't register gpio driver\n");
-		clk_disable_unprepare(gchip->clk);
+		if (gchip->clk)
+			clk_disable_unprepare(gchip->clk);
+		return ret;
 	}
 
-	return ret;
+	if (mb86s70_gpio_have_acpi(pdev))
+		acpi_gpiochip_request_interrupts(&gchip->gc);
+
+	return 0;
 }
 
 static int mb86s70_gpio_remove(struct platform_device *pdev)
 {
 	struct mb86s70_gpio_chip *gchip = platform_get_drvdata(pdev);
 
+	if (gchip->gc.irq.domain) {
+		acpi_gpiochip_free_interrupts(&gchip->gc);
+		irq_domain_remove(gchip->gc.irq.domain);
+	}
 	gpiochip_remove(&gchip->gc);
-	clk_disable_unprepare(gchip->clk);
+	if (gchip->clk)
+		clk_disable_unprepare(gchip->clk);
 
 	return 0;
 }
@@ -207,10 +238,19 @@  static const struct of_device_id mb86s70_gpio_dt_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, mb86s70_gpio_dt_ids);
 
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id mb86s70_gpio_acpi_ids[] = {
+	{ "SCX0007" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(acpi, mb86s70_gpio_acpi_ids);
+#endif
+
 static struct platform_driver mb86s70_gpio_driver = {
 	.driver = {
 		.name = "mb86s70-gpio",
 		.of_match_table = mb86s70_gpio_dt_ids,
+		.acpi_match_table = ACPI_PTR(mb86s70_gpio_acpi_ids),
 	},
 	.probe = mb86s70_gpio_probe,
 	.remove = mb86s70_gpio_remove,