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

[Nori, Sekhar]

Hi Sergei,

On Thu, Oct 07, 2010 at 20:35:28, Sergei Shtylyov wrote:

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

The issues down below aside, I think goto based error handling
is clearer to follow. Any objections to using goto based
error handing per-se?

Thanks,
Sekhar

>
> > +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
>
> _______________________________________________
> Davinci-linux-open-source mailing list
> Davinci-linux-open-source at linux.davincidsp.com
> http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
>