Re: [PATCH v2 04/17] fdt: Add basic support for decoding GPIO definitions

[Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>]

Hi Stephen,

On Mon, Dec 5, 2011 at 3:03 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> On 12/05/2011 03:52 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On Mon, Dec 5, 2011 at 2:22 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>> On 12/05/2011 02:56 PM, Simon Glass wrote:
>>>> Hi Stephen,
>>>>
>>>> On Mon, Dec 5, 2011 at 1:46 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>>>> 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 = <phandle gpio_num flags>;
>>>>>>
>>>>>> 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:
>>>>>>
>>>>>>    bit    meaning
>>>>>>    0      0=input, 1=output
>>>>>>    1      for output only: inital value of output
>>>>>>    2      0=polarity normal, 1=active 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-cells
>>>>> 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
>>>>   second cell is used to specify optional parameters:
>>>>   - 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