[U-Boot] [PATCH v3 4/4] dm:gpio:mxc add DT support

[Simon Glass]

Hi Peng,

On 21 January 2015 at 04:09, Peng Fan <Peng.Fan at freescale.com> wrote:
> This patch add DT support for mxc gpio driver.
>
> There are one place using CONFIG_OF_CONTROL macro.
> 1. The U_BOOT_DEVICES and mxc_plat array are complied out. To DT,
>    platdata is alloced using calloc, so there is no need to use mxc_plat.
>
> The following situations are tested, and all work fine:
> 1. with DM, without DT
> 2. with DM and DT
> 3. without DM
> Since device tree has not been upstreamed, if want to test this patch.
> The followings need to be done.
>  + pieces of code does not gpio_request when using gpio_direction_xxx and
>    etc, need to request gpio.
>  + move the gpio settings from board_early_init_f to board_init
>  + define CONFIG_DM ,CONFIG_DM_GPIO and CONFIG_OF_CONTROL
>  + Add device tree file and do related configuration in
>    `make ARCH=arm menuconfig`
> These will be done in future patches by step.
>
> Signed-off-by: Peng Fan <Peng.Fan at freescale.com>
> ---
>  drivers/gpio/mxc_gpio.c | 69 +++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 52 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpio/mxc_gpio.c b/drivers/gpio/mxc_gpio.c
> index c52dd19..0766b78 100644
> --- a/drivers/gpio/mxc_gpio.c
> +++ b/drivers/gpio/mxc_gpio.c
> @@ -151,6 +151,9 @@ int gpio_direction_output(unsigned gpio, int value)
>  #endif
>
>  #ifdef CONFIG_DM_GPIO
> +#include <fdtdec.h>
> +DECLARE_GLOBAL_DATA_PTR;
> +
>  static int mxc_gpio_is_output(struct gpio_regs *regs, int offset)
>  {
>         u32 val;
> @@ -259,23 +262,6 @@ static const struct dm_gpio_ops gpio_mxc_ops = {
>         .get_function           = mxc_gpio_get_function,
>  };
>
> -static const struct mxc_gpio_plat mxc_plat[] = {
> -       { 0, (struct gpio_regs *)GPIO1_BASE_ADDR },
> -       { 1, (struct gpio_regs *)GPIO2_BASE_ADDR },
> -       { 2, (struct gpio_regs *)GPIO3_BASE_ADDR },
> -#if defined(CONFIG_MX25) || defined(CONFIG_MX27) || defined(CONFIG_MX51) || \
> -               defined(CONFIG_MX53) || defined(CONFIG_MX6)
> -       { 3, (struct gpio_regs *)GPIO4_BASE_ADDR },
> -#endif
> -#if defined(CONFIG_MX27) || defined(CONFIG_MX53) || defined(CONFIG_MX6)
> -       { 4, (struct gpio_regs *)GPIO5_BASE_ADDR },
> -       { 5, (struct gpio_regs *)GPIO6_BASE_ADDR },
> -#endif
> -#if defined(CONFIG_MX53) || defined(CONFIG_MX6)
> -       { 6, (struct gpio_regs *)GPIO7_BASE_ADDR },
> -#endif
> -};
> -
>  static int mxc_gpio_probe(struct udevice *dev)
>  {
>         struct mxc_bank_info *bank = dev_get_priv(dev);
> @@ -296,12 +282,60 @@ static int mxc_gpio_probe(struct udevice *dev)
>         return 0;
>  }
>
> +static int mxc_gpio_bind(struct udevice *device)

s/device/dev/ as that is the convention.

> +{
> +       struct mxc_gpio_plat *plat = device->platdata;
> +       struct gpio_regs *regs;
> +
> +       if (plat)
> +               return 0;

Please add a comment as to why.

> +
> +       regs = dev_get_addr(device);
> +       if (!regs)
> +               return -ENXIO;

-ENODEV I think?

> +
> +       plat = calloc(1, sizeof(*plat));
> +       if (!plat)
> +               return -ENOMEM;

Can you use the auto-alloc feature instead? Trying to keep memory
allocations out of drivers unless it is for buffers, etc.

> +
> +       plat->regs = regs;
> +       plat->bank_index = device->req_seq;

Why store this? You can access it anytime using the device pointer.

> +       device->platdata = plat;
> +
> +       return 0;
> +}
> +
> +static const struct udevice_id mxc_gpio_ids[] = {
> +       { .compatible = "fsl,imx35-gpio" },
> +       { }
> +};
> +
>  U_BOOT_DRIVER(gpio_mxc) = {
>         .name   = "gpio_mxc",
>         .id     = UCLASS_GPIO,
>         .ops    = &gpio_mxc_ops,
>         .probe  = mxc_gpio_probe,
>         .priv_auto_alloc_size = sizeof(struct mxc_bank_info),
> +       .of_match = mxc_gpio_ids,

Masahiro added a function for this.:

       .of_match = of_match_ptr(mxc_gpio_ids),

But I'm not completely sure if this is wanted, since you include this
information even when not using device tree.

> +       .bind   = mxc_gpio_bind,
> +};
> +
> +#ifndef CONFIG_OF_CONTROL
> +static const struct mxc_gpio_plat mxc_plat[] = {
> +       { 0, (struct gpio_regs *)GPIO1_BASE_ADDR },
> +       { 1, (struct gpio_regs *)GPIO2_BASE_ADDR },
> +       { 2, (struct gpio_regs *)GPIO3_BASE_ADDR },
> +#if defined(CONFIG_MX25) || defined(CONFIG_MX27) || defined(CONFIG_MX51) || \
> +               defined(CONFIG_MX53) || defined(CONFIG_MX6)
> +       { 3, (struct gpio_regs *)GPIO4_BASE_ADDR },
> +#endif
> +#if defined(CONFIG_MX27) || defined(CONFIG_MX53) || defined(CONFIG_MX6)
> +       { 4, (struct gpio_regs *)GPIO5_BASE_ADDR },
> +       { 5, (struct gpio_regs *)GPIO6_BASE_ADDR },
> +#endif
> +#if defined(CONFIG_MX53) || defined(CONFIG_MX6)
> +       { 6, (struct gpio_regs *)GPIO7_BASE_ADDR },
> +#endif

Can we remove the casts by changing the type to ulong?

>  };
>
>  U_BOOT_DEVICES(mxc_gpios) = {
> @@ -321,3 +355,4 @@ U_BOOT_DEVICES(mxc_gpios) = {
>  #endif
>  };
>  #endif
> +#endif
> --
> 1.8.4
>
>

Regards,
Simon