On Thu, Oct 08, 2009 at 10:37:05AM +0530, Nori, Sekhar wrote: > 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?
#ifdef's in general are frowned upon, see Documentation/SubmittingPatches section 2.2. If you have to add them, you're only supposed to add chunks of code. You can look at any of the code in places like kernel/*.c. None of them use an #if to determine runtime paths (or shouldn't if you find a place the does)--they only add/remove pieces of code. > Is it because the new code may not have been as > well tested or is it because the runtime path is now slightly > longer? Files can quickly become unreadable when various people start adding #ifdef's. The file mentioned above talks about it a bit. > > > 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. Okay but this isn't coming from me. This is standard community doctrine. Mark -- _______________________________________________ Davinci-linux-open-source mailing list [email protected] http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
