"Nori, Sekhar" <[email protected]> writes: > On Wed, Oct 07, 2009 at 23:05:48, Mark A. Greer wrote: >> On Mon, Oct 05, 2009 at 11:12:17AM +0530, Nori, Sekhar wrote: >> > On Fri, Oct 02, 2009 at 02:24:17, Mark A. Greer wrote: >> >> > > I don't know Sekhar... This looks like a step backwards to me with all >> > > the #ifdef-ery. Why not keep the code as it is and change the #ifdefs >> > > that are already there (e.g., change the CONFIG_UI ones to CONFIG_FB, >> > > etc)? >> > >> > Mark, >> > >> > The only new #ifdef the patch introduces is for MMC/SD. That is >> > to make sure plugging in UI card does not get MMC to stop working. >> >> You have taken #ifdef's that included/excluded code and replaced them with >> #ifdef's that are a part of the runtime logic. That's typically frowned >> upon. That's my issue. > > Hmm. Honestly I did not know this was frowned upon. Why is it > frowned upon? Is it because the new code may not have been as > well tested or is it because the runtime path is now slightly > longer? > >> >> > I think the patch actually improves readability by removing the >> > existing #ifdefs present inside function body. >> >> I disagree plus the existing ifdef's do what #ifdef's are supposed to do, >> include or exclude chunks of code. Now, in addition to indirectly >> making them a part of the runtime logic, you added another level of >> indirection so people have to figure out exactly how 'HAS_LCD', etc. >> get defined (not difficult but another level just the same). > > I am not still convinced this is step in wrong direction. I will > wait for Kevin's thoughts on this before reposting a revision.
I tend to agree with Mark. >> >> > The HAS_XXX constructs are not new and have been used in other >> > DaVinci code. Have a look at support for DM644x EVM here: >> > >> > http://lxr.linux.no/linux+v2.6.31/arch/arm/mach-davinci/board-dm644x-evm.c#L609 >> >> Past mistakes are not valid justifications for future ones. :) > > Agreed, but consistency helps too. So, past mistakes need to be > corrected before newer mistakes can be made :) I must admit to having a strong dislike of the HAS_XXX #ifdeffery, so it should necessarly be used as the model to build on. If we didn't have mux conflicts, I'm sure we would all prefer to see this all done with runtime detection. To me all of this boils down to how to deal with mutually exclusive drivers. The existing approach was to let the Kconfig options under the main UI board one decide. The current one lets the driver options themselves decide. In the case of the user trying to figure out which things are conflicting and which to disable, having the options as a Kconfig 'choice' under the UI board seems much more straightforward to me. So, here's my proposal, which basically keeps the existing Kconfig options but adds the runtime detection of UI board as done in $SUBJECT patch. In Kconfig, have a main 'UI board' heading as we do now, but it should not be selectable since this can be done at runtime. Under there all the options should be listed and the mutually-exclusive ones handled using 'choice'. This way, the default config can always enable the NOR, NAND, MMC, fb drivers themselves and the mutual-exclusion is handled at the UI board level. Kevin _______________________________________________ Davinci-linux-open-source mailing list [email protected] http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
