Re: [PATCH v8] gpio: add a driver for the Synopsys DesignWare APB GPIO block

On Wed, Dec 4, 2013 at 5:56 AM, Mark Rutland [off-list ref] wrote:

On Tue, Dec 03, 2013 at 04:41:16PM +0000, Alan Tull wrote:

quoted

From: Jamie Iles <redacted>

The Synopsys DesignWare block is used in some ARM devices (picoxcell)
and can be configured to provide multiple banks of GPIO pins.

Signed-off-by: Alan Tull <redacted>
Reviewed-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>

v8:     - remove socfpga.dtsi changes
        - minor cleanup in devicetree documentation
v7:     - use irq_generic_chip
        - support one irq per gpio line or one irq for many
        - s/bank/port/ and other cleanup
v6:     - (atull) squash the set of patches
        - use linear irq domain
        - build fixes. Original driver was reviewed on v3.2.
        - Fix setting irq edge type for 'rising' and 'both'.
        - Support as a loadable module.
        - Use bgpio_chip's spinlock during register access.
        - Clean up register names to match spec
        - s/bank/port/ because register names use the word 'port'
        - s/nr-gpio/nr-gpios/
        - don't get/put the of_node
        - remove signoffs/acked-by's because of changes
        - other cleanup
v5:     - handle sparse bank population correctly
v3:     - depend on rather than select IRQ_DOMAIN
        - split IRQ support into a separate patch
v2:     - use Rob Herring's irqdomain in generic irq chip patches
        - use reg property to indicate bank index
        - support irqs on both edges based on LinusW's u300 driver
---
 .../devicetree/bindings/gpio/snps-dwapb-gpio.txt   |   57 +++
 drivers/gpio/Kconfig                               |    9 +
 drivers/gpio/Makefile                              |    1 +
 drivers/gpio/gpio-dwapb.c                          |  426 ++++++++++++++++++++
 4 files changed, 493 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
 create mode 100644 drivers/gpio/gpio-dwapb.c

diff --git a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
new file mode 100644
index 0000000..e7f144f
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt

@@ -0,0 +1,57 @@
+* Synopsys DesignWare APB GPIO controller
+
+Required properties:
+- compatible : Should be "snps,dw-apb-gpio"

s/be/contain/

quoted

+- reg : Address and length of the register set for the device

As this has children with reg entries, it should have #address-cells and
#size-cells (as the example does).

quoted

+
+The GPIO controller has a configurable number of ports, each of which are
+represented as child nodes with the following properties:
+
+Required properties:
+- compatible : "snps,dw-apb-gpio-port"
+- gpio-controller : Marks the device node as a gpio controller.
+- #gpio-cells : Should be two.  The first cell is the pin number and
+  the second cell is used to specify optional parameters (currently
+  unused).
+- reg : The integer port index of the port, a single cell.
+
+Optional properties:
+- interrupt-controller : The first port may be configured to be an interrupt
+controller.
+- #interrupt-cells : Specifies the number of cells needed to encode an
+interrupt.  Shall be set to 2.  The first cell defines the interrupt number,
+the second encodes the triger flags encoded as described in
+Documentation/devicetree/bindings/interrupts.txt
+- interrupt-parent : The parent interrupt controller.
+- interrupts : The interrupts to the parent controller raised when GPIOs
+generate the interrupts.
+- snps,nr-gpios : The number of pins in the port, a single cell.
+
+Example:
+
+gpio: gpio@20000 {
+       compatible = "snps,dw-apb-gpio";
+       reg = <0x20000 0x1000>;
+       #address-cells = <1>;
+       #size-cells = <0>;
+
+       porta: gpio-controller@0 {
+               compatible = "snps,dw-apb-gpio-port";
+               gpio-controller;
+               #gpio-cells = <2>;
+               snps,nr-gpio = <8>;
+               reg = <0>;
+               interrupt-controller;
+               #interrupt-cells = <2>;
+               interrupt-parent = <&vic1>;
+               interrupts = <0 1 2 3 4 5 6 7>;

Nit: please bracket list entries individually.

Otherwise this looks fine to me.

Thanks,
Mark.

Hi Mark,

Thanks for the feedback.  I'll include fixes in v9 which I'll post
within a few days.

Alan Tull