+ Felipe
On 3/23/2012 11:20 AM, Mohammed, Afzal wrote:
Hi Benoit,
On Fri, Mar 23, 2012 at 15:07:30, Cousson, Benoit wrote:
Final destination aimed for this driver is MFD.
Why? Are you sure this is appropriate? This is not really a
multifunction device but rather a bus device that can manage multiple
kind of devices.
Agree, this not an MFD, but can we call this a bus?, as there is
nothing like GPMC protocol. We considered it logically as MFD&
proceeded and there was a similar attempt for davinci EMIF [1,2].
But EMIF does not have anything to do in MFD either :-)
What was the feedback for this series?
We discussed that at Linaro connect, but it looks like MFD is becoming
the place for miscellaneous drivers that we do not know where to put.
Maybe we should introduce a driver/memory/ directory for memory
controller. At least for EMIF.
In the case of GMPC, it is slightly different because it can handle
NOR/NAND memory but as well behave like an ISA bus controller for
Ethernet ISA chip.
But since it can control several devices thanks the chipselects lines it
has, it behaves like a multi-protocol bus controller.
But in anycase, it does not look like an MFD for my point of view. For
me a MFD is like a small soc, it does contain several completely
unrelated block (Power, Audio, GPIO...), but does share some memory
space / IRQ lines.
Is this the only controller doing that kind of stuff in the kernel so far?
arch/arm/mach-omap2/gpmc.c | 1083 +++++++++++++++++++-------------
You should probably find the proper location first, move the code and
convert to driver.
I will let Tony comment but this is the strategy today for all this
pseudo driver that should not be in OMAP arch directory anymore.
Please let me know whether you have any suggestions on where GPMC driver
should live.
+ printk(KERN_DEBUG "GPMC CS%d: %-10s* %3d ns, %3d ticks>= %d\n",
Nit, but since you are cleaning extensively this code, you should use
pr_ macros instead or even dev_ macros since you do have a real driver
now with real devices now.
Sure, this was overlooked
+struct gpmc_cs_config {
+ u32 config1;
+ u32 config2;
+ u32 config3;
+ u32 config4;
+ u32 config5;
+ u32 config6;
+ u32 config7;
+ int is_valid;
+};
OK, so this code was just moved and not removed. Becasue of these big
code move, the patch is not super readable. We cannot really see what
part is new and what was changed.
Maybe you should try to split that in sevarl patches or minized the move.
Yes, I was really in two minds before the coding started. Lot of code in
this patch has been moved from one place to other, this was done to put
codes that handle similar things together, so that trees can be made
visible easily in the forest. And once the patch is applied, as similar
sections are together, it may be easy to make further changes
If an initial patch just to rearrange the code to have similar section
together& then new changes in a another patch, would that be fine?
Well, if this is just comestic, I will even do that after the driver
conversion. Because if you do that before you will move some piece of
code that you might completely delete later. So you should fix the code
first and then potentially, move some part if that will improve the
readability.
+static int __init gpmc_clk_init(void)
+{
+ char *ck = NULL;
+
+ if (cpu_is_omap24xx())
+ ck = "core_l3_ck";
+ else if (cpu_is_omap34xx())
+ ck = "gpmc_fck";
+ else if (cpu_is_omap44xx())
+ ck = "gpmc_ck";
Please don't do that anymore. The CLKDEV array is done to create alias
and avoid this kind of hacks. Moreover you should rely on hwmod for
device creation and thus main clock alias will already be populated for
free.
There are not added, they are existing code, result of rearranging the
code. These sections were given not given much importance as these won't
go into driver. But noted the point you are making.
The issue is that the cpu_is_XXX should not be accessed from outside
mach-omap2 directory, so you should get rid of that before trying to
move the gpmc in the XXX location.
Regards,
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
