Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752953AbcD2LP0 (ORCPT ); Fri, 29 Apr 2016 07:15:26 -0400 Received: from foss.arm.com ([217.140.101.70]:50202 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751634AbcD2LPO (ORCPT ); Fri, 29 Apr 2016 07:15:14 -0400 Date: Fri, 29 Apr 2016 12:15:01 +0100 From: Mark Rutland To: Christian Lamparter Cc: linux-gpio@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, =?utf-8?B?w4FsdmFybyBGZXJuw6FuZGV6?= Rojas , Kumar Gala , Alexander Shiyan , Ian Campbell , Pawel Moll , Rob Herring , Alexandre Courbot , Linus Walleij Subject: Re: [RFC v5 4/4] gpio: dt-bindings: add gpio-mmio bindings Message-ID: <20160429111500.GA23726@leverpostej> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6206 Lines: 161 Hi, As a general thing, please put the binding earlier in a series than code implemeting it, as that that makes it easier to review the series in-order (with context from the binding making code review easier). See Documentation/devicetree/bindings/submitting-patches.txt On Fri, Apr 29, 2016 at 02:53:17AM +0200, Christian Lamparter wrote: > From: Álvaro Fernández Rojas > > This patch adds the device tree bindings for the gpio-mmio. > The gpio-mmio is already part of a the GPIO generic library. > > Signed-off-by: Álvaro Fernández Rojas > Signed-off-by: Christian Lamparter > --- > .../devicetree/bindings/gpio/gpio-mmio.txt | 73 ++++++++++++++++++++++ > 1 file changed, 73 insertions(+) > create mode 100644 Documentation/devicetree/bindings/gpio/gpio-mmio.txt > > diff --git a/Documentation/devicetree/bindings/gpio/gpio-mmio.txt b/Documentation/devicetree/bindings/gpio/gpio-mmio.txt > new file mode 100644 > index 0000000..cc7f0b1 > --- /dev/null > +++ b/Documentation/devicetree/bindings/gpio/gpio-mmio.txt > @@ -0,0 +1,73 @@ > +Bindings for the generic driver for memory-mapped GPIO controllers. > + Bindings should be for hardware (either specific device models, or for classes), and not for Linux drivers. The latter is subject to arbitrary changes while the former is not, as old hardware continues to exist and does not change while drivers get completely reworked. So please frame this binding document in terms of the class of hardware you are trying to address. Please provide some introduction to the assumptions that you are making about said hardware, such that it's obvious when one needs a more specific binding. I share the same fears that Rob mentioned in that while this may appear simple now, the limitations are unclear, and this effectively prevents us from accurately/specifically describing some hardware. Regardless, I've taken the time to review the below, and I have a number of comments. > +Required properties: > + - compatible: should be "linux,gpio-mmio" Even with the above comments, this binding name would be valid. > + - reg-names: must contain > + "dat" - data register > + may contain > + "set" - data set register > + "clr" - data clear register > + "dirout" - direction output register > + "dirin" - direction input register Below, we mention endianness. Are registers an arbitrary number of bytes long? In what unit size are they grouped (e.g. {8,16,32,64}-bit)? What are the expectations for the set/clear/dirin/dirout registers? Are they write-one to modify, or write-zero to modify? For the names, don't bother abbreviating (i.e. use "data", "set", "clear"). These names might clash with the real names a specific HW model applies to its register sets, which means that this binding is exclusive w.r.t. a specific binding for a device (i.e. it cannot be extended with device-specific information). If we're happy with that, then we must call out the expected limitations on the use of this binding, or we end up with the not-so-simple-any-more issues Rob mentioned previously. > + - 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 Where the cells encode what? I'm guessing this is <$idx $flags>, but you should spell that out explicitly. > + - gpio-controller: Marks the device node as a gpio controller. > + > +Optional properties: > + - ngpio: specifies the number of gpio mapped in the register. > + - big-endian: force big endian register accesses. > + - big-endian-byte-order: assign GPIOs in reverse order. I cannot parse this last description. It needs a better wording. I guess this means that the indices (in the first GPIO cell) are applied in descending order for ascending chunks of the registers (and that is a poor description, too). > + - unreadable-reg-set: data set register is not readable. > + - read-output-reg-set: cache value set for reads. > + - unreadable-reg-dir: dirout/dirin register is not readable. > + - no-output: GPIOs are read-only. > + > +The GPIO generic library provides support for memory-mapped GPIO > +controllers. The configuration is detected by which resources are present. > +The simplest form of a GPIO controller that the driver support is just a > +single "dat" register, where GPIO state can be read and/or written. > +However, the driver supports far more: > + - 8/16/32/64 bits registers. The number of GPIOs is automatically > + determined by the width of the registers. > + - GPIO controllers with clear/set registers. > + - GPIO controllers with a single "dat" register. > + - Big endian bits/GPIOs ordering. Reword this in terms of the class of hardware you are trying to support, (rather than the specific library code you are using), and move it to the introduction at the top of the binding. Thanks, Mark. > + > +For setting GPIO's there are three configurations: > + 1. single input/output register resource (named "dat"), > + 2. set/clear pair (named "set" and "clr"), > + 3. single output register resource and single input resource > + ("set" and dat"). > + > +For setting the GPIO direction, there are three configurations: > + a. simple bidirectional GPIOs that requires no configuration. > + b. an output direction register (named "dirout") > + where a 1 bit indicates the GPIO is an output. > + c. an input direction register (named "dirin") > + where a 1 bit indicates the GPIO is an input. > + > +Examples: > + > + /* Configuration for single input/output register > + * for eight simple bidirectional GPIOs. > + */ > + gpio_a_1 { > + compatible = "linux,gpio-mmio"; > + reg = <0x18000000 0x1>; > + reg-names = "dat"; > + #gpio-cells = <2>; > + gpio-controller; > + }; > + > + /* Configuration for set/clear pair registers with > + * 32 output direction register GPIOs. > + */ > + gpio_b_2 { > + compatible = "linux,gpio-mmio"; > + reg = <0x18000000 0x4>, <0x18000010 0x4>, > + <0x18000004 0x4>, <0x18000008 0x4>; > + reg-names = "dat", "set", "clr", "dirout"; > + #gpio-cells = <2>; > + gpio-controller; > + }; > -- > 2.8.1 >