Hi Afzal,

On 06/14/2012 12:40 AM, Mohammed, Afzal wrote:

Hi Jon,

On Wed, Jun 13, 2012 at 22:08:47, Hunter, Jon wrote:

quoted

On 06/13/2012 12:03 AM, Mohammed, Afzal wrote:

quoted

quoted

As gpmc_onenand_setup is a callback by onenand driver, we would have
lost the opportunity to configure onenand before driver is probed.

Is that a problem? Looks like it is called early in the probe and so I
would hope no one is attempting to access the onenand itself before the
probe has completed.

During gpmc driver probe, it will configure all the connected peripherals,
if configuration details are not present at that point of time, gpmc driver
will cry out saying that configuration & timings has not been configured,
(please see holler if no configuration patch). 

Sorry, I am not sure if I am missing something here, but isn't the
chip-select requested during the gpmc probe? If so then we should not be
programming the gpmc registers at all until the chip-select has been
allocated. Hence, after the probe seems more appropriate.

And I do not see any reason
why gpmc driver should not be fed with async mode configuration initially,
as it has to be done always.

It is more of where you are doing it. I am not against putting in async
mode to begin with.

quoted

quoted

This would cause requirement of double GPMC configuring and we lost
the opportunity to configure GPMC before driver is probed.

I am not convinced we need to. Furthermore with your change you do not
actually set async mode in the onenand until _set_sync() is called.

Yes, setting async mode in onenand is done in set_sync function, and it is
always called by onenand driver indirectly.

Seems if setting async mode in onenand is taken out of set_sync & placed
it before set_sync invocation in gpmc_onenand_setup, intention will be
clear, right ? (even though sequence wise same thing is happening now)

Exactly.

quoted

quoted

And the first step for onenand configuration is always to set it
to async mode (with the way it is done now), so it seems reasonable
to rely on normal GPMC configuration for async & then do reconfigure
for sync.

Yes but as far as I can see, it seems that this is the intent of the
onenand_setup() function to perform the necessary initialisation.

I believe doing it in gpmc_onenand_init is better, due to the reasons
mentioned above as well as because function name will correspond to what
it is doing, i.e. initialization

If the chip-select is allocating during the probe, then I don't agree.

quoted

Have you tested onenand? Do you have a board with onenand?

Yes, on omap3evm, with help of local patches (as mainline doesn't have
those). It was mentioned in cover letter of gpmc driver conversion series

Great. Sorry but with 20+ patch spread across 3 series it is not always
easy to find the details. So ideally it should be mentioned in this
cover letter too.

Thanks
Jon