[U-Boot] [PATCH 1/2] gpio: Add DW APB GPIO driver

[Simon Glass]

Hi Marek,

On 5 August 2015 at 19:49, Marek Vasut <marex at denx.de> wrote:
> On Wednesday, August 05, 2015 at 04:39:33 PM, Simon Glass wrote:
>> Hi Marek,
>
> Hi Simon,
>
>> On 2 August 2015 at 18:16, Marek Vasut <marex at denx.de> wrote:
>> > On Monday, August 03, 2015 at 01:38:28 AM, Simon Glass wrote:
>> >> Hi Marek,
>> >
>> > Hi Simon,
>> >
>> > [...]
>> >
>> >> >> > +               if (!fdtdec_get_bool(blob, node,
>> >> >> > "gpio-controller")) +                       continue;
>> >> >> > +
>> >> >> > +               plat = NULL;
>> >> >> > +               plat = calloc(1, sizeof(*plat));
>> >> >>
>> >> >> I suppose this should use devm_alloc() now.
>> >> >
>> >> > Is that even in u-boot/master ? Also, I'm not sure it's a good idea to
>> >> > put this in if I use this driver in SPL.
>> >>
>> >> Yes it's very new. Only in dm/master.
>> >>
>> >> It's only compiled in when enabled, so you can still use it in SPL.
>> >>
>> >> Up to you. At some point I think we should convert all driver allocs
>> >> to use devm so that we know what allocation is going on.
>> >
>> > When is this landing in master ? I don't mind either way, but I can do
>> > a linting pass on the socfpga when things settle down too.
>>
>> Hopefully I'll have a pull request out on Friday.
>
> Cool!
>
>> >> >> > +               if (!plat)
>> >> >> > +                       return -ENOMEM;
>> >> >> > +
>> >> >> > +               plat->base = base;
>> >> >> > +               plat->bank = bank;
>> >> >> > +               plat->pins = fdtdec_get_int(blob, node,
>> >> >> > "snps,nr-gpios", 0); +               snprintf(plat->name,
>> >> >> > sizeof(plat->name) - 1, "%s-bank%i-", +
>> >> >> > name, bank);
>> >> >>
>> >> >> Why such a long name? That's going to be a pain to type in the 'gpio'
>> >> >> command.
>> >> >
>> >> > Do you have a suggestion please ?
>> >>
>> >> A, B, C is good if you have <=26 banks. Tegra does that.
>> >>
>> >> Exynos does PA0, PA1, PB0, PC0, PC1, etc.
>> >
>> > How do I identify which one is PA0/PA1/PA2 , shall I perform some
>> > transformation on the register address of the GPIO block or example ?
>> > But how can I assure that if the next SoCFPGA has these addresses
>> > completely different, these GPIO numbers will be stable ? Isn't it
>> > better to be explicit about the GPIO block ID then ?
>>
>> It's up to you. Normally each bank has a name and the datasheet
>> specifies it. In your case if not you could think about a naming
>> scheme.
>
> Can you please take a look into arch/arm/dts/socfpga.dtsi ?
> The system has three GPIO controllers (look for gpio0, gpio1, gpio2)
> and each of these controllers has one bank (porta, portb, portc) .
>
> I can name my gpios portxN , where x is either of a,b,c and N is the
> GPIO number. The problem is, I cannot determine in dwapb_gpio_bind()
> which one is "porta", "portb" and "portc" because all I have is the
> physical addess of the GPIO controller and the index of the bank in
> the namespace of that controller.
>
> Sure, I can do some sort of global counting in the driver, but I would
> like to avoid that sort of thing. I can also add some kind of ad-hoc DT
> prop, but that's also not a good idea I think. Do you have any suggestion
> for me please ?

One option is to use the device tree node name but it isn't very
friendly - gpio0 at xxxxx. You could perhaps add a new property like
'bank-name'?

>
>> > Also, remember that I have an FPGA in the same package, which has a lot
>> > of I/O.
>> >
>> >> > Also, I can as well use "gpio <operation> N" , where N is a number.
>> >
>> > What about this ? How does indexing the GPIOs with plain number fit into
>> > the picture please ?
>>
>> Driver model GPIO handles this automatically. If you can add different
>> numbers of GPIO blocks you might consider adding them as different
>> GPIO banks with their own names. The global number depends on the
>> probe order which depends on the bind order (i.e. device tree node
>> order). But names are safer than numbers - a small change can change
>> all the numbers. There is a function to get the global number given a
>> GPIO device and offset.
>
> I see, thanks for clarifying!

Regards,
Simon