From: "Premi, Sanjeev" <premi@ti.com> To: "Cousson, Benoit" <b-cousson@ti.com> Cc: "DebBarma, Tarun Kanti" <tarun.kanti@ti.com>, "linux-omap@vger.kernel.org" <linux-omap@vger.kernel.org>, "Hilman, Kevin" <khilman@ti.com>, "Shilimkar, Santosh" <santosh.shilimkar@ti.com>, "tony@atomide.com" <tony@atomide.com>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, "Varadarajan, Charulatha" <charu@ti.com>, Paul Walmsley <paul@pwsan.com> Subject: RE: [PATCH 02/15] OMAP2PLUS: GPIO: Fix non-wakeup GPIO and rev_ids Date: Thu, 26 May 2011 17:17:04 +0530 [thread overview] Message-ID: <B85A65D85D7EB246BE421B3FB0FBB593024D178110@dbde02.ent.ti.com> (raw) In-Reply-To: <4DDE2745.3030307@ti.com> > -----Original Message----- > From: Cousson, Benoit > Sent: Thursday, May 26, 2011 3:41 PM > To: Premi, Sanjeev > Cc: DebBarma, Tarun Kanti; linux-omap@vger.kernel.org; > Hilman, Kevin; Shilimkar, Santosh; tony@atomide.com; > linux-arm-kernel@lists.infradead.org; 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@vger.kernel.org > >> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of > >> DebBarma, Tarun Kanti > >> Sent: Tuesday, May 24, 2011 7:55 PM > >> > >> From: Charulatha V<charu@ti.com> > >> > >> 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@ti.com> > >> Cc: Cousson, Benoit<b-cousson@ti.com> > >> Cc: Paul Walmsley<paul@pwsan.com> > >> --- > >> 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] > >
WARNING: multiple messages have this Message-ID (diff)
From: premi@ti.com (Premi, Sanjeev) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 02/15] OMAP2PLUS: GPIO: Fix non-wakeup GPIO and rev_ids Date: Thu, 26 May 2011 17:17:04 +0530 [thread overview] Message-ID: <B85A65D85D7EB246BE421B3FB0FBB593024D178110@dbde02.ent.ti.com> (raw) In-Reply-To: <4DDE2745.3030307@ti.com> > -----Original Message----- > From: Cousson, Benoit > Sent: Thursday, May 26, 2011 3:41 PM > To: Premi, Sanjeev > Cc: DebBarma, Tarun Kanti; linux-omap at vger.kernel.org; > Hilman, Kevin; Shilimkar, Santosh; tony at atomide.com; > linux-arm-kernel at lists.infradead.org; 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 at vger.kernel.org > >> [mailto:linux-omap-owner at vger.kernel.org] On Behalf Of > >> DebBarma, Tarun Kanti > >> Sent: Tuesday, May 24, 2011 7:55 PM > >> > >> From: Charulatha V<charu@ti.com> > >> > >> 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@ti.com> > >> Cc: Cousson, Benoit<b-cousson@ti.com> > >> Cc: Paul Walmsley<paul@pwsan.com> > >> --- > >> 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] > >
next prev parent reply other threads:[~2011-05-26 11:47 UTC|newest] Thread overview: 118+ messages / expand[flat|nested] mbox.gz Atom feed top 2011-05-24 14:24 [PATCH 00/15] OMAP: GPIO: Cleanup OMAP GPIO driver Tarun Kanti DebBarma 2011-05-24 14:24 ` Tarun Kanti DebBarma 2011-05-24 14:24 ` [PATCH 01/15] OMAP: GPIO: Avoid cpu_is checks during module ena/disable Tarun Kanti DebBarma 2011-05-24 14:24 ` Tarun Kanti DebBarma 2011-05-25 21:19 ` Kevin Hilman 2011-05-25 21:19 ` Kevin Hilman 2011-05-26 9:38 ` Varadarajan, Charulatha 2011-05-26 9:38 ` Varadarajan, Charulatha 2011-05-24 14:24 ` [PATCH 02/15] OMAP2PLUS: GPIO: Fix non-wakeup GPIO and rev_ids Tarun Kanti DebBarma 2011-05-24 14:24 ` Tarun Kanti DebBarma 2011-05-25 21:34 ` Kevin Hilman 2011-05-25 21:34 ` Kevin Hilman 2011-05-26 9:38 ` Varadarajan, Charulatha 2011-05-26 9:38 ` Varadarajan, Charulatha 2011-05-26 17:15 ` Kevin Hilman 2011-05-26 17:15 ` Kevin Hilman 2011-05-26 17:39 ` Varadarajan, Charulatha 2011-05-26 17:39 ` Varadarajan, Charulatha 2011-05-26 18:32 ` Kevin Hilman 2011-05-26 18:32 ` Kevin Hilman 2011-05-26 9:23 ` Premi, Sanjeev 2011-05-26 9:23 ` Premi, Sanjeev 2011-05-26 9:43 ` Varadarajan, Charulatha 2011-05-26 9:43 ` Varadarajan, Charulatha 2011-05-26 10:11 ` Cousson, Benoit 2011-05-26 10:11 ` Cousson, Benoit 2011-05-26 11:47 ` Premi, Sanjeev [this message] 2011-05-26 11:47 ` Premi, Sanjeev 2011-05-26 12:11 ` Cousson, Benoit 2011-05-26 12:11 ` Cousson, Benoit 2011-05-26 12:38 ` Premi, Sanjeev 2011-05-26 12:38 ` Premi, Sanjeev 2011-05-26 12:46 ` Cousson, Benoit 2011-05-26 12:46 ` Cousson, Benoit 2011-05-26 13:19 ` Premi, Sanjeev 2011-05-26 13:19 ` Premi, Sanjeev 2011-05-26 13:38 ` B.J. Buchalter 2011-05-26 13:38 ` B.J. Buchalter 2011-05-26 14:12 ` Cousson, Benoit 2011-05-26 14:12 ` Cousson, Benoit 2011-05-24 14:24 ` [PATCH 03/15] OMAP: GPIO: Remove dependency on gpio_bank_count Tarun Kanti DebBarma 2011-05-24 14:24 ` Tarun Kanti DebBarma 2011-05-24 14:24 ` [PATCH 04/15] OMAP2PLUS: GPIO: Use flag to identify wkup dmn GPIO Tarun Kanti DebBarma 2011-05-24 14:24 ` Tarun Kanti DebBarma 2011-05-25 21:40 ` Kevin Hilman 2011-05-25 21:40 ` Kevin Hilman 2011-05-24 14:24 ` [PATCH 05/15] OMAP: GPIO: Make gpio_context part of gpio_bank structure Tarun Kanti DebBarma 2011-05-24 14:24 ` Tarun Kanti DebBarma 2011-05-25 21:41 ` Kevin Hilman 2011-05-25 21:41 ` Kevin Hilman 2011-05-26 9:58 ` Premi, Sanjeev 2011-05-26 9:58 ` Premi, Sanjeev 2011-05-26 10:07 ` Varadarajan, Charulatha 2011-05-26 10:07 ` Varadarajan, Charulatha 2011-05-26 9:59 ` Premi, Sanjeev 2011-05-26 9:59 ` Premi, Sanjeev 2011-05-24 14:24 ` [PATCH 06/15] OMAP4: GPIO: Save/restore context Tarun Kanti DebBarma 2011-05-24 14:24 ` Tarun Kanti DebBarma 2011-05-25 21:43 ` Kevin Hilman 2011-05-25 21:43 ` Kevin Hilman 2011-05-26 9:37 ` Varadarajan, Charulatha 2011-05-26 9:37 ` Varadarajan, Charulatha 2011-05-24 14:24 ` [PATCH 07/15] OMAP: GPIO: handle save/restore ctx in GPIO driver Tarun Kanti DebBarma 2011-05-24 14:24 ` Tarun Kanti DebBarma 2011-05-25 22:33 ` Kevin Hilman 2011-05-25 22:33 ` Kevin Hilman 2011-05-25 22:36 ` Kevin Hilman 2011-05-25 22:36 ` Kevin Hilman 2011-05-24 14:24 ` [PATCH 08/15] OMAP2+: GPIO: make workaround_enabled bank specific Tarun Kanti DebBarma 2011-05-24 14:24 ` Tarun Kanti DebBarma 2011-05-25 22:39 ` Kevin Hilman 2011-05-25 22:39 ` Kevin Hilman 2011-05-26 9:37 ` Varadarajan, Charulatha 2011-05-26 9:37 ` Varadarajan, Charulatha 2011-05-24 14:24 ` [PATCH 09/15] OMAP: GPIO: cleanup suspend and resume functions Tarun Kanti DebBarma 2011-05-24 14:24 ` Tarun Kanti DebBarma 2011-05-25 22:57 ` Kevin Hilman 2011-05-25 22:57 ` Kevin Hilman 2011-05-26 10:02 ` Varadarajan, Charulatha 2011-05-26 10:02 ` Varadarajan, Charulatha 2011-05-24 14:24 ` [PATCH 10/15] OMAP: GPIO: cleanup prepare/resume idle functions Tarun Kanti DebBarma 2011-05-24 14:24 ` Tarun Kanti DebBarma 2011-05-25 23:00 ` Kevin Hilman 2011-05-25 23:00 ` Kevin Hilman 2011-05-24 14:24 ` [PATCH 11/15] OMAP: GPIO: Remove hardcoded offsets in ctxt save/restore Tarun Kanti DebBarma 2011-05-24 14:24 ` Tarun Kanti DebBarma 2011-05-25 23:01 ` Kevin Hilman 2011-05-25 23:01 ` Kevin Hilman 2011-05-26 9:36 ` Varadarajan, Charulatha 2011-05-26 9:36 ` Varadarajan, Charulatha 2011-05-26 9:42 ` Premi, Sanjeev 2011-05-26 9:42 ` Premi, Sanjeev 2011-05-26 9:48 ` Varadarajan, Charulatha 2011-05-26 9:48 ` Varadarajan, Charulatha 2011-05-24 14:24 ` [PATCH 12/15] OMAP: GPIO: Fix: use wake set/clear regs Tarun Kanti DebBarma 2011-05-24 14:24 ` Tarun Kanti DebBarma 2011-05-25 23:14 ` Kevin Hilman 2011-05-25 23:14 ` Kevin Hilman 2011-05-26 9:36 ` Varadarajan, Charulatha 2011-05-26 9:36 ` Varadarajan, Charulatha 2011-05-24 14:24 ` [PATCH 13/15] OMAP: GPIO: clean set_gpio_triggering function Tarun Kanti DebBarma 2011-05-24 14:24 ` Tarun Kanti DebBarma 2011-05-25 23:27 ` Kevin Hilman 2011-05-25 23:27 ` Kevin Hilman 2011-05-26 9:55 ` Varadarajan, Charulatha 2011-05-26 9:55 ` Varadarajan, Charulatha 2011-05-24 14:24 ` [PATCH 14/15] OMAP: GPIO: Use memset for omap_gpio_reg_offs Tarun Kanti DebBarma 2011-05-24 14:24 ` Tarun Kanti DebBarma 2011-05-25 23:30 ` Kevin Hilman 2011-05-25 23:30 ` Kevin Hilman 2011-05-24 14:24 ` [PATCH 15/15] OMAP: GPIO: clean omap_gpio_mod_init function Tarun Kanti DebBarma 2011-05-24 14:24 ` Tarun Kanti DebBarma 2011-05-25 23:48 ` Kevin Hilman 2011-05-25 23:48 ` Kevin Hilman 2011-06-03 11:20 ` Varadarajan, Charulatha 2011-06-03 11:20 ` Varadarajan, Charulatha 2011-06-03 14:31 ` Kevin Hilman 2011-06-03 14:31 ` Kevin Hilman
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=B85A65D85D7EB246BE421B3FB0FBB593024D178110@dbde02.ent.ti.com \ --to=premi@ti.com \ --cc=b-cousson@ti.com \ --cc=charu@ti.com \ --cc=khilman@ti.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-omap@vger.kernel.org \ --cc=paul@pwsan.com \ --cc=santosh.shilimkar@ti.com \ --cc=tarun.kanti@ti.com \ --cc=tony@atomide.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.