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....@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. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot