> -----Original Message-----
> From: Kevin Hilman [mailto:khil...@deeprootsystems.com]
> Sent: Saturday, May 01, 2010 5:34 AM
> To: Varadarajan, Charulatha
> Cc: linux-omap@vger.kernel.org; Nayak, Rajendra; p...@pwsan.com;
> t...@atomide.com
> Subject: Re: [PATCH 9/9] OMAP:GPIO: Implement GPIO as a platform device
>
> Charulatha V <ch...@ti.com> 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 <ch...@ti.com>
> > Signed-off-by: Rajendra Nayak <rna...@ti.com>
> > ---
> > 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
