Re: [PATCH v5 06/14] ARM: OMAP2+: gpmc: CS configuration helper

[Jon Hunter <jon-hunter@ti.com>]

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