linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v9 0/4] Add gpio/pinconf support to Broadcom Cygnus SoC
@ 2015-02-10 22:16 Ray Jui
  2015-02-10 22:16 ` [PATCH v9 1/4] pinctrl: Cygnus: define Broadcom Cygnus GPIO/PINCONF binding Ray Jui
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Ray Jui @ 2015-02-10 22:16 UTC (permalink / raw)
  To: Linus Walleij, Alexandre Courbot, Rob Herring, Pawel Moll,
	Mark Rutland, Ian Campbell, Kumar Gala, Grant Likely,
	Christian Daudt, Matt Porter, Florian Fainelli, Russell King,
	Joe Perches, Arnd Bergmann
  Cc: Scott Branden, Dmitry Torokhov, Anatol Pomazau, linux-kernel,
	linux-arm-kernel, linux-gpio, bcm-kernel-feedback-list,
	devicetree, Ray Jui

This patchset contains the initial GPIO/PINCONF support for the Broadcom
Cygnus SoC.

Cygnus has 3 GPIO controllers: 1) the ASIU GPIO; 2) the chipCommonG GPIO;
and 3) the ALWAYS-ON GPIO. All 3 types of GPIO controllers are supported by
the this driver.

All 3 Cygnus GPIO controllers support basic PINCONF functions such as bias
pull up, pull down, and drive strength configurations, when these pins are
muxed to GPIO.

Pins from the ASIU GPIO can be individually muxed to GPIO function, through
interaction with the Cygnus IOMUX controller.

Note this patchset has a dependency on the other patchset "Add pinctrl support
to Broadcom Cygnus SoC" that is also under review

Changes from v8:
 - Add code in function 'cygnus_gpio_pinmux_add_range' to free pinmux
   node resource by calling of_node_put
 - Drop .suppress_bind_attrs = true, since this is already done in
   platform_driver_probe

Changes from v7:
 - Use 'bool' instead of 'int' for flag that indicates pinmux support in the
   driver
 - Call put_device to drop reference to the pinmux dev after call to
   of_find_device_by_node
 - Replace kasprintf with devm_kasprintf and remove memory deallocation logic
   in the driver
 - Set suppress_bind_attrs to true for the driver

Changes from v6:
 - Move the driver from drivers/gpio/* to drivers/pinctrl/* since this driver
   supports both GPIO and some basic PINCONF features
 - Support PINCONF features through standard DT subnodes properties including
   "bias-disable", "bias-pull-up", "bias-pull-down", and "drive-strength", by
   creating local PINCONF controller
 - Add support to allow individual ASIU GPIO pins to be muxed as GPIO, through
   interactions with the Cygnus IOMUX driver
 - Convert the driver to use standard GPIOCHIP_IRQ APIs. This helps to reduce
   customized code in the driver
 - Other miscellaneous imrpovements in the driver
 - Enable GPIO based phone hook detection support for BCM911360 phone factor
   board

Changes from v5:
 - Get rid of DT property "linux,gpio-base". Use dynamic allocation for GPIO base
   number

Changes from v4:
 - Use DT property "linux,gpio-base" to define GPIO base number
 - factorize common code to improve code readability and reduce code size
 - remove "bcm_" prefix on function and struct names
 - improve debugging prints
 - default GPIO_BCM_CYGNUS to y in Kconfig (it still depends on
   ARCH_BCM_CYGNUS). This way we do not need to select it from the
   arch/arm/mach-bcm/Kconfig
 - Get rid of redundant MAINTAINER entry for this driver. It will be maintained
   by Broadcom iProc/Cygnus maintainers
 - Update device tree document based on driver changes

Changes from v3:
 - Fix dt property tpyo
 - Fix incorrect GPIO compatible ID in device tree binding document example

Changes from v2:
 - Consolidate different compatible IDs into "brcm,cygnus-gpio"
 - Get rid of redundant "no-interrupt" property

Changes from v1:
 - Get rid of inline qualifier
 - Get rid of redundant check in the ISR
 - Other minor fixes to imrove code readability

Ray Jui (4):
  pinctrl: Cygnus: define Broadcom Cygnus GPIO/PINCONF binding
  pinctrl: cygnus: add gpio/pinconf driver
  ARM: dts: enable GPIO for Broadcom Cygnus
  ARM: dts: cygnus: enable GPIO based hook detection

 .../bindings/pinctrl/brcm,cygnus-gpio.txt          |  102 +++
 arch/arm/boot/dts/bcm-cygnus.dtsi                  |   33 +
 arch/arm/boot/dts/bcm911360_entphn.dts             |   13 +
 drivers/pinctrl/bcm/Kconfig                        |   22 +
 drivers/pinctrl/bcm/Makefile                       |    1 +
 drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c          |  908 ++++++++++++++++++++
 6 files changed, 1079 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/brcm,cygnus-gpio.txt
 create mode 100644 drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c

-- 
1.7.9.5


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v9 1/4] pinctrl: Cygnus: define Broadcom Cygnus GPIO/PINCONF binding
  2015-02-10 22:16 [PATCH v9 0/4] Add gpio/pinconf support to Broadcom Cygnus SoC Ray Jui
@ 2015-02-10 22:16 ` Ray Jui
  2015-02-10 22:16 ` [PATCH v9 2/4] pinctrl: cygnus: add gpio/pinconf driver Ray Jui
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Ray Jui @ 2015-02-10 22:16 UTC (permalink / raw)
  To: Linus Walleij, Alexandre Courbot, Rob Herring, Pawel Moll,
	Mark Rutland, Ian Campbell, Kumar Gala, Grant Likely,
	Christian Daudt, Matt Porter, Florian Fainelli, Russell King,
	Joe Perches, Arnd Bergmann
  Cc: Scott Branden, Dmitry Torokhov, Anatol Pomazau, linux-kernel,
	linux-arm-kernel, linux-gpio, bcm-kernel-feedback-list,
	devicetree, Ray Jui

Document the GPIO/PINCONF device tree binding for Broadcom Cygnus SoC

Signed-off-by: Ray Jui <rjui@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
---
 .../bindings/pinctrl/brcm,cygnus-gpio.txt          |  102 ++++++++++++++++++++
 1 file changed, 102 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/brcm,cygnus-gpio.txt

diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,cygnus-gpio.txt b/Documentation/devicetree/bindings/pinctrl/brcm,cygnus-gpio.txt
new file mode 100644
index 0000000..9b9196c
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/brcm,cygnus-gpio.txt
@@ -0,0 +1,102 @@
+Broadcom Cygnus GPIO/PINCONF Controller
+
+Required properties:
+
+- compatible:
+    Must be "brcm,cygnus-gpio"
+
+- reg:
+    Define the base and range of the I/O address space that contains the Cygnus
+GPIO/PINCONF controller registers
+
+- ngpios:
+    Total number of GPIOs the controller provides
+
+- #gpio-cells:
+    Must be two. The first cell is the GPIO pin number (within the
+controller's pin space) and the second cell is used for the following:
+    bit[0]: polarity (0 for active high and 1 for active low)
+
+- gpio-controller:
+    Specifies that the node is a GPIO controller
+
+Optional properties:
+
+- interrupts:
+    Interrupt ID
+
+- interrupt-controller:
+    Specifies that the node is an interrupt controller
+
+- pinmux:
+    Specifies the phandle to the IOMUX device, where pins can be individually
+muxed to GPIO
+
+Supported generic PINCONF properties in child nodes:
+
+- pins:
+    The list of pins (within the controller's own pin space) that properties
+in the node apply to. Pin names are "gpio-<pin>"
+
+- bias-disable:
+    Disable pin bias
+
+- bias-pull-up:
+    Enable internal pull up resistor
+
+- bias-pull-down:
+    Enable internal pull down resistor
+
+- drive-strength:
+    Valid drive strength values include 2, 4, 6, 8, 10, 12, 14, 16 (mA)
+
+Example:
+	gpio_ccm: gpio@1800a000 {
+		compatible = "brcm,cygnus-gpio";
+		reg = <0x1800a000 0x50>,
+		      <0x0301d164 0x20>;
+		ngpios = <24>;
+		#gpio-cells = <2>;
+		gpio-controller;
+		interrupts = <GIC_SPI 84 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-controller;
+
+		touch_pins: touch_pins {
+			pwr: pwr {
+				pins = "gpio-0";
+				drive-strength = <16>;
+			};
+
+			event: event {
+				pins = "gpio-1";
+				bias-pull-up;
+			};
+		};
+	};
+
+	gpio_asiu: gpio@180a5000 {
+		compatible = "brcm,cygnus-gpio";
+		reg = <0x180a5000 0x668>;
+		ngpios = <146>;
+		#gpio-cells = <2>;
+		gpio-controller;
+		interrupts = <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-controller;
+	};
+
+	/*
+	 * Touchscreen that uses the CCM GPIO 0 and 1
+	 */
+	tsc {
+		...
+		...
+		gpio-pwr = <&gpio_ccm 0 0>;
+		gpio-event = <&gpio_ccm 1 0>;
+	};
+
+	/* Bluetooth that uses the ASIU GPIO 5, with polarity inverted */
+	bluetooth {
+		...
+		...
+		bcm,rfkill-bank-sel = <&gpio_asiu 5 1>
+	}
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v9 2/4] pinctrl: cygnus: add gpio/pinconf driver
  2015-02-10 22:16 [PATCH v9 0/4] Add gpio/pinconf support to Broadcom Cygnus SoC Ray Jui
  2015-02-10 22:16 ` [PATCH v9 1/4] pinctrl: Cygnus: define Broadcom Cygnus GPIO/PINCONF binding Ray Jui
