[PATCH 2/3] ARM: OMAP2+: onenand: cleanup for gpmc driver conversion
jon-hunter at ti.com
Tue Jun 12 13:30:48 EDT 2012
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.
More information about the linux-arm-kernel