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

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