Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755708AbaBFWKB (ORCPT ); Thu, 6 Feb 2014 17:10:01 -0500 Received: from mail-qa0-f48.google.com ([209.85.216.48]:42893 "EHLO mail-qa0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751297AbaBFWJ6 (ORCPT ); Thu, 6 Feb 2014 17:09:58 -0500 MIME-Version: 1.0 In-Reply-To: <20140206170325.GF16551@e106331-lin.cambridge.arm.com> References: <1391702147-6617-1-git-send-email-delicious.quinoa@gmail.com> <20140206170325.GF16551@e106331-lin.cambridge.arm.com> Date: Thu, 6 Feb 2014 16:09:28 -0600 Message-ID: Subject: Re: [PATCH v10] gpio: add a driver for the Synopsys DesignWare APB GPIO block From: delicious quinoa To: Mark Rutland Cc: Linus Walleij , "linux-kernel@vger.kernel.org" , "linux-gpio@vger.kernel.org" , "linux-doc@vger.kernel.org" , Jamie Iles , "devicetree@vger.kernel.org" , Grant Likely , "rob.herring@calxeda.com" , Steffen Trumtrar , Sebastian Hesselbarth , Heiko Stuebner , Alan Tull , Dinh Nguyen , Yves Vandervennet Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Feb 6, 2014 at 11:03 AM, Mark Rutland wrote: > On Thu, Feb 06, 2014 at 03:55:47PM +0000, Alan Tull wrote: >> From: Jamie Iles >> >> 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: Jamie Iles >> Signed-off-by: Alan Tull >> Reviewed-by: Sebastian Hesselbarth >> >> v10: - in documentation nr-gpio -> nr-gpios >> v9: - cleanup in dt bindings doc >> - use of_get_child_count() >> 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 | 59 +++ >> drivers/gpio/Kconfig | 9 + >> drivers/gpio/Makefile | 1 + >> drivers/gpio/gpio-dwapb.c | 415 ++++++++++++++++++++ >> 4 files changed, 484 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..cb01f9f >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt >> @@ -0,0 +1,59 @@ >> +* Synopsys DesignWare APB GPIO controller >> + >> +Required properties: >> +- compatible : Should contain "snps,dw-apb-gpio" >> +- reg : Address and length of the register set for the device > > Presumably #address-cells and #size-cells should be described here? > >> + >> +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). > > Why not just have this as one cell for now if the second cell is unused? > > If it needs to be expanded the driver can read #gpio-cells to figure out > what to do at runtime, and it prevents crap DTSs in the mean time that > could get in the way if you need to use additional cells in future. > >> +- reg : The integer port index of the port, a single cell. >> +- #address-cells : should be 1. >> +- #size-cells : should be 0. > > As mentioned above, presumably #address-cells and #size-cells should be > in the parent node, as is the case in the example? > >> + >> +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. > > How many are expected? > > Otherwise, the binding looks ok to me. > > [...] > >> +static void dwapb_configure_irqs(struct dwapb_gpio *gpio, >> + struct dwapb_gpio_port *port) >> +{ >> + struct gpio_chip *gc = &port->bgc.gc; >> + struct device_node *node = gc->of_node; >> + struct irq_chip_generic *irq_gc; >> + unsigned int hwirq, ngpio = gc->ngpio; >> + struct irq_chip_type *ct; >> + int reg, err, irq; >> + >> + if (of_get_property(node, "interrupts", ®) == NULL) >> + return; > > of_get_property can take a NULL lenp (core OF code depends on this > fact), so you don't need the somewhat confusing ® here. > > Cheers, > Mark. Hi Mark, Thanks for the feedback. I have made the changes in v11, which I just sent out. Alan Tull aka delicious quinoa -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/