[PATCH v2 0/6] GPIO support for BRCMSTB

[PATCH v2 0/6] GPIO support for BRCMSTB

[Gregory Fong]

This patchset adds support for the GPIO controller (UPG GIO) used on Broadcom's
various BRCMSTB SoCs (BCM7XXX and others). It uses the "basic-mmio-gpio"
interface to try to reduce duplication of the base logic.

For all existing hardware, this block hooked up to the BCM7120 L2 IRQ
controller and so will require CONFIG_BCM7120_L2_IRQ=y.

New in v2:
- fix license mismatch as pointed out by Paul Bolle
- move select ARCH_WANT_OPTIONAL_GPIOLIB to separate patch
- change to have 32 lines per bank per Linus Walleij's comments
- allow this controller to be used as a wakeup source
- add default GPIO number for BRCMSTB

The device tree bindings from v1 were merged to the GPIO tree, so this patchset
only contains an addition to allow GPIOs to be used as a wakeup source
(patch 3).  The initial bindings from v1 can be found at
https://lkml.org/lkml/2015/5/6/200 .

Gregory Fong (6):
  gpio: Add GPIO support for Broadcom STB SoCs
  gpio: brcmstb: Add interrupt support
  dt-bindings: brcmstb-gpio: document properties for wakeup
  gpio: brcmstb: Allow GPIOs to be wakeup sources
  ARM: brcmstb: Select ARCH_WANT_OPTIONAL_GPIOLIB
  ARM: brcmstb: Add default gpio number

 .../devicetree/bindings/gpio/brcm,brcmstb-gpio.txt |  26 +-
 MAINTAINERS                                        |   7 +
 arch/arm/Kconfig                                   |   3 +-
 arch/arm/mach-bcm/Kconfig                          |   1 +
 drivers/gpio/Kconfig                               |   9 +
 drivers/gpio/Makefile                              |   1 +
 drivers/gpio/gpio-brcmstb.c                        | 581 +++++++++++++++++++++
 7 files changed, 626 insertions(+), 2 deletions(-)
 create mode 100644 drivers/gpio/gpio-brcmstb.c

-- 
1.9.1

[PATCH v2 1/6] gpio: Add GPIO support for Broadcom STB SoCs

[Gregory Fong]

This adds support for the GPIO IP "UPG GIO" used on Broadcom STB SoCs
(BCM7XXX and some others).  Uses basic_mmio_gpio to instantiate a
gpio_chip for each bank.  The driver assumes that it handles the base
set of GPIOs on the system and that it can start its numbering sequence
from 0, so any GPIO expanders used with it must dynamically assign GPIO
numbers after this driver has finished registering its GPIOs.

Does not implement the interrupt-controller portion yet, will be done in a
future commit.

List-usage-fixed-by: Brian Norris <computersforpeace@gmail.com>
Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com>
---
v2:
- change include to use <linux/gpio/driver.h> instead of <linux/gpio.h>
- get rid of unnecessary imask member in struct bank
- rename GPIO_PER_BANK -> MAX_GPIO_PER_BANK
- always have 32 GPIOs per bank and add 'width' member in struct bank to hold
  actual number of GPIOs in use
- mark of_match table as const

 MAINTAINERS                 |   7 ++
 drivers/gpio/Kconfig        |   8 ++
 drivers/gpio/Makefile       |   1 +
 drivers/gpio/gpio-brcmstb.c | 252 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 268 insertions(+)
 create mode 100644 drivers/gpio/gpio-brcmstb.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 198a429..1d1b6f1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2290,6 +2290,13 @@ N:	bcm9583*
 N:	bcm583*
 N:	bcm113*
 
+BROADCOM BRCMSTB GPIO DRIVER
+M:	Gregory Fong <gregory.0xf0@gmail.com>
+L:	bcm-kernel-feedback-list@broadcom.com>
+S:	Supported
+F:	drivers/gpio/gpio-brcmstb.c
+F:	Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt
+
 BROADCOM KONA GPIO DRIVER
 M:	Ray Jui <rjui@broadcom.com>
 L:	bcm-kernel-feedback-list@broadcom.com
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 5faaf5f..d86de6a 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -126,6 +126,14 @@ config GPIO_BCM_KONA
 	help
 	  Turn on GPIO support for Broadcom "Kona" chips.
 
+config GPIO_BRCMSTB
+	tristate "BRCMSTB GPIO support"
+	default y if ARCH_BRCMSTB
+	depends on OF_GPIO && (ARCH_BRCMSTB || COMPILE_TEST)
+	select GPIO_GENERIC
+	help
+	  Say yes here to enable GPIO support for Broadcom STB (BCM7XXX) SoCs.
+
 config GPIO_CLPS711X
 	tristate "CLPS711X GPIO support"
 	depends on ARCH_CLPS711X || COMPILE_TEST
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 1b55fda..893bbff 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_GPIO_ALTERA)  	+= gpio-altera.o
 obj-$(CONFIG_GPIO_AMD8111)	+= gpio-amd8111.o
 obj-$(CONFIG_GPIO_ARIZONA)	+= gpio-arizona.o
 obj-$(CONFIG_GPIO_BCM_KONA)	+= gpio-bcm-kona.o
+obj-$(CONFIG_GPIO_BRCMSTB)	+= gpio-brcmstb.o
 obj-$(CONFIG_GPIO_BT8XX)	+= gpio-bt8xx.o
 obj-$(CONFIG_GPIO_CLPS711X)	+= gpio-clps711x.o
 obj-$(CONFIG_GPIO_CS5535)	+= gpio-cs5535.o
diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
new file mode 100644
index 0000000..7a3cb1f
--- /dev/null
+++ b/drivers/gpio/gpio-brcmstb.c
@@ -0,0 +1,252 @@
+/*
+ * Copyright (C) 2015 Broadcom Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/bitops.h>
+#include <linux/gpio/driver.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/module.h>
+#include <linux/basic_mmio_gpio.h>
+
+#define GIO_BANK_SIZE           0x20
+#define GIO_ODEN(bank)          (((bank) * GIO_BANK_SIZE) + 0x00)
+#define GIO_DATA(bank)          (((bank) * GIO_BANK_SIZE) + 0x04)
+#define GIO_IODIR(bank)         (((bank) * GIO_BANK_SIZE) + 0x08)
+#define GIO_EC(bank)            (((bank) * GIO_BANK_SIZE) + 0x0c)
+#define GIO_EI(bank)            (((bank) * GIO_BANK_SIZE) + 0x10)
+#define GIO_MASK(bank)          (((bank) * GIO_BANK_SIZE) + 0x14)
+#define GIO_LEVEL(bank)         (((bank) * GIO_BANK_SIZE) + 0x18)
+#define GIO_STAT(bank)          (((bank) * GIO_BANK_SIZE) + 0x1c)
+
+struct brcmstb_gpio_bank {
+	struct list_head node;
+	int id;
+	struct bgpio_chip bgc;
+	struct brcmstb_gpio_priv *parent_priv;
+	u32 width;
+};
+
+struct brcmstb_gpio_priv {
+	struct list_head bank_list;
+	void __iomem *reg_base;
+	int num_banks;
+	struct platform_device *pdev;
+	int gpio_base;
+};
+
+#define MAX_GPIO_PER_BANK           32
+#define GPIO_BANK(gpio)         ((gpio) >> 5)
+/* assumes MAX_GPIO_PER_BANK is a multiple of 2 */
+#define GPIO_BIT(gpio)          ((gpio) & (MAX_GPIO_PER_BANK - 1))
+
+static inline struct brcmstb_gpio_bank *
+brcmstb_gpio_gc_to_bank(struct gpio_chip *gc)
+{
+	struct bgpio_chip *bgc = to_bgpio_chip(gc);
+	return container_of(bgc, struct brcmstb_gpio_bank, bgc);
+}
+
+static inline struct brcmstb_gpio_priv *
+brcmstb_gpio_gc_to_priv(struct gpio_chip *gc)
+{
+	struct brcmstb_gpio_bank *bank = brcmstb_gpio_gc_to_bank(gc);
+	return bank->parent_priv;
+}
+
+/* Make sure that the number of banks matches up between properties */
+static int brcmstb_gpio_sanity_check_banks(struct device *dev,
+		struct device_node *np, struct resource *res)
+{
+	int res_num_banks = resource_size(res) / GIO_BANK_SIZE;
+	int num_banks =
+		of_property_count_u32_elems(np, "brcm,gpio-bank-widths");
+
+	if (res_num_banks != num_banks) {
+		dev_err(dev, "Mismatch in banks: res had %d, bank-widths had %d\n",
+				res_num_banks, num_banks);
+		return -EINVAL;
+	} else {
+		return 0;
+	}
+}
+
+static int brcmstb_gpio_remove(struct platform_device *pdev)
+{
+	struct brcmstb_gpio_priv *priv = platform_get_drvdata(pdev);
+	struct list_head *pos;
+	struct brcmstb_gpio_bank *bank;
+	int ret = 0;
+
+	list_for_each(pos, &priv->bank_list) {
+		bank = list_entry(pos, struct brcmstb_gpio_bank, node);
+		ret = bgpio_remove(&bank->bgc);
+		if (ret)
+			dev_err(&pdev->dev, "gpiochip_remove fail in cleanup");
+	}
+	return ret;
+}
+
+static int brcmstb_gpio_of_xlate(struct gpio_chip *gc,
+		const struct of_phandle_args *gpiospec, u32 *flags)
+{
+	struct brcmstb_gpio_priv *priv = brcmstb_gpio_gc_to_priv(gc);
+	struct brcmstb_gpio_bank *bank = brcmstb_gpio_gc_to_bank(gc);
+	int offset;
+
+	if (gc->of_gpio_n_cells != 2) {
+		WARN_ON(1);
+		return -EINVAL;
+	}
+
+	if (WARN_ON(gpiospec->args_count < gc->of_gpio_n_cells))
+		return -EINVAL;
+
+	offset = gpiospec->args[0] - (gc->base - priv->gpio_base);
+	if (offset >= gc->ngpio)
+		return -EINVAL;
+
+	if (unlikely(offset >= bank->width)) {
+		dev_warn_ratelimited(&priv->pdev->dev,
+			"Received request for invalid GPIO offset %d\n",
+			gpiospec->args[0]);
+	}
+
+	if (flags)
+		*flags = gpiospec->args[1];
+
+	return offset;
+}
+
+static int brcmstb_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	void __iomem *reg_base;
+	struct brcmstb_gpio_priv *priv;
+	struct resource *res;
+	struct property *prop;
+	const __be32 *p;
+	u32 bank_width;
+	int err;
+	static int gpio_base;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	reg_base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(reg_base))
+		return PTR_ERR(reg_base);
+
+	priv->gpio_base = gpio_base;
+	priv->reg_base = reg_base;
+	priv->pdev = pdev;
+
+	INIT_LIST_HEAD(&priv->bank_list);
+	if (brcmstb_gpio_sanity_check_banks(dev, np, res))
+		return -EINVAL;
+
+	of_property_for_each_u32(np, "brcm,gpio-bank-widths", prop, p,
+			bank_width) {
+		struct brcmstb_gpio_bank *bank;
+		struct bgpio_chip *bgc;
+		struct gpio_chip *gc;
+
+		bank = devm_kzalloc(dev, sizeof(*bank), GFP_KERNEL);
+		if (!bank) {
+			err = -ENOMEM;
+			goto fail;
+		}
+
+		bank->parent_priv = priv;
+		bank->id = priv->num_banks;
+		if (bank_width <= 0 || bank_width > MAX_GPIO_PER_BANK) {
+			dev_err(dev, "Invalid bank width %d\n", bank_width);
+			goto fail;
+		} else {
+			bank->width = bank_width;
+		}
+
+		/*
+		 * Regs are 4 bytes wide, have data reg, no set/clear regs,
+		 * and direction bits have 0 = output and 1 = input
+		 */
+		bgc = &bank->bgc;
+		err = bgpio_init(bgc, dev, 4,
+				reg_base + GIO_DATA(bank->id),
+				NULL, NULL, NULL,
+				reg_base + GIO_IODIR(bank->id), 0);
+		if (err) {
+			dev_err(dev, "bgpio_init() failed\n");
+			goto fail;
+		}
+
+		gc = &bgc->gc;
+		gc->of_node = np;
+		gc->owner = THIS_MODULE;
+		gc->label = np->full_name;
+		gc->base = gpio_base;
+		gc->of_gpio_n_cells = 2;
+		gc->of_xlate = brcmstb_gpio_of_xlate;
+		/* not all ngpio lines are valid, will use bank width later */
+		gc->ngpio = MAX_GPIO_PER_BANK;
+
+		err = gpiochip_add(gc);
+		if (err) {
+			dev_err(dev, "Could not add gpiochip for bank %d\n",
+					bank->id);
+			goto fail;
+		}
+		gpio_base += gc->ngpio;
+		dev_dbg(dev, "bank=%d, base=%d, ngpio=%d, width=%d\n", bank->id,
+			gc->base, gc->ngpio, bank->width);
+
+		/* Everything looks good, so add bank to list */
+		list_add(&bank->node, &priv->bank_list);
+
+		priv->num_banks++;
+	}
+
+	dev_info(dev, "Registered %d banks (GPIO(s): %d-%d)\n",
+			priv->num_banks, priv->gpio_base, gpio_base - 1);
+
+	platform_set_drvdata(pdev, priv);
+
+	return 0;
+
+fail:
+	(void) brcmstb_gpio_remove(pdev);
+	return err;
+}
+
+static const struct of_device_id brcmstb_gpio_of_match[] = {
+	{ .compatible = "brcm,brcmstb-gpio" },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, brcmstb_gpio_of_match);
+
+static struct platform_driver brcmstb_gpio_driver = {
+	.driver = {
+		.name = "brcmstb-gpio",
+		.of_match_table = brcmstb_gpio_of_match,
+	},
+	.probe = brcmstb_gpio_probe,
+	.remove = brcmstb_gpio_remove,
+};
+module_platform_driver(brcmstb_gpio_driver);
+
+MODULE_AUTHOR("Gregory Fong");
+MODULE_DESCRIPTION("Driver for Broadcom BRCMSTB SoC UPG GPIO");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

[PATCH v2 2/6] gpio: brcmstb: Add interrupt support

[Gregory Fong]

Create an irq_chip for each GIO block.  Uses chained IRQ handling since
known uses of this block have a BCM7120 L2 interrupt controller as a
parent.  Supports interrupts for all GPIOs.

In the IRQ handler, we check for raised IRQs for invalid GPIOs and warn
(ratelimited) if they're encountered.

Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com>
---
v2:
- since imask member of bank struct was removed, just read and write from mask
  reg and don't maintain a shadow
- warn on invalid IRQs
- move some irq setup to a separate function since probe is getting unwieldy

 drivers/gpio/Kconfig        |   1 +
 drivers/gpio/gpio-brcmstb.c | 276 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 277 insertions(+)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index d86de6a..7249dba 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -131,6 +131,7 @@ config GPIO_BRCMSTB
 	default y if ARCH_BRCMSTB
 	depends on OF_GPIO && (ARCH_BRCMSTB || COMPILE_TEST)
 	select GPIO_GENERIC
+	select IRQ_DOMAIN
 	help
 	  Say yes here to enable GPIO support for Broadcom STB (BCM7XXX) SoCs.
 
diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
index 7a3cb1f..b9962ff 100644
--- a/drivers/gpio/gpio-brcmstb.c
+++ b/drivers/gpio/gpio-brcmstb.c
@@ -17,6 +17,8 @@
 #include <linux/of_irq.h>
 #include <linux/module.h>
 #include <linux/basic_mmio_gpio.h>
+#include <linux/irqdomain.h>
+#include <linux/irqchip/chained_irq.h>
 
 #define GIO_BANK_SIZE           0x20
 #define GIO_ODEN(bank)          (((bank) * GIO_BANK_SIZE) + 0x00)
@@ -40,7 +42,11 @@ struct brcmstb_gpio_priv {
 	struct list_head bank_list;
 	void __iomem *reg_base;
 	int num_banks;
+	int num_gpios;
 	struct platform_device *pdev;
+	struct irq_chip irq_chip;
+	struct irq_domain *irq_domain;
+	int parent_irq;
 	int gpio_base;
 };
 
@@ -63,6 +69,231 @@ brcmstb_gpio_gc_to_priv(struct gpio_chip *gc)
 	return bank->parent_priv;
 }
 
