linux-kernel - Re: [PATCH 2/2] gpio: dt-bindings: add brcm,bcm6345-gpio bindings

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]

Date:	Wed, 3 Aug 2016 13:19:12 +0200
From:	Jonas Gorski <jonas.gorski@...il.com>
To:	Álvaro Fernández Rojas <noltari@...il.com>
Cc:	"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Alexandre Courbot <gnurou@...il.com>, chunkeey@...glemail.com,
	Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [PATCH 2/2] gpio: dt-bindings: add brcm,bcm6345-gpio bindings

Hi,

On 3 August 2016 at 13:02, Álvaro Fernández Rojas <noltari@...il.com> wrote:
> Subject: [PATCH 2/2] gpio: dt-bindings: add brcm,bcm6345-gpio bindings

Minor nitpick: I think the bindings should come first, then the
consumer. We shouldn't add support for it without having okayed the
binding.

> This patch adds the device tree bindings for the Broadcom's BCM6345
> memory-mapped GPIO controllers.
>
> The gpios will be supported by gpio-mmio code of the
> GPIO generic library.
>
> Signed-off-by: Álvaro Fernández Rojas <noltari@...il.com>
> ---
>  .../devicetree/bindings/gpio/brcm,bcm6345-gpio.txt | 46 ++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/brcm,bcm6345-gpio.txt
>
> diff --git a/Documentation/devicetree/bindings/gpio/brcm,bcm6345-gpio.txt b/Documentation/devicetree/bindings/gpio/brcm,bcm6345-gpio.txt
> new file mode 100644
> index 0000000..48e35ec
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/brcm,bcm6345-gpio.txt
> @@ -0,0 +1,46 @@
> +Bindings for the Broadcom's brcm,bcm6345-gpio memory-mapped GPIO controllers.
> +
> +These bindings should only be used for BCM6338 and BCM6345 SoCs, since newer
> +ones need a proper pinctrl driver.

There's nothing wrong with using it on newer SoCs to have gpio now and
then later replacing it in the dts file with a pinctrl node once the
bindings/drivers are available. We won't/can't drop support for the
gpio-driver, as 6338/6345 don't have pinctrl anyway. So older dts's
with gpio nodes will continue to work, and newer dts's will use the
pinctrl drivers.

> +BCM6338 have 8-bit data and dirout registers, where GPIO state can be read
> +and/or written, and the direction changed from input to output.
> +BCM6345 have 16-bit data and dirout registers, where GPIO state can be read
> +and/or written, and the direction changed from input to output.
> +
> +Required properties:
> +       - compatible: should be "brcm,bcm6345-gpio"
> +       - reg-names: must contain
> +               "dat" - data register
> +               "dirout" - direction (output) register
> +       - reg: address + size pairs describing the GPIO register sets;
> +               order must correspond with the order of entries in reg-names
> +       - #gpio-cells: must be set to 2. The first cell is the pin number and
> +                       the second cell is used to specify the gpio polarity:
> +                       0 = active high
> +                       1 = active low
> +       - gpio-controller: Marks the device node as a gpio controller.
> +
> +Optional properties:
> +       - big-endian: Memory is big endian.

I think we want native-endian here.

> +
> +Examples:
> +       - BCM6338:
> +       gpio0: gpio-controller@...e0407 {

Since both have only one bank, it might make sense to call it just
"gpio" instead of numbering it.

> +               compatible = "brcm,bcm6345-gpio";
> +               reg-names = "dirout", "dat";
> +               reg = <0xfffe0407 1>, <0xfffe040f 1>;
> +
> +               #gpio-cells = <2>;
> +               gpio-controller;
> +       };
> +
> +       - BCM6345:
> +       gpio0: gpio-controller@...e0406 {
> +               compatible = "brcm,bcm6345-gpio";
> +               reg-names = "dirout", "dat";
> +               reg = <0xfffe0406 2>, <0xfffe040a 2>;
> +               big-endian;
> +
> +               #gpio-cells = <2>;
> +               gpio-controller;
> +       };

Apart from those (minor) nitpicks, it looks good to me.


Regards
Jonas