Re: [PATCH 10/13 v3] OMAP: GPIO: Add gpio dev_attr and correct clks in OMAP4 hwmod struct

["Cousson, Benoit" <b-cousson@ti.com>]

On 6/16/2010 8:54 AM, Varadarajan, Charulatha wrote:
>
>> From: Cousson, Benoit
>> Sent: Tuesday, June 15, 2010 10:10 PM
>>
>> Hi Charu,
>>
>> On 6/15/2010 5:05 PM, Varadarajan, Charulatha wrote:
>>> From: Charulatha V<charu@ti.com>
>>>
>>> This patch adds gpio_dev_attr to OMAP4 gpio hwmod structure. This patch
>>> also corrects the gpio .main_clk and .clk fields in gpio hwmod structures.
>>>
>>> Signed-off-by: Charulatha V<charu@ti.com>
>>> ---
>>>    arch/arm/mach-omap2/omap_hwmod_44xx_data.c |   37 +++++++++++++++++++---------
>>>    1 files changed, 25 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c b/arch/arm/mach-
>> omap2/omap_hwmod_44xx_data.c
>>> index 20f5f8c..b4c0878 100644
>>> --- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
>>> +++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
>>> @@ -22,6 +22,7 @@
>>>
>>>    #include<plat/omap_hwmod.h>
>>>    #include<plat/cpu.h>
>>> +#include<plat/gpio.h>
>>>
>>>    #include "omap_hwmod_common_data.h"
>>>
>>> @@ -1272,6 +1273,12 @@ static struct omap_hwmod omap44xx_fdif_hwmod = {
>>>     * general purpose io module
>>>     */
>>>
>>> +static struct omap_gpio_dev_attr gpio_dev_attr = {
>>> +	.gpio_bank_count = 6,
>>
>> Why do you need that information?
>> The point is that this struct is in theory a per instance data not a
>> global one. If needed, you should be able to get that from the number of
>> iteration done during the init of hwmods.
>
> Even though it is possible to get it through number of iterations, more efficient approach would be to keep this information here. My point is that this information can be auto-generated and hence it is better to keep it in hwmod database itself.

It is still a duplication of an information we already have indirectly. 
The number of GPIO bank is exactly the number of hwmod gpio instances. 
You do not need any other parameters to get that.

> FYI, in a meeting we recently had with Kevin, we discussed about "global data" information getting duplicated for each instance and it was agreed that it is okay to have pointers duplicated for each instance.

I'm not OK with that. If we start mixing the pure IP specific 
information with the global one it will be a mess, and it will prevent 
the easy reuse accross SoC familly.
hwmod structure is an IP information. The number of GPIO is an 
integration information that has nothing to do inside the hwmod. If we 
want to add an extra instance of GPIO in OMAP4440 for example, we will 
not be able to reuse the current 4430 data.

So please, don't do that.

BTW, you didn't answer the first answer, do you really need that?

>>
>>> +	.gpio_bank_bits = 32,
>>> +	.dbck_flag = true,
>>> +};
>>
>> Since this structure is the same than OMAP3, you should maybe consider
>> sharing it.
>
> They are in different _data.c files. I feel that it will be good if they are kept decoupled as we need not have to mix up different SoC data. Here it's only a coincidence that OMAP3 and OMAP4 have same data

No it is not a coincidence, it is because we are re-using the same IP 
implementation. In that case, it is not a big deal, but you should try 
to reuse as most as you can already existing data.

Benoit

>
>>
>>> +
>>>    static struct omap_hwmod_class_sysconfig omap44xx_gpio_sysc = {
>>>    	.rev_offs	= 0x0000,
>>>    	.sysc_offs	= 0x0010,
>>> @@ -1305,7 +1312,7 @@ static struct omap_hwmod_addr_space omap44xx_gpio1_addrs[]
>> = {
>>>    static struct omap_hwmod_ocp_if omap44xx_l4_wkup__gpio1 = {
>>>    	.master		=&omap44xx_l4_wkup_hwmod,
>>>    	.slave		=&omap44xx_gpio1_hwmod,
>>> -	.clk		= "l4_wkup_clk_mux_ck",
>>> +	.clk		= "gpio1_ick",
>>>    	.addr		= omap44xx_gpio1_addrs,
>>>    	.addr_cnt	= ARRAY_SIZE(omap44xx_gpio1_addrs),
>>>    	.user		= OCP_USER_MPU | OCP_USER_SDMA,
>>> @@ -1325,7 +1332,7 @@ static struct omap_hwmod omap44xx_gpio1_hwmod = {
>>>    	.class		=&omap44xx_gpio_hwmod_class,
>>>    	.mpu_irqs	= omap44xx_gpio1_irqs,
>>>    	.mpu_irqs_cnt	= ARRAY_SIZE(omap44xx_gpio1_irqs),
>>> -	.main_clk	= "gpio1_ick",
>>> +	.main_clk	= NULL,
>>
>> Removing the line is enough. It will be null by default.
>
> Agreed.
>
>> I'm still not 100% sure that this is the good way to control the GPIO
>> module mode, but at least this is cleaner than the previous clock mapping.
>>
>> Benoit
>