Hi Afzal, On 06/13/2012 12:50 AM, Mohammed, Afzal wrote: > Hi Jon, > > On Tue, Jun 12, 2012 at 23:39:32, Hunter, Jon wrote: >> On 06/12/2012 07:58 AM, Mohammed, Afzal wrote: > >>> Thinking again over it, I am feeling above is sufficient, reason same as >>> said earlier, to keep code simple & currently this is sufficient to >>> handle GPMC bit patterns for IPs presently available. What you are >>> suggesting is to take care of the imaginary case when new GPMC IP happens >>> with new device type or size, I think that should be handled when such a >>> scenario happens. Probably, it is better here to add a comment to make >>> intention clear. >> >> That is one possibility but I think that more important reasons are ... >> >> 1. Readability, at first it appears that we are always configuring the >> CS for NAND. However, this is not the case. Maybe a comment here can >> help as you mentioned. > > As far as the users of GPMC is considered there is no confusion. Eg. For > device type, they have been provided with two macros, > > GPMC_CONFIG1_DEVICETYPE_NOR, GPMC_CONFIG1_DEVICETYPE_NAND > > So for NOR, user can feel satisfied by giving NOR flag, little does he know > that driver doesn't do anything with the flag, but he still gets what he want > NOR flag is defined as zero. Yes even if he doesn't specify any type, device > type will be set as NOR, but then that is the default - the only other option Sure, but reviewing the function it still looks odd from a readability standpoint. At least it made me think "what is going on here ...". So a comment is definitely needed. >> >> 2. A bad setting in the configuration passed. Hopefully, people will >> stick to the flags but we know that we expect the device type to be a 0 >> or 2 and so should we check? > > Value of device type is something driver has to worry about, not its users, > they have been provided with two flags, one for NAND & other for NOR. Yes, but the driver does not seem to worry about it. In other words, there is no error checking. Ok so not a big deal a comment should suffice. Jon -- 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
