Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755990AbbEUStH (ORCPT ); Thu, 21 May 2015 14:49:07 -0400 Received: from mail-wi0-f172.google.com ([209.85.212.172]:37523 "EHLO mail-wi0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754484AbbEUStD (ORCPT ); Thu, 21 May 2015 14:49:03 -0400 Date: Thu, 21 May 2015 20:48:55 +0200 From: Rabin Vincent To: Linus Walleij Cc: Alexandre Courbot , "devicetree@vger.kernel.org" , "linux-gpio@vger.kernel.org" , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH] gpio: add ETRAXFS GPIO driver Message-ID: <20150521184855.GA6159@debian> References: <1431728843-21583-1-git-send-email-rabin@rab.in> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3571 Lines: 101 On Tue, May 19, 2015 at 11:39:01AM +0200, Linus Walleij wrote: > On Sat, May 16, 2015 at 12:27 AM, Rabin Vincent wrote: > > +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@b001a000 { > > + 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. At least to me, this: + i2c { + compatible = "i2c-gpio"; + gpios = <&gio 0xD 5 0>, <&gio 0xD 6 0>; + i2c-gpio,delay-us = <2>; which immediately shows that it's port D pins 5 and 6 which are being used, and which matches the naming in the schematics and the chip documentation is clearly preferable to this: + gpios = <&gio 101 0>, <&gio 102 0>; which uses made up numbers with no relation to any documentation and which probably requires the use of a calculator to determine if the correct pins are being used. (btw, the ports have varying numbers of GPIOs and none of them have 32). > > 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. Yes, but I did it the way I did for the reasons below. > 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. The binding in the patch matches the hardware. The hardware is described as one IP with several ports and not several instances of the same IP. The registers are also just 3 per port in the same region. Creating one instance of the device for handling each port, seems like useless overhead at best and, because it doesn't even match how the hardware looks like, quite wrong anyway. Only port A has interrupt support; this is not implemented in the current driver. BTW, the documentation for the chip is available here (GIO starts at page 647 and its registers at page 895): http://www.axis.com/files/manuals/etrax_fs_des_ref-070821.pdf > > +struct etraxfs_gpio_port { > > + const char *label; > > + unsigned int oe; > > + unsigned int dout; > > + unsigned int din; > > consider using u32 for these. Why? These are just offsets to the base address so there's no reason they _have_ to be 32 bits so u32 seems semantically wrong. > > + unsigned int ngpio; > > +}; > > + > > +struct etraxfs_gpio_info { > > + int num_ports; > > unsigned? OK. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/