linux-kernel - Re: [PATCH] gpio: add ETRAXFS GPIO driver

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] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 19 May 2015 11:39:01 +0200
From:	Linus Walleij <linus.walleij@...aro.org>
To:	Rabin Vincent <rabin@....in>
Cc:	Alexandre Courbot <gnurou@...il.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] gpio: add ETRAXFS GPIO driver

On Sat, May 16, 2015 at 12:27 AM, Rabin Vincent <rabin@....in> wrote:

> Add a GPIO driver for the General I/O block on Axis ETRAX FS SoCs.
>
> Signed-off-by: Rabin Vincent <rabin@....in>

Nice!

(...)
> +++ b/Documentation/devicetree/bindings/gpio/gpio-etraxfs.txt
> @@ -0,0 +1,21 @@
> +Axis ETRAX FS General I/O controller bindings
> +
> +Required properties:
> +
> +- compatible:
> +  - "axis,etraxfs-gio"
> +- reg: Physical base address and length of the controller's registers.
> +- #gpio-cells: Should be 3
> +  - The first cell is the port number (hex).
> +  - The seconds cell is the gpio offset number.
> +  - The third cell is reserved and is currently unused.
> +- gpio-controller: Marks the device node as a GPIO controller.
> +
> +Example:
> +
> +       gio: gpio@...1a000 {
> +               compatible = "axis,etraxfs-gio";
> +               reg = <0xb001a000 0x1000>;
> +               gpio-controller;
> +               #gpio-cells = <3>;
> +       };

Three cells is rather unusual, is it the best arrangement?

Usually it's just offset+flags (your flags are ununsed I see).
And then you could divide offset by num gpios per bank
(I guess 32) in the driver to get bank number.

The other obvious question is whether you considered the
design pattern of using one DT node per bank, so you
have offset 0..31 (I guess) on each device, simplifying things
with two cells.

The latter design pattern is usually recommended unless
there is a "strong" coupling between the banks, such as
if they all share the same IRQ line so they need the
same interrupt handler, or share other common registers.

> +config GPIO_ETRAXFS
> +       bool "Axis ETRAX FS General I/O"
> +       depends on CRIS || COMPILE_TEST
> +       depends on OF
> +       select GPIO_GENERIC

Very nice to have generic for this.

> +struct etraxfs_gpio_port {
> +       const char *label;
> +       unsigned int oe;
> +       unsigned int dout;
> +       unsigned int din;

consider using u32 for these.

> +       unsigned int ngpio;
> +};
> +
> +struct etraxfs_gpio_info {
> +       int num_ports;

unsigned?

> +       const struct etraxfs_gpio_port *ports;
> +};

Yours,
Linus Walleij
--
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/