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 > > @@ -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. Regards Afzal -- 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
