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

[Marek Vasut <marex@denx.de>]

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@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@denx.de>
> > Cc: Simon Glass <sjg@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.

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

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