Re: [PATCH 5/9] OMAP:GPIO: Introduce support for OMAP2PLUS chip specific GPIO

2010-05-12 Thread Kevin Hilman
"Varadarajan, Charulatha"  writes:

> Tony/Kevin,
>
>> > > +{
>> > > +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;
>> > > +
>> > > +if (gpio_init())
>> > > +return -EINVAL;
>> > > +
>> > > +early_platform_driver_register_all("earlygpio");
>> > > +early_platform_driver_probe("earlygpio", gpio_bank_count, 0);
>> > > +return 0;
>> > > +}
>> >
>> > Then please replace this init with something like:
>> 
>> Okay.
>> 
>> >
>> > #ifdef CONFIG_ARCH_OMAP2
>> > int __init omap242x_gpio_init(void)
>> > {
>> >if (!cpu_is_omap2420())
>> >return -EINVAL;
>> >
>> >gpio_bank_count = 4;
>> >
>> >return gpio_init(METHOD_GPIO_24XX);
>> > }
>> > subsys_initcall(omap242x_gpio_init);
>> >
>> > int __init omap243x_gpio_init(void)
>> > {
>> >if (!cpu_is_omap2430())
>> >return -EINVAL;
>> >
>> >gpio_bank_count = 5;
>> >
>> >return gpio_init(METHOD_GPIO_24XX);
>> > }
>> > subsys_initcall(omap243x_gpio_init);
>> > #endif
>> >
>> > #ifdef CONFIG_ARCH_OMAP3
>> > int __init omap34xx_gpio_init(void)
>> > {
>> >if (!cpu_is_omap34xx())
>> >return -EINVAL;
>> >
>> >gpio_bank_count = OMAP34X_NR_GPIOS;
>> >
>> >return gpio_init(METHOD_GPIO_34XX);
>> > }
>> > subsys_initcall(omap34xx_gpio_init);
>> > #endif
>> > ...
>> >
>> > This way it will be more future proof when new omaps get added
>> > and the if else stuff disappears. Also then you'll have an omap
>> > specific function to initialize the gpio stuff.
>> >
>> > Note that then early_platform_driver_register_all and
>> > early_platform_driver_probe can be moved to gpio_init.
>> >
>> > With multi-omap build the subsys_initcall runs for all of the
>> > selected platforms, but returns early except for the machine
>> > we're running on. All the code is optimized out for omap
>> > specific product kernels.
>> 
>> Okay. Will do the needful and send new patch series in 2 weeks.
>
> subsys_initcall is not sufficient for SoC specific gpio_init as it needs
> to be done before machine_init functions access gpio APIs. Hence I am 
> making SoC specific gpio_init as postcore_initcall.

OK.  Please add a comment at the postcore_initcall() location with the
details as to why it is needed and what it needs to go before etc.

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


RE: [PATCH 5/9] OMAP:GPIO: Introduce support for OMAP2PLUS chip specific GPIO

2010-05-12 Thread Varadarajan, Charulatha
Tony/Kevin,

> > > +{
> > > + 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;
> > > +
> > > + if (gpio_init())
> > > + return -EINVAL;
> > > +
> > > + early_platform_driver_register_all("earlygpio");
> > > + early_platform_driver_probe("earlygpio", gpio_bank_count, 0);
> > > + return 0;
> > > +}
> >
> > Then please replace this init with something like:
> 
> Okay.
> 
> >
> > #ifdef CONFIG_ARCH_OMAP2
> > int __init omap242x_gpio_init(void)
> > {
> > if (!cpu_is_omap2420())
> > return -EINVAL;
> >
> > gpio_bank_count = 4;
> >
> > return gpio_init(METHOD_GPIO_24XX);
> > }
> > subsys_initcall(omap242x_gpio_init);
> >
> > int __init omap243x_gpio_init(void)
> > {
> > if (!cpu_is_omap2430())
> > return -EINVAL;
> >
> > gpio_bank_count = 5;
> >
> > return gpio_init(METHOD_GPIO_24XX);
> > }
> > subsys_initcall(omap243x_gpio_init);
> > #endif
> >
> > #ifdef CONFIG_ARCH_OMAP3
> > int __init omap34xx_gpio_init(void)
> > {
> > if (!cpu_is_omap34xx())
> > return -EINVAL;
> >
> > gpio_bank_count = OMAP34X_NR_GPIOS;
> >
> > return gpio_init(METHOD_GPIO_34XX);
> > }
> > subsys_initcall(omap34xx_gpio_init);
> > #endif
> > ...
> >
> > This way it will be more future proof when new omaps get added
> > and the if else stuff disappears. Also then you'll have an omap
> > specific function to initialize the gpio stuff.
> >
> > Note that then early_platform_driver_register_all and
> > early_platform_driver_probe can be moved to gpio_init.
> >
> > With multi-omap build the subsys_initcall runs for all of the
> > selected platforms, but returns early except for the machine
> > we're running on. All the code is optimized out for omap
> > specific product kernels.
> 
> Okay. Will do the needful and send new patch series in 2 weeks.

