[U-Boot] [RFC PATCH v2 13/13] tegra: Convert tegra GPIO driver to use driver model

[Simon Glass]

HI Stephen,

On 5 May 2014 15:14, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 05/05/2014 01:53 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On 5 May 2014 11:29, Stephen Warren <swarren at wwwdotorg.org> wrote:
>>> On 05/05/2014 10:09 AM, Simon Glass wrote:
>>>> This is an implementation of GPIOs for Tegra that uses driver model. It has
>>>> been tested on trimslice and also using the new iotrace feature.
>>>>
>>>> The implementation uses a top-level GPIO device (which has no actual GPIOS).
>>>> Under this all the banks are created as separate GPIO devices.
>>>
>>> I don't think that dividing the GPIOs into banks is appropriate. While
>>> the textual naming of Tegra GPIOs is based on the bank concept, I think
>>> that division should only apply to the textual naming, and not any data
>>> structures or device registration, etc. In fact, I often refer to Tegra
>>> GPIOs using their integer ID rather than their name, and dividing the
>>> controller into separate banks probably technically disallows this,
>>> since there would be no architected guarantee that the numbering of the
>>> banks would be sequential. Contiguous numbering of all GPIOs within the
>>> controller is also useful for correlation with pinmux numbering of pins.
>>
>> This is how the GPIO uclass works at present. Do you think that should
>> change also? It would mean enhancing the uclass to support multiple
>> GPIO bank names, with each having a number of GPIOs attached. This is
>> the sort of complexity that adds to code size. On the other hand it's
>> something that we may want to do anyway as more SOC's GPIO drivers are
>> converted.
>
> Surely this means simplifying the core to completely remove any concept
> of GPIO banks. I really don't see any need for a 2-level hierarchy. Just
> have one struct/object/... that represents a GPIO
> controller/module/chip/... Each of those has N GPIOs numbered 0..N.
> Linux has managed to get away without a 2-level controller/bank
> approach, so there's certainly evidence it works in practice.

I think you have it backwards...the current implementation has a
single level of hierarchy. Each driver handles one bank (or 'port' in
the case of Tegra). What you are talking about is having a single
driver handle multiple banks, thus requiring that the driver have a
second level to deal with banks, over and above the device. We might
end up with that, but I would prefer to move to it when we have
evidence that it is a general need.

>
>>> ...
>>>> Since driver model is not yet available before relocation, or in SPL, a
>>>> special function is provided for seaboard's SPL code.
>>>
>>> Perhaps we should just remove that code from Seaboard, since sharing the
>>> UART/SPI pins really isn't a useful feature.
>>
>> saveenv to SPI won't work without it though.
>
> The environment is stored in eMMC.
>
> SPI simply doesn't work sanely on Seaboard, and we should just rip out
> any support for it at all. On Seaboard itself, people can just set the
> jumpers to ignore SPI completely. On Springbank, something similar can
> be done with the pull-up/down resistors. I've been using my Springbank
> without any of the resistors present, while disables SPI completely, for
> years without issue...

OK, how about you submit a patch to do that, and then I can drop my
special case (or maybe not if your other patch goes in)?

>
>>>> +static char *gpio_port_name(int base_port)
>>>>  {
>>>> +     char *name, *s;
>>>> +
>>>> +     name = malloc(3);
>>>
>>> It seems like this should be statically allocated (e.g. as part of *plat
>>> in gpio_tegra_bind() or something?). Still, if we stop splitting things
>>> into banks, we can completely get rid of this.
>>
>> No, we still need the name so that 'gpio input T3' works corrrectly.
>
> Why do we need GPIO names at all? "gpio input 155" works just as well,
> and to be honest is often a lot more convenient that trying to maps
> things back to a name.

Eh? We need to support named GPIOs in U-Boot. 155 is a meaningless
number which drivers people back and forth to the datasheets, their
calculators, a long table, etc. Even the Tegra device tree has moved
away from numbers to GPIO names, I notice.

Regards,
Simon