[U-Boot] [PATCH v2] dm:gpio:mxc add DT support

[Igor Grinberg <grinberg@compulab.co.il>]

On 01/20/15 09:15, Peng Fan wrote:
> This patch add DT support for mxc gpio driver.
> 
> Include a bank_index entry in platdata. This can avoid using
> `plat - mxc_plat` to calculate bank number. Also this can simplify code.

Although, I don't insist, but I would recommend to split the patch into 2:
the bank_index rework and the DT support addition.

> 
> There are two places still using CONFIG_OF_CONTROL macro, just to
> shrink code size if only support DM but not support DT.
> 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.
> 2. add a function mxc_get_gpio_addr to get "reg" property if DT support.
>    If no DT, this function just returns NULL.

I have some comments on this one, please see below...

> 
> The following situations are tested:
> 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@freescale.com>
> ---
> 
> Changes v2:
>  1. remove uneccessary #ifdef
>  2. add more stuff in commit log
>  3. include a new function mxc_get_gpio_addr to get register base.
>     This function is different for DT and not DT, by `#ifdef`.
>     If using one implementation for DT and not DT, final image will be big.
>  4. include a new entry in platdata, named bank_index. it can simplify DT
>     support. To no DT, bank_index is static initilized; to DT, bank_index
>     is get from device's req_seq.
> 
>  drivers/gpio/mxc_gpio.c | 89 +++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 71 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpio/mxc_gpio.c b/drivers/gpio/mxc_gpio.c
> index 8bb9e39..5826620 100644
> --- a/drivers/gpio/mxc_gpio.c
> +++ b/drivers/gpio/mxc_gpio.c
> @@ -23,6 +23,7 @@ enum mxc_gpio_direction {
>  #define GPIO_PER_BANK			32
>  
>  struct mxc_gpio_plat {
> +	int bank_index;
>  	struct gpio_regs *regs;
>  };
>  
> @@ -150,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;
> @@ -258,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[] = {
> -	{ (struct gpio_regs *)GPIO1_BASE_ADDR },
> -	{ (struct gpio_regs *)GPIO2_BASE_ADDR },
> -	{ (struct gpio_regs *)GPIO3_BASE_ADDR },
> -#if defined(CONFIG_MX25) || defined(CONFIG_MX27) || defined(CONFIG_MX51) || \
> -		defined(CONFIG_MX53) || defined(CONFIG_MX6)
> -	{ (struct gpio_regs *)GPIO4_BASE_ADDR },
> -#endif
> -#if defined(CONFIG_MX27) || defined(CONFIG_MX53) || defined(CONFIG_MX6)
> -	{ (struct gpio_regs *)GPIO5_BASE_ADDR },
> -	{ (struct gpio_regs *)GPIO6_BASE_ADDR },
> -#endif
> -#if defined(CONFIG_MX53) || defined(CONFIG_MX6)
> -	{ (struct gpio_regs *)GPIO7_BASE_ADDR },
> -#endif
> -};
> -
>  static int mxc_gpio_probe(struct udevice *dev)
>  {
>  	struct mxc_bank_info *bank = dev_get_priv(dev);
> @@ -283,7 +270,7 @@ static int mxc_gpio_probe(struct udevice *dev)
>  	int banknum;
>  	char name[18], *str;
>  
> -	banknum = plat - mxc_plat;
> +	banknum = plat->bank_index;
>  	sprintf(name, "GPIO%d_", banknum + 1);
>  	str = strdup(name);
>  	if (!str)
> @@ -295,12 +282,77 @@ static int mxc_gpio_probe(struct udevice *dev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_OF_CONTROL
> +static struct gpio_regs *mxc_get_gpio_addr(struct udevice *device)
> +{
> +	fdt_addr_t addr;
> +	addr = fdtdec_get_addr(gd->fdt_blob, device->of_offset, "reg");
> +	if (addr == FDT_ADDR_T_NONE)
> +		return NULL;
> +	else
> +		return (struct gpio_regs *)addr;
> +}
> +#else
> +static struct gpio_regs *mxc_get_gpio_addr(struct udevice *device)
> +{
> +	return NULL;
> +}
> +#endif

In general, I'm fine with this concept, but I believe we should implement
a stub for fdtdec_get_addr() function in the fdtdec.h (say just returning
FDT_ADDR_T_NONE), as otherwise we might end up with multiple drivers
implementing the same noop callback just to work around a poor fdtdec_*()
interface.

> +
> +static int mxc_gpio_bind(struct udevice *device)
> +{
> +	struct mxc_gpio_plat *plat = device->platdata;
> +	struct gpio_regs *regs;
> +
> +	if (plat)
> +		return 0;
> +
> +	regs = mxc_get_gpio_addr(device);
> +	if (!regs)
> +		return -ENXIO;
> +
> +	plat = calloc(1, sizeof(*plat));
> +	if (!plat)
> +		return -ENOMEM;
> +
> +	plat->regs = regs;
> +	plat->bank_index = device->req_seq;
> +	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,
> +	.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
>  };
>  
>  U_BOOT_DEVICES(mxc_gpios) = {
> @@ -320,3 +372,4 @@ U_BOOT_DEVICES(mxc_gpios) = {
>  #endif
>  };
>  #endif
> +#endif
> 

-- 
Regards,
Igor.