@ 2015-02-10 22:16 ` Ray Jui
  2015-03-04  9:48   ` Linus Walleij
  2015-02-10 22:16 ` [PATCH v9 3/4] ARM: dts: enable GPIO for Broadcom Cygnus Ray Jui
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 9+ messages in thread
From: Ray Jui @ 2015-02-10 22:16 UTC (permalink / raw)
  To: Linus Walleij, Alexandre Courbot, Rob Herring, Pawel Moll,
	Mark Rutland, Ian Campbell, Kumar Gala, Grant Likely,
	Christian Daudt, Matt Porter, Florian Fainelli, Russell King,
	Joe Perches, Arnd Bergmann
  Cc: Scott Branden, Dmitry Torokhov, Anatol Pomazau, linux-kernel,
	linux-arm-kernel, linux-gpio, bcm-kernel-feedback-list,
	devicetree, Ray Jui

This adds the initial support of the Broadcom Cygnus GPIO/PINCONF driver
that supports all 3 GPIO controllers on Cygnus including the ASIU GPIO
controller, the chipCommonG GPIO controller, and the always-on GPIO
controller. Basic PINCONF configurations such as bias pull up/down, and
drive strength are also supported in this driver.

Pins from the ASIU GPIO controller can be individually muxed to GPIO
function, through interaction with the Cygnus IOMUX controller

Signed-off-by: Ray Jui <rjui@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
---
 drivers/pinctrl/bcm/Kconfig               |   22 +
 drivers/pinctrl/bcm/Makefile              |    1 +
 drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c |  908 +++++++++++++++++++++++++++++
 3 files changed, 931 insertions(+)
 create mode 100644 drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c

diff --git a/drivers/pinctrl/bcm/Kconfig b/drivers/pinctrl/bcm/Kconfig
index eb13201..cd11d4d 100644
--- a/drivers/pinctrl/bcm/Kconfig
+++ b/drivers/pinctrl/bcm/Kconfig
@@ -20,6 +20,28 @@ config PINCTRL_BCM2835
 	select PINMUX
 	select PINCONF
 
+config PINCTRL_CYGNUS_GPIO
+	bool "Broadcom Cygnus GPIO (with PINCONF) driver"
+	depends on OF_GPIO && ARCH_BCM_CYGNUS
+	select GPIOLIB_IRQCHIP
+	select PINCONF
+	select GENERIC_PINCONF
+	default ARCH_BCM_CYGNUS
+	help
+	  Say yes here to enable the Broadcom Cygnus GPIO driver.
+
+	  The Broadcom Cygnus SoC has 3 GPIO controllers including the ASIU
+	  GPIO controller (ASIU), the chipCommonG GPIO controller (CCM), and
+	  the always-ON GPIO controller (CRMU/AON). All 3 GPIO controllers are
+	  supported by this driver.
+
+	  All 3 Cygnus GPIO controllers support basic PINCONF functions such
+	  as bias pull up, pull down, and drive strength configurations, when
+	  these pins are muxed to GPIO.
+
+	  Pins from the ASIU GPIO can be individually muxed to GPIO function,
+	  through interaction with the Cygnus IOMUX controller.
+
 config PINCTRL_CYGNUS_MUX
 	bool "Broadcom Cygnus IOMUX driver"
 	depends on (ARCH_BCM_CYGNUS || COMPILE_TEST)
diff --git a/drivers/pinctrl/bcm/Makefile b/drivers/pinctrl/bcm/Makefile
index bb6beb6..2b2f70e 100644
--- a/drivers/pinctrl/bcm/Makefile
+++ b/drivers/pinctrl/bcm/Makefile
@@ -2,4 +2,5 @@
 
 obj-$(CONFIG_PINCTRL_BCM281XX)		+= pinctrl-bcm281xx.o
 obj-$(CONFIG_PINCTRL_BCM2835)		+= pinctrl-bcm2835.o
+obj-$(CONFIG_PINCTRL_CYGNUS_GPIO)	+= pinctrl-cygnus-gpio.o
 obj-$(CONFIG_PINCTRL_CYGNUS_MUX)	+= pinctrl-cygnus-mux.o
diff --git a/drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c b/drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c
new file mode 100644
index 0000000..f2e5aec
--- /dev/null
+++ b/drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c
@@ -0,0 +1,908 @@
+/*
+ * Copyright (C) 2014-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.
+ *
+ * This file contains the Broadcom Cygnus GPIO driver that supports 3
+ * GPIO controllers on Cygnus including the ASIU GPIO controller, the
+ * chipCommonG GPIO controller, and the always-on GPIO controller. Basic
+ * PINCONF such as bias pull up/down, and drive strength are also supported
+ * in this driver.
+ *
+ * Pins from the ASIU GPIO can be individually muxed to GPIO function,
+ * through the interaction with the Cygnus IOMUX controller
+ */
+
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/gpio.h>
+#include <linux/ioport.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinconf-generic.h>
+
+#include "../pinctrl-utils.h"
+
+#define CYGNUS_GPIO_DATA_IN_OFFSET   0x00
+#define CYGNUS_GPIO_DATA_OUT_OFFSET  0x04
+#define CYGNUS_GPIO_OUT_EN_OFFSET    0x08
+#define CYGNUS_GPIO_IN_TYPE_OFFSET   0x0c
+#define CYGNUS_GPIO_INT_DE_OFFSET    0x10
+#define CYGNUS_GPIO_INT_EDGE_OFFSET  0x14
+#define CYGNUS_GPIO_INT_MSK_OFFSET   0x18
+#define CYGNUS_GPIO_INT_STAT_OFFSET  0x1c
+#define CYGNUS_GPIO_INT_MSTAT_OFFSET 0x20
+#define CYGNUS_GPIO_INT_CLR_OFFSET   0x24
+#define CYGNUS_GPIO_PAD_RES_OFFSET   0x34
+#define CYGNUS_GPIO_RES_EN_OFFSET    0x38
+
+/* drive strength control for ASIU GPIO */
+#define CYGNUS_GPIO_ASIU_DRV0_CTRL_OFFSET 0x58
+
+/* drive strength control for CCM/CRMU (AON) GPIO */
+#define CYGNUS_GPIO_DRV0_CTRL_OFFSET  0x00
+
+#define GPIO_BANK_SIZE 0x200
+#define NGPIOS_PER_BANK 32
+#define GPIO_BANK(pin) ((pin) / NGPIOS_PER_BANK)
+
+#define CYGNUS_GPIO_REG(pin, reg) (GPIO_BANK(pin) * GPIO_BANK_SIZE + (reg))
+#define CYGNUS_GPIO_SHIFT(pin) ((pin) % NGPIOS_PER_BANK)
+
+#define GPIO_DRV_STRENGTH_BIT_SHIFT  20
+#define GPIO_DRV_STRENGTH_BITS       3
+#define GPIO_DRV_STRENGTH_BIT_MASK   ((1 << GPIO_DRV_STRENGTH_BITS) - 1)
+
+/*
+ * Cygnus GPIO core
+ *
+ * @dev: pointer to device
+ * @base: I/O register base for Cygnus GPIO controller
+ * @io_ctrl: I/O register base for certain type of Cygnus GPIO controller that
+ * has the PINCONF support implemented outside of the GPIO block
+ * @lock: lock to protect access to I/O registers
+ * @gc: GPIO chip
+ * @num_banks: number of GPIO banks, each bank supports up to 32 GPIOs
+ * @pinmux_is_supported: flag to indicate this GPIO controller contains pins
+ * that can be individually muxed to GPIO
+ * @pctl: pointer to pinctrl_dev
+ * @pctldesc: pinctrl descriptor
+ */
+struct cygnus_gpio {
+	struct device *dev;
+
+	void __iomem *base;
+	void __iomem *io_ctrl;
+
+	spinlock_t lock;
+
+	struct gpio_chip gc;
+	unsigned num_banks;
+
+	bool pinmux_is_supported;
+
+	struct pinctrl_dev *pctl;
+	struct pinctrl_desc pctldesc;
+};
+
+static struct cygnus_gpio *to_cygnus_gpio(struct gpio_chip *gc)
+{
+	return container_of(gc, struct cygnus_gpio, gc);
+}
+
+/*
+ * Mapping from PINCONF pins to GPIO pins is 1-to-1
+ */
+static unsigned cygnus_pin_to_gpio(unsigned pin)
+{
+	return pin;
+}
+
+static u32 cygnus_readl(struct cygnus_gpio *chip, unsigned int offset)
+{
+	return readl(chip->base + offset);
+}
+
+static void cygnus_writel(struct cygnus_gpio *chip, unsigned int offset,
+			  u32 val)
+{
+	writel(val, chip->base + offset);
+}
+
+/**
+ *  cygnus_set_bit - set or clear one bit (corresponding to the GPIO pin) in a
+ *  Cygnus GPIO register
+ *
+ *  @cygnus_gpio: Cygnus GPIO device
+ *  @reg: register offset
+ *  @gpio: GPIO pin
+ *  @set: set or clear. 1 - set; 0 -clear
+ */
+static void cygnus_set_bit(struct cygnus_gpio *chip, unsigned int reg,
+			   unsigned gpio, int set)
+{
+	unsigned int offset = CYGNUS_GPIO_REG(gpio, reg);
+	unsigned int shift = CYGNUS_GPIO_SHIFT(gpio);
+	u32 val;
+
+	val = cygnus_readl(chip, offset);
+	if (set)
+		val |= BIT(shift);
+	else
+		val &= ~BIT(shift);
+	cygnus_writel(chip, offset, val);
+}
+
+static int cygnus_get_bit(struct cygnus_gpio *chip, unsigned int reg,
+			  unsigned gpio)
+{
+	unsigned int offset = CYGNUS_GPIO_REG(gpio, reg);
+	unsigned int shift = CYGNUS_GPIO_SHIFT(gpio);
+	u32 val;
+
+	val = cygnus_readl(chip, offset) & BIT(shift);
+	if (val)
+		return 1;
+	else
+		return 0;
+}
+
+static void cygnus_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
+{
+	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
+	struct cygnus_gpio *chip = to_cygnus_gpio(gc);
+	struct irq_chip *irq_chip = irq_desc_get_chip(desc);
+	int i, bit;
+
+	chained_irq_enter(irq_chip, desc);
+
+	/* go through the entire GPIO banks and handle all interrupts */
+	for (i = 0; i < chip->num_banks; i++) {
+		unsigned long val = cygnus_readl(chip,
+				(i * GPIO_BANK_SIZE) +
+				CYGNUS_GPIO_INT_MSTAT_OFFSET);
+
+		for_each_set_bit(bit, &val, NGPIOS_PER_BANK) {
+			unsigned pin = NGPIOS_PER_BANK * i + bit;
+			int child_irq = irq_find_mapping(gc->irqdomain, pin);
+
+			/*
+			 * Clear the interrupt before invoking the
+			 * handler, so we do not leave any window
+			 */
+			cygnus_writel(chip, (i * GPIO_BANK_SIZE) +
+				      CYGNUS_GPIO_INT_CLR_OFFSET, BIT(bit));
+
+			generic_handle_irq(child_irq);
+		}
+	}
+
+	chained_irq_exit(irq_chip, desc);
+}
+
+
+static void cygnus_gpio_irq_ack(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct cygnus_gpio *chip = to_cygnus_gpio(gc);
+	unsigned gpio = d->hwirq;
+	unsigned int offset = CYGNUS_GPIO_REG(gpio,
+			CYGNUS_GPIO_INT_CLR_OFFSET);
+	unsigned int shift = CYGNUS_GPIO_SHIFT(gpio);
+	u32 val = BIT(shift);
+
+	cygnus_writel(chip, offset, val);
+}
+
+/**
+ *  cygnus_gpio_irq_set_mask - mask/unmask a GPIO interrupt
+ *
+ *  @d: IRQ chip data
+ *  @mask: mask/unmask GPIO interrupt. 0 - mask (disable); 1 - unmask (enable)
+ */
+static void cygnus_gpio_irq_set_mask(struct irq_data *d, int mask)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct cygnus_gpio *chip = to_cygnus_gpio(gc);
+	unsigned gpio = d->hwirq;
+
+	cygnus_set_bit(chip, CYGNUS_GPIO_INT_MSK_OFFSET, gpio, mask);
+}
+
+static void cygnus_gpio_irq_mask(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct cygnus_gpio *chip = to_cygnus_gpio(gc);
+	unsigned long flags;
+
+	spin_lock_irqsave(&chip->lock, flags);
+	cygnus_gpio_irq_set_mask(d, 0);
+	spin_unlock_irqrestore(&chip->lock, flags);
+}
+
+static void cygnus_gpio_irq_unmask(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct cygnus_gpio *chip = to_cygnus_gpio(gc);
+	unsigned long flags;
+
+	spin_lock_irqsave(&chip->lock, flags);
+	cygnus_gpio_irq_set_mask(d, 1);
+	spin_unlock_irqrestore(&chip->lock, flags);
+}
+
+static int cygnus_gpio_irq_set_type(struct irq_data *d, unsigned int type)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct cygnus_gpio *chip = to_cygnus_gpio(gc);
+	unsigned gpio = d->hwirq;
+	int int_type = 0, dual_edge = 0, edge_lvl = 0;
+	unsigned long flags;
+
+	switch (type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_EDGE_RISING:
+		edge_lvl = 1;
+		break;
+
+	case IRQ_TYPE_EDGE_FALLING:
+		break;
+
+	case IRQ_TYPE_EDGE_BOTH:
+		dual_edge = 1;
+		break;
+
+	case IRQ_TYPE_LEVEL_HIGH:
+		int_type = 1;
+		edge_lvl = 1;
+		break;
+
+	case IRQ_TYPE_LEVEL_LOW:
+		int_type = 1;
+		break;
+
+	default:
+		dev_err(chip->dev, "invalid GPIO IRQ type 0x%x\n",
+			type);
+		return -EINVAL;
+	}
+
+	spin_lock_irqsave(&chip->lock, flags);
+	cygnus_set_bit(chip, CYGNUS_GPIO_IN_TYPE_OFFSET, gpio, int_type);
+	cygnus_set_bit(chip, CYGNUS_GPIO_INT_DE_OFFSET, gpio, dual_edge);
+	cygnus_set_bit(chip, CYGNUS_GPIO_INT_EDGE_OFFSET, gpio,
+		       edge_lvl);
+	spin_unlock_irqrestore(&chip->lock, flags);
+
+	dev_dbg(chip->dev,
+		"gpio:%u set int_type:%d dual_edge:%d edge_lvl:%d\n",
+		gpio, int_type, dual_edge, edge_lvl);
+
+	return 0;
+}
+
+static struct irq_chip cygnus_gpio_irq_chip = {
+	.name = "bcm-cygnus-gpio",
+	.irq_ack = cygnus_gpio_irq_ack,
+	.irq_mask = cygnus_gpio_irq_mask,
+	.irq_unmask = cygnus_gpio_irq_unmask,
+	.irq_set_type = cygnus_gpio_irq_set_type,
+};
+
+/*
+ * Request the Cygnus IOMUX pinmux controller to mux individual pins to GPIO
+ */
+static int cygnus_gpio_request(struct gpio_chip *gc, unsigned offset)
+{
+	struct cygnus_gpio *chip = to_cygnus_gpio(gc);
+	unsigned gpio = gc->base + offset;
+
+	/* not all Cygnus GPIO pins can be muxed individually */
+	if (!chip->pinmux_is_supported)
+		return 0;
+
+	return pinctrl_request_gpio(gpio);
+}
+
+static void cygnus_gpio_free(struct gpio_chip *gc, unsigned offset)
+{
+	struct cygnus_gpio *chip = to_cygnus_gpio(gc);
+	unsigned gpio = gc->base + offset;
+
+	if (!chip->pinmux_is_supported)
+		return;
+
+	pinctrl_free_gpio(gpio);
+}
+
+static int cygnus_gpio_direction_input(struct gpio_chip *gc, unsigned gpio)
+{
+	struct cygnus_gpio *chip = to_cygnus_gpio(gc);
+	unsigned long flags;
+
+	spin_lock_irqsave(&chip->lock, flags);
+	cygnus_set_bit(chip, CYGNUS_GPIO_OUT_EN_OFFSET, gpio, 0);
+	spin_unlock_irqrestore(&chip->lock, flags);
+
+	dev_dbg(chip->dev, "gpio:%u set input\n", gpio);
+
+	return 0;
+}
+
+static int cygnus_gpio_direction_output(struct gpio_chip *gc, unsigned gpio,
+					int value)
+{
+	struct cygnus_gpio *chip = to_cygnus_gpio(gc);
+	unsigned long flags;
+
+	spin_lock_irqsave(&chip->lock, flags);
+	cygnus_set_bit(chip, CYGNUS_GPIO_OUT_EN_OFFSET, gpio, 1);
+	cygnus_set_bit(chip, CYGNUS_GPIO_DATA_OUT_OFFSET, gpio, value);
+	spin_unlock_irqrestore(&chip->lock, flags);
+
+	dev_dbg(chip->dev, "gpio:%u set output, value:%d\n", gpio, value);
+
+	return 0;
+}
+
+static void cygnus_gpio_set(struct gpio_chip *gc, unsigned gpio, int value)
+{
+	struct cygnus_gpio *chip = to_cygnus_gpio(gc);
+	unsigned long flags;
+
+	spin_lock_irqsave(&chip->lock, flags);
+	cygnus_set_bit(chip, CYGNUS_GPIO_DATA_OUT_OFFSET, gpio, value);
+	spin_unlock_irqrestore(&chip->lock, flags);
+
+	dev_dbg(chip->dev, "gpio:%u set, value:%d\n", gpio, value);
+}
+
+static int cygnus_gpio_get(struct gpio_chip *gc, unsigned gpio)
+{
+	struct cygnus_gpio *chip = to_cygnus_gpio(gc);
+	unsigned int offset = CYGNUS_GPIO_REG(gpio,
+					      CYGNUS_GPIO_DATA_IN_OFFSET);
+	unsigned int shift = CYGNUS_GPIO_SHIFT(gpio);
+
+	return !!(cygnus_readl(chip, offset) & BIT(shift));
+}
+
+static int cygnus_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	return 1;
+}
+
+/*
+ * Only one group: "gpio_grp", since this local pinctrl device only performs
+ * GPIO specific PINCONF configurations
+ */
+static const char *cygnus_get_group_name(struct pinctrl_dev *pctldev,
+					 unsigned selector)
+{
+	return "gpio_grp";
+}
+
+static const struct pinctrl_ops cygnus_pctrl_ops = {
+	.get_groups_count = cygnus_get_groups_count,
+	.get_group_name = cygnus_get_group_name,
+	.dt_node_to_map = pinconf_generic_dt_node_to_map_pin,
+	.dt_free_map = pinctrl_utils_dt_free_map,
+};
+
+static int cygnus_gpio_set_pull(struct cygnus_gpio *chip, unsigned gpio,
+				int disable, int pull_up)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&chip->lock, flags);
+
+	if (disable) {
+		cygnus_set_bit(chip, CYGNUS_GPIO_RES_EN_OFFSET, gpio, 0);
+	} else {
+		cygnus_set_bit(chip, CYGNUS_GPIO_PAD_RES_OFFSET, gpio,
+			       pull_up);
+		cygnus_set_bit(chip, CYGNUS_GPIO_RES_EN_OFFSET, gpio, 1);
+	}
+
+	spin_unlock_irqrestore(&chip->lock, flags);
+
+	dev_dbg(chip->dev, "gpio:%u set pullup:%d\n", gpio, pull_up);
+
+	return 0;
+}
+
+static void cygnus_gpio_get_pull(struct cygnus_gpio *chip, unsigned gpio,
+				 int *disable, int *pull_up)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&chip->lock, flags);
+	*disable = !cygnus_get_bit(chip, CYGNUS_GPIO_RES_EN_OFFSET, gpio);
+	*pull_up = cygnus_get_bit(chip, CYGNUS_GPIO_PAD_RES_OFFSET, gpio);
+	spin_unlock_irqrestore(&chip->lock, flags);
+}
+
+static int cygnus_gpio_set_strength(struct cygnus_gpio *chip, unsigned gpio,
+				    unsigned strength)
+{
+	void __iomem *base;
+	unsigned int i, offset, shift;
+	u32 val;
+	unsigned long flags;
+
+	/* make sure drive strength is supported */
+	if (strength < 2 ||  strength > 16 || (strength % 2))
+		return -ENOTSUPP;
+
+	if (chip->io_ctrl) {
+		base = chip->io_ctrl;
+		offset = CYGNUS_GPIO_DRV0_CTRL_OFFSET;
+	} else {
+		base = chip->base;
+		offset = CYGNUS_GPIO_REG(gpio,
+					 CYGNUS_GPIO_ASIU_DRV0_CTRL_OFFSET);
+	}
+
+	shift = CYGNUS_GPIO_SHIFT(gpio);
+
+	dev_dbg(chip->dev, "gpio:%u set drive strength:%d mA\n", gpio,
+		strength);
+
+	spin_lock_irqsave(&chip->lock, flags);
+	strength = (strength / 2) - 1;
+	for (i = 0; i < GPIO_DRV_STRENGTH_BITS; i++) {
+		val = readl(base + offset);
+		val &= ~BIT(shift);
+		val |= ((strength >> i) & 0x1) << shift;
+		writel(val, base + offset);
+		offset += 4;
+	}
+	spin_unlock_irqrestore(&chip->lock, flags);
+
+	return 0;
+}
+
+static int cygnus_gpio_get_strength(struct cygnus_gpio *chip, unsigned gpio,
+				    u16 *strength)
+{
+	void __iomem *base;
+	unsigned int i, offset, shift;
+	u32 val;
+	unsigned long flags;
+
+	if (chip->io_ctrl) {
+		base = chip->io_ctrl;
+		offset = CYGNUS_GPIO_DRV0_CTRL_OFFSET;
+	} else {
+		base = chip->base;
+		offset = CYGNUS_GPIO_REG(gpio,
+					 CYGNUS_GPIO_ASIU_DRV0_CTRL_OFFSET);
+	}
+
+	shift = CYGNUS_GPIO_SHIFT(gpio);
+
+	spin_lock_irqsave(&chip->lock, flags);
+	*strength = 0;
+	for (i = 0; i < GPIO_DRV_STRENGTH_BITS; i++) {
+		val = readl(base + offset) & BIT(shift);
+		val >>= shift;
+		*strength += (val << i);
+		offset += 4;
+	}
+
+	/* convert to mA */
+	*strength = (*strength + 1) * 2;
+	spin_unlock_irqrestore(&chip->lock, flags);
+
+	return 0;
+}
+
+static int cygnus_pin_config_get(struct pinctrl_dev *pctldev, unsigned pin,
+				 unsigned long *config)
+{
+	struct cygnus_gpio *chip = pinctrl_dev_get_drvdata(pctldev);
+	enum pin_config_param param = pinconf_to_config_param(*config);
+	unsigned gpio = cygnus_pin_to_gpio(pin);
+	u16 arg;
+	int disable, pull_up, ret;
+
+	switch (param) {
+	case PIN_CONFIG_BIAS_DISABLE:
+		cygnus_gpio_get_pull(chip, gpio, &disable, &pull_up);
+		if (disable)
+			return 0;
+		else
+			return -EINVAL;
+
+	case PIN_CONFIG_BIAS_PULL_UP:
+		cygnus_gpio_get_pull(chip, gpio, &disable, &pull_up);
+		if (!disable && pull_up)
+			return 0;
+		else
+			return -EINVAL;
+
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		cygnus_gpio_get_pull(chip, gpio, &disable, &pull_up);
+		if (!disable && !pull_up)
+			return 0;
+		else
+			return -EINVAL;
+
+	case PIN_CONFIG_DRIVE_STRENGTH:
+		ret = cygnus_gpio_get_strength(chip, gpio, &arg);
+		if (ret)
+			return ret;
+		else
+			*config = pinconf_to_config_packed(param, arg);
+
+		return 0;
+
+	default:
+		return -ENOTSUPP;
+	}
+
+	return -ENOTSUPP;
+}
+
+static int cygnus_pin_config_set(struct pinctrl_dev *pctldev, unsigned pin,
+				 unsigned long *configs, unsigned num_configs)
+{
+	struct cygnus_gpio *chip = pinctrl_dev_get_drvdata(pctldev);
+	enum pin_config_param param;
+	u16 arg;
+	unsigned i, gpio = cygnus_pin_to_gpio(pin);
+	int ret = -ENOTSUPP;
+
+	for (i = 0; i < num_configs; i++) {
+		param = pinconf_to_config_param(configs[i]);
+		arg = pinconf_to_config_argument(configs[i]);
+
+		switch (param) {
+		case PIN_CONFIG_BIAS_DISABLE:
+			ret = cygnus_gpio_set_pull(chip, gpio, 1, 0);
+			if (ret < 0)
+				goto out;
+			break;
+
+		case PIN_CONFIG_BIAS_PULL_UP:
+			ret = cygnus_gpio_set_pull(chip, gpio, 0, 1);
+			if (ret < 0)
+				goto out;
+			break;
+
+		case PIN_CONFIG_BIAS_PULL_DOWN:
+			ret = cygnus_gpio_set_pull(chip, gpio, 0, 0);
+			if (ret < 0)
+				goto out;
+			break;
+
+		case PIN_CONFIG_DRIVE_STRENGTH:
+			ret = cygnus_gpio_set_strength(chip, gpio, arg);
+			if (ret < 0)
+				goto out;
+			break;
+
+		default:
+			dev_err(chip->dev, "invalid configuration\n");
+			return -ENOTSUPP;
+		}
+	} /* for each config */
+
+out:
+	return ret;
+}
+
+static const struct pinconf_ops cygnus_pconf_ops = {
+	.is_generic = true,
+	.pin_config_get = cygnus_pin_config_get,
+	.pin_config_set = cygnus_pin_config_set,
+};
+
+/*
+ * Map a GPIO in the local gpio_chip pin space to a pin in the Cygnus IOMUX
+ * pinctrl pin space
+ */
+struct cygnus_gpio_pin_range {
+	unsigned offset;
+	unsigned pin_base;
+	unsigned num_pins;
+};
+
+#define CYGNUS_PINRANGE(o, p, n) { .offset = o, .pin_base = p, .num_pins = n }
+
+/*
+ * Pin mapping table for mapping local GPIO pins to Cygnus IOMUX pinctrl pins
+ */
+static const struct cygnus_gpio_pin_range cygnus_gpio_pintable[] = {
+	CYGNUS_PINRANGE(0, 42, 1),
+	CYGNUS_PINRANGE(1, 44, 3),
+	CYGNUS_PINRANGE(4, 48, 1),
+	CYGNUS_PINRANGE(5, 50, 3),
+	CYGNUS_PINRANGE(8, 126, 1),
+	CYGNUS_PINRANGE(9, 155, 1),
+	CYGNUS_PINRANGE(10, 152, 1),
+	CYGNUS_PINRANGE(11, 154, 1),
+	CYGNUS_PINRANGE(12, 153, 1),
+	CYGNUS_PINRANGE(13, 127, 3),
+	CYGNUS_PINRANGE(16, 140, 1),
+	CYGNUS_PINRANGE(17, 145, 7),
+	CYGNUS_PINRANGE(24, 130, 10),
+	CYGNUS_PINRANGE(34, 141, 4),
+	CYGNUS_PINRANGE(38, 54, 1),
+	CYGNUS_PINRANGE(39, 56, 3),
+	CYGNUS_PINRANGE(42, 60, 3),
+	CYGNUS_PINRANGE(45, 64, 3),
+	CYGNUS_PINRANGE(48, 68, 2),
+	CYGNUS_PINRANGE(50, 84, 6),
+	CYGNUS_PINRANGE(56, 94, 6),
+	CYGNUS_PINRANGE(62, 72, 1),
+	CYGNUS_PINRANGE(63, 70, 1),
+	CYGNUS_PINRANGE(64, 80, 1),
+	CYGNUS_PINRANGE(65, 74, 3),
+	CYGNUS_PINRANGE(68, 78, 1),
+	CYGNUS_PINRANGE(69, 82, 1),
+	CYGNUS_PINRANGE(70, 156, 17),
+	CYGNUS_PINRANGE(87, 104, 12),
+	CYGNUS_PINRANGE(99, 102, 2),
+	CYGNUS_PINRANGE(101, 90, 4),
+	CYGNUS_PINRANGE(105, 116, 10),
+	CYGNUS_PINRANGE(123, 11, 1),
+	CYGNUS_PINRANGE(124, 38, 4),
+	CYGNUS_PINRANGE(128, 43, 1),
+	CYGNUS_PINRANGE(129, 47, 1),
+	CYGNUS_PINRANGE(130, 49, 1),
+	CYGNUS_PINRANGE(131, 53, 1),
+	CYGNUS_PINRANGE(132, 55, 1),
+	CYGNUS_PINRANGE(133, 59, 1),
+	CYGNUS_PINRANGE(134, 63, 1),
+	CYGNUS_PINRANGE(135, 67, 1),
+	CYGNUS_PINRANGE(136, 71, 1),
+	CYGNUS_PINRANGE(137, 73, 1),
+	CYGNUS_PINRANGE(138, 77, 1),
+	CYGNUS_PINRANGE(139, 79, 1),
+	CYGNUS_PINRANGE(140, 81, 1),
+	CYGNUS_PINRANGE(141, 83, 1),
+	CYGNUS_PINRANGE(142, 10, 1)
+};
+
+/*
+ * The Cygnus IOMUX controller mainly supports group based mux configuration,
+ * but certain pins can be muxed to GPIO individually. Only the ASIU GPIO
+ * controller can support this, so it's an optional configuration
+ *
+ * Return -ENODEV means no support and that's fine
+ */
+static int cygnus_gpio_pinmux_add_range(struct cygnus_gpio *chip)
+{
+	struct device_node *node = chip->dev->of_node;
+	struct device_node *pinmux_node;
+	struct platform_device *pinmux_pdev;
+	struct gpio_chip *gc = &chip->gc;
+	int i, ret = 0;
+
+	/* parse DT to find the phandle to the pinmux controller */
+	pinmux_node = of_parse_phandle(node, "pinmux", 0);
+	if (!pinmux_node)
+		return -ENODEV;
+
+	pinmux_pdev = of_find_device_by_node(pinmux_node);
+	/* no longer need the pinmux node */
+	of_node_put(pinmux_node);
+	if (!pinmux_pdev) {
+		dev_err(chip->dev, "failed to get pinmux device\n");
+		return -EINVAL;
+	}
+
+	/* now need to create the mapping between local GPIO and PINMUX pins */
+	for (i = 0; i < ARRAY_SIZE(cygnus_gpio_pintable); i++) {
+		ret = gpiochip_add_pin_range(gc, dev_name(&pinmux_pdev->dev),
+					     cygnus_gpio_pintable[i].offset,
+					     cygnus_gpio_pintable[i].pin_base,
+					     cygnus_gpio_pintable[i].num_pins);
+		if (ret) {
+			dev_err(chip->dev, "unable to add GPIO pin range\n");
+			goto err_put_device;
+		}
+	}
+
+	chip->pinmux_is_supported = (ret == 0);
+
+	/* no need for pinmux_pdev device reference anymore */
+	put_device(&pinmux_pdev->dev);
+	return 0;
+
+err_put_device:
+	put_device(&pinmux_pdev->dev);
+	gpiochip_remove_pin_ranges(gc);
+	return ret;
+}
+
+static void cygnus_gpio_pinmux_remove_range(struct cygnus_gpio *chip)
+{
+	struct gpio_chip *gc = &chip->gc;
+
+	if (chip->pinmux_is_supported)
+		gpiochip_remove_pin_ranges(gc);
+}
+
+/*
+ * Cygnus GPIO controller supports some PINCONF related configurations such as
+ * pull up, pull down, and drive strength, when the pin is configured to GPIO
+ *
+ * Here a local pinctrl device is created with simple 1-to-1 pin mapping to the
+ * local GPIO pins
+ */
+static int cygnus_gpio_register_pinconf(struct cygnus_gpio *chip)
+{
+	struct pinctrl_desc *pctldesc = &chip->pctldesc;
+	struct pinctrl_pin_desc *pins;
+	struct gpio_chip *gc = &chip->gc;
+	int i;
+
+	pins = devm_kcalloc(chip->dev, gc->ngpio, sizeof(*pins), GFP_KERNEL);
+	if (!pins)
+		return -ENOMEM;
+
+	for (i = 0; i < gc->ngpio; i++) {
+		pins[i].number = i;
+		pins[i].name = devm_kasprintf(chip->dev, GFP_KERNEL,
+					      "gpio-%d", i);
+		if (!pins[i].name)
+			return -ENOMEM;
+	}
+
+	pctldesc->name = dev_name(chip->dev);
+	pctldesc->pctlops = &cygnus_pctrl_ops;
+	pctldesc->pins = pins;
+	pctldesc->npins = gc->ngpio;
+	pctldesc->confops = &cygnus_pconf_ops;
+
+	chip->pctl = pinctrl_register(pctldesc, chip->dev, chip);
+	if (!chip->pctl) {
+		dev_err(chip->dev, "unable to register pinctrl device\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void cygnus_gpio_unregister_pinconf(struct cygnus_gpio *chip)
+{
+	if (chip->pctl)
+		pinctrl_unregister(chip->pctl);
+}
+
+static const struct of_device_id cygnus_gpio_of_match[] = {
+	{ .compatible = "brcm,cygnus-gpio" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, cygnus_gpio_of_match);
+
+static int cygnus_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	struct cygnus_gpio *chip;
+	struct gpio_chip *gc;
+	u32 ngpios;
+	int irq, ret;
+
+	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	chip->dev = dev;
+	platform_set_drvdata(pdev, chip);
+
+	if (of_property_read_u32(dev->of_node, "ngpios", &ngpios)) {
+		dev_err(dev, "missing ngpios DT property\n");
+		return -ENODEV;
+	}
+	chip->num_banks = (ngpios + NGPIOS_PER_BANK - 1) / NGPIOS_PER_BANK;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	chip->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(chip->base)) {
+		dev_err(dev, "unable to map I/O memory\n");
+		return PTR_ERR(chip->base);
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (res) {
+		chip->io_ctrl = devm_ioremap_resource(dev, res);
+		if (IS_ERR(chip->io_ctrl)) {
+			dev_err(dev, "unable to map I/O memory\n");
+			return PTR_ERR(chip->io_ctrl);
+		}
+	}
+
+	spin_lock_init(&chip->lock);
+
+	gc = &chip->gc;
+	gc->base = -1;
+	gc->ngpio = ngpios;
+	gc->label = dev_name(dev);
+	gc->dev = dev;
+	gc->of_node = dev->of_node;
+	gc->request = cygnus_gpio_request;
+	gc->free = cygnus_gpio_free;
+	gc->direction_input = cygnus_gpio_direction_input;
+	gc->direction_output = cygnus_gpio_direction_output;
+	gc->set = cygnus_gpio_set;
+	gc->get = cygnus_gpio_get;
+
+	ret = gpiochip_add(gc);
+	if (ret < 0) {
+		dev_err(dev, "unable to add GPIO chip\n");
+		return ret;
+	}
+
+	ret = cygnus_gpio_pinmux_add_range(chip);
+	if (ret && ret != -ENODEV) {
+		dev_err(dev, "unable to add GPIO pin range\n");
+		goto err_rm_gpiochip;
+	}
+
+	ret = cygnus_gpio_register_pinconf(chip);
+	if (ret) {
+		dev_err(dev, "unable to register pinconf\n");
+		goto err_rm_range;
+	}
+
+	/* optional GPIO interrupt support */
+	irq = platform_get_irq(pdev, 0);
+	if (irq) {
+		ret = gpiochip_irqchip_add(gc, &cygnus_gpio_irq_chip, 0,
+					   handle_simple_irq, IRQ_TYPE_NONE);
+		if (ret) {
+			dev_err(dev, "no GPIO irqchip\n");
+			goto err_unregister_pinconf;
+		}
+
+		gpiochip_set_chained_irqchip(gc, &cygnus_gpio_irq_chip, irq,
+					     cygnus_gpio_irq_handler);
+	}
+
+	return 0;
+
+err_unregister_pinconf:
+	cygnus_gpio_unregister_pinconf(chip);
+
+err_rm_range:
+	cygnus_gpio_pinmux_remove_range(chip);
+
+err_rm_gpiochip:
+	gpiochip_remove(gc);
+
+	return ret;
+}
+
+static struct platform_driver cygnus_gpio_driver = {
+	.driver = {
+		.name = "cygnus-gpio",
+		.of_match_table = cygnus_gpio_of_match,
+	},
+	.probe = cygnus_gpio_probe,
+};
+
+static int __init cygnus_gpio_init(void)
+{
+	return platform_driver_probe(&cygnus_gpio_driver, cygnus_gpio_probe);
+}
+arch_initcall_sync(cygnus_gpio_init);
+
+MODULE_AUTHOR("Ray Jui <rjui@broadcom.com>");
+MODULE_DESCRIPTION("Broadcom Cygnus GPIO Driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v9 3/4] ARM: dts: enable GPIO for Broadcom Cygnus
  2015-02-10 22:16 [PATCH v9 0/4] Add gpio/pinconf support to Broadcom Cygnus SoC Ray Jui
  2015-02-10 22:16 ` [PATCH v9 1/4] pinctrl: Cygnus: define Broadcom Cygnus GPIO/PINCONF binding Ray Jui
  2015-02-10 22:16 ` [PATCH v9 2/4] pinctrl: cygnus: add gpio/pinconf driver Ray Jui
@ 2015-02-10 22:16 ` Ray Jui
  2015-02-10 22:16 ` [PATCH v9 4/4] ARM: dts: cygnus: enable GPIO based hook detection Ray Jui
  2015-02-25 19:30 ` [PATCH v9 0/4] Add gpio/pinconf support to Broadcom Cygnus SoC Dmitry Torokhov
  4 siblings, 0 replies; 9+ messages in thread