+static void brcmstb_gpio_set_imask(struct brcmstb_gpio_bank *bank,
+		unsigned int offset, bool enable)
+{
+	struct bgpio_chip *bgc = &bank->bgc;
+	struct brcmstb_gpio_priv *priv = bank->parent_priv;
+	u32 mask = bgc->pin2mask(bgc, offset);
+	u32 imask;
+	unsigned long flags;
+
+	spin_lock_irqsave(&bgc->lock, flags);
+	imask = bgc->read_reg(priv->reg_base + GIO_MASK(bank->id));
+	if (enable)
+		imask |= mask;
+	else
+		imask &= ~mask;
+	bgc->write_reg(priv->reg_base + GIO_MASK(bank->id), imask);
+	spin_unlock_irqrestore(&bgc->lock, flags);
+}
+
+static int brcmstb_gpio_to_irq(struct gpio_chip *gc, unsigned gc_offset)
+{
+	struct brcmstb_gpio_priv *priv = brcmstb_gpio_gc_to_priv(gc);
+	/* gc_offset is relative to this gpio_chip; want real offset */
+	int offset = gc_offset + (gc->base - priv->gpio_base);
+
+	if (offset >= priv->num_gpios)
+		return -ENXIO;
+	return irq_create_mapping(priv->irq_domain, offset);
+}
+
+/* -------------------- IRQ chip functions -------------------- */
+
+static int brcmstb_gpio_hwirq_to_offset(irq_hw_number_t hwirq,
+		struct brcmstb_gpio_bank *bank)
+{
+	return hwirq - (bank->bgc.gc.base - bank->parent_priv->gpio_base);
+}
+
+static void brcmstb_gpio_irq_mask(struct irq_data *d)
+{
+	struct brcmstb_gpio_bank *bank = irq_data_get_irq_chip_data(d);
+	int offset = brcmstb_gpio_hwirq_to_offset(d->hwirq, bank);
+
+	brcmstb_gpio_set_imask(bank, offset, false);
+}
+
+static void brcmstb_gpio_irq_unmask(struct irq_data *d)
+{
+	struct brcmstb_gpio_bank *bank = irq_data_get_irq_chip_data(d);
+	int offset = brcmstb_gpio_hwirq_to_offset(d->hwirq, bank);
+
+	brcmstb_gpio_set_imask(bank, offset, true);
+}
+
+static int brcmstb_gpio_irq_set_type(struct irq_data *d, unsigned int type)
+{
+	struct brcmstb_gpio_bank *bank = irq_data_get_irq_chip_data(d);
+	struct brcmstb_gpio_priv *priv = bank->parent_priv;
+	u32 mask = BIT(brcmstb_gpio_hwirq_to_offset(d->hwirq, bank));
+	u32 edge_insensitive, iedge_insensitive;
+	u32 edge_config, iedge_config;
+	u32 level, ilevel;
+	unsigned long flags;
+
+	switch (type) {
+	case IRQ_TYPE_LEVEL_LOW:
+		level = 0;
+		edge_config = 0;
+		edge_insensitive = 0;
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		level = mask;
+		edge_config = 0;
+		edge_insensitive = 0;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		level = 0;
+		edge_config = 0;
+		edge_insensitive = 0;
+		break;
+	case IRQ_TYPE_EDGE_RISING:
+		level = 0;
+		edge_config = mask;
+		edge_insensitive = 0;
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		level = 0;
+		edge_config = 0;  /* don't care, but want known value */
+		edge_insensitive = mask;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	spin_lock_irqsave(&bank->bgc.lock, flags);
+
+	iedge_config = bank->bgc.read_reg(priv->reg_base +
+			GIO_EC(bank->id)) & ~mask;
+	iedge_insensitive = bank->bgc.read_reg(priv->reg_base +
+			GIO_EI(bank->id)) & ~mask;
+	ilevel = bank->bgc.read_reg(priv->reg_base +
+			GIO_LEVEL(bank->id)) & ~mask;
+
+	bank->bgc.write_reg(priv->reg_base + GIO_EC(bank->id),
+			iedge_config | edge_config);
+	bank->bgc.write_reg(priv->reg_base + GIO_EI(bank->id),
+			iedge_insensitive | edge_insensitive);
+	bank->bgc.write_reg(priv->reg_base + GIO_LEVEL(bank->id),
+			ilevel | level);
+
+	spin_unlock_irqrestore(&bank->bgc.lock, flags);
+	return 0;
+}
+
+static void brcmstb_gpio_irq_bank_handler(int irq,
+		struct brcmstb_gpio_bank *bank)
+{
+	struct brcmstb_gpio_priv *priv = bank->parent_priv;
+	void __iomem *reg_base = priv->reg_base;
+	unsigned long status;
+	unsigned long flags;
+
+	spin_lock_irqsave(&bank->bgc.lock, flags);
+	while ((status = bank->bgc.read_reg(reg_base + GIO_STAT(bank->id)) &
+			 bank->bgc.read_reg(reg_base + GIO_MASK(bank->id)))) {
+		int bit;
+		for_each_set_bit(bit, &status, 32) {
+			int hwirq = bank->bgc.gc.base -
+				priv->gpio_base + bit;
+			int child_irq =
+				irq_find_mapping(priv->irq_domain,
+						 hwirq);
+			u32 stat = bank->bgc.read_reg(reg_base +
+						      GIO_STAT(bank->id));
+			if (bit >= bank->width)
+				dev_warn(&priv->pdev->dev,
+					 "IRQ for invalid GPIO (bank=%d, offset=%d)\n",
+					 bank->id, bit);
+			bank->bgc.write_reg(reg_base + GIO_STAT(bank->id),
+					    stat | BIT(bit));
+			generic_handle_irq(child_irq);
+		}
+	}
+	spin_unlock_irqrestore(&bank->bgc.lock, flags);
+}
+
+/* Each UPG GIO block has one IRQ for all banks */
+static void brcmstb_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
+{
+	struct brcmstb_gpio_priv *priv = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct list_head *pos;
+
+	chained_irq_enter(chip, desc);
+	list_for_each(pos, &priv->bank_list) {
+		struct brcmstb_gpio_bank *bank =
+			list_entry(pos, struct brcmstb_gpio_bank, node);
+		brcmstb_gpio_irq_bank_handler(irq, bank);
+	}
+	chained_irq_exit(chip, desc);
+}
+
+static struct brcmstb_gpio_bank *brcmstb_gpio_hwirq_to_bank(
+		struct brcmstb_gpio_priv *priv, irq_hw_number_t hwirq)
+{
+	struct list_head *pos;
+	int i = 0;
+
+	/* banks are in descending order */
+	list_for_each_prev(pos, &priv->bank_list) {
+		struct brcmstb_gpio_bank *bank =
+			list_entry(pos, struct brcmstb_gpio_bank, node);
+		i += bank->bgc.gc.ngpio;
+		if (hwirq < i)
+			return bank;
+	}
+	return NULL;
+}
+
+/*
+ * This lock class tells lockdep that GPIO irqs are in a different
+ * category than their parents, so it won't report false recursion.
+ */
+static struct lock_class_key brcmstb_gpio_irq_lock_class;
+
+
+static int brcmstb_gpio_irq_map(struct irq_domain *d, unsigned int irq,
+		irq_hw_number_t hwirq)
+{
+	struct brcmstb_gpio_priv *priv = d->host_data;
+	struct brcmstb_gpio_bank *bank =
+		brcmstb_gpio_hwirq_to_bank(priv, hwirq);
+	struct platform_device *pdev = priv->pdev;
+	int ret;
+
+	if (!bank)
+		return -EINVAL;
+
+	dev_dbg(&pdev->dev, "Mapping irq %d for gpio line %d (bank %d)\n",
+		irq, (int)hwirq, bank->id);
+	ret = irq_set_chip_data(irq, bank);
+	if (ret < 0)
+		return ret;
+	irq_set_lockdep_class(irq, &brcmstb_gpio_irq_lock_class);
+	irq_set_chip_and_handler(irq, &priv->irq_chip, handle_simple_irq);
+#ifdef CONFIG_ARM
+	set_irq_flags(irq, IRQF_VALID);
+#else
+	irq_set_noprobe(irq);
+#endif
+	return 0;
+}
+
+static void brcmstb_gpio_irq_unmap(struct irq_domain *d, unsigned int irq)
+{
+	irq_set_chip_and_handler(irq, NULL, NULL);
+	irq_set_chip_data(irq, NULL);
+}
+
+static struct irq_domain_ops brcmstb_gpio_irq_domain_ops = {
+	.map = brcmstb_gpio_irq_map,
+	.unmap = brcmstb_gpio_irq_unmap,
+	.xlate = irq_domain_xlate_twocell,
+};
+
 /* Make sure that the number of banks matches up between properties */
 static int brcmstb_gpio_sanity_check_banks(struct device *dev,
 		struct device_node *np, struct resource *res)
@@ -127,6 +358,32 @@ static int brcmstb_gpio_of_xlate(struct gpio_chip *gc,
 	return offset;
 }
 
+/* priv->parent_irq and priv->num_gpios must be set before calling */
+static int brcmstb_gpio_irq_setup(struct platform_device *pdev,
+		struct brcmstb_gpio_priv *priv)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+
+	priv->irq_chip.name = dev_name(dev);
+	priv->irq_chip.irq_mask = brcmstb_gpio_irq_mask;
+	priv->irq_chip.irq_unmask = brcmstb_gpio_irq_unmask;
+	priv->irq_chip.irq_set_type = brcmstb_gpio_irq_set_type;
+	priv->irq_domain =
+		irq_domain_add_linear(np, priv->num_gpios,
+				      &brcmstb_gpio_irq_domain_ops,
+				      priv);
+	if (!priv->irq_domain) {
+		dev_err(dev, "Couldn't allocate IRQ domain\n");
+		return -ENXIO;
+	}
+	irq_set_chained_handler(priv->parent_irq,
+				brcmstb_gpio_irq_handler);
+	irq_set_handler_data(priv->parent_irq, priv);
+
+	return 0;
+}
+
 static int brcmstb_gpio_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -153,6 +410,16 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
 	priv->reg_base = reg_base;
 	priv->pdev = pdev;
 
+	if (of_find_property(np, "interrupt-controller", NULL)) {
+		priv->parent_irq = platform_get_irq(pdev, 0);
+		if (priv->parent_irq < 0) {
+			dev_err(dev, "Couldn't get IRQ");
+			return -ENOENT;
+		}
+	} else {
+		priv->parent_irq = -ENOENT;
+	}
+
 	INIT_LIST_HEAD(&priv->bank_list);
 	if (brcmstb_gpio_sanity_check_banks(dev, np, res))
 		return -EINVAL;
@@ -201,6 +468,8 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
 		gc->of_xlate = brcmstb_gpio_of_xlate;
 		/* not all ngpio lines are valid, will use bank width later */
 		gc->ngpio = MAX_GPIO_PER_BANK;
+		if (priv->parent_irq >= 0)
+			gc->to_irq = brcmstb_gpio_to_irq;
 
 		err = gpiochip_add(gc);
 		if (err) {
@@ -218,6 +487,13 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
 		priv->num_banks++;
 	}
 
+	priv->num_gpios = gpio_base - priv->gpio_base;
+	if (priv->parent_irq >= 0) {
+		err = brcmstb_gpio_irq_setup(pdev, priv);
+		if (err)
+			goto fail;
+	}
+
 	dev_info(dev, "Registered %d banks (GPIO(s): %d-%d)\n",
 			priv->num_banks, priv->gpio_base, gpio_base - 1);
 
-- 
1.9.1

[PATCH v2 3/6] dt-bindings: brcmstb-gpio: document properties for wakeup

[Gregory Fong]

