Skip to contentSkip to conversation

Add gpio/pinconf support to Broadcom Cygnus SoC

PATCH In review Ray Jui 3 participants 9 messages Feb 10, 2015 – Mar 5, 2015

Subsystems (4)
Device tree bindings GPIO Pin control Broadcom iproc Arm architecture
lore
Share & tools
lore

Ray Jui

author maintainer
#0 lore raw

v9 cover Add gpio/pinconf support to Broadcom Cygnus SoC

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
Signature · 1 line
1.7.9.5
author maintainer
#1 lore raw

PATCH v9 1/4 pinctrl: Cygnus: define Broadcom Cygnus GPIO/PINCONF binding

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

Signed-off-by Ray Jui <[email protected]>
Reviewed-by Scott Branden <[email protected]>
---
 .../bindings/pinctrl/brcm,cygnus-gpio.txt          |  102 ++++++++++++++++++++
 1 file changed, 102 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/brcm,cygnus-gpio.txt
Documentation/devicetree/bindings/pinctrl/brcm,cygnus-gpio.txt new file +102 -0
new file mode 100644
index 0000000..9b9196c
@@ -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>
+	}
Signature · 1 line
1.7.9.5
author maintainer
#2 lore raw

PATCH v9 2/4 pinctrl: cygnus: add gpio/pinconf driver

In reply to 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 <[email protected]>
Reviewed-by Scott Branden <[email protected]>
---
 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
drivers/pinctrl/bcm/Kconfig +22 -0
index eb13201..cd11d4d 100644
@@ -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)
drivers/pinctrl/bcm/Makefile +1 -0
index bb6beb6..2b2f70e 100644
@@ -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
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c new file +908 -0

908 lines not on this page · raw message

Signature · 1 line
1.7.9.5
#3 lore raw

Re: [PATCH v9 2/4] pinctrl: cygnus: add gpio/pinconf driver

Quoted 11 lines · 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 <[email protected]>
Reviewed-by Scott Branden <[email protected]>

OK!

Quoted 1 line
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +0 -0

(...)

drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c line 28

Quoted 6 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +6 -0
+#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>

drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c line 123

Quoted 10 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +10 -0
+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.

drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c line 135

Quoted 2 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +2 -0
+static void cygnus_set_bit(struct cygnus_gpio *chip, unsigned int reg,
+                          unsigned gpio, int set)

"set" should be bool, right?

Quoted 5 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +5 -0
+{
+       unsigned int offset = CYGNUS_GPIO_REG(gpio, reg);
+       unsigned int shift = CYGNUS_GPIO_SHIFT(gpio);
+       u32 val;
+

drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c line 150

Quoted 10 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +10 -0
+       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?

drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c line 160

Quoted 10 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +10 -0
+{
+       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.

drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c line 252

Quoted 6 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +6 -0
+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.

drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c line 257

Quoted 5 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +5 -0
+       unsigned long flags;
+
+       switch (type & IRQ_TYPE_SENSE_MASK) {
+       case IRQ_TYPE_EDGE_RISING:
+               edge_lvl = 1;

= true;

etc.

Quoted 37 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +37 -0
+               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;
+}

(...)

Quoted 15 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +15 -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);
+}
+

drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c line 328

Quoted 10 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +10 -0
+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!

drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c line 611

Quoted 5 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +5 -0
+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.

drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c line 623

Quoted 11 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +11 -0
+/*
+ * 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...

Quoted 84 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +84 -0
+/*
+ * 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 */

drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c line 718

Quoted 10 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +10 -0
+       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.

Quoted 1 line

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

Just = true;

You cannot get here with ret != 0.

drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c line 738

Quoted 7 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +7 -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.

+

drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c line 884

Quoted 5 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +5 -0
+err_unregister_pinconf:
+       cygnus_gpio_unregister_pinconf(chip);
+
+err_rm_range:
+       cygnus_gpio_pinmux_remove_range(chip);

Not needed I think.

drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c line 887

Quoted 3 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +3 -0
+
+err_rm_gpiochip:
+       gpiochip_remove(gc);

Because this will do it.

drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c line 554

Quoted 3 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +3 -0
+
+       return ret;
+}

Apart from that this looks really good!

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