From: Ray Jui @ 2015-02-10 22:16 UTC (permalink / raw)
  To: Linus Walleij, Alexandre Courbot, Rob Herring, Pawel Moll,
	Mark Rutland, Ian Campbell, Kumar Gala, Grant Likely,
	Christian Daudt, Matt Porter, Florian Fainelli, Russell King,
	Joe Perches, Arnd Bergmann
  Cc: Scott Branden, Dmitry Torokhov, Anatol Pomazau, linux-kernel,
	linux-arm-kernel, linux-gpio, bcm-kernel-feedback-list,
	devicetree, Ray Jui

This enables all 3 GPIO controllers including the ASIU GPIO, the
chipcommonG GPIO, and the ALWAYS-ON GPIO, for Broadcom Cygnus SoC

Signed-off-by: Ray Jui <rjui@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
---
 arch/arm/boot/dts/bcm-cygnus.dtsi |   33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi b/arch/arm/boot/dts/bcm-cygnus.dtsi
index b014ce5..a3b8621 100644
--- a/arch/arm/boot/dts/bcm-cygnus.dtsi
+++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
@@ -60,6 +60,39 @@
 		      <0x0301d24c 0x2c>;
 	};
 
+	gpio_crmu: gpio@03024800 {
+		compatible = "brcm,cygnus-gpio";
+		reg = <0x03024800 0x50>,
+		      <0x03024008 0x18>;
+		ngpios = <6>;
+		#gpio-cells = <2>;
+		gpio-controller;
+	};
+
+	gpio_ccm: gpio@1800a000 {
+		compatible = "brcm,cygnus-gpio";
+		reg = <0x1800a000 0x50>,
+		      <0x0301d164 0x20>;
+		ngpios = <24>;
+		#gpio-cells = <2>;
+		gpio-controller;
+		interrupts = <GIC_SPI 84 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-controller;
+	};
+
+	gpio_asiu: gpio@180a5000 {
+		compatible = "brcm,cygnus-gpio";
+		reg = <0x180a5000 0x668>;
+		ngpios = <146>;
+		#gpio-cells = <2>;
+		gpio-controller;
+
+		pinmux = <&pinctrl>;
+
+		interrupt-controller;
+		interrupts = <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH>;
+	};
+
 	amba {
 		#address-cells = <1>;
 		#size-cells = <1>;
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v9 4/4] ARM: dts: cygnus: enable GPIO based hook detection
  2015-02-10 22:16 [PATCH v9 0/4] Add gpio/pinconf support to Broadcom Cygnus SoC Ray Jui
                   ` (2 preceding siblings ...)
  2015-02-10 22:16 ` [PATCH v9 3/4] ARM: dts: enable GPIO for Broadcom Cygnus Ray Jui
