> -----Original Message----- > From: Cousson, Benoit > Sent: Thursday, May 26, 2011 3:41 PM > To: Premi, Sanjeev > Cc: DebBarma, Tarun Kanti; linux-omap@xxxxxxxxxxxxxxx; > Hilman, Kevin; Shilimkar, Santosh; tony@xxxxxxxxxxx; > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; 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: linux-omap-owner@xxxxxxxxxxxxxxx > >> [mailto:linux-omap-owner@xxxxxxxxxxxxxxx] On Behalf Of > >> DebBarma, Tarun Kanti > >> Sent: Tuesday, May 24, 2011 7:55 PM > >> > >> From: Charulatha V<charu@xxxxxx> > >> > >> 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<charu@xxxxxx> > >> Cc: Cousson, Benoit<b-cousson@xxxxxx> > >> Cc: Paul Walmsley<paul@xxxxxxxxx> > >> --- > >> 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
