[U-Boot] [PATCH v2] dm:gpio:mxc add DT support
Igor Grinberg
grinberg at compulab.co.il
Tue Jan 20 15:39:24 CET 2015
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 at 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.
More information about the U-Boot
mailing list