@ 2015-02-10 22:16 ` Ray Jui
  2015-02-25 19:30 ` [PATCH v9 0/4] Add gpio/pinconf support to Broadcom Cygnus SoC Dmitry Torokhov
  4 siblings, 0 replies; 9+ messages in thread
From: Ray Jui @ 2015-02-10 22:16 UTC (permalink / raw)
  To: Linus Walleij, Alexandre Courbot, Rob Herring, Pawel Moll,
	Mark Rutland, Ian Campbell, Kumar Gala, Grant Likely,
	Christian Daudt, Matt Porter, Florian Fainelli, Russell King,
	Joe Perches, Arnd Bergmann
  Cc: Scott Branden, Dmitry Torokhov, Anatol Pomazau, linux-kernel,
	linux-arm-kernel, linux-gpio, bcm-kernel-feedback-list,
	devicetree, Ray Jui

This enables GPIO based phone hook detection for Broadcom BCM911360
phone factor board (bcm911360_entphn)

Signed-off-by: Ray Jui <rjui@broadcom.com>
---
 arch/arm/boot/dts/bcm911360_entphn.dts |   13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/arm/boot/dts/bcm911360_entphn.dts b/arch/arm/boot/dts/bcm911360_entphn.dts
index d2ee952..7db4843 100644
--- a/arch/arm/boot/dts/bcm911360_entphn.dts
+++ b/arch/arm/boot/dts/bcm911360_entphn.dts
@@ -33,6 +33,7 @@
 /dts-v1/;
 
 #include "bcm-cygnus.dtsi"