subsys_initcall is not sufficient for SoC specific gpio_init as it needs
to be done before machine_init functions access gpio APIs. Hence I am 
making SoC specific gpio_init as postcore_initcall.


> 

--
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


Re: [PATCH 5/9] OMAP:GPIO: Introduce support for OMAP2PLUS chip specific GPIO

2010-05-11 Thread Kevin Hilman
"Varadarajan, Charulatha"  writes:


>> > -Original Message-
>> > From: Kevin Hilman [mailto:khil...@deeprootsystems.com]
>> > Sent: Saturday, May 01, 2010 4:33 AM
>> > To: Varadarajan, Charulatha
>> > Cc: linux-omap@vger.kernel.org; Nayak, Rajendra; p...@pwsan.com;
>> t...@atomide.com
>> > Subject: Re: [PATCH 5/9] OMAP:GPIO: Introduce support for OMAP2PLUS chip
>> specific
>> > GPIO
>> >
>> > Charulatha V  writes:
>> >
>> > > This patch adds support for handling GPIO as a HWMOD adapted
>> > > platform device for OMAP2PLUS chips.
>> > >
>> > > Signed-off-by: Charulatha V 
>> > > ---
>> > >  arch/arm/mach-omap2/gpio.c |  101
>> 
>> > >  1 files changed, 101 insertions(+), 0 deletions(-)
>> > >  create mode 100644 arch/arm/mach-omap2/gpio.c
>> > >
>> > > diff --git a/arch/arm/mach-omap2/gpio.c b/arch/arm/mach-omap2/gpio.c
>> > > new file mode 100644
>> > > index 000..6424050
>> > > --- /dev/null
>> > > +++ b/arch/arm/mach-omap2/gpio.c
>> > > @@ -0,0 +1,101 @@
>
> ..[snip]..
>
>> > > +if (cpu_is_omap24xx() || cpu_is_omap34xx())
>> > > +pdata->method = METHOD_GPIO_24XX;
>> > > +if (cpu_is_omap44xx())
>> > > +pdata->method = METHOD_GPIO_44XX;
>> > > +pdata->virtual_irq_start = IH_GPIO_BASE + 32 * i;
>> > > +pdata->device_enable = omap_device_enable;
>> > > +pdata->device_idle = omap_device_idle;
>> > > +pdata->device_shutdown = omap_device_shutdown;
>> >
>> > These aren't valid for GPIO1 which is in WKUP.  Maybe we need
>> > to check if the hwmod is not in wkup_pwrdm before setting these?
>> 
>> I need to check how to implement this.
>> 
>
> There are two ways to implement this:
> 1. Use a flag in dev_attr of the device to indicate if the device belongs to
>WKUP domain
> 2. We can add an API in powerdomain FW to provide information if the 
>device belongs to a "always_on" domain or otherwise, and use 
>this API for each device.
>

Actually, as I think about this more, my initial comment is wrong.

the omap_device* functions are perfectly valid for GPIO1 as they have
independent control that is managed by hwmod, so please ignore my
comment.  Your approach is fine.

Thanks,

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


RE: [PATCH 5/9] OMAP:GPIO: Introduce support for OMAP2PLUS chip specific GPIO

