> -----Original Message-----
> From: Cousson, Benoit
> Sent: Thursday, May 26, 2011 3:41 PM
> To: Premi, Sanjeev
> Cc: DebBarma, Tarun Kanti; [email protected];
> Hilman, Kevin; Shilimkar, Santosh; [email protected];
> [email protected]; Varadarajan,
> Charulatha; Paul Walmsley
> Subject: Re: [PATCH 02/15] OMAP2PLUS: GPIO: Fix non-wakeup
> GPIO and rev_ids
>
> On 5/26/2011 11:23 AM, Premi, Sanjeev wrote:
> >> From: [email protected]
> >> [mailto:[email protected]] On Behalf Of
> >> DebBarma, Tarun Kanti
> >> Sent: Tuesday, May 24, 2011 7:55 PM
> >>
> >> From: Charulatha V<[email protected]>
> >>
> >> Non-wakeup GPIOs are available only in OMAP2420 and OMAP3430. But
> >> the GPIO driver initializes the non-wakeup GPIO bits for OMAP24xx
> >> (bothe OMAP 2420 and 2430)& not for OMAP3 which is incorrect.
> >>
> >> Fix the above by providing non-wakeup GPIO information
> through pdata
> >> specific to the SoC.
> >>
> >> The GPIO rev id provided in the hwmod database is the same
> >> for OMAP2420
> >> and OMAP2430. Change the GPIO rev ids in hwmod database as
> given below
> >> so that it can be used to identify OMAP2420 and OMAP2430.
> >> OMAP2420 - 0
> >> OMAP2430 - 1
> >> OMAP3 - 2
> >> OMAP4 - 3
> >
> > [sp] Magic numbers should be avoided.
> > Suggest using something like:
> > #define GPIO_REV_2420 0
> > #define GPIO_REV_2430 1
> > #define GPIO_REV_34XX 2
> > #define GPIO_REV_44xx 3
>
> Well, it is not a magic number, it is a revision id that is
> incremented
> each time there is a difference in the IP.
> The OMAP version -> IP version link is done at hwmod level.
> The driver
> does not have to know it is an OMAP3 or OMAP4. In that case we did
> change the IP version for every revisions, but OMAP5 will use
> the OMAP4
> revision.
> I'm not even considering all the Davinci variants that are not named
> OMAP but do use OMAP IPs... as you already know...
> That can provide a rather confusing information for my point of view.
>
> Whereas a comment like that will give you the exhaustive information.
>
> 0: OMAP2420
> 1: OMAP2430
> 2: OMAP3, DMxxx
> 3: OMAP4, OMAP5, DM816x
>
> That being said, some drivers already did that, so I'm not opposed to
> such change. I just think it is not the best approach.
> At least it gives a pointer to the TRM that contains the IP doc.
[sp] Benoit, your are right from generation perspectove where the
it is easier to increment numbers but when we use comparison
code - as in this patch, comparison against a number isn't
readable.
As example:
+ switch (oh->class->rev) { ## This is auto-generated.
+ case 0: ## But this is our code.
I am recommending this to read as:
+ switch (oh->class->rev) { ## This is auto-generated.
+ case GPIO_REV_2420: ## More readable.
~sanjeev
>
> > We don't have to refer back to this comment while
> reading the code.
> > I also believed that HWMODs were auto generated.
> > Can the changes to structures in this patch recreated
> using current
> > scripts?
>
> Only OMAP4 & 5 are generated today, and in anycase this
> information is
> some custom data added by driver owner. So we just have to
> update that
> information.
> I cannot really use the real HW IP version because it is
> irrelevant most
> of the time. Maybe in the future we will be able to force the
> HW team to
> provide relevant data in that field :-)
>
> Regards,
> Benoit
>
>
> >
> > ~sanjeev
> >
> >>
> >> Signed-off-by: Charulatha V<[email protected]>
> >> Cc: Cousson, Benoit<[email protected]>
> >> Cc: Paul Walmsley<[email protected]>
> >> ---
> >> arch/arm/mach-omap2/gpio.c | 26
> >> ++++++++++++++++++++++++--
> >> arch/arm/mach-omap2/omap_hwmod_2430_data.c | 2 +-
> >> arch/arm/mach-omap2/omap_hwmod_3xxx_data.c | 2 +-
> >> arch/arm/mach-omap2/omap_hwmod_44xx_data.c | 2 +-
> >> arch/arm/plat-omap/include/plat/gpio.h | 1 +
> >> drivers/gpio/gpio_omap.c | 11 +++--------
> >> 6 files changed, 31 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/arch/arm/mach-omap2/gpio.c
> b/arch/arm/mach-omap2/gpio.c
> >> index 0446bd1..6cd26b4 100644
> >> --- a/arch/arm/mach-omap2/gpio.c
> >> +++ b/arch/arm/mach-omap2/gpio.c
> >> @@ -56,6 +56,28 @@ static int omap2_gpio_dev_init(struct
> >> omap_hwmod *oh, void *unused)
> >> return -ENOMEM;
> >> }
> >>
> >> + switch (oh->class->rev) {
> >> + case 0:
> >> + if (id == 1)
> >> + /* non-wakeup GPIO pins for OMAP2420 Bank1 */
> >> + pdata->non_wakeup_gpios = 0xe203ffc0;
> >> + else if (id == 2)
> >> + /* non-wakeup GPIO pins for OMAP2420 Bank2 */
> >> + pdata->non_wakeup_gpios = 0x08700040;
> >> + break;
> >> + case 2:
> >> + if (id == 2)
> >> + /* non-wakeup GPIO pins for OMAP3 Bank2 */
> >> + pdata->non_wakeup_gpios = 0x00000001;
> >> + else if (id == 6)
> >> + /* non-wakeup GPIO pins for OMAP3 Bank6 */
> >> + pdata->non_wakeup_gpios = 0x08000000;
> >> + break;
> >
> > [sp] Where is the description on non-wakeup GPIOs in OMAP3?
> > Here is text from AM37x TRM:
> > [quote ...only relevant text]
> > Each GPIO module provides 32 dedicated
> general-purpose pins with input
> > and output capabilities; .... These pins can be
> configured for the
> > following applications:
> > - Data input (capture)/output (drive)
> > - Keyboard interface with a debounce cell
> > - Interrupt generation in ....
> > - Wake-up request generation in idle mode
> > [/quote]
> > Otherwise, what are the GPIO2_WAKEUPENABLE (0x4905 0020) and
> > GPIO6_WAKEUPENABLE (0x4905 8020) meant for?
> >
> >> + default:
> >> + /* No non-wakeup GPIO pins for other SoCs */
> >> + break;
> >> + }
> >> +
> >
> > ~sanjeev
> >
> > [snip]...[snip]
>
> --
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
