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

[Simon Glass]

Hi Marek,

On 2 August 2015 at 16:19, Marek Vasut <marex at denx.de> wrote:
> On Sunday, August 02, 2015 at 11:28:13 PM, Simon Glass wrote:
>> Hi Marek,
>
> Hi,
>
>> On 27 July 2015 at 14:44, Marek Vasut <marex at denx.de> wrote:
>> > Add driver for the DesignWare APB GPIO IP block.
>> > This driver is DM capable and probes from DT.
>> >
>> > Signed-off-by: Marek Vasut <marex at denx.de>
>> > Cc: Simon Glass <sjg at chromium.org>
>
> [...]
>
>> > +#define GPIO_SWPORTA_DR                0x00
>> > +#define GPIO_SWPORTA_DDR       0x04
>> > +#define GPIO_INTEN             0x30
>> > +#define GPIO_INTMASK           0x34
>> > +#define GPIO_INTTYPE_LEVEL     0x38
>> > +#define GPIO_INT_POLARITY      0x3c
>> > +#define GPIO_INTSTATUS         0x40
>> > +#define GPIO_PORTA_DEBOUNCE    0x48
>> > +#define GPIO_PORTA_EOI         0x4c
>> > +#define GPIO_EXT_PORTA         0x50
>>
>> Should use C structure, right?
>
> My understanding is that we no longer want C structs . Tom ?
>
> [...]
>
>> > +static const struct dm_gpio_ops gpio_dwapb_ops = {
>> > +       .direction_input        = dwapb_gpio_direction_input,
>> > +       .direction_output       = dwapb_gpio_direction_output,
>> > +       .get_value              = dwapb_gpio_get_value,
>> > +       .set_value              = dwapb_gpio_set_value,
>>
>> Do you want to implement .function?
>
> No, the pinmuxing on SoCFPGA is still in a weird state.
>
>> > +};
>> > +
>> > +static int gpio_dwapb_probe(struct udevice *dev)
>> > +{
>> > +       struct gpio_dev_priv *priv = dev_get_uclass_priv(dev);
>> > +       struct gpio_dwapb_platdata *plat = dev->platdata;
>> > +
>> > +       if (!plat)
>> > +               return 0;
>> > +
>> > +       priv->gpio_count = plat->pins;
>> > +       priv->bank_name = plat->name;
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +static int gpio_dwapb_bind(struct udevice *dev)
>> > +{
>> > +       struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
>> > +       const void *blob = gd->fdt_blob;
>> > +       struct udevice *subdev;
>> > +       fdt_addr_t base;
>> > +       int node, bank = 0;
>> > +       const char *name;
>> > +
>> > +       /* If this is a child device, there is nothing to do here */
>> > +       if (plat)
>> > +               return 0;
>> > +
>> > +       base = fdtdec_get_addr(blob, dev->of_offset, "reg");
>> > +       if (base == FDT_ADDR_T_NONE) {
>> > +               debug("Can't get the GPIO register base address\n");
>> > +               return -ENXIO;
>> > +       }
>> > +
>> > +       name = fdt_get_name(blob, dev->of_offset, NULL);
>> > +
>> > +       for (node = fdt_first_subnode(blob, dev->of_offset);
>> > +            node > 0;
>> > +            node = fdt_next_subnode(blob, node)) {
>> > +               int ret;
>> > +
>> > +               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.

>
>> > +               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.

>
> Also, I can as well use "gpio <operation> N" , where N is a number.
>
>> > +
>> > +               ret = device_bind(dev, dev->driver, plat->name,
>> > +                                 plat, -1, &subdev);
>> > +               if (ret)
>> > +                       return ret;
>> > +
>> > +               subdev->of_offset = node;
>> > +               bank++;
>> > +       }
>> > +
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +static const struct udevice_id gpio_dwapb_ids[] = {
>> > +       { .compatible = "snps,dw-apb-gpio" },
>> > +       { }
>> > +};
>> > +
>> > +U_BOOT_DRIVER(gpio_dwapb) = {
>> > +       .name           = "gpio-dwapb",
>> > +       .id             = UCLASS_GPIO,
>> > +       .of_match       = gpio_dwapb_ids,
>> > +       .ops            = &gpio_dwapb_ops,
>> > +       .bind           = gpio_dwapb_bind,
>> > +       .probe          = gpio_dwapb_probe,
>> > +};
>> > --
>> > 2.1.4
>>
>> Regards,
>> Simon

Regards,
Simon