2010-05-11 Thread Varadarajan, Charulatha
Kevin,

> > -Original Message-
> > From: Kevin Hilman [mailto:khil...@deeprootsystems.com]
> > Sent: Saturday, May 01, 2010 4:33 AM
> > To: Varadarajan, Charulatha
> > Cc: linux-omap@vger.kernel.org; Nayak, Rajendra; p...@pwsan.com;
> t...@atomide.com
> > Subject: Re: [PATCH 5/9] OMAP:GPIO: Introduce support for OMAP2PLUS chip
> specific
> > GPIO
> >
> > Charulatha V  writes:
> >
> > > This patch adds support for handling GPIO as a HWMOD adapted
> > > platform device for OMAP2PLUS chips.
> > >
> > > Signed-off-by: Charulatha V 
> > > ---
> > >  arch/arm/mach-omap2/gpio.c |  101
> 
> > >  1 files changed, 101 insertions(+), 0 deletions(-)
> > >  create mode 100644 arch/arm/mach-omap2/gpio.c
> > >
> > > diff --git a/arch/arm/mach-omap2/gpio.c b/arch/arm/mach-omap2/gpio.c
> > > new file mode 100644
> > > index 000..6424050
> > > --- /dev/null
> > > +++ b/arch/arm/mach-omap2/gpio.c
> > > @@ -0,0 +1,101 @@

..[snip]..

> > > + if (cpu_is_omap24xx() || cpu_is_omap34xx())
> > > + pdata->method = METHOD_GPIO_24XX;
> > > + if (cpu_is_omap44xx())
> > > + pdata->method = METHOD_GPIO_44XX;
> > > + pdata->virtual_irq_start = IH_GPIO_BASE + 32 * i;
> > > + pdata->device_enable = omap_device_enable;
> > > + pdata->device_idle = omap_device_idle;
> > > + pdata->device_shutdown = omap_device_shutdown;
> >
> > These aren't valid for GPIO1 which is in WKUP.  Maybe we need
> > to check if the hwmod is not in wkup_pwrdm before setting these?
> 
> I need to check how to implement this.
> 

There are two ways to implement this:
1. Use a flag in dev_attr of the device to indicate if the device belongs to
   WKUP domain
2. We can add an API in powerdomain FW to provide information if the 
   device belongs to a "always_on" domain or otherwise, and use 
   this API for each device.

Kindly suggest. 

-V Charulatha


--
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


RE: [PATCH 5/9] OMAP:GPIO: Introduce support for OMAP2PLUS chip specific GPIO

2010-05-06 Thread Varadarajan, Charulatha