+#include "dt-bindings/input/input.h"
 
 / {
 	model = "Cygnus Enterprise Phone (BCM911360_ENTPHN)";
@@ -50,4 +51,16 @@
 	uart3: serial@18023000 {
 		status = "okay";
 	};
+
+	gpio_keys {
+		compatible = "gpio-keys";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		hook {
+			label = "HOOK";
+			linux,code = <KEY_O>;
+			gpios = <&gpio_asiu 48 0>;
+		};
+	};
 };
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v9 0/4] Add gpio/pinconf support to Broadcom Cygnus SoC
  2015-02-10 22:16 [PATCH v9 0/4] Add gpio/pinconf support to Broadcom Cygnus SoC Ray Jui
                   ` (3 preceding siblings ...)
  2015-02-10 22:16 ` [PATCH v9 4/4] ARM: dts: cygnus: enable GPIO based hook detection Ray Jui
@ 2015-02-25 19:30 ` Dmitry Torokhov
  4 siblings, 0 replies; 9+ messages in thread
From: Dmitry Torokhov @ 2015-02-25 19:30 UTC (permalink / raw)
  To: Ray Jui
  Cc: Linus Walleij, Alexandre Courbot, Rob Herring, Pawel Moll,
	Mark Rutland, Ian Campbell, Kumar Gala, Grant Likely,
	Christian Daudt, Matt Porter, Florian Fainelli, Russell King,
	Joe Perches, Arnd Bergmann, Scott Branden, Anatol Pomazau,
	linux-kernel, linux-arm-kernel, linux-gpio,
	bcm-kernel-feedback-list, devicetree

On Tue, Feb 10, 2015 at 02:16:20PM -0800, Ray Jui wrote:
> This patchset contains the initial GPIO/PINCONF support for the Broadcom
> Cygnus SoC.
> 
> Cygnus has 3 GPIO controllers: 1) the ASIU GPIO; 2) the chipCommonG GPIO;
> and 3) the ALWAYS-ON GPIO. All 3 types of GPIO controllers are supported by
> the this driver.
> 
> All 3 Cygnus GPIO controllers support basic PINCONF functions such as bias
> pull up, pull down, and drive strength configurations, when these pins are
> muxed to GPIO.
> 
> Pins from the ASIU GPIO can be individually muxed to GPIO function, through
> interaction with the Cygnus IOMUX controller.
> 
> Note this patchset has a dependency on the other patchset "Add pinctrl support
> to Broadcom Cygnus SoC" that is also under review
> 
> Changes from v8:
>  - Add code in function 'cygnus_gpio_pinmux_add_range' to free pinmux
>    node resource by calling of_node_put
>  - Drop .suppress_bind_attrs = true, since this is already done in
>    platform_driver_probe
> 
> Changes from v7:
>  - Use 'bool' instead of 'int' for flag that indicates pinmux support in the
>    driver
>  - Call put_device to drop reference to the pinmux dev after call to
>    of_find_device_by_node
>  - Replace kasprintf with devm_kasprintf and remove memory deallocation logic
>    in the driver
>  - Set suppress_bind_attrs to true for the driver
> 
> Changes from v6:
>  - Move the driver from drivers/gpio/* to drivers/pinctrl/* since this driver
>    supports both GPIO and some basic PINCONF features
>  - Support PINCONF features through standard DT subnodes properties including
>    "bias-disable", "bias-pull-up", "bias-pull-down", and "drive-strength", by
>    creating local PINCONF controller
>  - Add support to allow individual ASIU GPIO pins to be muxed as GPIO, through
>    interactions with the Cygnus IOMUX driver
>  - Convert the driver to use standard GPIOCHIP_IRQ APIs. This helps to reduce
>    customized code in the driver
>  - Other miscellaneous imrpovements in the driver
>  - Enable GPIO based phone hook detection support for BCM911360 phone factor
>    board
> 
> Changes from v5:
>  - Get rid of DT property "linux,gpio-base". Use dynamic allocation for GPIO base
>    number
> 
> Changes from v4:
>  - Use DT property "linux,gpio-base" to define GPIO base number
>  - factorize common code to improve code readability and reduce code size
>  - remove "bcm_" prefix on function and struct names
>  - improve debugging prints
>  - default GPIO_BCM_CYGNUS to y in Kconfig (it still depends on
>    ARCH_BCM_CYGNUS). This way we do not need to select it from the
>    arch/arm/mach-bcm/Kconfig
>  - Get rid of redundant MAINTAINER entry for this driver. It will be maintained
>    by Broadcom iProc/Cygnus maintainers
>  - Update device tree document based on driver changes
> 
> Changes from v3:
>  - Fix dt property tpyo
>  - Fix incorrect GPIO compatible ID in device tree binding document example
> 
> Changes from v2:
>  - Consolidate different compatible IDs into "brcm,cygnus-gpio"
>  - Get rid of redundant "no-interrupt" property
> 
> Changes from v1:
>  - Get rid of inline qualifier
>  - Get rid of redundant check in the ISR
>  - Other minor fixes to imrove code readability

FWIW I tested this series on BCM958305K SVK.

Tested-by: Dmitry Torokhov <dtor@chromium.org>

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v9 2/4] pinctrl: cygnus: add gpio/pinconf driver
  2015-02-10 22:16 ` [PATCH v9 2/4] pinctrl: cygnus: add gpio/pinconf driver Ray Jui
