lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CANk1AXRx1aYSNHJ41ru_4EU61YoGOyno_qGfApjZe2iiq00RPQ@mail.gmail.com>
Date:	Thu, 6 Feb 2014 16:09:28 -0600
From:	delicious quinoa <delicious.quinoa@...il.com>
To:	Mark Rutland <mark.rutland@....com>
Cc:	Linus Walleij <linus.walleij@...aro.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
	"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 v10] gpio: add a driver for the Synopsys DesignWare APB
 GPIO block

On Thu, Feb 6, 2014 at 11:03 AM, Mark Rutland <mark.rutland@....com> wrote:
> On Thu, Feb 06, 2014 at 03:55:47PM +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: Jamie Iles <jamie@...ieiles.com>
>> Signed-off-by: Alan Tull <atull@...era.com>
>> Reviewed-by: Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>
>>
>> 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", &reg) == 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 &reg 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@...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

Powered by Openwall GNU/*/Linux Powered by OpenVZ