Re: [PATCH 2/4] ARM: mxc: migrate mach-mx5 gpio driver to gpio-mxc

[Olof Johansson]

[was offline last night, apologies for the latency]

On Tue, May 31, 2011 at 9:08 AM, Shawn Guo <shawn.guo@xxxxxxxxxxxxx> wrote:
> On Tue, May 31, 2011 at 05:27:55PM +0200, Arnd Bergmann wrote:
>> On Tuesday 31 May 2011, Shawn Guo wrote:
>> >
>> > On Tue, May 31, 2011 at 01:28:17PM +0200, Arnd Bergmann wrote:
>> > > On Tuesday 31 May 2011, Shawn Guo wrote:
>> > > > > Just open-code the mxc_add_mxc_gpio() by moving the individual calls to
>> > > > > mxc_add_gpio() into the respective callers. Having a global
>> > > > > mxc_add_mxc_gpio() function that does something different for each
>> > > > > caller seems entirely pointless to me.
>> > > > >
>> > > > Right now, mxc_add_mxc_gpio() is a postcore_initcall.  Moving
>> > > > individual mxc_add_gpio() call into irq_init function does not work.
>> > > > And I need to find a proper caller for each SoC to call mxc_add_gpio
>> > > > to register gpio devices.
>> > >
>> > > Why not init_machine? That is an arch_initcall(), so it's probably close
>> > > enough.
>> > >
>> > The init_machine is mostly a board specific function than SoC specific
>> > one.  That is to say we will call mxc_add_gpio() in every single board
>> > init function even for the same SoC.
>>
>> But the machine is the only place that knows what SOC is being used.
>> Your patch right now detects it by looking at the CPU type that is
>> also being set by the board file using the init_early call, which
>> is a bit silly.
>>
> I would say that the CPU type is being set by SoC file (e.g.
> mach-mx5/mm.c) than board file.  All the things in mach-mx5/mm.c are
> common to any mx51 based boards.

The thing is that all initialization today starts at the board file.
Instead of setting a variable that determines code paths in
asyncronous initcalls later, you might as well call the appropriate
initcall functions from common functions called by board files.

So, you'd just add a call to a common per-soc init in each of the
per-board machine_init functions. Over time it'll be a good place to
add code that would otherwise be duplicated per board of the same soc
family.

>> I would leave e.g. the imx51_register_gpios() in place, but only
>> change the definition and the caller to
>>
>> void __init imx51_register_gpios(void)
>> {
>>         mxc_add_gpio(0, MX51_GPIO1_BASE_ADDR, SZ_16K, MX51_MXC_INT_GPIO1_LOW, MX51_MXC_INT_GPIO1_HIGH);
>>         mxc_add_gpio(1, MX51_GPIO2_BASE_ADDR, SZ_16K, MX51_MXC_INT_GPIO2_LOW, MX51_MXC_INT_GPIO2_HIGH);
>>         mxc_add_gpio(2, MX51_GPIO3_BASE_ADDR, SZ_16K, MX51_MXC_INT_GPIO3_LOW, MX51_MXC_INT_GPIO3_HIGH);
>>         mxc_add_gpio(3, MX51_GPIO4_BASE_ADDR, SZ_16K, MX51_MXC_INT_GPIO4_LOW, MX51_MXC_INT_GPIO4_HIGH);
>> }
>>
>> Then you can call that function from each i.mx51 based board, or
>> from a new imx51_soc_init() function that groups multiple such
>> functions.
>>
> It will anyway touch every single board file (a lot) to have this SoC
> specific device registration plugged in.  If you still think it's
> worthy to do so, I can turn around.

I personally prefer the imx51_soc_init() Arnd proposed above, since
there's a chance you will want to add more things to it over time. It
is worth adding it now, it's just a one-line change per board and it
gives you a good place to hook in other per-family setup that might be
needed later.


-Olof
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/