> -----Original Message----- > From: Kevin Hilman [mailto:khilman@xxxxxxxxxxxxxxxxxxx] > Sent: Saturday, May 01, 2010 5:34 AM > To: Varadarajan, Charulatha > Cc: linux-omap@xxxxxxxxxxxxxxx; Nayak, Rajendra; paul@xxxxxxxxx; tony@xxxxxxxxxxx > Subject: Re: [PATCH 9/9] OMAP:GPIO: Implement GPIO as a platform device > > Charulatha V <charu@xxxxxx> writes: > > > This patch implements GPIO as a early platform device. Also it > > implements OMAP2PLUS specific GPIO as HWMOD FW adapted driver. > > Should include a summary explanation of why you're converting to an > early platform device as well. Okay. > > > Inorder to convert GPIO as platform device, modifications are > > required in clockxxxx_data.c files so that device names can be > > used to obtain clock instead of getting clocks by name/NULL ptr. > > ok > > > Currently early platform device register does not do device_pm_init. > > Hence pm_runtime functions are not used to enable the GPIO device > > since gpio is early platform device. > > OK for now, since this isn't using runtime PM, but maybe we need a > late_initcall() here to do the device_pm_init() + pm_runtime_enable() > > This change log needs more description of the intended init sequence. > Right now it seems that there are multiple init paths. Now that GPIO > is an early_platform_device, we should be able to at least make > omap_gpio_init() static and remove its usage from all the board files. This was the implementation that I initially did in my previous patch series. The following needs to be considered: 1. omap2_init_devices() might be too late for early_gpio_init() to be placed in it. 2. omap_init_irq() needs to be completed before calling early_gpio_init(). So, if early_gpio_init() is called from omap2_init_common_hw(), we need to have omap_init_irq() called before omap2_init_common_hw(). But Tony objected this approach mentioning that board might not boot up as omap2_init_common_hw() has to be done asap. That's why, I had not moved the omap_gpio_init() usage from board files. > > Also, the driver and device separation and init is totally mixed > together and very confusing. The platform_driver is in Agreed. Please see my comment at the end of this patch in reply to your other similar comment. > plat-omap/gpio.c and should be doing the driver init: > [early_]platform_driver_register() and _probe(). The platform_device > setup is in mach-omapX/gpio*.c and where the device init should be, in > this case early_platform_add_devices(). In early_gpio_init, omap_device_build() (device specific) and early_platform_driver_register_all() (driver specific), _probe() are done in a sequence. This is because early_gpio_init needs to be called asap and if we try to split this device specific and driver specific early_inits, we will end up having two init calls for each early drivers. I feel that multiple function calls for early_init (one for driver probe and one for device register) should be avoided as we need to find the right place to call them. > > > > Signed-off-by: Charulatha V <charu@xxxxxx> > > Signed-off-by: Rajendra Nayak <rnayak@xxxxxx> > > --- > > arch/arm/mach-omap1/Makefile | 6 + > > arch/arm/mach-omap1/clock_data.c | 2 +- > > arch/arm/mach-omap2/Makefile | 2 +- > > arch/arm/mach-omap2/clock2420_data.c | 10 +- > > arch/arm/mach-omap2/clock2430_data.c | 14 +- > > arch/arm/mach-omap2/clock3xxx_data.c | 24 +- > > arch/arm/mach-omap2/clock44xx_data.c | 24 +- > > arch/arm/plat-omap/gpio.c | 405 ++++++++++++-------------------- > > arch/arm/plat-omap/include/plat/gpio.h | 21 ++ > > 9 files changed, 220 insertions(+), 288 deletions(-) > > [...] > > > @@ -1621,6 +1501,34 @@ static void __init omap_gpio_show_rev(void) > > */ > > static struct lock_class_key gpio_lock_class; > > > > +static int init_gpio_info(void) > > +{ > > + gpio_bank_bits = 32; > > + > > + if (cpu_is_omap15xx()) { > > + gpio_bank_count = 2; > > + gpio_bank_bits = 16; > > + } else if (cpu_is_omap16xx()) { > > + gpio_bank_count = 5; > > + gpio_bank_bits = 16; > > + } else if (cpu_is_omap7xx()) > > + gpio_bank_count = 7; > > + else if (cpu_is_omap242x()) > > + gpio_bank_count = 4; > > + else if (cpu_is_omap243x()) > > + gpio_bank_count = 5; > > + else if (cpu_is_omap34xx() || cpu_is_omap44xx()) > > + gpio_bank_count = OMAP34XX_NR_GPIOS; > > Both the bank count and bank bits could be part of platform_data > and set in the SoC specific init. This is the GPIO driver part > and we're trying to make this as SoC independent as possible. Anytime > you need to add a cpu_is* or #ifdef in this code indicates something > that should be part of SoC specific init and passed in. bank count and bank bits are not specific to each device, but SoC specific. Hence I did not consider passing it as part of platform_data, because this information would be duplicated across all devices in that SoC. It would be good if we have a way to pass the SoC specific data which is common for all the devices rather than duplicating them by sending them via platform_data. Anyways, I can move it to platform_data if there is no other way. > > > + gpio_bank = kzalloc(gpio_bank_count * sizeof(struct gpio_bank), > > + GFP_KERNEL); > > + if (!gpio_bank) { > > + pr_err("Memory allocation failed for gpio_bank\n"); > > + return -ENOMEM; > > + } > > + return 0; > > +} > > + > > static void omap_gpio_mod_init(struct gpio_bank *bank, int id) > > { > > if (cpu_class_is_omap2()) { > > @@ -1686,16 +1594,9 @@ static void omap_gpio_mod_init(struct gpio_bank *bank, > int id) > > > > static void __init omap_gpio_chip_init(struct gpio_bank *bank) > > { > > - int j, gpio_bank_bits = 16; > > + int j; > > static int gpio; > > > > - if (cpu_is_omap7xx() && bank->method == METHOD_GPIO_7XX) > > - gpio_bank_bits = 32; /* 7xx has 32-bit GPIOs */ > > - > > - if ((bank->method == METHOD_GPIO_24XX) || > > - (bank->method == METHOD_GPIO_44XX)) > > - gpio_bank_bits = 32; > > - > > bank->mod_usage = 0; > > /* REVISIT eventually switch from OMAP-specific gpio structs > > * over to the generic ones > > @@ -1737,140 +1638,103 @@ static void __init omap_gpio_chip_init(struct > gpio_bank *bank) > > set_irq_data(bank->irq, bank); > > } > > > > -static int __init _omap_gpio_init(void) > > +static inline void get_gpio_dbck(struct platform_device *pdev, > > + struct gpio_bank *bank) > > { > > - int i; > > - int gpio = 0; > > + if (cpu_is_omap34xx() || cpu_is_omap44xx()) { > > + bank->dbck = clk_get(&pdev->dev, "dbck"); > > + if (IS_ERR(bank->dbck)) > > + pr_err("GPIO: Could not get dbck\n"); > > + } > > +} > > +static int __devinit omap_gpio_probe(struct platform_device *pdev) > > +{ > > + static int gpio_init_done; > > + struct omap_gpio_platform_data *pdata; > > + int id; > > struct gpio_bank *bank; > > - int bank_size = SZ_8K; /* Module 4KB + L4 4KB except on omap1 */ > > - char clk_name[11]; > > + struct resource *res; > > > > - initialized = 1; > > + if (!gpio_init_done) > > + init_gpio_info(); > > > > -#if defined(CONFIG_ARCH_OMAP1) > > - if (cpu_is_omap15xx()) { > > - gpio_ick = clk_get(NULL, "arm_gpio_ck"); > > - if (IS_ERR(gpio_ick)) > > - printk("Could not get arm_gpio_ck\n"); > > - else > > - clk_enable(gpio_ick); > > + if (!pdev || !pdev->dev.platform_data) { > > + pr_err("GPIO device initialize without" > > + "platform data\n"); > > + return -EINVAL; > > } > > -#endif > > -#if defined(CONFIG_ARCH_OMAP2) > > - if (cpu_class_is_omap2()) { > > - gpio_ick = clk_get(NULL, "gpios_ick"); > > - if (IS_ERR(gpio_ick)) > > - printk("Could not get gpios_ick\n"); > > - else > > - clk_enable(gpio_ick); > > - gpio_fck = clk_get(NULL, "gpios_fck"); > > - if (IS_ERR(gpio_fck)) > > - printk("Could not get gpios_fck\n"); > > - else > > - clk_enable(gpio_fck); > > > > - /* > > - * On 2430 & 3430 GPIO 5 uses CORE L4 ICLK > > - */ > > -#if defined(CONFIG_ARCH_OMAP2430) > > - if (cpu_is_omap2430()) { > > - gpio5_ick = clk_get(NULL, "gpio5_ick"); > > - if (IS_ERR(gpio5_ick)) > > - printk("Could not get gpio5_ick\n"); > > - else > > - clk_enable(gpio5_ick); > > - gpio5_fck = clk_get(NULL, "gpio5_fck"); > > - if (IS_ERR(gpio5_fck)) > > - printk("Could not get gpio5_fck\n"); > > - else > > - clk_enable(gpio5_fck); > > - } > > -#endif > > - } > > -#endif > > + pdata = pdev->dev.platform_data; > > > > -#if defined(CONFIG_ARCH_OMAP3) || defined(CONFIG_ARCH_OMAP4) > > - if (cpu_is_omap34xx() || cpu_is_omap44xx()) { > > - for (i = 0; i < OMAP34XX_NR_GPIOS; i++) { > > - sprintf(clk_name, "gpio%d_ick", i + 1); > > - gpio_iclks[i] = clk_get(NULL, clk_name); > > - if (IS_ERR(gpio_iclks[i])) > > - printk(KERN_ERR "Could not get %s\n", clk_name); > > - else > > - clk_enable(gpio_iclks[i]); > > - } > > + id = pdev->id; > > + if (id > gpio_bank_count) { > > + pr_err("Invalid GPIO device id (%d)\n", id); > > + return -EINVAL; > > } > > -#endif > > > > + bank = &gpio_bank[id]; > > > > -#ifdef CONFIG_ARCH_OMAP15XX > > - if (cpu_is_omap15xx()) { > > - gpio_bank_count = 2; > > - gpio_bank = gpio_bank_1510; > > - bank_size = SZ_2K; > > - } > > -#endif > > -#if defined(CONFIG_ARCH_OMAP16XX) > > - if (cpu_is_omap16xx()) { > > - gpio_bank_count = 5; > > - gpio_bank = gpio_bank_1610; > > - bank_size = SZ_2K; > > - } > > -#endif > > -#if defined(CONFIG_ARCH_OMAP730) || defined(CONFIG_ARCH_OMAP850) > > - if (cpu_is_omap7xx()) { > > - gpio_bank_count = 7; > > - gpio_bank = gpio_bank_7xx; > > - bank_size = SZ_2K; > > - } > > -#endif > > -#ifdef CONFIG_ARCH_OMAP2 > > - if (cpu_is_omap242x()) { > > - gpio_bank_count = 4; > > - gpio_bank = gpio_bank_242x; > > - } > > - if (cpu_is_omap243x()) { > > - gpio_bank_count = 5; > > - gpio_bank = gpio_bank_243x; > > - } > > -#endif > > -#ifdef CONFIG_ARCH_OMAP3 > > - if (cpu_is_omap34xx()) { > > - gpio_bank_count = OMAP34XX_NR_GPIOS; > > - gpio_bank = gpio_bank_34xx; > > + if (bank->initialized == 1) { > > is_early_platform_device() can be used to tell if the _probe() is > being called as part of early platform driver probe or "regular". Okay. > > > + /* > > + * Currently, for early platform_devices, > > + * clk_get() using dev ptr does not seem to be working > > + * Hence getting dbck during regular device probe > > + */ > > Need a better explanation here. "does not seem to work" is not > quite good enough. Okay. The reason for "does not seem to work" is that device_initialize() is not called as part of omap_early_device_register and the clk_get could not find the clock using dev pointer. > > > + get_gpio_dbck(pdev, bank); > > In any case, maybe it's better to defer the clk_get() into > the debounce setup and only use if needed. Then, this needs to be done only in omap_set_gpio_debounce() which does not have pdev->dev info. I can take care of this. > > > + return 0; > > } > > -#endif > > -#ifdef CONFIG_ARCH_OMAP4 > > - if (cpu_is_omap44xx()) { > > - gpio_bank_count = OMAP34XX_NR_GPIOS; > > - gpio_bank = gpio_bank_44xx; > > + > > + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > > + if (unlikely(!res)) { > > + pr_err("GPIO Bank %i Invalid IRQ resource\n", id); > > + return -ENODEV; > > } > > -#endif > > - for (i = 0; i < gpio_bank_count; i++) { > > + bank->irq = res->start; > > + bank->virtual_irq_start = pdata->virtual_irq_start; > > + bank->base = pdata->base; > > Why are you using pdata->base for OMAP2+ base addresses... Thanks for pointing this. I will correct it. > > > + bank->method = pdata->method; > > > > - bank = &gpio_bank[i]; > > - spin_lock_init(&bank->lock); > > + spin_lock_init(&bank->lock); > > > > + if (cpu_class_is_omap2()) { > > + bank->device_enable = pdata->device_enable; > > + bank->device_idle = pdata->device_idle; > > + bank->device_shutdown = pdata->device_shutdown; > > + pdata->device_enable(pdev); > > + } else if (cpu_class_is_omap1()) { > > /* Static mapping, never released */ > > - bank->base = ioremap(bank->pbase, bank_size); > > - if (!bank->base) { > > - printk(KERN_ERR "Could not ioremap gpio bank%i\n", i); > > - continue; > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > ... and doing it the right way here for OMAP1? > > platform_get_resource() should be used for both. Yes. > > > + if (unlikely(!res)) { > > + pr_err("GPIO Bank %i Invalid mem resource\n", id); > > + return -ENODEV; > > } > > > > - omap_gpio_mod_init(bank, i); > > - omap_gpio_chip_init(bank); > > + bank->base = ioremap(res->start, resource_size(res)); > > + if (!bank->base) { > > + pr_err("Could not ioremap gpio bank%i\n", id); > > + return -ENOMEM; > > + } > > > > - if (cpu_is_omap34xx() || cpu_is_omap44xx()) { > > - sprintf(clk_name, "gpio%d_dbck", i + 1); > > - bank->dbck = clk_get(NULL, clk_name); > > - if (IS_ERR(bank->dbck)) > > - printk(KERN_ERR "Could not get %s\n", clk_name); > > + if (cpu_is_omap15xx() && (id == 0)) { > > + static struct clk *gpio_clk; > > + gpio_clk = clk_get(&pdev->dev, "arm_gpio_ck"); > > + if (IS_ERR(gpio_clk)) > > + pr_err("Could not get arm_gpio_ck\n"); > > + else > > + clk_enable(gpio_clk); > > } > > } > > > > - omap_gpio_show_rev(); > > + omap_gpio_mod_init(bank, id); > > + omap_gpio_chip_init(bank); > > > > + if (!gpio_init_done) { > > + omap_gpio_show_rev(); > > + gpio_init_done = 1; > > + } > > + > > + bank->initialized = 1; > > I think you can drop this flag, and use is_early_platform_device() if > needed. Sure. > > > return 0; > > } > > > > @@ -2210,16 +2074,42 @@ void omap_gpio_restore_context(void) > > } > > #endif > > > > -/* > > - * This may get called early from board specific init > > - * for boards that have interrupts routed via FPGA. > > - */ > > +static struct platform_driver omap_gpio_driver = { > > + .probe = omap_gpio_probe, > > + .driver = { > > + .name = "omap-gpio", > > + }, > > +}; > > + > > +int __init omap_gpio_drv_reg(void) > > +{ > > + return platform_driver_register(&omap_gpio_driver); > > +} > > + > > +early_platform_init("earlygpio", &omap_gpio_driver); > > + > > int __init omap_gpio_init(void) > > { > > - if (!initialized) > > - return _omap_gpio_init(); > > - else > > + int ret = 0; > > + > > + if (initialized) > > return 0; > > + > > +#ifdef CONFIG_ARCH_OMAP1 > > + if (cpu_is_omap7xx()) > > + ret = omap7xx_gpio_init(); > > + if (cpu_is_omap15xx()) > > + ret = omap15xx_gpio_init(); > > + if (cpu_is_omap16xx()) > > + ret = omap16xx_gpio_init(); > > +#endif > > +#ifdef CONFIG_ARCH_OMAP2PLUS > > + if (cpu_class_is_omap2()) > > + ret = omap2_gpio_init(); > > +#endif > > No thanks. driver init should not be calling device init. The device > init should have it's own initcall or be called by other early device > init. Agreed. omap_gpio_init() does nothing but calling device_init(). But having omap_gpio_init() isn't a problem for OMAP2+, whereas it would break multi-omap build for OMAP1 because there are 3 files in mach-omap1 for each omap. 1. I can club all the 3 gpio files in mach-omap1 in to one file mach-omap1/gpio.c and use omap_gpio_init() in it. But the patch size and file size might be much bigger as the platform_data for all OMAP1 GPIO devices would be available in this file. 2. (or) While maintaining 3 different files for mach-omap1/gpio_xxx.c, modify the board files (about 20 files) of mach-omap1 and make use of SoC specific omap_gpio_init. 3. (or) Call gpio_init() from omap_init_common_hw() by removing the calls from board files and do a cpu_is check (for mach-omap1) in it. Please suggest if we have any other ways to handle this and the better way to handle this. > > > + initialized = 1; > > + > > + return ret; > > } > > > > static int __init omap_gpio_sysinit(void) > > @@ -2227,7 +2117,10 @@ static int __init omap_gpio_sysinit(void) > > int ret = 0; > > > > if (!initialized) > > - ret = _omap_gpio_init(); > > + ret = omap_gpio_init(); > > + > > + if (!ret) > > + ret = omap_gpio_drv_reg(); > > > > mpuio_init(); > > Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
