Re: [PATCH 2/2] davinci: Platform support for OMAP-L137/AM17x NOR flash driver

[Sergei Shtylyov <sshtylyov@mvista.com>]

Hello.

Savinay Dharmappa wrote:

> From: David Griego <dgriego@mvista.com>

    And that's after I have told you this code is not authored by David, but by 
Aleksey Makarov... :-(

> Adds platform support for OMAP-L137/AM17x NOR flash driver.

> Also, configures chip select 3 to control NOR flash's upper
> address lines.

> Signed-off-by: Aleksey Makarov <amakarov@ru.mvista.com>
> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
> Signed-off-by: Savinay Dharmappa <savinay.dharmappa@ti.com>

    Moreover, I'm seeing that you've done some incorrect changes to the 
previously correct code. NAK.

[...]

>  config MACH_DAVINCI_DA850_EVM
> diff --git a/arch/arm/mach-davinci/board-da830-evm.c b/arch/arm/mach-davinci/board-da830-evm.c
> index 1bb89d3..9f18efc 100644
> --- a/arch/arm/mach-davinci/board-da830-evm.c
> +++ b/arch/arm/mach-davinci/board-da830-evm.c
[...]
> @@ -429,6 +431,226 @@ static inline void da830_evm_init_nand(int mux_mode)
[...]
> +static void da830_evm_nor_done(void *data)
> +{
> +	clk_disable(da830_evm_nor.clk);
> +	clk_put(da830_evm_nor.clk);
> +	iounmap(da830_evm_nor.latch.addr);
> +	release_mem_region(DA8XX_AEMIF_CS3_BASE, PAGE_SIZE);
> +	iounmap(da830_evm_nor.aemif.addr);
> +	release_mem_region(DA8XX_AEMIF_CTL_BASE, SZ_32K);
> +}

    Why you've changed this function which was useful also for the error cleanup 
before?

> +static int da830_evm_nor_init(void *data, int cs)
> +{
> +	/* Turn on AEMIF clocks */
> +	da830_evm_nor.clk = clk_get(NULL, "aemif");
> +	if (IS_ERR(da830_evm_nor.clk)) {
> +		pr_err("%s: could not get AEMIF clock\n", __func__);
> +		da830_evm_nor.clk = NULL;
> +		return -ENODEV;
> +	}
> +	clk_enable(da830_evm_nor.clk);
> +
> +	da830_evm_nor.aemif.res = request_mem_region(DA8XX_AEMIF_CTL_BASE,
> +						     SZ_32K, "AEMIF control");
> +	if (da830_evm_nor.aemif.res == NULL) {
> +		pr_err("%s: could not request AEMIF control region\n",
> +			__func__);
> +		goto err_aemif_region;

    I wonder why you used goto's at all. Anyway, the error cleanup code is 
completely broken now.

> +	}
> +
> +	da830_evm_nor.aemif.addr = ioremap_nocache(DA8XX_AEMIF_CTL_BASE,
> +						   SZ_32K);
> +	if (da830_evm_nor.aemif.addr == NULL) {
> +		pr_err("%s: could not remap AEMIF control region\n", __func__);
> +		goto err_aemif_ioremap;
> +	}
> +
> +	/* Setup AEMIF -- timings, etc. */
> +
> +	/* Set maximum wait cycles */
> +	davinci_aemif_setup_timing(&da830_evm_norflash_timing,
> +						da830_evm_nor.aemif.addr, cs);
> +
> +	davinci_aemif_setup_timing(&da830_evm_norflash_timing,
> +					da830_evm_nor.aemif.addr, cs + 1);
> +
> +	/* Setup the window to access the latch */
> +	da830_evm_nor.latch.res =
> +		request_mem_region(DA8XX_AEMIF_CS3_BASE, PAGE_SIZE,
> +				   "DA830 UI NOR address latch");
> +	if (da830_evm_nor.latch.res == NULL) {
> +		pr_err("%s: could not request address latch region\n",
> +			__func__);
> +		goto err_latch_region;
> +	}
> +
> +	da830_evm_nor.latch.addr =
> +		ioremap_nocache(DA8XX_AEMIF_CS3_BASE, PAGE_SIZE);
> +	if (da830_evm_nor.latch.addr == NULL) {
> +		pr_err("%s: could not remap address latch region\n", __func__);
> +		goto err_latch_ioremap;
> +	}
> +	return 0;
> +
> +err_aemif_region:
> +	release_mem_region(DA8XX_AEMIF_CTL_BASE, SZ_32K);

    Why release what you've just failed to request?!

> +	da830_evm_nor.aemif.res = NULL;

    Useless assignment.

> +	return -EBUSY;

    And you're not calling clk_disable().

> +err_aemif_ioremap:
> +	iounmap(da830_evm_nor.aemif.addr);

    Why unmap what you've just failed to map?! da830_evm_nor.aemif.addr is NULL.

> +	da830_evm_nor.aemif.addr = NULL;

    Useless assignment.

> +	return -ENOMEM;

    You're not calling release_mem_region() and clk_disable().

> +err_latch_region:
> +	release_mem_region(DA8XX_AEMIF_CS3_BASE, PAGE_SIZE);

    Why release what you've just failed to request?!

> +	da830_evm_nor.latch.res = NULL;

    Useless assginment.

> +	return -EBUSY;

    You're not calling iounmap() and release_mem_region() for the NOR flash 
region and also not calling clk_disable().

> +
> +err_latch_ioremap:
> +	iounmap(da830_evm_nor.latch.addr);

    Why unmap what you've just failed to map?! da830_evm_nor.latch.addr is NULL.

> +	da830_evm_nor.latch.addr = NULL;

    Useless assginment.

> +	return -ENOMEM;

    You're not release_mem_region() for the latch region, not calling iounmap() 
and release_mem_region() for the NOR flash region and also not calling 
clk_disable().

> +}
[...]

    Your changes made me doubt that you actually understood the code well enough 
before doing them...

WBR, Sergei