Yours,
Linus Walleij

author maintainer
#4 lore raw

Re: [PATCH v9 2/4] pinctrl: cygnus: add gpio/pinconf driver

Hi Linus,

Quoted 328 lines · Linus Walleij
Quoted 11 lines · 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 <[email protected]>
Reviewed-by Scott Branden <[email protected]>

OK!

Quoted 1 line
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +0 -0

(...)

drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c line 28

Quoted 6 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +6 -0
+#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>

drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c line 123

Quoted 10 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +10 -0
+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.

drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c line 135

Quoted 2 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +2 -0
+static void cygnus_set_bit(struct cygnus_gpio *chip, unsigned int reg,
+                          unsigned gpio, int set)

"set" should be bool, right?

Quoted 5 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +5 -0
+{
+       unsigned int offset = CYGNUS_GPIO_REG(gpio, reg);
+       unsigned int shift = CYGNUS_GPIO_SHIFT(gpio);
+       u32 val;
+

drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c line 150

Quoted 10 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +10 -0
+       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?

drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c line 160

Quoted 10 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +10 -0
+{
+       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.

drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c line 252

Quoted 6 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +6 -0
+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.

drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c line 257

Quoted 5 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +5 -0
+       unsigned long flags;
+
+       switch (type & IRQ_TYPE_SENSE_MASK) {
+       case IRQ_TYPE_EDGE_RISING:
+               edge_lvl = 1;

= true;

etc.

Quoted 37 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +37 -0
+               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;
+}

(...)

Quoted 15 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +15 -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);
+}
+

drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c line 328

Quoted 10 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +10 -0
+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!

drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c line 611

Quoted 5 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +5 -0
+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.

drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c line 623

Quoted 11 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +11 -0
+/*
+ * 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...

Quoted 84 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +84 -0
+/*
+ * 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 */

drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c line 718

Quoted 10 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +10 -0
+       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.

Quoted 1 line

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

Just = true;

You cannot get here with ret != 0.

drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c line 738

Quoted 7 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +7 -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.

+

drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c line 884

Quoted 5 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +5 -0
+err_unregister_pinconf:
+       cygnus_gpio_unregister_pinconf(chip);
+
+err_rm_range:
+       cygnus_gpio_pinmux_remove_range(chip);

Not needed I think.

drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c line 887

Quoted 3 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +3 -0
+
+err_rm_gpiochip:
+       gpiochip_remove(gc);

Because this will do it.

drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c line 554

Quoted 3 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +3 -0
+
+       return ret;
+}

Apart from that this looks really good!

Quoted 5 lines

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

author maintainer
#5 lore raw

Re: [PATCH v9 2/4] pinctrl: cygnus: add gpio/pinconf driver

Hi Linus,

Quoted 9 lines
Quoted 9 lines

drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c line 28

Quoted 6 lines
drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c new file +6 -0
+#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.

Quoted 10 lines
Quoted 2 lines

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

author maintainer
#6 lore raw

PATCH v9 3/4 ARM: dts: enable GPIO for Broadcom Cygnus

In reply to 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 <[email protected]>
Reviewed-by Scott Branden <[email protected]>
---
 arch/arm/boot/dts/bcm-cygnus.dtsi |   33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)
arch/arm/boot/dts/bcm-cygnus.dtsi +33 -0
index b014ce5..a3b8621 100644
@@ -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>;
Signature · 1 line
1.7.9.5
author maintainer
#7 lore raw

PATCH v9 4/4 ARM: dts: cygnus: enable GPIO based hook detection

In reply to Ray Jui

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

Signed-off-by Ray Jui <[email protected]>
---
 arch/arm/boot/dts/bcm911360_entphn.dts |   13 +++++++++++++
 1 file changed, 13 insertions(+)
arch/arm/boot/dts/bcm911360_entphn.dts +13 -0
index d2ee952..7db4843 100644
@@ -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>;
+		};
+	};
 };
Signature · 1 line
1.7.9.5
Tested-by on the v9 series
#8 lore raw
In reply to Ray Jui
Quoted 66 lines · 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

Quoted 8 lines

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 <[email protected]>

Thanks.

Signature · 1 line
Dmitry