Some brcmstb GPIO controllers can be used to wake from suspend, so use the
de facto standard property 'wakeup-source' to mark the nodes of controllers
with that capability.

Also document interrupts-extended, which will be used for wakeup handling
because the interrupt parent for the wake IRQ is different from the regular
IRQ.

Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com>
---
New in v2.

 .../devicetree/bindings/gpio/brcm,brcmstb-gpio.txt | 26 +++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt b/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt
index 435f1bc..568814f 100644
--- a/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt
@@ -33,6 +33,12 @@ Optional properties:
 - interrupt-parent:
     phandle of the parent interrupt controller
 
+- interrupts-extended:
+    Alternate form of specifying interrupts and parents that allows for
+    multiple parents.  This takes precedence over 'interrupts' and
+    'interrupt-parent'.  This probably must be used if the wakeup-source
+    property is provided because that may have a different interrupt parent.
+
 - #interrupt-cells:
     Should be <2>.  The first cell is the GPIO number, the second should specify
     flags.  The following subset of flags is supported:
@@ -48,7 +54,10 @@ Optional properties:
     Marks the device node as an interrupt controller
 
 - interrupt-names:
-    The name of the IRQ resource used by this controller
+    The names of the IRQ resources used by this controller
+
+- wakeup-source:
+    GPIOs for this controller can be used as a wakeup source
 
 Example:
 	upg_gio: gpio@f040a700 {
@@ -63,3 +72,18 @@ Example:
 		interrupt-names = "upg_gio";
 		brcm,gpio-bank-widths = <0x20 0x20 0x20 0x18>;
 	};
+
+	upg_gio_aon: gpio@f04172c0 {
+		#gpio-cells = <0x2>;
+		#interrupt-cells = <0x2>;
+		compatible = "brcm,bcm7445-gpio", "brcm,brcmstb-gpio";
+		gpio-controller;
+		interrupt-controller;
+		reg = <0xf04172c0 0x40>;
+		interrupt-parent = <0xc>;
+		interrupts = <0x6>;
+		interrupts-extended = <0xc 0x6 0xa 0x5>;
+		interrupt-names = "upg_gio_aon", "upg_gio_aon_wakeup";
+		wakeup-source;
+		brcm,gpio-bank-widths = <0x12 0x4>;
+	};
-- 
1.9.1

[PATCH v2 4/6] gpio: brcmstb: Allow GPIOs to be wakeup sources

[Gregory Fong]

Several drivers (e.g. gpio-keys) allow for GPIOs to be configured as
wakeup sources, and this GPIO controller supports that through a
separate interrupt path.

The de-facto standard DT property "wakeup-source" is checked, since
that indicates whether the GPIO controller hardware can wake.  Uses
the IRQCHIP_MASK_ON_SUSPEND irq_chip flag because UPG GIO doesn't have
any of its own wakeup source configuration.

Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com>
---
New in v2.

 drivers/gpio/gpio-brcmstb.c | 53 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
index b9962ff..2598c1e 100644
--- a/drivers/gpio/gpio-brcmstb.c
+++ b/drivers/gpio/gpio-brcmstb.c
@@ -19,6 +19,7 @@
 #include <linux/basic_mmio_gpio.h>
 #include <linux/irqdomain.h>
 #include <linux/irqchip/chained_irq.h>
+#include <linux/interrupt.h>
 
 #define GIO_BANK_SIZE           0x20
 #define GIO_ODEN(bank)          (((bank) * GIO_BANK_SIZE) + 0x00)
@@ -48,6 +49,7 @@ struct brcmstb_gpio_priv {
 	struct irq_domain *irq_domain;
 	int parent_irq;
 	int gpio_base;
+	int parent_wake_irq;
 };
 
 #define MAX_GPIO_PER_BANK           32
@@ -183,6 +185,31 @@ static int brcmstb_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 	return 0;
 }
 