@ 2015-03-04  9:48   ` Linus Walleij
  2015-03-04 17:37     ` Ray Jui
  0 siblings, 1 reply; 9+ messages in thread
From: Linus Walleij @ 2015-03-04  9:48 UTC (permalink / raw)
  To: Ray Jui
  Cc: Alexandre Courbot, Rob Herring, Pawel Moll, Mark Rutland,
	Ian Campbell, Kumar Gala, Grant Likely, Christian Daudt,
	Matt Porter, Florian Fainelli, Russell King, Joe Perches,
	Arnd Bergmann, Scott Branden, Dmitry Torokhov, Anatol Pomazau,
	linux-kernel, linux-arm-kernel, linux-gpio,
	bcm-kernel-feedback-list, devicetree

On Tue, Feb 10, 2015 at 11:16 PM, Ray Jui <rjui@broadcom.com> wrote:

> This adds the initial support of the Broadcom Cygnus GPIO/PINCONF driver
> that supports all 3 GPIO controllers on Cygnus including the ASIU GPIO
> controller, the chipCommonG GPIO controller, and the always-on GPIO
> controller. Basic PINCONF configurations such as bias pull up/down, and
> drive strength are also supported in this driver.
>
> Pins from the ASIU GPIO controller can be individually muxed to GPIO
> function, through interaction with the Cygnus IOMUX controller
>
> Signed-off-by: Ray Jui <rjui@broadcom.com>
> Reviewed-by: Scott Branden <sbranden@broadcom.com>

OK!

> diff --git a/drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c b/drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c
(...)

> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/gpio.h>

This should be:
#include <linux/gpio/driver.h>

> +static u32 cygnus_readl(struct cygnus_gpio *chip, unsigned int offset)
> +{
> +       return readl(chip->base + offset);
> +}
> +
> +static void cygnus_writel(struct cygnus_gpio *chip, unsigned int offset,
> +                         u32 val)
> +{
> +       writel(val, chip->base + offset);
> +}

If these look like so, just use readl(val, chip->base +
offset)/writel() at all sites.
These functions just adds a pointless layer of abstraction.

> +static void cygnus_set_bit(struct cygnus_gpio *chip, unsigned int reg,
> +                          unsigned gpio, int set)

"set" should be bool, right?

> +{
> +       unsigned int offset = CYGNUS_GPIO_REG(gpio, reg);
> +       unsigned int shift = CYGNUS_GPIO_SHIFT(gpio);
> +       u32 val;
> +
> +       val = cygnus_readl(chip, offset);
> +       if (set)
> +               val |= BIT(shift);
> +       else
> +               val &= ~BIT(shift);
> +       cygnus_writel(chip, offset, val);
> +}
> +
> +static int cygnus_get_bit(struct cygnus_gpio *chip, unsigned int reg,
> +                         unsigned gpio)

This should be bool right, not int?

> +{
> +       unsigned int offset = CYGNUS_GPIO_REG(gpio, reg);
> +       unsigned int shift = CYGNUS_GPIO_SHIFT(gpio);
> +       u32 val;
> +
> +       val =
> +       if (val)
> +               return 1;
> +       else
> +               return 0;

Just:
return !!(cygnus_readl(chip, offset) & BIT(shift));

Both of these are a bit overzealous like the readl/writel functions above,
consider just inlining them.

> +static int cygnus_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct cygnus_gpio *chip = to_cygnus_gpio(gc);
> +       unsigned gpio = d->hwirq;
> +       int int_type = 0, dual_edge = 0, edge_lvl = 0;

It looks like some of these should be bool.

> +       unsigned long flags;
> +
> +       switch (type & IRQ_TYPE_SENSE_MASK) {
> +       case IRQ_TYPE_EDGE_RISING:
> +               edge_lvl = 1;

= true;

etc.

> +               break;
> +
> +       case IRQ_TYPE_EDGE_FALLING:
> +               break;
> +
> +       case IRQ_TYPE_EDGE_BOTH:
> +               dual_edge = 1;
> +               break;
> +
> +       case IRQ_TYPE_LEVEL_HIGH:
> +               int_type = 1;
> +               edge_lvl = 1;
> +               break;
> +
> +       case IRQ_TYPE_LEVEL_LOW:
> +               int_type = 1;
> +               break;
> +
> +       default:
> +               dev_err(chip->dev, "invalid GPIO IRQ type 0x%x\n",
> +                       type);
> +               return -EINVAL;
> +       }
> +
> +       spin_lock_irqsave(&chip->lock, flags);
> +       cygnus_set_bit(chip, CYGNUS_GPIO_IN_TYPE_OFFSET, gpio, int_type);
> +       cygnus_set_bit(chip, CYGNUS_GPIO_INT_DE_OFFSET, gpio, dual_edge);
> +       cygnus_set_bit(chip, CYGNUS_GPIO_INT_EDGE_OFFSET, gpio,
> +                      edge_lvl);
> +       spin_unlock_irqrestore(&chip->lock, flags);
> +
> +       dev_dbg(chip->dev,
> +               "gpio:%u set int_type:%d dual_edge:%d edge_lvl:%d\n",
> +               gpio, int_type, dual_edge, edge_lvl);
> +
> +       return 0;
> +}

(...)
> +/*
> + * Request the Cygnus IOMUX pinmux controller to mux individual pins to GPIO
> + */
> +static int cygnus_gpio_request(struct gpio_chip *gc, unsigned offset)
> +{
> +       struct cygnus_gpio *chip = to_cygnus_gpio(gc);
> +       unsigned gpio = gc->base + offset;
> +
> +       /* not all Cygnus GPIO pins can be muxed individually */
> +       if (!chip->pinmux_is_supported)
> +               return 0;
> +
> +       return pinctrl_request_gpio(gpio);
> +}
> +
> +static void cygnus_gpio_free(struct gpio_chip *gc, unsigned offset)
> +{
> +       struct cygnus_gpio *chip = to_cygnus_gpio(gc);
> +       unsigned gpio = gc->base + offset;
> +
> +       if (!chip->pinmux_is_supported)
> +               return;
> +
> +       pinctrl_free_gpio(gpio);
> +}

Nice!

> +static const struct pinconf_ops cygnus_pconf_ops = {
> +       .is_generic = true,
> +       .pin_config_get = cygnus_pin_config_get,
> +       .pin_config_set = cygnus_pin_config_set,
> +};

This pin config thing looks really nice.

> +/*
> + * Map a GPIO in the local gpio_chip pin space to a pin in the Cygnus IOMUX
> + * pinctrl pin space
> + */
> +struct cygnus_gpio_pin_range {
> +       unsigned offset;
> +       unsigned pin_base;
> +       unsigned num_pins;
> +};
> +
> +#define CYGNUS_PINRANGE(o, p, n) { .offset = o, .pin_base = p, .num_pins = n }

Aha so this mapping is really very sparse...

> +/*
> + * Pin mapping table for mapping local GPIO pins to Cygnus IOMUX pinctrl pins
> + */
> +static const struct cygnus_gpio_pin_range cygnus_gpio_pintable[] = {
> +       CYGNUS_PINRANGE(0, 42, 1),
> +       CYGNUS_PINRANGE(1, 44, 3),
> +       CYGNUS_PINRANGE(4, 48, 1),
> +       CYGNUS_PINRANGE(5, 50, 3),
> +       CYGNUS_PINRANGE(8, 126, 1),
> +       CYGNUS_PINRANGE(9, 155, 1),
> +       CYGNUS_PINRANGE(10, 152, 1),
> +       CYGNUS_PINRANGE(11, 154, 1),
> +       CYGNUS_PINRANGE(12, 153, 1),
> +       CYGNUS_PINRANGE(13, 127, 3),
> +       CYGNUS_PINRANGE(16, 140, 1),
> +       CYGNUS_PINRANGE(17, 145, 7),
> +       CYGNUS_PINRANGE(24, 130, 10),
> +       CYGNUS_PINRANGE(34, 141, 4),
> +       CYGNUS_PINRANGE(38, 54, 1),
> +       CYGNUS_PINRANGE(39, 56, 3),
> +       CYGNUS_PINRANGE(42, 60, 3),
> +       CYGNUS_PINRANGE(45, 64, 3),
> +       CYGNUS_PINRANGE(48, 68, 2),
> +       CYGNUS_PINRANGE(50, 84, 6),
> +       CYGNUS_PINRANGE(56, 94, 6),
> +       CYGNUS_PINRANGE(62, 72, 1),
> +       CYGNUS_PINRANGE(63, 70, 1),
> +       CYGNUS_PINRANGE(64, 80, 1),
> +       CYGNUS_PINRANGE(65, 74, 3),
> +       CYGNUS_PINRANGE(68, 78, 1),
> +       CYGNUS_PINRANGE(69, 82, 1),
> +       CYGNUS_PINRANGE(70, 156, 17),
> +       CYGNUS_PINRANGE(87, 104, 12),
> +       CYGNUS_PINRANGE(99, 102, 2),
> +       CYGNUS_PINRANGE(101, 90, 4),
> +       CYGNUS_PINRANGE(105, 116, 10),
> +       CYGNUS_PINRANGE(123, 11, 1),
> +       CYGNUS_PINRANGE(124, 38, 4),
> +       CYGNUS_PINRANGE(128, 43, 1),
> +       CYGNUS_PINRANGE(129, 47, 1),
> +       CYGNUS_PINRANGE(130, 49, 1),
> +       CYGNUS_PINRANGE(131, 53, 1),
> +       CYGNUS_PINRANGE(132, 55, 1),
> +       CYGNUS_PINRANGE(133, 59, 1),
> +       CYGNUS_PINRANGE(134, 63, 1),
> +       CYGNUS_PINRANGE(135, 67, 1),
> +       CYGNUS_PINRANGE(136, 71, 1),
> +       CYGNUS_PINRANGE(137, 73, 1),
> +       CYGNUS_PINRANGE(138, 77, 1),
> +       CYGNUS_PINRANGE(139, 79, 1),
> +       CYGNUS_PINRANGE(140, 81, 1),
> +       CYGNUS_PINRANGE(141, 83, 1),
> +       CYGNUS_PINRANGE(142, 10, 1)
> +};
> +
> +/*
> + * The Cygnus IOMUX controller mainly supports group based mux configuration,
> + * but certain pins can be muxed to GPIO individually. Only the ASIU GPIO
> + * controller can support this, so it's an optional configuration
> + *
> + * Return -ENODEV means no support and that's fine
> + */
> +static int cygnus_gpio_pinmux_add_range(struct cygnus_gpio *chip)
> +{
> +       struct device_node *node = chip->dev->of_node;
> +       struct device_node *pinmux_node;
> +       struct platform_device *pinmux_pdev;
> +       struct gpio_chip *gc = &chip->gc;
> +       int i, ret = 0;
> +
> +       /* parse DT to find the phandle to the pinmux controller */
> +       pinmux_node = of_parse_phandle(node, "pinmux", 0);
> +       if (!pinmux_node)
> +               return -ENODEV;
> +
> +       pinmux_pdev = of_find_device_by_node(pinmux_node);
> +       /* no longer need the pinmux node */
> +       of_node_put(pinmux_node);
> +       if (!pinmux_pdev) {
> +               dev_err(chip->dev, "failed to get pinmux device\n");
> +               return -EINVAL;
> +       }
> +
> +       /* now need to create the mapping between local GPIO and PINMUX pins */
> +       for (i = 0; i < ARRAY_SIZE(cygnus_gpio_pintable); i++) {
> +               ret = gpiochip_add_pin_range(gc, dev_name(&pinmux_pdev->dev),
> +                                            cygnus_gpio_pintable[i].offset,
> +                                            cygnus_gpio_pintable[i].pin_base,
> +                                            cygnus_gpio_pintable[i].num_pins);
> +               if (ret) {
> +                       dev_err(chip->dev, "unable to add GPIO pin range\n");
> +                       goto err_put_device;
> +               }
> +       }

This is really nice. Awesome job, exactly how it should be done,
even if it's a bit complex.

> +       chip->pinmux_is_supported = (ret == 0);

Just = true;

You cannot get here with ret != 0.

> +static void cygnus_gpio_pinmux_remove_range(struct cygnus_gpio *chip)
> +{
> +       struct gpio_chip *gc = &chip->gc;
> +
> +       if (chip->pinmux_is_supported)
> +               gpiochip_remove_pin_ranges(gc);
> +}

You don't need to do this. Look in gpiochip_remove() and you see it
will remove the range for you.

 +
> +err_unregister_pinconf:
> +       cygnus_gpio_unregister_pinconf(chip);
> +
> +err_rm_range:
> +       cygnus_gpio_pinmux_remove_range(chip);

Not needed I think.

> +
> +err_rm_gpiochip:
> +       gpiochip_remove(gc);

Because this will do it.

> +
> +       return ret;
> +}

Apart from that this looks really good!

