Hi Peng, On 24 January 2015 at 07:34, Peng Fan <b51...@freescale.com> wrote: > Hi Simon, > > > On 1/23/2015 5:26 AM, Simon Glass wrote: >> >> Hi Peng, >> >> On 21 January 2015 at 04:09, Peng Fan <peng....@freescale.com> wrote: >>> >>> This patch add DT support for mxc gpio driver. >>> >>> There are one place using CONFIG_OF_CONTROL macro. >>> 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. >>> >>> The following situations are tested, and all work fine: >>> 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> >>> --- >>> drivers/gpio/mxc_gpio.c | 69 >>> +++++++++++++++++++++++++++++++++++++------------ >>> 1 file changed, 52 insertions(+), 17 deletions(-) >>> >>> diff --git a/drivers/gpio/mxc_gpio.c b/drivers/gpio/mxc_gpio.c >>> index c52dd19..0766b78 100644 >>> --- a/drivers/gpio/mxc_gpio.c >>> +++ b/drivers/gpio/mxc_gpio.c >>> @@ -151,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; >>> @@ -259,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[] = { >>> - { 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 >>> -}; >>> - >>> static int mxc_gpio_probe(struct udevice *dev) >>> { >>> struct mxc_bank_info *bank = dev_get_priv(dev); >>> @@ -296,12 +282,60 @@ static int mxc_gpio_probe(struct udevice *dev) >>> return 0; >>> } >>> >>> +static int mxc_gpio_bind(struct udevice *device) >> >> s/device/dev/ as that is the convention. > > Will fix this. >> >> >>> +{ >>> + struct mxc_gpio_plat *plat = device->platdata; >>> + struct gpio_regs *regs; >>> + >>> + if (plat) >>> + return 0; >> >> Please add a comment as to why. > > Ok. >> >> >>> + >>> + regs = dev_get_addr(device); >>> + if (!regs) >>> + return -ENXIO; >> >> -ENODEV I think? > > Yeah. Right. >>> >>> + >>> + plat = calloc(1, sizeof(*plat)); >>> + if (!plat) >>> + return -ENOMEM; >> >> Can you use the auto-alloc feature instead? Trying to keep memory >> allocations out of drivers unless it is for buffers, etc. > > I want the DM code can support DT and no DT. To no DT, platdata is defined > in U_BOOT_DEVICES. > If using auto-alloc feature, but DT is not supported, is it conflict with > platdata in U_BOOT_DEVICES?
Yes it will. But please add a TODO in the code to remove this when every board is converted to driver model and you don't need this. >> >> >>> + >>> + plat->regs = regs; >>> + plat->bank_index = device->req_seq; >> >> Why store this? You can access it anytime using the device pointer. > > To no DT, bank_index is statically intialized in mxc_plat array. I do not > want to introudce `#ifdef CONFIG_OF_CONTROL` in probe function and introudce > `if (dev->of_offset == -1)`, so > store it to bank_index. > To no DT, `if(plat) return 0;` will return. So plat->bank_index = > device->req_seq will only effects for DT. > Just want to support DT and no DT. OK I think I understand. >> >> >>> + 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, >> >> Masahiro added a function for this.: >> >> .of_match = of_match_ptr(mxc_gpio_ids), >> >> But I'm not completely sure if this is wanted, since you include this >> information even when not using device tree. > > Thanks,I'll try this. I am not sure whether using of_match_ptr will make > compiler complain mxc_gpio_ids `defined but not used` for no DT, since > `#ifdef xx` is not recommended to compiled out `mxc_gpio_ids` for no DT. Yes it will. What you have is OK. >> >> >>> + .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 >> >> Can we remove the casts by changing the type to ulong? > > Hmm, this patch is just to make this driver can support DT. This will > introduce more change. Also, changing the type to ulong will change > mxc_gpio_plat struct. > If change the type to ulong, functions in this driver that uses platdata > will casts it to `struct gpio_regs *`. So i'd like not to remove the casts, > since remove them in mxc_plat will introduce casts in other functions which > use the platdata. OK. Something to think about later. In general a long line of casts indicates something should be fixed! Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot