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. > > > 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 :) Thanks, Sekhar _______________________________________________ Davinci-linux-open-source mailing list [email protected] http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
