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