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