+static int brcmstb_gpio_irq_set_wake(struct irq_data *d, unsigned int enable)
+{
+	struct brcmstb_gpio_bank *bank = irq_data_get_irq_chip_data(d);
+	struct brcmstb_gpio_priv *priv = bank->parent_priv;
+
+	/*
+	 * Only enable wake IRQ once for however many hwirqs can wake
+	 * since they all use the same wake IRQ.  Mask will be set
+	 * up appropriately thanks to IRQCHIP_MASK_ON_SUSPEND flag.
+	 */
+	if (enable)
+		enable_irq_wake(priv->parent_wake_irq);
+	else
+		disable_irq_wake(priv->parent_wake_irq);
+	return 0;
+}
+
+static irqreturn_t brcmstb_gpio_wake_irq_handler(int irq, void *data)
+{
+	struct brcmstb_gpio_priv *priv = data;
+
+	pm_wakeup_event(&priv->pdev->dev, 0);
+	return IRQ_HANDLED;
+}
+
 static void brcmstb_gpio_irq_bank_handler(int irq,
 		struct brcmstb_gpio_bank *bank)
 {
@@ -369,6 +396,32 @@ static int brcmstb_gpio_irq_setup(struct platform_device *pdev,
 	priv->irq_chip.irq_mask = brcmstb_gpio_irq_mask;
 	priv->irq_chip.irq_unmask = brcmstb_gpio_irq_unmask;
 	priv->irq_chip.irq_set_type = brcmstb_gpio_irq_set_type;
+
+	/* Ensures that all non-wakeup IRQs are disabled at suspend */
+	priv->irq_chip.flags = IRQCHIP_MASK_ON_SUSPEND;
+
+	if (IS_ENABLED(CONFIG_PM_SLEEP) &&
+			of_get_property(np, "wakeup-source", NULL)) {
+		priv->parent_wake_irq = platform_get_irq(pdev, 1);
+		if (priv->parent_wake_irq < 0)  {
+			dev_warn(dev,
+				"Couldn't get wake IRQ - GPIOs will not be able to wake from sleep");
+		} else {
+			int err = devm_request_irq(dev, priv->parent_wake_irq,
+					brcmstb_gpio_wake_irq_handler, 0,
+					"brcmstb-gpio-wake", priv);
+
+			if (err < 0) {
+				dev_err(dev, "Couldn't request wake IRQ");
+				return err;
+			}
+
+			device_set_wakeup_capable(dev, true);
+			device_wakeup_enable(dev);
+			priv->irq_chip.irq_set_wake = brcmstb_gpio_irq_set_wake;
+		}
+	}
+
 	priv->irq_domain =
 		irq_domain_add_linear(np, priv->num_gpios,
 				      &brcmstb_gpio_irq_domain_ops,
-- 
1.9.1

[PATCH v2 5/6] ARM: brcmstb: Select ARCH_WANT_OPTIONAL_GPIOLIB

[Gregory Fong]

Select ARCH_WANT_OPTIONAL_GPIOLIB from BRCMSTB to allow GPIOLIB and
GPIO_BRCMSTB to be enabled.

Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com>
---
v2: this was split out so that it can go through the ARM SoC tree.

 arch/arm/mach-bcm/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig
index 8b11f44..0ac9e4b3 100644
--- a/arch/arm/mach-bcm/Kconfig
+++ b/arch/arm/mach-bcm/Kconfig
@@ -144,6 +144,7 @@ config ARCH_BRCMSTB
 	select BRCMSTB_GISB_ARB
 	select BRCMSTB_L2_IRQ
 	select BCM7120_L2_IRQ
+	select ARCH_WANT_OPTIONAL_GPIOLIB
 	help
 	  Say Y if you intend to run the kernel on a Broadcom ARM-based STB
 	  chipset.
-- 
1.9.1

[PATCH v2 6/6] ARM: brcmstb: Add default gpio number

[Gregory Fong]

Out of the brcmstb SoCs that I know, BCM3390 has the largest numbers
of GPIOs, with its
- 320 "peripheral" GPIOs
- 5*32 = 160 UPG GPIOs (counting unused lines, which do get counted)
- 2*32 = 64 UPG AON GPIOs (counting unused lines)
Total: 544

I suspect that the upper limit will only need to be higher in the
future, so set it to 1024.

Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com>
---
New in v2.

 arch/arm/Kconfig | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index e717642..bbe6bf7 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1509,7 +1509,8 @@ config ARM_PSCI
 # selected platforms.
 config ARCH_NR_GPIO
 	int
-	default 1024 if ARCH_SHMOBILE || ARCH_TEGRA || ARCH_ZYNQ
+	default 1024 if ARCH_BRCMSTB || ARCH_SHMOBILE || ARCH_TEGRA || \
+		ARCH_ZYNQ
 	default 512 if ARCH_EXYNOS || ARCH_KEYSTONE || SOC_OMAP5 || \
 		SOC_DRA7XX || ARCH_S3C24XX || ARCH_S3C64XX || ARCH_S5PV210
 	default 416 if ARCH_SUNXI
-- 
1.9.1

Re: [PATCH v2 0/6] GPIO support for BRCMSTB

[Florian Fainelli]

On 28/05/15 19:14, Gregory Fong wrote:
> This patchset adds support for the GPIO controller (UPG GIO) used on Broadcom's
> various BRCMSTB SoCs (BCM7XXX and others). It uses the "basic-mmio-gpio"
> interface to try to reduce duplication of the base logic.
> 
> For all existing hardware, this block hooked up to the BCM7120 L2 IRQ
> controller and so will require CONFIG_BCM7120_L2_IRQ=y.

Unless there are objections, I will take patches 5 and 6 and apply them
to soc/next.

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

Thanks!

> 
> New in v2:
> - fix license mismatch as pointed out by Paul Bolle
> - move select ARCH_WANT_OPTIONAL_GPIOLIB to separate patch
> - change to have 32 lines per bank per Linus Walleij's comments
> - allow this controller to be used as a wakeup source
> - add default GPIO number for BRCMSTB
> 
> The device tree bindings from v1 were merged to the GPIO tree, so this patchset
> only contains an addition to allow GPIOs to be used as a wakeup source
> (patch 3).  The initial bindings from v1 can be found at
> https://lkml.org/lkml/2015/5/6/200 .
> 
> Gregory Fong (6):
>   gpio: Add GPIO support for Broadcom STB SoCs
>   gpio: brcmstb: Add interrupt support
>   dt-bindings: brcmstb-gpio: document properties for wakeup
>   gpio: brcmstb: Allow GPIOs to be wakeup sources
>   ARM: brcmstb: Select ARCH_WANT_OPTIONAL_GPIOLIB
>   ARM: brcmstb: Add default gpio number
> 
>  .../devicetree/bindings/gpio/brcm,brcmstb-gpio.txt |  26 +-
>  MAINTAINERS                                        |   7 +
>  arch/arm/Kconfig                                   |   3 +-
>  arch/arm/mach-bcm/Kconfig                          |   1 +
>  drivers/gpio/Kconfig                               |   9 +
>  drivers/gpio/Makefile                              |   1 +
>  drivers/gpio/gpio-brcmstb.c                        | 581 +++++++++++++++++++++
>  7 files changed, 626 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/gpio/gpio-brcmstb.c
> 


-- 
Florian

Re: [PATCH v2 5/6] ARM: brcmstb: Select ARCH_WANT_OPTIONAL_GPIOLIB

[Florian Fainelli]

On 28/05/15 19:14, Gregory Fong wrote:
> Select ARCH_WANT_OPTIONAL_GPIOLIB from BRCMSTB to allow GPIOLIB and
> GPIO_BRCMSTB to be enabled.
> 
> Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com>

Applied to soc/next, thanks!
-- 
Florian

Re: [PATCH v2 6/6] ARM: brcmstb: Add default gpio number

[Florian Fainelli]

On 28/05/15 19:14, Gregory Fong wrote:
> Out of the brcmstb SoCs that I know, BCM3390 has the largest numbers
> of GPIOs, with its
> - 320 "peripheral" GPIOs
> - 5*32 = 160 UPG GPIOs (counting unused lines, which do get counted)
> - 2*32 = 64 UPG AON GPIOs (counting unused lines)
> Total: 544
> 
> I suspect that the upper limit will only need to be higher in the
> future, so set it to 1024.
> 
> Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com>

Applied to soc/next, thanks!
-- 
Florian

Re: [PATCH v2 2/6] gpio: brcmstb: Add interrupt support

[Brian Norris]

A few small comments:

On Thu, May 28, 2015 at 07:14:06PM -0700, Gregory Fong wrote:
> v2:
> - since imask member of bank struct was removed, just read and write from mask
>   reg and don't maintain a shadow

^^ this comment may be addressing what I'm going to ask about below? Not
sure why this was changed, actually.

> - warn on invalid IRQs
> - move some irq setup to a separate function since probe is getting unwieldy
> 
>  drivers/gpio/Kconfig        |   1 +
>  drivers/gpio/gpio-brcmstb.c | 276 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 277 insertions(+)
> 
...
> diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
> index 7a3cb1f..b9962ff 100644
> --- a/drivers/gpio/gpio-brcmstb.c
> +++ b/drivers/gpio/gpio-brcmstb.c
...
> @@ -63,6 +69,231 @@ brcmstb_gpio_gc_to_priv(struct gpio_chip *gc)
...
> +static void brcmstb_gpio_irq_bank_handler(int irq,
> +		struct brcmstb_gpio_bank *bank)
> +{
> +	struct brcmstb_gpio_priv *priv = bank->parent_priv;
> +	void __iomem *reg_base = priv->reg_base;
> +	unsigned long status;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&bank->bgc.lock, flags);
> +	while ((status = bank->bgc.read_reg(reg_base + GIO_STAT(bank->id)) &
> +			 bank->bgc.read_reg(reg_base + GIO_MASK(bank->id)))) {

In case you do run this loop multiple times (multiple interrupts in
progress?), wouldn't it make sense to stash the mask exactly once,
outside the loop? It's probably not a real big deal in practice, I
guess.

> +		int bit;
> +		for_each_set_bit(bit, &status, 32) {
> +			int hwirq = bank->bgc.gc.base -
> +				priv->gpio_base + bit;
> +			int child_irq =
> +				irq_find_mapping(priv->irq_domain,
> +						 hwirq);
> +			u32 stat = bank->bgc.read_reg(reg_base +
> +						      GIO_STAT(bank->id));
> +			if (bit >= bank->width)
> +				dev_warn(&priv->pdev->dev,
> +					 "IRQ for invalid GPIO (bank=%d, offset=%d)\n",
> +					 bank->id, bit);
> +			bank->bgc.write_reg(reg_base + GIO_STAT(bank->id),
> +					    stat | BIT(bit));
> +			generic_handle_irq(child_irq);
> +		}
> +	}
> +	spin_unlock_irqrestore(&bank->bgc.lock, flags);
> +}
...
> @@ -153,6 +410,16 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
>  	priv->reg_base = reg_base;
>  	priv->pdev = pdev;
>  
> +	if (of_find_property(np, "interrupt-controller", NULL)) {

of_property_read_bool()?

> +		priv->parent_irq = platform_get_irq(pdev, 0);
> +		if (priv->parent_irq < 0) {
> +			dev_err(dev, "Couldn't get IRQ");
> +			return -ENOENT;
> +		}
> +	} else {
> +		priv->parent_irq = -ENOENT;
> +	}
> +
>  	INIT_LIST_HEAD(&priv->bank_list);
>  	if (brcmstb_gpio_sanity_check_banks(dev, np, res))
>  		return -EINVAL;

Otherwise, looks OK to my inexpert eyes.

Reviewed-by: Brian Norris <computersforpeace@gmail.com>

Re: [PATCH v2 3/6] dt-bindings: brcmstb-gpio: document properties for wakeup

[Brian Norris]

On Thu, May 28, 2015 at 07:14:07PM -0700, Gregory Fong wrote:
> Some brcmstb GPIO controllers can be used to wake from suspend, so use the
> de facto standard property 'wakeup-source' to mark the nodes of controllers
> with that capability.
> 
> Also document interrupts-extended, which will be used for wakeup handling
> because the interrupt parent for the wake IRQ is different from the regular
> IRQ.
> 
> Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com>
> ---
> New in v2.
> 
>  .../devicetree/bindings/gpio/brcm,brcmstb-gpio.txt | 26 +++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt b/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt
> index 435f1bc..568814f 100644
> --- a/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt
> +++ b/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt
> @@ -33,6 +33,12 @@ Optional properties:
>  - interrupt-parent:
>      phandle of the parent interrupt controller
>  
> +- interrupts-extended:
> +    Alternate form of specifying interrupts and parents that allows for
> +    multiple parents.  This takes precedence over 'interrupts' and
> +    'interrupt-parent'.  This probably must be used if the wakeup-source
> +    property is provided because that may have a different interrupt parent.
> +

"This probably must be used" seems a little awkward, especially when
you're just explaining an implementation detail of our SoCs, rather than
something unique about this binding. Maybe:

  "Wakeup-capable GPIO controllers often route their wakeup interrupt
  lines through a different interrupt controller than the primary
  interrupt line, making this property necessary."

>  - #interrupt-cells:
>      Should be <2>.  The first cell is the GPIO number, the second should specify
>      flags.  The following subset of flags is supported:
> @@ -48,7 +54,10 @@ Optional properties:
>      Marks the device node as an interrupt controller
>  
>  - interrupt-names:
> -    The name of the IRQ resource used by this controller
> +    The names of the IRQ resources used by this controller

If you're specifying names, you should list them here.

> +
> +- wakeup-source:
> +    GPIOs for this controller can be used as a wakeup source
>  
>  Example:
>  	upg_gio: gpio@f040a700 {
> @@ -63,3 +72,18 @@ Example:
>  		interrupt-names = "upg_gio";
>  		brcm,gpio-bank-widths = <0x20 0x20 0x20 0x18>;
>  	};
> +
> +	upg_gio_aon: gpio@f04172c0 {
> +		#gpio-cells = <0x2>;
> +		#interrupt-cells = <0x2>;

Might use decimal instead of hex for the above 2 lines?

> +		compatible = "brcm,bcm7445-gpio", "brcm,brcmstb-gpio";
> +		gpio-controller;
> +		interrupt-controller;
> +		reg = <0xf04172c0 0x40>;
> +		interrupt-parent = <0xc>;

That should be a phandle, not an int (I realize phandles resolve down to
an integer, but we're speaking DTS, not DTB).

> +		interrupts = <0x6>;
> +		interrupts-extended = <0xc 0x6 0xa 0x5>;

Same here (phandles).

Also, even though the interrupt binding semantics specify precedence
between interrupts and interrupts-extended, I'd think an example should
stick to one or the other, no?

> +		interrupt-names = "upg_gio_aon", "upg_gio_aon_wakeup";
> +		wakeup-source;
> +		brcm,gpio-bank-widths = <0x12 0x4>;
> +	};

Reviewed-by: Brian Norris <computersforpeace@gmail.com>

Re: [PATCH v2 4/6] gpio: brcmstb: Allow GPIOs to be wakeup sources

[Brian Norris]

On Thu, May 28, 2015 at 07:14:08PM -0700, Gregory Fong wrote:
> Several drivers (e.g. gpio-keys) allow for GPIOs to be configured as
> wakeup sources, and this GPIO controller supports that through a
> separate interrupt path.
> 
> The de-facto standard DT property "wakeup-source" is checked, since
> that indicates whether the GPIO controller hardware can wake.  Uses
> the IRQCHIP_MASK_ON_SUSPEND irq_chip flag because UPG GIO doesn't have
> any of its own wakeup source configuration.
> 
> Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com>
> ---
> New in v2.
> 
>  drivers/gpio/gpio-brcmstb.c | 53 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
> index b9962ff..2598c1e 100644
> --- a/drivers/gpio/gpio-brcmstb.c
> +++ b/drivers/gpio/gpio-brcmstb.c
> @@ -19,6 +19,7 @@
>  #include <linux/basic_mmio_gpio.h>
>  #include <linux/irqdomain.h>
>  #include <linux/irqchip/chained_irq.h>
> +#include <linux/interrupt.h>
>  
>  #define GIO_BANK_SIZE           0x20
>  #define GIO_ODEN(bank)          (((bank) * GIO_BANK_SIZE) + 0x00)
> @@ -48,6 +49,7 @@ struct brcmstb_gpio_priv {
>  	struct irq_domain *irq_domain;
>  	int parent_irq;
>  	int gpio_base;
> +	int parent_wake_irq;
>  };
>  
>  #define MAX_GPIO_PER_BANK           32
> @@ -183,6 +185,31 @@ static int brcmstb_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>  	return 0;
>  }
>  
> +static int brcmstb_gpio_irq_set_wake(struct irq_data *d, unsigned int enable)
> +{
> +	struct brcmstb_gpio_bank *bank = irq_data_get_irq_chip_data(d);
> +	struct brcmstb_gpio_priv *priv = bank->parent_priv;
> +
> +	/*
> +	 * Only enable wake IRQ once for however many hwirqs can wake
> +	 * since they all use the same wake IRQ.  Mask will be set
> +	 * up appropriately thanks to IRQCHIP_MASK_ON_SUSPEND flag.
> +	 */
> +	if (enable)
> +		enable_irq_wake(priv->parent_wake_irq);
> +	else
> +		disable_irq_wake(priv->parent_wake_irq);
> +	return 0;
> +}
> +
> +static irqreturn_t brcmstb_gpio_wake_irq_handler(int irq, void *data)
> +{
> +	struct brcmstb_gpio_priv *priv = data;
> +
> +	pm_wakeup_event(&priv->pdev->dev, 0);
> +	return IRQ_HANDLED;
> +}
> +
>  static void brcmstb_gpio_irq_bank_handler(int irq,
>  		struct brcmstb_gpio_bank *bank)
>  {
> @@ -369,6 +396,32 @@ static int brcmstb_gpio_irq_setup(struct platform_device *pdev,
>  	priv->irq_chip.irq_mask = brcmstb_gpio_irq_mask;
>  	priv->irq_chip.irq_unmask = brcmstb_gpio_irq_unmask;
>  	priv->irq_chip.irq_set_type = brcmstb_gpio_irq_set_type;
> +
> +	/* Ensures that all non-wakeup IRQs are disabled at suspend */
> +	priv->irq_chip.flags = IRQCHIP_MASK_ON_SUSPEND;
> +
> +	if (IS_ENABLED(CONFIG_PM_SLEEP) &&
> +			of_get_property(np, "wakeup-source", NULL)) {

of_property_read_bool()?

> +		priv->parent_wake_irq = platform_get_irq(pdev, 1);
> +		if (priv->parent_wake_irq < 0)  {
> +			dev_warn(dev,
> +				"Couldn't get wake IRQ - GPIOs will not be able to wake from sleep");
> +		} else {
> +			int err = devm_request_irq(dev, priv->parent_wake_irq,
> +					brcmstb_gpio_wake_irq_handler, 0,
> +					"brcmstb-gpio-wake", priv);
> +
> +			if (err < 0) {
> +				dev_err(dev, "Couldn't request wake IRQ");
> +				return err;
> +			}
> +
> +			device_set_wakeup_capable(dev, true);
> +			device_wakeup_enable(dev);

Might want to move these two lines above the devm_request_irq(), so that
you're ready to record PM events immediately at probe time. This is
important when waking from S5 states, where we sometimes want to be able
to check the /sys/devices/.../wakeup_count stats to see what woke us up.

> +			priv->irq_chip.irq_set_wake = brcmstb_gpio_irq_set_wake;
> +		}
> +	}
> +
>  	priv->irq_domain =
>  		irq_domain_add_linear(np, priv->num_gpios,
>  				      &brcmstb_gpio_irq_domain_ops,

With that:

Reviewed-by: Brian Norris <computersforpeace@gmail.com>

Re: [PATCH v2 3/6] dt-bindings: brcmstb-gpio: document properties for wakeup

[Gregory Fong]

On Fri, May 29, 2015 at 5:36 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
> On Thu, May 28, 2015 at 07:14:07PM -0700, Gregory Fong wrote:
>> Some brcmstb GPIO controllers can be used to wake from suspend, so use the
>> de facto standard property 'wakeup-source' to mark the nodes of controllers
>> with that capability.
>>
>> Also document interrupts-extended, which will be used for wakeup handling
>> because the interrupt parent for the wake IRQ is different from the regular
>> IRQ.
>>
>> Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com>
>> ---
>> New in v2.
>>
>>  .../devicetree/bindings/gpio/brcm,brcmstb-gpio.txt | 26 +++++++++++++++++++++-
>>  1 file changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt b/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt
>> index 435f1bc..568814f 100644
>> --- a/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt
>> +++ b/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt
>> @@ -33,6 +33,12 @@ Optional properties:
>>  - interrupt-parent:
>>      phandle of the parent interrupt controller
>>
>> +- interrupts-extended:
>> +    Alternate form of specifying interrupts and parents that allows for
>> +    multiple parents.  This takes precedence over 'interrupts' and
>> +    'interrupt-parent'.  This probably must be used if the wakeup-source
>> +    property is provided because that may have a different interrupt parent.
>> +
>
> "This probably must be used" seems a little awkward, especially when
> you're just explaining an implementation detail of our SoCs, rather than
> something unique about this binding. Maybe:
>
>   "Wakeup-capable GPIO controllers often route their wakeup interrupt
>   lines through a different interrupt controller than the primary
>   interrupt line, making this property necessary."

That wording does seem better, will change.

>
>>  - #interrupt-cells:
>>      Should be <2>.  The first cell is the GPIO number, the second should specify
>>      flags.  The following subset of flags is supported:
>> @@ -48,7 +54,10 @@ Optional properties:
>>      Marks the device node as an interrupt controller
>>
>>  - interrupt-names:
>> -    The name of the IRQ resource used by this controller
>> +    The names of the IRQ resources used by this controller
>
> If you're specifying names, you should list them here.

I was wondering about that.  Some bindings have them listed, some
don't.  In this case I know what names currently exist but there could
certainly be different ones in the future.  How does that work?  Or am
I misunderstanding what this field is used for?  Where are the
documented rules for this?

>
>> +
>> +- wakeup-source:
>> +    GPIOs for this controller can be used as a wakeup source
>>
>>  Example:
>>       upg_gio: gpio@f040a700 {
>> @@ -63,3 +72,18 @@ Example:
>>               interrupt-names = "upg_gio";
>>               brcm,gpio-bank-widths = <0x20 0x20 0x20 0x18>;
>>       };
>> +
>> +     upg_gio_aon: gpio@f04172c0 {
>> +             #gpio-cells = <0x2>;
>> +             #interrupt-cells = <0x2>;
>
> Might use decimal instead of hex for the above 2 lines?

Sure.

>
>> +             compatible = "brcm,bcm7445-gpio", "brcm,brcmstb-gpio";
>> +             gpio-controller;
>> +             interrupt-controller;
>> +             reg = <0xf04172c0 0x40>;
>> +             interrupt-parent = <0xc>;
>
> That should be a phandle, not an int (I realize phandles resolve down to
> an integer, but we're speaking DTS, not DTB).

OK.

>
>> +             interrupts = <0x6>;
>> +             interrupts-extended = <0xc 0x6 0xa 0x5>;
>
> Same here (phandles).
>
> Also, even though the interrupt binding semantics specify precedence
> between interrupts and interrupts-extended, I'd think an example should
> stick to one or the other, no?

This is the output that we actually get from the bootloader.  But
regardless, IMO the example should have both cases: precedence is
well-defined, both sets of information are valid, and the driver can
handle the case where interrupts-extended is not an understood
property.

>
>> +             interrupt-names = "upg_gio_aon", "upg_gio_aon_wakeup";
>> +             wakeup-source;
>> +             brcm,gpio-bank-widths = <0x12 0x4>;
>> +     };
>
> Reviewed-by: Brian Norris <computersforpeace@gmail.com>

Thanks for the review,
Gregory

Re: [PATCH v2 2/6] gpio: brcmstb: Add interrupt support

[Gregory Fong]

On Fri, May 29, 2015 at 5:10 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
> A few small comments:
>
> On Thu, May 28, 2015 at 07:14:06PM -0700, Gregory Fong wrote:
>> v2:
>> - since imask member of bank struct was removed, just read and write from mask
>>   reg and don't maintain a shadow
>
> ^^ this comment may be addressing what I'm going to ask about below? Not
> sure why this was changed, actually.

Yes, see below...

>
>> - warn on invalid IRQs
>> - move some irq setup to a separate function since probe is getting unwieldy
>>
>>  drivers/gpio/Kconfig        |   1 +
>>  drivers/gpio/gpio-brcmstb.c | 276 ++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 277 insertions(+)
>>
> ...
>> diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
>> index 7a3cb1f..b9962ff 100644
>> --- a/drivers/gpio/gpio-brcmstb.c
>> +++ b/drivers/gpio/gpio-brcmstb.c
> ...
>> @@ -63,6 +69,231 @@ brcmstb_gpio_gc_to_priv(struct gpio_chip *gc)
> ...
>> +static void brcmstb_gpio_irq_bank_handler(int irq,
>> +             struct brcmstb_gpio_bank *bank)
>> +{
>> +     struct brcmstb_gpio_priv *priv = bank->parent_priv;
>> +     void __iomem *reg_base = priv->reg_base;
>> +     unsigned long status;
>> +     unsigned long flags;
>> +
>> +     spin_lock_irqsave(&bank->bgc.lock, flags);
>> +     while ((status = bank->bgc.read_reg(reg_base + GIO_STAT(bank->id)) &
>> +                      bank->bgc.read_reg(reg_base + GIO_MASK(bank->id)))) {
>
> In case you do run this loop multiple times (multiple interrupts in
> progress?), wouldn't it make sense to stash the mask exactly once,
> outside the loop? It's probably not a real big deal in practice, I
> guess.

I made this change after Linus's remark at
https://lkml.org/lkml/2015/5/12/303 on v1, which I agree with mostly
since it's a premature optimization---I haven't determined whether
keeping a shadow mask actually helps performance at all in practice,
and better to keep it simpler without actual data.

>
>> +             int bit;
>> +             for_each_set_bit(bit, &status, 32) {
>> +                     int hwirq = bank->bgc.gc.base -
>> +                             priv->gpio_base + bit;
>> +                     int child_irq =
>> +                             irq_find_mapping(priv->irq_domain,
>> +                                              hwirq);
>> +                     u32 stat = bank->bgc.read_reg(reg_base +
>> +                                                   GIO_STAT(bank->id));
>> +                     if (bit >= bank->width)
>> +                             dev_warn(&priv->pdev->dev,
>> +                                      "IRQ for invalid GPIO (bank=%d, offset=%d)\n",
>> +                                      bank->id, bit);
>> +                     bank->bgc.write_reg(reg_base + GIO_STAT(bank->id),
>> +                                         stat | BIT(bit));
>> +                     generic_handle_irq(child_irq);
>> +             }
>> +     }
>> +     spin_unlock_irqrestore(&bank->bgc.lock, flags);
>> +}
> ...
>> @@ -153,6 +410,16 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
>>       priv->reg_base = reg_base;
>>       priv->pdev = pdev;
>>
>> +     if (of_find_property(np, "interrupt-controller", NULL)) {
>
> of_property_read_bool()?

OK.

>
>> +             priv->parent_irq = platform_get_irq(pdev, 0);
>> +             if (priv->parent_irq < 0) {
>> +                     dev_err(dev, "Couldn't get IRQ");
>> +                     return -ENOENT;
>> +             }
>> +     } else {
>> +             priv->parent_irq = -ENOENT;
>> +     }
>> +
>>       INIT_LIST_HEAD(&priv->bank_list);
>>       if (brcmstb_gpio_sanity_check_banks(dev, np, res))
>>               return -EINVAL;
>
> Otherwise, looks OK to my inexpert eyes.
>
> Reviewed-by: Brian Norris <computersforpeace@gmail.com>

Re: [PATCH v2 3/6] dt-bindings: brcmstb-gpio: document properties for wakeup

[Brian Norris]

On Fri, May 29, 2015 at 05:57:50PM -0700, Gregory Fong wrote:
> On Fri, May 29, 2015 at 5:36 PM, Brian Norris
> <computersforpeace@gmail.com> wrote:
> > On Thu, May 28, 2015 at 07:14:07PM -0700, Gregory Fong wrote:
> >> --- a/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt
> >> +++ b/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt
> >> @@ -33,6 +33,12 @@ Optional properties:
...
> >>  - #interrupt-cells:
> >>      Should be <2>.  The first cell is the GPIO number, the second should specify
> >>      flags.  The following subset of flags is supported:
> >> @@ -48,7 +54,10 @@ Optional properties:
> >>      Marks the device node as an interrupt controller
> >>
> >>  - interrupt-names:
> >> -    The name of the IRQ resource used by this controller
> >> +    The names of the IRQ resources used by this controller
> >
> > If you're specifying names, you should list them here.
> 
> I was wondering about that.  Some bindings have them listed, some
> don't.  In this case I know what names currently exist but there could
> certainly be different ones in the future.  How does that work?  Or am
> I misunderstanding what this field is used for?  Where are the
> documented rules for this?

The only documentation I see is:
Documentation/devicetree/bindings/resource-names.txt

That documents the basics of the *-names properties, not their expected
usage.

In practice, they're only useful if you have enough optional resources
that fixed indexing isn't sufficient, and you need to use
platform_get_resource_byname().

So IMO, their purposes seems to be one of these:
(1) functional (e.g., for get_resource_byname(), when you have more than
    one optional resource)
(2) self-documentation (which might run counter to #1, as you begin
    generating too many unique names)
(3) no purpose

So IMO, if you ever want (1), they shouldn't have instance-specific
naming, but should use something generic to the device class. Otherwise,
they are just self-documentation, and aren't functionally useful. So
IMO, these sorts of names:

	interrupt-names = "upg_gio_aon", "upg_gio_aon_wakeup";

work better as functional descriptions:

	interrupt-names = "gio", "wakeup";

But in the end, I wouldn't foresee you needing to do (1), so you're left
with (2) or (3), at which point I'm not sure if you should even mention
the property.

Just my 2 cents (and those cents may not even be worth face value),
Brian

Re: [PATCH v2 3/6] dt-bindings: brcmstb-gpio: document properties for wakeup

[Gregory Fong]

On Fri, May 29, 2015 at 6:37 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
> On Fri, May 29, 2015 at 05:57:50PM -0700, Gregory Fong wrote:
>> On Fri, May 29, 2015 at 5:36 PM, Brian Norris
>> <computersforpeace@gmail.com> wrote:
>> > On Thu, May 28, 2015 at 07:14:07PM -0700, Gregory Fong wrote:
>> >> --- a/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt
>> >> +++ b/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt
>> >> @@ -33,6 +33,12 @@ Optional properties:
> ...
>> >>  - #interrupt-cells:
>> >>      Should be <2>.  The first cell is the GPIO number, the second should specify
>> >>      flags.  The following subset of flags is supported:
>> >> @@ -48,7 +54,10 @@ Optional properties:
>> >>      Marks the device node as an interrupt controller
>> >>
>> >>  - interrupt-names:
>> >> -    The name of the IRQ resource used by this controller
>> >> +    The names of the IRQ resources used by this controller
>> >
>> > If you're specifying names, you should list them here.
>>
>> I was wondering about that.  Some bindings have them listed, some
>> don't.  In this case I know what names currently exist but there could
>> certainly be different ones in the future.  How does that work?  Or am
>> I misunderstanding what this field is used for?  Where are the
>> documented rules for this?
>
> The only documentation I see is:
> Documentation/devicetree/bindings/resource-names.txt
>
> That documents the basics of the *-names properties, not their expected
> usage.
>
> In practice, they're only useful if you have enough optional resources
> that fixed indexing isn't sufficient, and you need to use
> platform_get_resource_byname().
>
> So IMO, their purposes seems to be one of these:
> (1) functional (e.g., for get_resource_byname(), when you have more than
>     one optional resource)
> (2) self-documentation (which might run counter to #1, as you begin
>     generating too many unique names)
> (3) no purpose
>
> So IMO, if you ever want (1), they shouldn't have instance-specific
> naming, but should use something generic to the device class. Otherwise,
> they are just self-documentation, and aren't functionally useful. So
> IMO, these sorts of names:
>
>         interrupt-names = "upg_gio_aon", "upg_gio_aon_wakeup";
>
> work better as functional descriptions:
>
>         interrupt-names = "gio", "wakeup";
>
> But in the end, I wouldn't foresee you needing to do (1), so you're left
> with (2) or (3), at which point I'm not sure if you should even mention
> the property.

I'm fine with leaving out the interrupt-names property, since we're
not using it here anyway.  Unless there are serious objections, I'll
plan on remove it from the bindings doc next round.

Thanks,
Gregory

Re: [PATCH v2 2/6] gpio: brcmstb: Add interrupt support

[Linus Walleij]

On Fri, May 29, 2015 at 4:14 AM, Gregory Fong <gregory.0xf0@gmail.com> wrote:

> Create an irq_chip for each GIO block.  Uses chained IRQ handling since
> known uses of this block have a BCM7120 L2 interrupt controller as a
> parent.  Supports interrupts for all GPIOs.
>
> In the IRQ handler, we check for raised IRQs for invalid GPIOs and warn
> (ratelimited) if they're encountered.
>
> Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com>
> ---
> v2:
> - since imask member of bank struct was removed, just read and write from mask
>   reg and don't maintain a shadow
> - warn on invalid IRQs
> - move some irq setup to a separate function since probe is getting unwieldy

This patch basically painfully reimplements GPIOLIB_IRQCHIP.

Is there a reason for not using that generic mechanism?

> +static int brcmstb_gpio_to_irq(struct gpio_chip *gc, unsigned gc_offset)
> +{
> +       struct brcmstb_gpio_priv *priv = brcmstb_gpio_gc_to_priv(gc);
> +       /* gc_offset is relative to this gpio_chip; want real offset */
> +       int offset = gc_offset + (gc->base - priv->gpio_base);
> +
> +       if (offset >= priv->num_gpios)
> +               return -ENXIO;
> +       return irq_create_mapping(priv->irq_domain, offset);

It is a no-no to rely on .to_irq() to be called before using an IRQ
from a GPIO chip.

The gpiochip and irq_chip APIs are orthogonal, and a user may
request an interrupt from the irqchip without calling .to_irq() on any
GPIO line first.

This should only use irq_find_mapping().

irq_create_mapping() needs to be called on all available
offsets during probe.

If the module is removable or at error path it also needs
to call irq_dispose_mapping() on all lines. Also this is handled
by GPIOLIB_IRQCHIP, and that is why it should always be
used, if possible.

Yours,
Linus Walleij

Re: [PATCH v2 1/6] gpio: Add GPIO support for Broadcom STB SoCs

[Linus Walleij]

On Fri, May 29, 2015 at 4:14 AM, Gregory Fong <gregory.0xf0@gmail.com> wrote:

> This adds support for the GPIO IP "UPG GIO" used on Broadcom STB SoCs
> (BCM7XXX and some others).  Uses basic_mmio_gpio to instantiate a
> gpio_chip for each bank.  The driver assumes that it handles the base
> set of GPIOs on the system and that it can start its numbering sequence
> from 0, so any GPIO expanders used with it must dynamically assign GPIO
> numbers after this driver has finished registering its GPIOs.
>
> Does not implement the interrupt-controller portion yet, will be done in a
> future commit.
>
> List-usage-fixed-by: Brian Norris <computersforpeace@gmail.com>
> Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com>
> ---
> v2:
> - change include to use <linux/gpio/driver.h> instead of <linux/gpio.h>
> - get rid of unnecessary imask member in struct bank
> - rename GPIO_PER_BANK -> MAX_GPIO_PER_BANK
> - always have 32 GPIOs per bank and add 'width' member in struct bank to hold
>   actual number of GPIOs in use
> - mark of_match table as const

This v2 version of 1/5 applied.

I recognize that you need one driver for all banks. I'm still hesitant
about the complexity it brings to e.g. the IRQ handling.

Another possibility would be to spawn one driver per bank anyways,
then have the IRQ requested as IRQF_SHARED so the IRQ
handler for the shared IRQ would walk all drivers IRQ handlers
and eventually figure out what GPIO fired it. But this
implementation is quicker I guess.

Yours,
Linus Walleij

Re: [PATCH v2 6/6] ARM: brcmstb: Add default gpio number

[Linus Walleij]

On Fri, May 29, 2015 at 4:14 AM, Gregory Fong <gregory.0xf0@gmail.com> wrote:

> Out of the brcmstb SoCs that I know, BCM3390 has the largest numbers
> of GPIOs, with its
> - 320 "peripheral" GPIOs
> - 5*32 = 160 UPG GPIOs (counting unused lines, which do get counted)
> - 2*32 = 64 UPG AON GPIOs (counting unused lines)
> Total: 544
>
> I suspect that the upper limit will only need to be higher in the
> future, so set it to 1024.
>
> Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v2 5/6] ARM: brcmstb: Select ARCH_WANT_OPTIONAL_GPIOLIB

[Linus Walleij]

On Fri, May 29, 2015 at 4:14 AM, Gregory Fong <gregory.0xf0@gmail.com> wrote:

> Select ARCH_WANT_OPTIONAL_GPIOLIB from BRCMSTB to allow GPIOLIB and
> GPIO_BRCMSTB to be enabled.
>
> Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v2 4/6] gpio: brcmstb: Allow GPIOs to be wakeup sources

[Linus Walleij]

On Fri, May 29, 2015 at 4:14 AM, Gregory Fong <gregory.0xf0@gmail.com> wrote:

> Several drivers (e.g. gpio-keys) allow for GPIOs to be configured as
> wakeup sources, and this GPIO controller supports that through a
> separate interrupt path.
>
> The de-facto standard DT property "wakeup-source" is checked, since
> that indicates whether the GPIO controller hardware can wake.  Uses
> the IRQCHIP_MASK_ON_SUSPEND irq_chip flag because UPG GIO doesn't have
> any of its own wakeup source configuration.
>
> Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com>

(...)
> +       if (enable)
> +               enable_irq_wake(priv->parent_wake_irq);
> +       else
> +               disable_irq_wake(priv->parent_wake_irq);
> +       return 0;

No error handling? If the code assumes these calls will
always succeed, atleast write that in a comment.

Yours,
Linus Walleij

Re: [PATCH v2 4/6] gpio: brcmstb: Allow GPIOs to be wakeup sources

[Gregory Fong]

On Fri, May 29, 2015 at 5:43 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
> On Thu, May 28, 2015 at 07:14:08PM -0700, Gregory Fong wrote:
>> Several drivers (e.g. gpio-keys) allow for GPIOs to be configured as
>> wakeup sources, and this GPIO controller supports that through a
>> separate interrupt path.
>>
>> The de-facto standard DT property "wakeup-source" is checked, since
>> that indicates whether the GPIO controller hardware can wake.  Uses
>> the IRQCHIP_MASK_ON_SUSPEND irq_chip flag because UPG GIO doesn't have
>> any of its own wakeup source configuration.
>>
>> Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com>
>> ---
>> New in v2.
>>
>>  drivers/gpio/gpio-brcmstb.c | 53 +++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 53 insertions(+)
>>
>> diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
>> index b9962ff..2598c1e 100644
>> --- a/drivers/gpio/gpio-brcmstb.c
>> +++ b/drivers/gpio/gpio-brcmstb.c
>> [...]
>> @@ -369,6 +396,32 @@ static int brcmstb_gpio_irq_setup(struct platform_device *pdev,
>>       priv->irq_chip.irq_mask = brcmstb_gpio_irq_mask;
>>       priv->irq_chip.irq_unmask = brcmstb_gpio_irq_unmask;
>>       priv->irq_chip.irq_set_type = brcmstb_gpio_irq_set_type;
>> +
>> +     /* Ensures that all non-wakeup IRQs are disabled at suspend */
>> +     priv->irq_chip.flags = IRQCHIP_MASK_ON_SUSPEND;
>> +
>> +     if (IS_ENABLED(CONFIG_PM_SLEEP) &&
>> +                     of_get_property(np, "wakeup-source", NULL)) {
>
> of_property_read_bool()?

Will change.

>
>> +             priv->parent_wake_irq = platform_get_irq(pdev, 1);
>> +             if (priv->parent_wake_irq < 0)  {
>> +                     dev_warn(dev,
>> +                             "Couldn't get wake IRQ - GPIOs will not be able to wake from sleep");
>> +             } else {
>> +                     int err = devm_request_irq(dev, priv->parent_wake_irq,
>> +                                     brcmstb_gpio_wake_irq_handler, 0,
>> +                                     "brcmstb-gpio-wake", priv);
>> +
>> +                     if (err < 0) {
>> +                             dev_err(dev, "Couldn't request wake IRQ");
>> +                             return err;
>> +                     }
>> +
>> +                     device_set_wakeup_capable(dev, true);
>> +                     device_wakeup_enable(dev);
>
> Might want to move these two lines above the devm_request_irq(), so that
> you're ready to record PM events immediately at probe time. This is
> important when waking from S5 states, where we sometimes want to be able
> to check the /sys/devices/.../wakeup_count stats to see what woke us up.

Makes sense.  We'll also need a reboot notifier for S5 to work.

>
>> +                     priv->irq_chip.irq_set_wake = brcmstb_gpio_irq_set_wake;
>> +             }
>> +     }
>> +
>>       priv->irq_domain =
>>               irq_domain_add_linear(np, priv->num_gpios,
>>                                     &brcmstb_gpio_irq_domain_ops,
>
> With that:
>
> Reviewed-by: Brian Norris <computersforpeace@gmail.com>

Thanks,
Gregory

Re: [PATCH v2 4/6] gpio: brcmstb: Allow GPIOs to be wakeup sources

[Gregory Fong]

On Tue, Jun 2, 2015 at 6:45 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, May 29, 2015 at 4:14 AM, Gregory Fong <gregory.0xf0@gmail.com> wrote:
>
>> Several drivers (e.g. gpio-keys) allow for GPIOs to be configured as
>> wakeup sources, and this GPIO controller supports that through a
>> separate interrupt path.
>>
>> The de-facto standard DT property "wakeup-source" is checked, since
>> that indicates whether the GPIO controller hardware can wake.  Uses
>> the IRQCHIP_MASK_ON_SUSPEND irq_chip flag because UPG GIO doesn't have
>> any of its own wakeup source configuration.
>>
>> Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com>
>
> (...)
>> +       if (enable)
>> +               enable_irq_wake(priv->parent_wake_irq);
>> +       else
>> +               disable_irq_wake(priv->parent_wake_irq);
>> +       return 0;
>
> No error handling? If the code assumes these calls will
> always succeed, atleast write that in a comment.

Will add error handling.

Thanks,
Gregory