Re: [PATCH 2/3] ARM: OMAP2+: onenand: cleanup for gpmc driver conversion

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

On 06/12/2012 01:16 AM, Mohammed, Afzal wrote:
> Hi Jon,
> 
> On Tue, Jun 12, 2012 at 00:06:30, Hunter, Jon wrote:
> 
>> I agree with getting rid of the first instance at the beginning of
>> _set_async_mode, but why get rid of the above one? Are you assuming that
>> by default it is in async mode? Could be nice to keep it to be explicit.
> 
> Second one is achieved below

Hmmm ... it seems odd to put the commands for setting async in the set
sync. I see what you are doing but surely someone should call set async
and the set sync.

>>> @@ -191,19 +181,21 @@ static int omap2_onenand_set_sync_mode(struct omap_onenand_platform_data *cfg,
>>>  	u32 reg;
>>>  	bool clk_dep = false;
>>>  
>>> +	/* Ensure sync read and sync write are disabled */
>>> +	reg = readw(onenand_base + ONENAND_REG_SYS_CFG1);
>>> +	reg &= ~ONENAND_SYS_CFG1_SYNC_READ & ~ONENAND_SYS_CFG1_SYNC_WRITE;
>>> +	writew(reg, onenand_base + ONENAND_REG_SYS_CFG1);
>>> +
>>
>> Should we only set these after we have checked the flags and depending
>> on which flags are set?
> 
> Even for sync mode, setting async mode initially is required
> 
>>> @@ -421,6 +413,8 @@ void __init gpmc_onenand_init(struct omap_onenand_platform_data *_onenand_data)
>>>  	gpmc_onenand_resource.end = gpmc_onenand_resource.start +
>>>  							ONENAND_IO_SIZE - 1;
>>>  
>>> +	omap2_onenand_set_async_mode(gpmc_onenand_data->cs);
>>> +
>>
>> What about putting this in the gpmc_onenand_setup() function before the
>> call to _set_sync_mode? Seems nice to have these together.
> 
> Intention of this patch is to setup GPMC async mode before driver starts
> its job so that reconfiguring GPMC needs to be to be done only once (this
> helps in avoiding issues when it has to work with gpmc driver).
> 
> With the existing code, set_async was done as part of set_sync, hence
> requiring GPMC to be configured twice after driver takes control, with
> your suggestion too, GPMC would have to be configured twice.

I am just suggesting that you place the call to set_async_mode in the
gpmc_onenand_setup() instead of the gpmc_onenand_init() and remove the
calls from set_sync (like you have done). So I don't see that these
would configure the GPMC twice.

Jon