[PATCH 0/2] GPIO: hlwd: Implement IRQ support

[PATCH 0/2] GPIO: hlwd: Implement IRQ support

[Jonathan Neuschäfer]

This patchset implements IRQ support for the Hollywood GPIO controller
(Hollywood is one of the chips in the Nintendo Wii game console).

Together with two device tree patches[1], I can use the buttons (via the
gpio-keys driver) and the photoelectric sensor in the disc slot (via
gpio-event-mon).

[1]: https://www.spinics.net/lists/kernel/msg3010459.html

Jonathan Neuschäfer (2):
  gpio: hlwd: Add basic IRQ support
  gpio: hlwd: Implement edge trigger emulation

 drivers/gpio/Kconfig     |   1 +
 drivers/gpio/gpio-hlwd.c | 193 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 193 insertions(+), 1 deletion(-)

-- 
2.20.1

[PATCH 1/2] gpio: hlwd: Add basic IRQ support

[Jonathan Neuschäfer]

This patch implements level-triggered IRQs in the Hollywood GPIO driver.
Edge triggered interrupts are not supported in this GPIO controller, so
I moved their emulation into a separate patch.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---

I'm not entirely sure about the locking, but lockdep doesn't complain.
---
 drivers/gpio/Kconfig     |   1 +
 drivers/gpio/gpio-hlwd.c | 136 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 136 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index b5a2845347ec..f77180dd87bd 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -258,6 +258,7 @@ config GPIO_HLWD
 	tristate "Nintendo Wii (Hollywood) GPIO"
 	depends on OF_GPIO
 	select GPIO_GENERIC
+	select GPIOLIB_IRQCHIP
 	help
 	  Select this to support the GPIO controller of the Nintendo Wii.
 
diff --git a/drivers/gpio/gpio-hlwd.c b/drivers/gpio/gpio-hlwd.c
index a63136a68ba3..9a93546f3d11 100644
--- a/drivers/gpio/gpio-hlwd.c
+++ b/drivers/gpio/gpio-hlwd.c
@@ -48,9 +48,107 @@
 
 struct hlwd_gpio {
 	struct gpio_chip gpioc;
+	struct irq_chip irqc;
 	void __iomem *regs;
+	int irq;
 };
 
