[U-Boot] [PATCH] dm:gpio:mxc get configuration from dtb

[Igor Grinberg]

Hi Peng Fan,

On 01/19/15 08:11, Peng Fan wrote:
> This patch supports getting gpios' configuration from dtb.
> CONFIG_OF_CONTROL is used to indicated which part is for device tree,
> and which is not.

Can we please avoid the DT #ifdeffery in the drivers?
and just implement the needed API stubs?

> 
> This patch is already tested on mx6sxsabresd board. 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`
> 
> Signed-off-by: Peng Fan <Peng.Fan at freescale.com>
> ---
>  drivers/gpio/mxc_gpio.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
> 
> diff --git a/drivers/gpio/mxc_gpio.c b/drivers/gpio/mxc_gpio.c
> index 8bb9e39..8603068 100644
> --- a/drivers/gpio/mxc_gpio.c
> +++ b/drivers/gpio/mxc_gpio.c
> @@ -14,6 +14,10 @@
>  #include <asm/arch/imx-regs.h>
>  #include <asm/gpio.h>
>  #include <asm/io.h>
> +#ifdef CONFIG_OF_CONTROL
> +#include <fdtdec.h>
> +DECLARE_GLOBAL_DATA_PTR;
> +#endif

Is there a problem  to include fdtdec.h with no CONFIG_OF_CONTROL?

>  
>  enum mxc_gpio_direction {
>  	MXC_GPIO_DIRECTION_IN,
> @@ -258,6 +262,7 @@ static const struct dm_gpio_ops gpio_mxc_ops = {
>  	.get_function		= mxc_gpio_get_function,
>  };
>  
> +#ifndef CONFIG_OF_CONTROL
>  static const struct mxc_gpio_plat mxc_plat[] = {
>  	{ (struct gpio_regs *)GPIO1_BASE_ADDR },
>  	{ (struct gpio_regs *)GPIO2_BASE_ADDR },
> @@ -274,6 +279,7 @@ static const struct mxc_gpio_plat mxc_plat[] = {
>  	{ (struct gpio_regs *)GPIO7_BASE_ADDR },
>  #endif
>  };
> +#endif

I think instead of compiling this out, you should reuse it.

>  
>  static int mxc_gpio_probe(struct udevice *dev)
>  {
> @@ -283,7 +289,19 @@ static int mxc_gpio_probe(struct udevice *dev)
>  	int banknum;
>  	char name[18], *str;
>  
> +#ifdef CONFIG_OF_CONTROL
> +	/*
> +	 * In dts file add:
> +	 * aliases {
> +	 *	gpio0 = &gpio1;
> +	 *	gpio1 = &gpio2;
> +	 *	.....
> +	 * };
> +	 * Then set banknum accoring dev's seq number. */
> +	banknum = dev->seq;
> +#else
>  	banknum = plat - mxc_plat;
> +#endif

Well, I never liked too much that plat - mxc-plat logic, but
still I think, you should be able to have one implementation for both cases?
There is a good example in doc/driver-model/spi-howto.txt.

>  	sprintf(name, "GPIO%d_", banknum + 1);
>  	str = strdup(name);
>  	if (!str)
> @@ -295,14 +313,71 @@ static int mxc_gpio_probe(struct udevice *dev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_OF_CONTROL
> +static int mxc_gpio_bind(struct udevice *device)
> +{
> +	struct mxc_gpio_plat *plat = device->platdata;
> +	struct gpio_regs *ctrl;
> +
> +	if (plat)
> +		return 0;
> +	/*
> +	 * In the dts file, gpiox bank are as following:
> +	 *	gpio1: gpio at 0209c000 {
> +	 *		compatible = "fsl,imx6q-gpio", "fsl,imx35-gpio";
> +	 *		reg = <0x0209c000 0x4000>;
> +	 *		interrupts = <0 66 0x04 0 67 0x04>;
> +	 *		gpio-controller;
> +	 *		#gpio-cells = <2>;
> +	 *		interrupt-controller;
> +	 *		#interrupt-cells = <2>;
> +	 *	};
> +	 *
> +	 *	gpio2: gpio at 020a0000 {
> +	 *		compatible = "fsl,imx6q-gpio", "fsl,imx35-gpio";
> +	 *		reg = <0x020a0000 0x4000>;
> +	 *		interrupts = <0 68 0x04 0 69 0x04>;
> +	 *		gpio-controller;
> +	 *		#gpio-cells = <2>;
> +	 *		interrupt-controller;
> +	 *		#interrupt-cells = <2>;
> +	 *	};
> +	 *
> +	 * gpio1 is the 1st bank, gpio2 is the 2nd bank and gpio3 ....
> +	 */

The above comment looks not belonging here... but in documentation/dts itself?

> +
> +	ctrl = (struct gpio_regs *)fdtdec_get_addr(gd->fdt_blob,
> +						   device->of_offset, "reg");

How about implementing a stub for this returning NULL?

> +	plat = calloc(1, sizeof(*plat));
> +	if (!plat)
> +		return -ENOMEM;
> +
> +	plat->regs = ctrl;
> +
> +	device->platdata = plat;
> +
> +	return 0;
> +}
> +
> +static const struct udevice_id mxc_gpio_ids[] = {
> +	{ .compatible = "fsl,imx35-gpio" },
> +	{ }
> +};
> +#endif
> +
>  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),
> +#ifdef CONFIG_OF_CONTROL
> +	.of_match = mxc_gpio_ids,
> +	.bind	= mxc_gpio_bind,
> +#endif
>  };
>  
> +#ifndef CONFIG_OF_CONTROL
>  U_BOOT_DEVICES(mxc_gpios) = {
>  	{ "gpio_mxc", &mxc_plat[0] },
>  	{ "gpio_mxc", &mxc_plat[1] },
> @@ -320,3 +395,4 @@ U_BOOT_DEVICES(mxc_gpios) = {
>  #endif
>  };
>  #endif
> +#endif
> 

-- 
Regards,
Igor.