On 06/12/2012 07:58 AM, Mohammed, Afzal wrote: > Hi Jon, > > On Tue, Jun 12, 2012 at 14:10:08, Mohammed, Afzal wrote: > >>>> + l |= conf & GPMC_CONFIG1_DEVICETYPE_NAND; >>>> + l |= conf & GPMC_CONFIG1_DEVICESIZE_16; >>> >>> I can see that it works to use the above definitions as masks because of >>> the possible values that can be programmed into these fields. However, >>> from a read-ability standpoint this is unclear and requires people to >>> review the documentation to understand what you are doing here. >>> Furthermore, if any new device-types or sizes were added in the future >>> this could lead to bugs. Hence, it would be better to define a mask for >>> these fields. >> >> I had thought about it initially, but then it was felt it will >> lead to a less simple code, that path was not taken, let me >> revisit this. > > 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. 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? Cheers 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
