From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932429AbcHKNzK (ORCPT ); Thu, 11 Aug 2016 09:55:10 -0400 Received: from mail-wm0-f66.google.com ([74.125.82.66]:32847 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932174AbcHKNzF (ORCPT ); Thu, 11 Aug 2016 09:55:05 -0400 Subject: Re: [PATCH v2 1/2] gpio: dt-bindings: add brcm,bcm6345-gpio bindings To: Linus Walleij , "devicetree@vger.kernel.org" References: <1470225957-5692-1-git-send-email-noltari@gmail.com> Cc: "linux-gpio@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Alexandre Courbot , Christian Lamparter From: =?UTF-8?Q?=c3=81lvaro_Fern=c3=a1ndez_Rojas?= Message-ID: <08a9662d-d021-335f-27b5-0e1968954289@gmail.com> Date: Thu, 11 Aug 2016 15:54:53 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Linus, El 11/08/2016 a las 13:45, Linus Walleij escribió: > All devicetree binding patches must be sent to the devicetree@vger.kernel.org > mailing list so include them on subsequent posts of this patch. > > On Wed, Aug 3, 2016 at 2:05 PM, Álvaro Fernández Rojas > wrote: > >> 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 >> --- >> v2: add improvements suggested by Jonas Gorski: >> - use native-endian instead of big-endian. > > What is this? Does it come from some other existing binding? > I was feeling native endian is the default unless LE or BE is > explicitly specified... Not really, even if the kernel is configured as big endian, either native-endian or big-endian are needed. The default is little-endian if nothing else is specified: https://github.com/torvalds/linux/blob/master/drivers/of/base.c#L598 > >> +Required properties: >> + - compatible: should be "brcm,bcm6345-gpio" >> + - reg-names: must contain >> + "dat" - data register >> + "dirout" - direction (output) register > > I don't like this and don't understand why you can't just cover > all GPIO registers with a single reg property. Because we want to add a simple gpio driver for these old SoCs with the newer ones having a different and more complex gpio/pinctrl driver. > >> + - 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. > > Reference the standard bindings in gpio.txt for cells and controller. Sure, I just copy & pasted this from the other gpio mmio driver: https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/gpio/wd%2Cmbl-gpio.txt#L17 > >> +Optional properties: >> + - native-endian: use native endian memory. > > That is weird. Explain why this is needed. (explained above) > >> + - BCM6345: >> + gpio: gpio-controller@fffe0406 { >> + compatible = "brcm,bcm6345-gpio"; >> + reg-names = "dirout", "dat"; >> + reg = <0xfffe0406 2>, <0xfffe040a 2>; > > Also I do not understand this at all. Why pick out two 16bit registers? > Surely the rest of the registers at 0xfffe0400 must be GPIO registers > as well? Are they not? This is the layout for BCM6345: https://gitlab.labs.nic.cz/turris/openwrt/blob/8bfda76892a39c033b5abf2c17035a444fffad08/target/linux/brcm63xx-2.6/files/include/asm-mips/mach-bcm963xx/6345_map_part.h#L141 And this is the layout for BCM6338: https://gitlab.labs.nic.cz/turris/openwrt/blob/8bfda76892a39c033b5abf2c17035a444fffad08/target/linux/brcm63xx-2.6/files/include/asm-mips/mach-bcm963xx/6338_map_part.h#L202 > > If they are, cover them all with a single reg property. > > If they are not all GPIO registers, use MFD syscon for this and access > the constituent parts through that. I don't think that's the case since we're using a generic mmio binding. > > Yours, > Linus Walleij > Regards, Álvaro.