+static void hlwd_gpio_irqhandler(struct irq_desc *desc)
+{
+	struct hlwd_gpio *hlwd =
+		gpiochip_get_data(irq_desc_get_handler_data(desc));
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	unsigned long flags;
+	unsigned long pending;
+	int hwirq;
+
+	spin_lock_irqsave(&hlwd->gpioc.bgpio_lock, flags);
+	pending = hlwd->gpioc.read_reg(hlwd->regs + HW_GPIOB_INTFLAG);
+	pending &= hlwd->gpioc.read_reg(hlwd->regs + HW_GPIOB_INTMASK);
+	spin_unlock_irqrestore(&hlwd->gpioc.bgpio_lock, flags);
+
+	chained_irq_enter(chip, desc);
+
+	for_each_set_bit(hwirq, &pending, 32) {
+		int irq = irq_find_mapping(hlwd->gpioc.irq.domain, hwirq);
+
+		generic_handle_irq(irq);
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+static void hlwd_gpio_irq_ack(struct irq_data *data)
+{
+	struct hlwd_gpio *hlwd =
+		gpiochip_get_data(irq_data_get_irq_chip_data(data));
+
+	hlwd->gpioc.write_reg(hlwd->regs + HW_GPIOB_INTFLAG, BIT(data->hwirq));
+}
+
+static void hlwd_gpio_irq_mask(struct irq_data *data)
+{
+	struct hlwd_gpio *hlwd =
+		gpiochip_get_data(irq_data_get_irq_chip_data(data));
+	unsigned long flags;
+	u32 mask;
+
+	spin_lock_irqsave(&hlwd->gpioc.bgpio_lock, flags);
+	mask = hlwd->gpioc.read_reg(hlwd->regs + HW_GPIOB_INTMASK);
+	mask &= ~BIT(data->hwirq);
+	hlwd->gpioc.write_reg(hlwd->regs + HW_GPIOB_INTMASK, mask);
+	spin_unlock_irqrestore(&hlwd->gpioc.bgpio_lock, flags);
+}
+
+static void hlwd_gpio_irq_unmask(struct irq_data *data)
+{
+	struct hlwd_gpio *hlwd =
+		gpiochip_get_data(irq_data_get_irq_chip_data(data));
+	unsigned long flags;
+	u32 mask;
+
+	spin_lock_irqsave(&hlwd->gpioc.bgpio_lock, flags);
+	mask = hlwd->gpioc.read_reg(hlwd->regs + HW_GPIOB_INTMASK);
+	mask |= BIT(data->hwirq);
+	hlwd->gpioc.write_reg(hlwd->regs + HW_GPIOB_INTMASK, mask);
+	spin_unlock_irqrestore(&hlwd->gpioc.bgpio_lock, flags);
+}
+
+static void hlwd_gpio_irq_enable(struct irq_data *data)
+{
+	hlwd_gpio_irq_ack(data);
+	hlwd_gpio_irq_unmask(data);
+}
+
+static int hlwd_gpio_irq_set_type(struct irq_data *data, unsigned int flow_type)
+{
+	struct hlwd_gpio *hlwd =
+		gpiochip_get_data(irq_data_get_irq_chip_data(data));
+	unsigned long flags;
+	u32 level;
+
+	spin_lock_irqsave(&hlwd->gpioc.bgpio_lock, flags);
+
+	switch (flow_type) {
+	case IRQ_TYPE_LEVEL_HIGH:
+		level = hlwd->gpioc.read_reg(hlwd->regs + HW_GPIOB_INTLVL);
+		level |= BIT(data->hwirq);
+		hlwd->gpioc.write_reg(hlwd->regs + HW_GPIOB_INTLVL, level);
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		level = hlwd->gpioc.read_reg(hlwd->regs + HW_GPIOB_INTLVL);
+		level &= ~BIT(data->hwirq);
+		hlwd->gpioc.write_reg(hlwd->regs + HW_GPIOB_INTLVL, level);
+		break;
+	default:
+		spin_unlock_irqrestore(&hlwd->gpioc.bgpio_lock, flags);
+		return -EINVAL;
+	}
+
+	spin_unlock_irqrestore(&hlwd->gpioc.bgpio_lock, flags);
+	return 0;
+}
+
 static int hlwd_gpio_probe(struct platform_device *pdev)
 {
 	struct hlwd_gpio *hlwd;
@@ -92,7 +190,43 @@ static int hlwd_gpio_probe(struct platform_device *pdev)
 		ngpios = 32;
 	hlwd->gpioc.ngpio = ngpios;
 
-	return devm_gpiochip_add_data(&pdev->dev, &hlwd->gpioc, hlwd);
+	res = devm_gpiochip_add_data(&pdev->dev, &hlwd->gpioc, hlwd);
+	if (res)
+		return res;
+
+	/* Mask and ack all interrupts */
+	iowrite32be(0, hlwd->regs + HW_GPIOB_INTMASK);
+	iowrite32be(0xffffffff, hlwd->regs + HW_GPIOB_INTFLAG);
+
+	/*
+	 * If this GPIO controller is not marked as an interrupt controller in
+	 * the DT, return.
+	 */
+	if (!of_property_read_bool(pdev->dev.of_node, "interrupt-controller"))
+		return 0;
+
+	hlwd->irq = platform_get_irq(pdev, 0);
+	if (hlwd->irq < 0) {
+		dev_info(&pdev->dev, "platform_get_irq returned %d\n",
+			 hlwd->irq);
+		return hlwd->irq;
+	}
+
+	hlwd->irqc.name = "GPIO";
+	hlwd->irqc.irq_mask = hlwd_gpio_irq_mask;
+	hlwd->irqc.irq_unmask = hlwd_gpio_irq_unmask;
+	hlwd->irqc.irq_enable = hlwd_gpio_irq_enable;
+	hlwd->irqc.irq_set_type = hlwd_gpio_irq_set_type;
+
+	res = gpiochip_irqchip_add(&hlwd->gpioc, &hlwd->irqc, 0,
+				   handle_level_irq, IRQ_TYPE_NONE);
+	if (res)
+		return res;
+
+	gpiochip_set_chained_irqchip(&hlwd->gpioc, &hlwd->irqc,
+				     hlwd->irq, hlwd_gpio_irqhandler);
+
+	return 0;
 }
 
 static const struct of_device_id hlwd_gpio_match[] = {
-- 
2.20.1

[PATCH 2/2] gpio: hlwd: Implement edge trigger emulation

[Jonathan Neuschäfer]

Like the Spreadtrum EIC driver[1], this driver needs to emulate edge
triggered interrupts to support the generic gpio-keys driver.

[1]: https://www.spinics.net/lists/kernel/msg2764576.html

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
 drivers/gpio/gpio-hlwd.c | 57 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/drivers/gpio/gpio-hlwd.c b/drivers/gpio/gpio-hlwd.c
index 9a93546f3d11..3d85e6a71b35 100644
--- a/drivers/gpio/gpio-hlwd.c
+++ b/drivers/gpio/gpio-hlwd.c
@@ -51,6 +51,8 @@ struct hlwd_gpio {
 	struct irq_chip irqc;
 	void __iomem *regs;
 	int irq;
+	u32 edge_emulation;
+	u32 rising_edge, falling_edge;
 };
 
 static void hlwd_gpio_irqhandler(struct irq_desc *desc)
@@ -61,10 +63,37 @@ static void hlwd_gpio_irqhandler(struct irq_desc *desc)
 	unsigned long flags;
 	unsigned long pending;
 	int hwirq;
+	u32 emulated_pending;
 
 	spin_lock_irqsave(&hlwd->gpioc.bgpio_lock, flags);
 	pending = hlwd->gpioc.read_reg(hlwd->regs + HW_GPIOB_INTFLAG);
 	pending &= hlwd->gpioc.read_reg(hlwd->regs + HW_GPIOB_INTMASK);
+
+	/* Treat interrupts due to edge trigger emulation separately */
+	emulated_pending = hlwd->edge_emulation & pending;
+	pending &= ~emulated_pending;
+	if (emulated_pending) {
+		u32 level, rising, falling;
+
+		level = hlwd->gpioc.read_reg(hlwd->regs + HW_GPIOB_INTLVL);
+		rising = level & emulated_pending;
+		falling = ~level & emulated_pending;
+
+		/* Invert the levels */
+		hlwd->gpioc.write_reg(hlwd->regs + HW_GPIOB_INTLVL,
+				level ^ emulated_pending);
+
+		/* Ack all emulated-edge interrupts */
+		hlwd->gpioc.write_reg(hlwd->regs + HW_GPIOB_INTFLAG,
+				emulated_pending);
+
+		/* Signal interrupts only on the correct edge */
+		rising &= hlwd->rising_edge;
+		falling &= hlwd->falling_edge;
+
+		/* Mark emulated interrupts as pending */
+		pending |= rising | falling;
+	}
 	spin_unlock_irqrestore(&hlwd->gpioc.bgpio_lock, flags);
 
 	chained_irq_enter(chip, desc);
@@ -120,6 +149,27 @@ static void hlwd_gpio_irq_enable(struct irq_data *data)
 	hlwd_gpio_irq_unmask(data);
 }
 
+static void hlwd_gpio_irq_setup_emulation(struct hlwd_gpio *hlwd, int hwirq,
+					  unsigned int flow_type)
+{
+	u32 level, state;
+
+	/* Set the trigger level to the inactive level */
+	level = hlwd->gpioc.read_reg(hlwd->regs + HW_GPIOB_INTLVL);
+	state = hlwd->gpioc.read_reg(hlwd->regs + HW_GPIOB_IN) & BIT(hwirq);
+	level &= ~BIT(hwirq);
+	level |= state ^ BIT(hwirq);
+	hlwd->gpioc.write_reg(hlwd->regs + HW_GPIOB_INTLVL, level);
+
+	hlwd->edge_emulation |= BIT(hwirq);
+	hlwd->rising_edge &= ~BIT(hwirq);
+	hlwd->falling_edge &= ~BIT(hwirq);
+	if (flow_type & IRQ_TYPE_EDGE_RISING)
+		hlwd->rising_edge |= BIT(hwirq);
+	if (flow_type & IRQ_TYPE_EDGE_FALLING)
+		hlwd->falling_edge |= BIT(hwirq);
+}
+
 static int hlwd_gpio_irq_set_type(struct irq_data *data, unsigned int flow_type)
 {
 	struct hlwd_gpio *hlwd =
@@ -129,6 +179,8 @@ static int hlwd_gpio_irq_set_type(struct irq_data *data, unsigned int flow_type)
 
 	spin_lock_irqsave(&hlwd->gpioc.bgpio_lock, flags);
 
+	hlwd->edge_emulation &= ~BIT(data->hwirq);
+
 	switch (flow_type) {
 	case IRQ_TYPE_LEVEL_HIGH:
 		level = hlwd->gpioc.read_reg(hlwd->regs + HW_GPIOB_INTLVL);
@@ -140,6 +192,11 @@ static int hlwd_gpio_irq_set_type(struct irq_data *data, unsigned int flow_type)
 		level &= ~BIT(data->hwirq);
 		hlwd->gpioc.write_reg(hlwd->regs + HW_GPIOB_INTLVL, level);
 		break;
+	case IRQ_TYPE_EDGE_RISING:
+	case IRQ_TYPE_EDGE_FALLING:
+	case IRQ_TYPE_EDGE_BOTH:
+		hlwd_gpio_irq_setup_emulation(hlwd, data->hwirq, flow_type);
+		break;
 	default:
 		spin_unlock_irqrestore(&hlwd->gpioc.bgpio_lock, flags);
 		return -EINVAL;
-- 
2.20.1

Re: [PATCH 1/2] gpio: hlwd: Add basic IRQ support

[Linus Walleij]

Hi Jonathan,

thanks for the patch! It is looking very good.
Some minor comments:

On Sun, Jan 13, 2019 at 3:00 PM Jonathan Neuschäfer
<j.neuschaefer@gmx.net> wrote:

>         select GPIO_GENERIC
> +       select GPIOLIB_IRQCHIP

Nice!

> +static void hlwd_gpio_irqhandler(struct irq_desc *desc)
> +{
> +       struct hlwd_gpio *hlwd =
> +               gpiochip_get_data(irq_desc_get_handler_data(desc));
> +       struct irq_chip *chip = irq_desc_get_chip(desc);
> +       unsigned long flags;
> +       unsigned long pending;
> +       int hwirq;
> +
> +       spin_lock_irqsave(&hlwd->gpioc.bgpio_lock, flags);
> +       pending = hlwd->gpioc.read_reg(hlwd->regs + HW_GPIOB_INTFLAG);
> +       pending &= hlwd->gpioc.read_reg(hlwd->regs + HW_GPIOB_INTMASK);

Please just access IO directly instead through the helper.
ioread32be()/iowrite32be() I suppose?

> +static void hlwd_gpio_irq_ack(struct irq_data *data)
> +{
> +       struct hlwd_gpio *hlwd =
> +               gpiochip_get_data(irq_data_get_irq_chip_data(data));
> +
> +       hlwd->gpioc.write_reg(hlwd->regs + HW_GPIOB_INTFLAG, BIT(data->hwirq));

Dito.

> +       spin_lock_irqsave(&hlwd->gpioc.bgpio_lock, flags);
> +       mask = hlwd->gpioc.read_reg(hlwd->regs + HW_GPIOB_INTMASK);
> +       mask &= ~BIT(data->hwirq);
> +       hlwd->gpioc.write_reg(hlwd->regs + HW_GPIOB_INTMASK, mask);

Dito.

> +       spin_lock_irqsave(&hlwd->gpioc.bgpio_lock, flags);
> +       mask = hlwd->gpioc.read_reg(hlwd->regs + HW_GPIOB_INTMASK);
> +       mask |= BIT(data->hwirq);
> +       hlwd->gpioc.write_reg(hlwd->regs + HW_GPIOB_INTMASK, mask);

Dito.

> +       switch (flow_type) {
> +       case IRQ_TYPE_LEVEL_HIGH:
> +               level = hlwd->gpioc.read_reg(hlwd->regs + HW_GPIOB_INTLVL);
> +               level |= BIT(data->hwirq);
> +               hlwd->gpioc.write_reg(hlwd->regs + HW_GPIOB_INTLVL, level);
> +               break;
> +       case IRQ_TYPE_LEVEL_LOW:
> +               level = hlwd->gpioc.read_reg(hlwd->regs + HW_GPIOB_INTLVL);
> +               level &= ~BIT(data->hwirq);
> +               hlwd->gpioc.write_reg(hlwd->regs + HW_GPIOB_INTLVL, level);
> +               break;

Dito.

> +       hlwd->irqc.name = "GPIO";

What about using something device-unique?

hlwd->irqc.name = dev_name(&pdev->dev);

for example?

otherwise it looks fine!

Yours,
Linus Walleij

Re: [PATCH 1/2] gpio: hlwd: Add basic IRQ support

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 1002 bytes --]

Hi,

On Mon, Jan 14, 2019 at 11:20:49AM +0100, Linus Walleij wrote:
> Hi Jonathan,
> 
> thanks for the patch! It is looking very good.
> Some minor comments:
> 
> On Sun, Jan 13, 2019 at 3:00 PM Jonathan Neuschäfer
[...]
> > +       spin_lock_irqsave(&hlwd->gpioc.bgpio_lock, flags);
> > +       pending = hlwd->gpioc.read_reg(hlwd->regs + HW_GPIOB_INTFLAG);
> > +       pending &= hlwd->gpioc.read_reg(hlwd->regs + HW_GPIOB_INTMASK);
> 
> Please just access IO directly instead through the helper.
> ioread32be()/iowrite32be() I suppose?

Ok.  Yes, 32-bit big endian is correct.

[...]
> > +       hlwd->irqc.name = "GPIO";
> 
> What about using something device-unique?
> 
> hlwd->irqc.name = dev_name(&pdev->dev);
> 
> for example?

Good idea, especially considering that the Wii U (the successor of the
Wii) has two of these GPIO controllers.

> otherwise it looks fine!

Cool, I'll send a v2 with these two points addressed.



Thanks,
Jonathan Neuschäfer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]