[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CANk1AXRVO+=U=0zLR=orU49hn4nOmx6pZH-42tpbGk4oF5L9Og@mail.gmail.com>
Date: Thu, 5 Dec 2013 16:55:32 -0600
From: delicious quinoa <delicious.quinoa@...il.com>
To: Mark Rutland <mark.rutland@....com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
Linus Walleij <linus.walleij@...ricsson.com>,
"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
Jamie Iles <jamie@...ieiles.com>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
Grant Likely <grant.likely@...retlab.ca>,
"rob.herring@...xeda.com" <rob.herring@...xeda.com>,
Steffen Trumtrar <s.trumtrar@...gutronix.de>,
Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
Heiko Stuebner <heiko@...ech.de>, Alan Tull <atull@...era.com>,
Dinh Nguyen <dinguyen@...era.com>,
Yves Vandervennet <rocket.yvanderv@...il.com>
Subject: 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 <mark.rutland@....com> wrote:
> On Tue, Dec 03, 2013 at 04:41:16PM +0000, Alan Tull wrote:
>> From: Jamie Iles <jamie@...ieiles.com>
>>
>> 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 <atull@...era.com>
>> Reviewed-by: Sebastian Hesselbarth <sebastian.hesselbarth@...il.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/
>
>> +- 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).
>
>> +
>> +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@...00 {
>> + 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists