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

Simon Glass
2015-08-02T23:38:28Z

Hi Marek,

On 2 August 2015 at 16:19, Marek Vasut <marex@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@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.

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