| Message ID | 1421738127-26421-1-git-send-email-Peng.Fan@freescale.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
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 >
Hi, Igor On 1/20/2015 10:39 PM, Igor Grinberg wrote: > 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. Ok. I can split this in v3 patch. > >> 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... Reply 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. I tried to implement a stub function in fdtdec.h like this: __weak fdt_addr_t fdtdec_get_addr_wrap(xxxx) { return FDT_ADDR_T_NONE; } And in driver code, implement non weak version as following: #ifdef CONFIG_OF_CONTROL fdt_addr_t fdtdec_get_addr_wrap(xxxx) { .......... } #endif But gcc complains about conficting types, since we have a weak implementation in header file and a strong implementation in c file. If the weak one is in fdtxx c file, no error, but i thinke this is not a good idea to put this in fdtxx c file. If we do not want DT, but only DM, DT code should not be compiled into the final image. I tried another way, add the following piece code in driver/core/device.c and function prototype in device.h, " #ifdef CONFIG_OF_CONTROL void *dev_reg_addr(struct udevice *dev) { fdt_addr_t addr; addr = fdtdev_get_addr(gd->fdt_blob, dev->of_offset, "reg"); if (addr == FDT_ADDR_T_NONE) return NULL; else return (void *)addr } #else void *dev_reg_addr(struct udevice *dev) { return NULL; } #endif " I think `#ifdef` is needed here. I think this way is better that put stub function in fdtdec.h. Using this way, the driver code can just `add = dev_reg_addr(device)` to get reg address. Maybe the upper piece code should be put in a new file named device-util.c in directory device/core but not device.c? Comments? >> + >> +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, Peng.
[...] >>> @@ -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. > I tried to implement a stub function in fdtdec.h like this: > __weak fdt_addr_t fdtdec_get_addr_wrap(xxxx) > { > return FDT_ADDR_T_NONE; > } > And in driver code, implement non weak version as following: > #ifdef CONFIG_OF_CONTROL > fdt_addr_t fdtdec_get_addr_wrap(xxxx) > { > .......... > } > #endif > But gcc complains about conficting types, since we have a weak > implementation in header file and a strong implementation in c file. > If the weak one is in fdtxx c file, no error, but i thinke this is not > a good idea to put this in fdtxx c file. If we do not want DT, > but only DM, DT code should not be compiled into the final image. Right. Putting the __weak function inside fdtxx c file will not work either as it is not compiled for !CONFIG_OF_CONTROL. > > I tried another way, add the following piece code in > driver/core/device.c and function prototype in device.h, > " > #ifdef CONFIG_OF_CONTROL > void *dev_reg_addr(struct udevice *dev) > { > fdt_addr_t addr; > > addr = fdtdev_get_addr(gd->fdt_blob, dev->of_offset, "reg"); > if (addr == FDT_ADDR_T_NONE) > return NULL; > else > return (void *)addr > } > #else > void *dev_reg_addr(struct udevice *dev) > { > return NULL; > } > #endif > " > I think `#ifdef` is needed here. I think this way is better that put > stub function in fdtdec.h. Using this way, the driver code can just > `add = dev_reg_addr(device)` to get reg address. You will need to check "if (!add) ..." in the driver anyway... Yes, I agree - abstracting the dev_reg_addr() function is a great idea! It will improve the situation for all drivers that will use dev_get_addr(). Also, I think that in *addition* to the above, implementing a stub for fdtdev_get_addr() in fdtdec.h will make it even better, so you will not need the ifdef in driver/core/device.c too and also improve the fdtdec interface flexibility for any other (whatever will it be) case the driver/other code will need to call fdtdev_get_addr() explicitly. Having said the above, I must say that I'm really a fan of how Linux interfaces deals with the CONFIG_* options, especially DT related ones. So, I think that implementing your idea in driver/core/device.c is good enough for merging. Implementing the stub in fdtdec.h can be a bonus for all of us... > Maybe the upper piece code should be put in a new file named > device-util.c in directory device/core but not device.c? > Well, I think new file will not have any real improvement over the above ideas and concepts. [...]
Hi Igor, On 1/21/2015 5:18 PM, Igor Grinberg wrote: > [...] > >>>> @@ -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. >> I tried to implement a stub function in fdtdec.h like this: >> __weak fdt_addr_t fdtdec_get_addr_wrap(xxxx) >> { >> return FDT_ADDR_T_NONE; >> } >> And in driver code, implement non weak version as following: >> #ifdef CONFIG_OF_CONTROL >> fdt_addr_t fdtdec_get_addr_wrap(xxxx) >> { >> .......... >> } >> #endif >> But gcc complains about conficting types, since we have a weak >> implementation in header file and a strong implementation in c file. >> If the weak one is in fdtxx c file, no error, but i thinke this is not >> a good idea to put this in fdtxx c file. If we do not want DT, >> but only DM, DT code should not be compiled into the final image. > Right. Putting the __weak function inside fdtxx c file will not work either > as it is not compiled for !CONFIG_OF_CONTROL. > >> I tried another way, add the following piece code in >> driver/core/device.c and function prototype in device.h, >> " >> #ifdef CONFIG_OF_CONTROL >> void *dev_reg_addr(struct udevice *dev) >> { >> fdt_addr_t addr; >> >> addr = fdtdev_get_addr(gd->fdt_blob, dev->of_offset, "reg"); >> if (addr == FDT_ADDR_T_NONE) >> return NULL; >> else >> return (void *)addr >> } >> #else >> void *dev_reg_addr(struct udevice *dev) >> { >> return NULL; >> } >> #endif >> " >> I think `#ifdef` is needed here. I think this way is better that put >> stub function in fdtdec.h. Using this way, the driver code can just >> `add = dev_reg_addr(device)` to get reg address. > You will need to check "if (!add) ..." in the driver anyway... Yeah. > > Yes, I agree - abstracting the dev_reg_addr() function is a great idea! > It will improve the situation for all drivers that will use dev_get_addr(). ok. I'll use the upper code and make a single patch in v3 patch set. > > Also, I think that in *addition* to the above, implementing a stub for > fdtdev_get_addr() in fdtdec.h will make it even better, so you will > not need the ifdef in driver/core/device.c too and also improve the > fdtdec interface flexibility for any other (whatever will it be) case > the driver/other code will need to call fdtdev_get_addr() explicitly. > > Having said the above, I must say that I'm really a fan of how Linux > interfaces deals with the CONFIG_* options, especially DT related ones. > > So, I think that implementing your idea in driver/core/device.c is > good enough for merging. Thanks. > Implementing the stub in fdtdec.h can be a bonus for all of us... I does not come out a good idea about this, so this will not be in the v3 patch set. Make patch small:) >> Maybe the upper piece code should be put in a new file named >> device-util.c in directory device/core but not device.c? >> > Well, I think new file will not have any real improvement over the > above ideas and concepts. > > [...] > > Thanks, Peng.
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 + +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
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. 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. 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(-)