On 06/12/2012 01:16 AM, Mohammed, Afzal wrote:

Hi Jon,

On Tue, Jun 12, 2012 at 00:06:30, Hunter, Jon wrote:

quoted

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.

quoted

quoted

@@ -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

quoted

quoted

@@ -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