If you resend with the above nitpicks fixed I will merge this.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v9 2/4] pinctrl: cygnus: add gpio/pinconf driver
  2015-03-04  9:48   ` Linus Walleij
@ 2015-03-04 17:37     ` Ray Jui
  2015-03-05  5:01       ` Ray Jui
  0 siblings, 1 reply; 9+ messages in thread
From: Ray Jui @ 2015-03-04 17:37 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Alexandre Courbot, Rob Herring, Pawel Moll, Mark Rutland,
	Ian Campbell, Kumar Gala, Grant Likely, Christian Daudt,
	Matt Porter, Florian Fainelli, Russell King, Joe Perches,
	Arnd Bergmann, Scott Branden, Dmitry Torokhov, Anatol Pomazau,
	linux-kernel, linux-arm-kernel, linux-gpio,
	bcm-kernel-feedback-list, devicetree

Hi Linus,

On 3/4/2015 1:48 AM, Linus Walleij wrote:
> On Tue, Feb 10, 2015 at 11:16 PM, Ray Jui <rjui@broadcom.com> wrote:
> 
>> This adds the initial support of the Broadcom Cygnus GPIO/PINCONF driver
>> that supports all 3 GPIO controllers on Cygnus including the ASIU GPIO
>> controller, the chipCommonG GPIO controller, and the always-on GPIO
>> controller. Basic PINCONF configurations such as bias pull up/down, and
>> drive strength are also supported in this driver.
>>
>> Pins from the ASIU GPIO controller can be individually muxed to GPIO
>> function, through interaction with the Cygnus IOMUX controller
>>
>> Signed-off-by: Ray Jui <rjui@broadcom.com>
>> Reviewed-by: Scott Branden <sbranden@broadcom.com>
> 
> OK!
> 
>> diff --git a/drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c b/drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c
> (...)
> 
>> +#include <linux/kernel.h>
>> +#include <linux/slab.h>
>> +#include <linux/module.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/gpio.h>
> 
> This should be:
> #include <linux/gpio/driver.h>
> 
>> +static u32 cygnus_readl(struct cygnus_gpio *chip, unsigned int offset)
>> +{
>> +       return readl(chip->base + offset);
>> +}
>> +
>> +static void cygnus_writel(struct cygnus_gpio *chip, unsigned int offset,
>> +                         u32 val)
>> +{
>> +       writel(val, chip->base + offset);
>> +}
> 
> If these look like so, just use readl(val, chip->base +
> offset)/writel() at all sites.
> These functions just adds a pointless layer of abstraction.
> 
>> +static void cygnus_set_bit(struct cygnus_gpio *chip, unsigned int reg,
>> +                          unsigned gpio, int set)
> 
> "set" should be bool, right?
> 
>> +{
>> +       unsigned int offset = CYGNUS_GPIO_REG(gpio, reg);
>> +       unsigned int shift = CYGNUS_GPIO_SHIFT(gpio);
>> +       u32 val;
>> +
>> +       val = cygnus_readl(chip, offset);
>> +       if (set)
>> +               val |= BIT(shift);
>> +       else
>> +               val &= ~BIT(shift);
>> +       cygnus_writel(chip, offset, val);
>> +}
>> +
>> +static int cygnus_get_bit(struct cygnus_gpio *chip, unsigned int reg,
>> +                         unsigned gpio)
> 
> This should be bool right, not int?
> 
>> +{
>> +       unsigned int offset = CYGNUS_GPIO_REG(gpio, reg);
>> +       unsigned int shift = CYGNUS_GPIO_SHIFT(gpio);
>> +       u32 val;
>> +
>> +       val =
>> +       if (val)
>> +               return 1;
>> +       else
>> +               return 0;
> 
> Just:
> return !!(cygnus_readl(chip, offset) & BIT(shift));
> 
> Both of these are a bit overzealous like the readl/writel functions above,
> consider just inlining them.
> 
>> +static int cygnus_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>> +{
>> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>> +       struct cygnus_gpio *chip = to_cygnus_gpio(gc);
>> +       unsigned gpio = d->hwirq;
>> +       int int_type = 0, dual_edge = 0, edge_lvl = 0;
> 
> It looks like some of these should be bool.
> 
>> +       unsigned long flags;
>> +
>> +       switch (type & IRQ_TYPE_SENSE_MASK) {
>> +       case IRQ_TYPE_EDGE_RISING:
>> +               edge_lvl = 1;
> 
> = true;
> 
> etc.
> 
>> +               break;
>> +
>> +       case IRQ_TYPE_EDGE_FALLING:
>> +               break;
>> +
>> +       case IRQ_TYPE_EDGE_BOTH:
>> +               dual_edge = 1;
>> +               break;
>> +
>> +       case IRQ_TYPE_LEVEL_HIGH:
>> +               int_type = 1;
>> +               edge_lvl = 1;
>> +               break;
>> +
>> +       case IRQ_TYPE_LEVEL_LOW:
>> +               int_type = 1;
>> +               break;
>> +
>> +       default:
>> +               dev_err(chip->dev, "invalid GPIO IRQ type 0x%x\n",
>> +                       type);
>> +               return -EINVAL;
>> +       }
>> +
>> +       spin_lock_irqsave(&chip->lock, flags);
>> +       cygnus_set_bit(chip, CYGNUS_GPIO_IN_TYPE_OFFSET, gpio, int_type);
>> +       cygnus_set_bit(chip, CYGNUS_GPIO_INT_DE_OFFSET, gpio, dual_edge);
>> +       cygnus_set_bit(chip, CYGNUS_GPIO_INT_EDGE_OFFSET, gpio,
>> +                      edge_lvl);
>> +       spin_unlock_irqrestore(&chip->lock, flags);
>> +
>> +       dev_dbg(chip->dev,
>> +               "gpio:%u set int_type:%d dual_edge:%d edge_lvl:%d\n",
>> +               gpio, int_type, dual_edge, edge_lvl);
>> +
>> +       return 0;
>> +}
> 
> (...)
>> +/*
>> + * Request the Cygnus IOMUX pinmux controller to mux individual pins to GPIO
>> + */
>> +static int cygnus_gpio_request(struct gpio_chip *gc, unsigned offset)
>> +{
>> +       struct cygnus_gpio *chip = to_cygnus_gpio(gc);
>> +       unsigned gpio = gc->base + offset;
>> +
>> +       /* not all Cygnus GPIO pins can be muxed individually */
>> +       if (!chip->pinmux_is_supported)
>> +               return 0;
>> +
>> +       return pinctrl_request_gpio(gpio);
>> +}
>> +
>> +static void cygnus_gpio_free(struct gpio_chip *gc, unsigned offset)
>> +{
>> +       struct cygnus_gpio *chip = to_cygnus_gpio(gc);
>> +       unsigned gpio = gc->base + offset;
>> +
>> +       if (!chip->pinmux_is_supported)
>> +               return;
>> +
>> +       pinctrl_free_gpio(gpio);
>> +}
> 
> Nice!
> 
>> +static const struct pinconf_ops cygnus_pconf_ops = {
>> +       .is_generic = true,
>> +       .pin_config_get = cygnus_pin_config_get,
>> +       .pin_config_set = cygnus_pin_config_set,
>> +};
> 
> This pin config thing looks really nice.
> 
>> +/*
>> + * Map a GPIO in the local gpio_chip pin space to a pin in the Cygnus IOMUX
>> + * pinctrl pin space
>> + */
>> +struct cygnus_gpio_pin_range {
>> +       unsigned offset;
>> +       unsigned pin_base;
>> +       unsigned num_pins;
>> +};
>> +
>> +#define CYGNUS_PINRANGE(o, p, n) { .offset = o, .pin_base = p, .num_pins = n }
> 
> Aha so this mapping is really very sparse...
> 
>> +/*
>> + * Pin mapping table for mapping local GPIO pins to Cygnus IOMUX pinctrl pins
>> + */
>> +static const struct cygnus_gpio_pin_range cygnus_gpio_pintable[] = {
>> +       CYGNUS_PINRANGE(0, 42, 1),
>> +       CYGNUS_PINRANGE(1, 44, 3),
>> +       CYGNUS_PINRANGE(4, 48, 1),
>> +       CYGNUS_PINRANGE(5, 50, 3),
>> +       CYGNUS_PINRANGE(8, 126, 1),
>> +       CYGNUS_PINRANGE(9, 155, 1),
>> +       CYGNUS_PINRANGE(10, 152, 1),
>> +       CYGNUS_PINRANGE(11, 154, 1),
>> +       CYGNUS_PINRANGE(12, 153, 1),
>> +       CYGNUS_PINRANGE(13, 127, 3),
>> +       CYGNUS_PINRANGE(16, 140, 1),
>> +       CYGNUS_PINRANGE(17, 145, 7),
>> +       CYGNUS_PINRANGE(24, 130, 10),
>> +       CYGNUS_PINRANGE(34, 141, 4),
>> +       CYGNUS_PINRANGE(38, 54, 1),
>> +       CYGNUS_PINRANGE(39, 56, 3),
>> +       CYGNUS_PINRANGE(42, 60, 3),
>> +       CYGNUS_PINRANGE(45, 64, 3),
>> +       CYGNUS_PINRANGE(48, 68, 2),
>> +       CYGNUS_PINRANGE(50, 84, 6),
>> +       CYGNUS_PINRANGE(56, 94, 6),
>> +       CYGNUS_PINRANGE(62, 72, 1),
>> +       CYGNUS_PINRANGE(63, 70, 1),
>> +       CYGNUS_PINRANGE(64, 80, 1),
>> +       CYGNUS_PINRANGE(65, 74, 3),
>> +       CYGNUS_PINRANGE(68, 78, 1),
>> +       CYGNUS_PINRANGE(69, 82, 1),
>> +       CYGNUS_PINRANGE(70, 156, 17),
>> +       CYGNUS_PINRANGE(87, 104, 12),
>> +       CYGNUS_PINRANGE(99, 102, 2),
>> +       CYGNUS_PINRANGE(101, 90, 4),
>> +       CYGNUS_PINRANGE(105, 116, 10),
>> +       CYGNUS_PINRANGE(123, 11, 1),
>> +       CYGNUS_PINRANGE(124, 38, 4),
>> +       CYGNUS_PINRANGE(128, 43, 1),
>> +       CYGNUS_PINRANGE(129, 47, 1),
>> +       CYGNUS_PINRANGE(130, 49, 1),
>> +       CYGNUS_PINRANGE(131, 53, 1),
>> +       CYGNUS_PINRANGE(132, 55, 1),
>> +       CYGNUS_PINRANGE(133, 59, 1),
>> +       CYGNUS_PINRANGE(134, 63, 1),
>> +       CYGNUS_PINRANGE(135, 67, 1),
>> +       CYGNUS_PINRANGE(136, 71, 1),
>> +       CYGNUS_PINRANGE(137, 73, 1),
>> +       CYGNUS_PINRANGE(138, 77, 1),
>> +       CYGNUS_PINRANGE(139, 79, 1),
>> +       CYGNUS_PINRANGE(140, 81, 1),
>> +       CYGNUS_PINRANGE(141, 83, 1),
>> +       CYGNUS_PINRANGE(142, 10, 1)
>> +};
>> +
>> +/*
>> + * The Cygnus IOMUX controller mainly supports group based mux configuration,
>> + * but certain pins can be muxed to GPIO individually. Only the ASIU GPIO
>> + * controller can support this, so it's an optional configuration
>> + *
>> + * Return -ENODEV means no support and that's fine
>> + */
>> +static int cygnus_gpio_pinmux_add_range(struct cygnus_gpio *chip)
>> +{
>> +       struct device_node *node = chip->dev->of_node;
>> +       struct device_node *pinmux_node;
>> +       struct platform_device *pinmux_pdev;
>> +       struct gpio_chip *gc = &chip->gc;
>> +       int i, ret = 0;
>> +
>> +       /* parse DT to find the phandle to the pinmux controller */
>> +       pinmux_node = of_parse_phandle(node, "pinmux", 0);
>> +       if (!pinmux_node)
>> +               return -ENODEV;
>> +
>> +       pinmux_pdev = of_find_device_by_node(pinmux_node);
>> +       /* no longer need the pinmux node */
>> +       of_node_put(pinmux_node);
>> +       if (!pinmux_pdev) {
>> +               dev_err(chip->dev, "failed to get pinmux device\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       /* now need to create the mapping between local GPIO and PINMUX pins */
>> +       for (i = 0; i < ARRAY_SIZE(cygnus_gpio_pintable); i++) {
>> +               ret = gpiochip_add_pin_range(gc, dev_name(&pinmux_pdev->dev),
>> +                                            cygnus_gpio_pintable[i].offset,
>> +                                            cygnus_gpio_pintable[i].pin_base,
>> +                                            cygnus_gpio_pintable[i].num_pins);
>> +               if (ret) {
>> +                       dev_err(chip->dev, "unable to add GPIO pin range\n");
>> +                       goto err_put_device;
>> +               }
>> +       }
> 
> This is really nice. Awesome job, exactly how it should be done,
> even if it's a bit complex.
> 
>> +       chip->pinmux_is_supported = (ret == 0);
> 
> Just = true;
> 
> You cannot get here with ret != 0.
> 
>> +static void cygnus_gpio_pinmux_remove_range(struct cygnus_gpio *chip)
>> +{
>> +       struct gpio_chip *gc = &chip->gc;
>> +
>> +       if (chip->pinmux_is_supported)
>> +               gpiochip_remove_pin_ranges(gc);
>> +}
> 
> You don't need to do this. Look in gpiochip_remove() and you see it
> will remove the range for you.
> 
>  +
>> +err_unregister_pinconf:
>> +       cygnus_gpio_unregister_pinconf(chip);
>> +
>> +err_rm_range:
>> +       cygnus_gpio_pinmux_remove_range(chip);
> 
> Not needed I think.
> 
>> +
>> +err_rm_gpiochip:
>> +       gpiochip_remove(gc);
> 
> Because this will do it.
> 
>> +
>> +       return ret;
>> +}
> 
> Apart from that this looks really good!
> 
> If you resend with the above nitpicks fixed I will merge this.
> 
> Yours,
> Linus Walleij
> 

Great! I will address all comments and re-send a BIG patch series with
the other pinctrl patch series consolidated.

Thanks!

Ray

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v9 2/4] pinctrl: cygnus: add gpio/pinconf driver
  2015-03-04 17:37     ` Ray Jui
@ 2015-03-05  5:01       ` Ray Jui
  0 siblings, 0 replies; 9+ messages in thread
From: Ray Jui @ 2015-03-05  5:01 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Alexandre Courbot, Rob Herring, Pawel Moll, Mark Rutland,
	Ian Campbell, Kumar Gala, Grant Likely, Christian Daudt,
	Matt Porter, Florian Fainelli, Russell King, Joe Perches,
	Arnd Bergmann, Scott Branden, Dmitry Torokhov, Anatol Pomazau,
	linux-kernel, linux-arm-kernel, linux-gpio,
	bcm-kernel-feedback-list, devicetree

Hi Linus,

>>> +#include <linux/kernel.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/module.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/io.h>
>>> +#include <linux/gpio.h>
>>
>> This should be:
>> #include <linux/gpio/driver.h>

In fact, just by trying it out I found I need linux/gpio.h for APIs like
gpiochip_add_pin_range, and etc.

>>
>>
> 
> Great! I will address all comments and re-send a BIG patch series with
> the other pinctrl patch series consolidated.
> 
> Thanks!
> 
> Ray
> 

I've consolidated this patch series with the IOMUX and sent out a
combined patch series earlier. I believe all other comments have been
addressed except the above include header one.

Thanks,

Ray

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2015-03-05  5:01 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-02-10 22:16 [PATCH v9 0/4] Add gpio/pinconf support to Broadcom Cygnus SoC Ray Jui
2015-02-10 22:16 ` [PATCH v9 1/4] pinctrl: Cygnus: define Broadcom Cygnus GPIO/PINCONF binding Ray Jui
2015-02-10 22:16 ` [PATCH v9 2/4] pinctrl: cygnus: add gpio/pinconf driver Ray Jui
2015-03-04  9:48   ` Linus Walleij
2015-03-04 17:37     ` Ray Jui
2015-03-05  5:01       ` Ray Jui
2015-02-10 22:16 ` [PATCH v9 3/4] ARM: dts: enable GPIO for Broadcom Cygnus Ray Jui
2015-02-10 22:16 ` [PATCH v9 4/4] ARM: dts: cygnus: enable GPIO based hook detection Ray Jui
2015-02-25 19:30 ` [PATCH v9 0/4] Add gpio/pinconf support to Broadcom Cygnus SoC Dmitry Torokhov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).