>
> 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@xxxxxx>
> >>>
> >>> 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@xxxxxx>
> >>> ---
> >>> 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.
I guess, irq no and dma channel information are also integration information that are auto-generated in 4430 database. Shall we consider gpio_bank_count too?
>
> So please, don't do that.
>
> BTW, you didn't answer the first answer, do you really need that?
>
It is used in save/restore context which would be called
from sram_idle path.
Considering implementing using iterations count, there would be an
additional loop that would do this counting and the final value
would be passed as pdata->gpio_bank_count through each device data.
static int gpio_bank_count;
void omap_get_gpio_count (void) {
gpio_bank_count++;
}
int omap2_init_gpio (struct omap_hwmod *oh, void*user)
{
...
...
pdata->gpio_bank_count = gpio_bank_count;
omap_device_build(...);
...
...
}
void gpio_init ()
{
...
...
omap_hwmod_for_each_by_class("gpio", &omap_get_gpio_count(oh, NULL));
...
...
omap_hwmod_for_each_by_class("gpio", &omap2_init_gpio(oh, NULL));
}
Do we need to consider this?
> >>
> >>> + .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.
They are in two different omap_hwmod_xxxx_data.c files. Is there a way to share this 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
> >
--
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