> -Original Message-
> From: Tony Lindgren [mailto:t...@atomide.com]
> Sent: Thursday, May 06, 2010 4:02 AM
> To: Varadarajan, Charulatha
> Cc: linux-omap@vger.kernel.org; Nayak, Rajendra; p...@pwsan.com;
> khil...@deeprootsystems.com
> Subject: Re: [PATCH 5/9] OMAP:GPIO: Introduce support for OMAP2PLUS chip 
> specific
> GPIO
> 
> * Charulatha V  [100422 08:50]:
> > This patch adds support for handling GPIO as a HWMOD adapted
> > platform device for OMAP2PLUS chips.
> 
> 
> 
> > +int __init gpio_init(void)
> > +{
> > +   int i = 0;
> > +   static int is_early_device = true;
> > +
> > +   do {
> > +   struct omap_device *od;
> > +   struct omap_hwmod *oh;
> > +   int hw_mod_name_len = 16;
> > +   int l;
> > +   char oh_name[hw_mod_name_len];
> > +   struct omap_gpio_platform_data *pdata;
> > +   char *name = "omap-gpio";
> > +
> > +   l = snprintf(oh_name, hw_mod_name_len, "gpio%d_hwmod", i + 1);
> > +   WARN(l >= hw_mod_name_len,
> > +   "String buffer overflow in GPIO device setup\n");
> > +
> > +   oh = omap_hwmod_lookup(oh_name);
> > +   if (!oh) {
> > +   pr_err("Could not look up %s\n", oh_name);
> > +   i++;
> > +   continue;
> > +   }
> > +
> > +   pdata = kzalloc(sizeof(struct omap_gpio_platform_data),
> > +   GFP_KERNEL);
> > +   if (!pdata) {
> > +   pr_err("Memory allocation failed gpio%d\n", i + 1);
> > +   return -ENOMEM;
> > +   }
> > +   pdata->base = oh->_rt_va;
> > +   pdata->irq = oh->mpu_irqs[0].irq;
> > +   if (cpu_is_omap24xx() || cpu_is_omap34xx())
> > +   pdata->method = METHOD_GPIO_24XX;
> > +   if (cpu_is_omap44xx())
> > +   pdata->method = METHOD_GPIO_44XX;
> > +   pdata->virtual_irq_start = IH_GPIO_BASE + 32 * i;
> > +   pdata->device_enable = omap_device_enable;
> > +   pdata->device_idle = omap_device_idle;
> > +   pdata->device_shutdown = omap_device_shutdown;
> > +
> > +   od = omap_device_build(name, i, oh, pdata,
> > +   sizeof(*pdata), omap_gpio_latency,
> > +   ARRAY_SIZE(omap_gpio_latency),
> > +   is_early_device);
> > +   WARN(IS_ERR(od), "Cant build omap_device for %s:%s.\n",
> > +   name, oh->name);
> > +
> > +   i++;
> > +   } while (i < gpio_bank_count);
> > +   is_early_device = false;
> > +
> > +   return 0;
> > +}
> > +arch_initcall(gpio_init);
> 
> You can get rid of most of the cpu_is_omap in gpio_init if you pass
> the method as a parameter to gpio_init(int method). We should only need
> to use cpu_is_omap exactly once for every init.

Okay.

> 
> > +int __init omap2_gpio_init(void)
> > +{
> > +   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;
> > +
> > +   if (gpio_init())
> > +   return -EINVAL;
> > +
> > +   early_platform_driver_register_all("earlygpio");
> > +   early_platform_driver_probe("earlygpio", gpio_bank_count, 0);
> > +   return 0;
> > +}
> 
> Then please replace this init with something like:

Okay.

> 
> #ifdef CONFIG_ARCH_OMAP2
> int __init omap242x_gpio_init(void)
> {
>   if (!cpu_is_omap2420())
>   return -EINVAL;
> 
>   gpio_bank_count = 4;
> 
>   return gpio_init(METHOD_GPIO_24XX);
> }
> subsys_initcall(omap242x_gpio_init);
> 
> int __init omap243x_gpio_init(void)
> {
>   if (!cpu_is_omap2430())
>   return -EINVAL;
> 
>   gpio_bank_count = 5;
> 
>   return gpio_init(METHOD_GPIO_24XX);
> }
> subsys_initcall(omap243x_gpio_init);
> #endif
> 
> #ifdef CONFIG_ARCH_OMAP3
> int __init omap34xx_gpio_init(void)
> {
>   if (!cpu_is_omap34xx())
>   return -EINVAL;
> 
>   gpio_bank_count = OMAP34X_NR_GPIOS;
> 
>   return gpio_init(METHOD_GPIO_34XX);
> }
> 

Re: [PATCH 5/9] OMAP:GPIO: Introduce support for OMAP2PLUS chip specific GPIO

2010-05-05 Thread Tony Lindgren
* Charulatha V  [100422 08:50]:
> This patch adds support for handling GPIO as a HWMOD adapted
> platform device for OMAP2PLUS chips.



