From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Subject: Re: [PATCH v2 04/17] fdt: Add basic support for decoding GPIO definitions Date: Mon, 5 Dec 2011 15:29:24 -0800 Message-ID: References: <1322878300-5551-1-git-send-email-sjg@chromium.org> <1322878300-5551-5-git-send-email-sjg@chromium.org> <4EDD3BA8.8040808@nvidia.com> <4EDD440C.80002@nvidia.com> <4EDD4DA7.6070902@nvidia.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <4EDD4DA7.6070902-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: devicetree-discuss-bounces+gldd-devicetree-discuss=m.gmane.org-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org Sender: devicetree-discuss-bounces+gldd-devicetree-discuss=m.gmane.org-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org To: Stephen Warren Cc: U-Boot Mailing List , Devicetree Discuss , Tom Warren , Albert ARIBAUD List-Id: devicetree@vger.kernel.org Hi Stephen, On Mon, Dec 5, 2011 at 3:03 PM, Stephen Warren wrote: > On 12/05/2011 03:52 PM, Simon Glass wrote: >> Hi Stephen, >> >> On Mon, Dec 5, 2011 at 2:22 PM, Stephen Warren wrot= e: >>> On 12/05/2011 02:56 PM, Simon Glass wrote: >>>> Hi Stephen, >>>> >>>> On Mon, Dec 5, 2011 at 1:46 PM, Stephen Warren wr= ote: >>>>> On 12/02/2011 07:11 PM, Simon Glass wrote: >>>>>> This adds some support into fdtdec for reading GPIO definitions from >>>>>> the fdt. We permit up to FDT_GPIO_MAX GPIOs in the system. Each GPIO >>>>>> is of the form: >>>>>> >>>>>> gpio-function-name =3D ; >>>>>> >>>>>> where: >>>>>> >>>>>> phandle is a pointer to the GPIO node >>>>>> gpio_num is the number of the GPIO (0 to 223) >>>>>> flags is some flags, proposed as follows: >>>>>> >>>>>> =A0 =A0bit =A0 =A0meaning >>>>>> =A0 =A00 =A0 =A0 =A00=3Dinput, 1=3Doutput >>>>>> =A0 =A01 =A0 =A0 =A0for output only: inital value of output >>>>>> =A0 =A02 =A0 =A0 =A00=3Dpolarity normal, 1=3Dactive low (inverted) >>>>> >>>>> The meaning of the flags (and even whether there are any flags any if= so >>>>> how many cells there are to contain them) is defined by the GPIO >>>>> controller's binding. It's not something that can be interpreted in >>>>> isolation by a generic DT parsing function. See for example #gpio-cel= ls >>>>> in tegra20.dtsi's gpio node and kernel file >>>>> Documentation/devicetree/bindings/gpio/gpio_nvidia.txt. >>>> >>>> I see this in my version: >>>> >>>> Required properties: >>>> - compatible : "nvidia,tegra20-gpio" >>>> - #gpio-cells : Should be two. The first cell is the pin number and the >>>> =A0 second cell is used to specify optional parameters: >>>> =A0 - bit 0 specifies polarity (0 for normal, 1 for inverted) >>>> - gpio-controller : Marks the device node as a GPIO controller. >>>> >>>> so how do I go about adding the other two bits? >>> >>> I don't think you would. Input vs. output and output value are set up by >>> APIs such as gpio_direction_input/output based on what the driver wants >>> to do with the GPIOs. >> >> Fair enough. I am wanting to create a way for more information to be >> provided about a GPIO so that it can be set up automatically ready for >> use (reduces code size). > > At least in this case, I don't think it makes sense to do that. The FDT > is about representing that a particular GPIO is a VBUS GPIO. That > doesn't mean the GPIO /has/ to be an output driven high; that's only > true if the driver is enabled and chooses to configure that port as a > host port, not a device port. > > If you wanted to represent GPIOs that were always configured to a > specific output value in DT, I think that'd be an unrelated binding > somewhere other than the USB bus's vbus-gpios property, since it'd have > a completely different semantic meaning. I feel that it is useful to have a generic gpio setup function which can at least set gpio to input or output. We could even make it optional with an additional flag. IMO a lot of the reason for only having one flag is that no one can OR together two decimal numbers (i.e. we need symbolic constants in the fdt). But for now I will drop this comment and the fdtdec_setup_gpio() function. This will increase code size slightly since every driver will need to call gpio_direction_input/output() manually. > >>> include/asm-generic/gpio.h seems to use an int to represent a GPIO. I'd >>> suggest these APIs do the same, rather than use a u8. >> >> Do you mean the fdt_gpio_state structure? > > Yes. > >> I have not used u8 for any >> function calls and would not. >> >> This adds 3 bytes for every entry. What is the benefit? People get >> upset when we waste memory! > > Well, U-boot has already chosen to use an int to represent a GPIO ID. > Given that, I assert that all places that store a GPIO ID should use the > same type. And realistically, we're only talking about a handful of > instances here, and any bloat is completely limited to those platforms > that use this feature, and linear with the number of GPIOs. OK I have changed this. Each structure element is now 4 bytes (50%) bigger than before but I don't think this will add up to more than a few hundred bytes all up. Regards, Simon > > -- > nvpublic