From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Cousson, Benoit" Subject: Re: [PATCH 02/15] OMAP2PLUS: GPIO: Fix non-wakeup GPIO and rev_ids Date: Thu, 26 May 2011 14:46:40 +0200 Message-ID: <4DDE4BB0.7010809@ti.com> References: <1306247094-25372-1-git-send-email-tarun.kanti@ti.com> <1306247094-25372-3-git-send-email-tarun.kanti@ti.com> <4DDE2745.3030307@ti.com> <4DDE435C.8060506@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from devils.ext.ti.com ([198.47.26.153]:36779 "EHLO devils.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757443Ab1EZMqs (ORCPT ); Thu, 26 May 2011 08:46:48 -0400 In-Reply-To: Sender: linux-omap-owner@vger.kernel.org List-Id: linux-omap@vger.kernel.org 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 On 5/26/2011 2:38 PM, Premi, Sanjeev wrote: >> -----Original Message----- >> From: Cousson, Benoit >> Sent: Thursday, May 26, 2011 5: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 1:47 PM, Premi, Sanjeev wrote: >>>> -----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 >>>>>> >>>>>> 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 >> >> This is not even related to generation. A version number is a number. >> If OMAP2 is using GPIO v1.6 and OMAP3 the version v2.2, only that IP >> version is relevant for the driver. >> For the documentation point of view, since we do not have a >> per IP TRM, >> then it can make sense to know where that IP is documented. But as I >> said a comment is enough and will allow a much more relevant level of >> details information than a define. >> >>> 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. >> >> More readable, maybe, but not necessarily relevant nor >> accurate if the >> same IP version is used in another chip or revision. >> >> What IP version is used in a DM816x? GPIO_REV_3430, GPIO_REV_4430? > > How is value 0 accurate? 0 is just the revision number that can give you the exact list of Soc that does use that IP. Thanks to an exhaustive comment. 0: OMAP2420 1: OMAP2430 2: OMAP3, DMxxx 3: OMAP4, OMAP5, DM816x BTW, I'm still waiting the answer for previous question... Regards, Benoit From mboxrd@z Thu Jan 1 00:00:00 1970 From: b-cousson@ti.com (Cousson, Benoit) Date: Thu, 26 May 2011 14:46:40 +0200 Subject: [PATCH 02/15] OMAP2PLUS: GPIO: Fix non-wakeup GPIO and rev_ids In-Reply-To: References: <1306247094-25372-1-git-send-email-tarun.kanti@ti.com> <1306247094-25372-3-git-send-email-tarun.kanti@ti.com> <4DDE2745.3030307@ti.com> <4DDE435C.8060506@ti.com> Message-ID: <4DDE4BB0.7010809@ti.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 5/26/2011 2:38 PM, Premi, Sanjeev wrote: >> -----Original Message----- >> From: Cousson, Benoit >> Sent: Thursday, May 26, 2011 5: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 1:47 PM, Premi, Sanjeev wrote: >>>> -----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 >>>>>> >>>>>> 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 >> >> This is not even related to generation. A version number is a number. >> If OMAP2 is using GPIO v1.6 and OMAP3 the version v2.2, only that IP >> version is relevant for the driver. >> For the documentation point of view, since we do not have a >> per IP TRM, >> then it can make sense to know where that IP is documented. But as I >> said a comment is enough and will allow a much more relevant level of >> details information than a define. >> >>> 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. >> >> More readable, maybe, but not necessarily relevant nor >> accurate if the >> same IP version is used in another chip or revision. >> >> What IP version is used in a DM816x? GPIO_REV_3430, GPIO_REV_4430? > > How is value 0 accurate? 0 is just the revision number that can give you the exact list of Soc that does use that IP. Thanks to an exhaustive comment. 0: OMAP2420 1: OMAP2430 2: OMAP3, DMxxx 3: OMAP4, OMAP5, DM816x BTW, I'm still waiting the answer for previous question... Regards, Benoit