> +int __init gpio_init(void)
> +{
> + int i = 0;
> + static int is_early_device = true;
> +
> + do {
> + struct omap_device *od;
> + struct omap_hwmod *oh;
> + int hw_mod_name_len = 16;
> + int l;
> + char oh_name[hw_mod_name_len];
> + struct omap_gpio_platform_data *pdata;
> + char *name = "omap-gpio";
> +
> + l = snprintf(oh_name, hw_mod_name_len, "gpio%d_hwmod", i + 1);
> + WARN(l >= hw_mod_name_len,
> + "String buffer overflow in GPIO device setup\n");
> +
> + oh = omap_hwmod_lookup(oh_name);
> + if (!oh) {
> + pr_err("Could not look up %s\n", oh_name);
> + i++;
> + continue;
> + }
> +
> + pdata = kzalloc(sizeof(struct omap_gpio_platform_data),
> + GFP_KERNEL);
> + if (!pdata) {
> + pr_err("Memory allocation failed gpio%d\n", i + 1);
> + return -ENOMEM;
> + }
> + pdata->base = oh->_rt_va;
> + pdata->irq = oh->mpu_irqs[0].irq;
> + if (cpu_is_omap24xx() || cpu_is_omap34xx())
> + pdata->method = METHOD_GPIO_24XX;
> + if (cpu_is_omap44xx())
> + pdata->method = METHOD_GPIO_44XX;
> + pdata->virtual_irq_start = IH_GPIO_BASE + 32 * i;
> + pdata->device_enable = omap_device_enable;
> + pdata->device_idle = omap_device_idle;
> + pdata->device_shutdown = omap_device_shutdown;
> +
> + od = omap_device_build(name, i, oh, pdata,
> + sizeof(*pdata), omap_gpio_latency,
> + ARRAY_SIZE(omap_gpio_latency),
> + is_early_device);
> + WARN(IS_ERR(od), "Cant build omap_device for %s:%s.\n",
> + name, oh->name);
> +
> + i++;
> + } while (i < gpio_bank_count);
> + is_early_device = false;
> +
> + return 0;
> +}
> +arch_initcall(gpio_init);

You can get rid of most of the cpu_is_omap in gpio_init if you pass
the method as a parameter to gpio_init(int method). We should only need
to use cpu_is_omap exactly once for every init.

> +int __init omap2_gpio_init(void)
> +{
> + 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;
> +
> + if (gpio_init())
> + return -EINVAL;
> +
> + early_platform_driver_register_all("earlygpio");
> + early_platform_driver_probe("earlygpio", gpio_bank_count, 0);
> + return 0;
> +}

Then please replace this init with something like:

#ifdef CONFIG_ARCH_OMAP2
int __init omap242x_gpio_init(void)
{
if (!cpu_is_omap2420())
return -EINVAL;

gpio_bank_count = 4;

return gpio_init(METHOD_GPIO_24XX);
}
subsys_initcall(omap242x_gpio_init);

int __init omap243x_gpio_init(void)
{
if (!cpu_is_omap2430())
return -EINVAL;

gpio_bank_count = 5;

return gpio_init(METHOD_GPIO_24XX);
}
subsys_initcall(omap243x_gpio_init);
#endif

#ifdef CONFIG_ARCH_OMAP3
int __init omap34xx_gpio_init(void)
{
if (!cpu_is_omap34xx())
return -EINVAL;

gpio_bank_count = OMAP34X_NR_GPIOS;

return gpio_init(METHOD_GPIO_34XX);
}
subsys_initcall(omap34xx_gpio_init);
#endif
...

This way it will be more future proof when new omaps get added
and the if else stuff disappears. Also then you'll have an omap
specific function to initialize the gpio stuff.

Note that then early_platform_driver_register_all and
early_platform_driver_probe can be moved to gpio_init.

With multi-omap build the subsys_initcall runs for all of the
selected platforms, but returns early except for the machine
we're running on. All the code is optimized out for omap
specific product kernels.

Regards,

Tony
--
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


RE: [PATCH 5/9] OMAP:GPIO: Introduce support for OMAP2PLUS chip specific GPIO

2010-05-04 Thread Varadarajan, Charulatha


> -Original Message-
> From: Kevin Hilman [mailto:khil...@deeprootsystems.com]
> Sent: Saturday, May 01, 2010 4:33 AM
> To: Varadarajan, Charulatha
> Cc: linux-omap@vger.kernel.org; Nayak, Rajendra; p...@pwsan.com; 
> t...@atomide.com
> Subject: Re: [PATCH 5/9] OMAP:GPIO: Introduce support for OMAP2PLUS chip 
> specific
> GPIO
> 
> Charulatha V  writes:
> 
> > This patch adds support for handling GPIO as a HWMOD adapted
> > platform device for OMAP2PLUS chips.
> >
> > Signed-off-by: Charulatha V 
> > ---
> >  arch/arm/mach-omap2/gpio.c |  101 
> > 
> >  1 files changed, 101 insertions(+), 0 deletions(-)
> >  create mode 100644 arch/arm/mach-omap2/gpio.c
> >
> > diff --git a/arch/arm/mach-omap2/gpio.c b/arch/arm/mach-omap2/gpio.c
> > new file mode 100644
> > index 000..6424050
> > --- /dev/null
> > +++ b/arch/arm/mach-omap2/gpio.c
> > @@ -0,0 +1,101 @@
> > +/*
> > + * gpio.c - OMAP2PLUS-specific gpio code
> > + *
> > + * Copyright (C) 2010 Texas Instruments, Inc.
> > + *
> > + * Author:
> > + * Charulatha V 
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +static int gpio_bank_count;
> > +
> > +struct omap_device_pm_latency omap_gpio_latency[] = {
> > +   [0] = {
> > +   .deactivate_func = omap_device_idle_hwmods,
> > +   .activate_func   = omap_device_enable_hwmods,
> > +   .flags   = OMAP_DEVICE_LATENCY_AUTO_ADJUST,
> > +   },
> > +};
> > +
> > +int __init gpio_init(void)
> > +{
> > +   int i = 0;
> > +   static int is_early_device = true;
> > +
> > +   do {
> > +   struct omap_device *od;
> > +   struct omap_hwmod *oh;
> > +   int hw_mod_name_len = 16;
> > +   int l;
> > +   char oh_name[hw_mod_name_len];
> > +   struct omap_gpio_platform_data *pdata;
> > +   char *name = "omap-gpio";
> > +
> > +   l = snprintf(oh_name, hw_mod_name_len, "gpio%d_hwmod", i + 1);
> > +   WARN(l >= hw_mod_name_len,
> > +   "String buffer overflow in GPIO device setup\n");
> > +
> 
> Rather than iterating over the name, you should have a hwmod_class for
> GPIOs and use omap_hwmod_for_each_by_class() to iterate over all the
> hwmods in the GPIO class.

Ok.

> 
> > +   oh = omap_hwmod_lookup(oh_name);
> > +   if (!oh) {
> > +   pr_err("Could not look up %s\n", oh_name);
> > +   i++;
> > +   continue;
> > +   }
> > +
> > +   pdata = kzalloc(sizeof(struct omap_gpio_platform_data),
> > +   GFP_KERNEL);
> > +   if (!pdata) {
> > +   pr_err("Memory allocation failed gpio%d\n", i + 1);
> > +   return -ENOMEM;
> > +   }
> > +   pdata->base = oh->_rt_va;
> > +   pdata->irq = oh->mpu_irqs[0].irq;
> 
> base address and IRQ should not be part of platform_data.   platform
> driver should be using platform_get_resource() for both.

Agreed. Will modify.

> 
> > +   if (cpu_is_omap24xx() || cpu_is_omap34xx())
> > +   pdata->method = METHOD_GPIO_24XX;
> > +   if (cpu_is_omap44xx())
> > +   pdata->method = METHOD_GPIO_44XX;
> > +   pdata->virtual_irq_start = IH_GPIO_BASE + 32 * i;
> > +   pdata->device_enable = omap_device_enable;
> > +   pdata->device_idle = omap_device_idle;
> > +   pdata->device_shutdown = omap_device_shutdown;
> 
> These aren't valid for GPIO1 which is in WKUP.  Maybe we need
> to check if the hwmod is not in wkup_pwrdm before setting these?

I need to check how to implement this. 

> 
> > +   od = omap_device_build(name, i, oh, pdata,
> > +   sizeof(*pdata), omap_gpio_latency,
> > +   ARRAY_SIZE(omap_gpio_latency),
> > +   is_early_device);
> > +   WARN(IS_ERR(od), "Cant build omap_device for %s:%s.\n",
> > +  

Re: [PATCH 5/9] OMAP:GPIO: Introduce support for OMAP2PLUS chip specific GPIO

2010-04-30 Thread Kevin Hilman
Charulatha V  writes:

> This patch adds support for handling GPIO as a HWMOD adapted
> platform device for OMAP2PLUS chips.
>
> Signed-off-by: Charulatha V 
> ---
>  arch/arm/mach-omap2/gpio.c |  101 
> 
>  1 files changed, 101 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/mach-omap2/gpio.c
>
> diff --git a/arch/arm/mach-omap2/gpio.c b/arch/arm/mach-omap2/gpio.c
> new file mode 100644
> index 000..6424050
> --- /dev/null
> +++ b/arch/arm/mach-omap2/gpio.c
> @@ -0,0 +1,101 @@
> +/*
> + * gpio.c - OMAP2PLUS-specific gpio code
> + *
> + * Copyright (C) 2010 Texas Instruments, Inc.
> + *
> + * Author:
> + *   Charulatha V 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +static int gpio_bank_count;
> +
> +struct omap_device_pm_latency omap_gpio_latency[] = {
> + [0] = {
> + .deactivate_func = omap_device_idle_hwmods,
> + .activate_func   = omap_device_enable_hwmods,
> + .flags   = OMAP_DEVICE_LATENCY_AUTO_ADJUST,
> + },
> +};
> +
> +int __init gpio_init(void)
> +{
> + int i = 0;
> + static int is_early_device = true;
> +
> + do {
> + struct omap_device *od;
> + struct omap_hwmod *oh;
> + int hw_mod_name_len = 16;
> + int l;
> + char oh_name[hw_mod_name_len];
> + struct omap_gpio_platform_data *pdata;
> + char *name = "omap-gpio";
> +
> + l = snprintf(oh_name, hw_mod_name_len, "gpio%d_hwmod", i + 1);
> + WARN(l >= hw_mod_name_len,
> + "String buffer overflow in GPIO device setup\n");
> +

Rather than iterating over the name, you should have a hwmod_class for
GPIOs and use omap_hwmod_for_each_by_class() to iterate over all the
hwmods in the GPIO class.

> + oh = omap_hwmod_lookup(oh_name);
> + if (!oh) {
> + pr_err("Could not look up %s\n", oh_name);
> + i++;
> + continue;
> + }
> +
> + pdata = kzalloc(sizeof(struct omap_gpio_platform_data),
> + GFP_KERNEL);
> + if (!pdata) {
> + pr_err("Memory allocation failed gpio%d\n", i + 1);
> + return -ENOMEM;
> + }
> + pdata->base = oh->_rt_va;
> + pdata->irq = oh->mpu_irqs[0].irq;

base address and IRQ should not be part of platform_data.   platform
driver should be using platform_get_resource() for both.

> + if (cpu_is_omap24xx() || cpu_is_omap34xx())
> + pdata->method = METHOD_GPIO_24XX;
> + if (cpu_is_omap44xx())
> + pdata->method = METHOD_GPIO_44XX;
> + pdata->virtual_irq_start = IH_GPIO_BASE + 32 * i;
> + pdata->device_enable = omap_device_enable;
> + pdata->device_idle = omap_device_idle;
> + pdata->device_shutdown = omap_device_shutdown;

These aren't valid for GPIO1 which is in WKUP.  Maybe we need
to check if the hwmod is not in wkup_pwrdm before setting these?

> + od = omap_device_build(name, i, oh, pdata,
> + sizeof(*pdata), omap_gpio_latency,
> + ARRAY_SIZE(omap_gpio_latency),
> + is_early_device);
> + WARN(IS_ERR(od), "Cant build omap_device for %s:%s.\n",
> + name, oh->name);
> +
> + i++;
> + } while (i < gpio_bank_count);
> + is_early_device = false;
> +
> + return 0;
> +}
> +arch_initcall(gpio_init);
> +
> +int __init omap2_gpio_init(void)
> +{
> + 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;

The count should be gathered after iterating over the GPIO hwmod class.
and should not need cpu_is checks.

> + if (gpio_init())
> + return -EINVAL;
> +
> + early_platform_driver_register_all("earlygpio");
> + early_platform_driver_probe("earlygpio", gpio_bank_count, 0);
> + return 0;
> +}
> -- 
> 1.6.3.3

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