Re: [PATCH] [MTD] [NAND] GPIO NAND flash driver

["Mike Frysinger" <vapier.adi@gmail.com>]

On Sun, Oct 12, 2008 at 04:02, Mike Rapoport wrote:
> Mike Frysinger wrote:
>>> The problem is that a write to GPIO may pass a write to the static
>>> memory regions, or vice versa.  So, what we do is we insert a read
>>> with a dependency in the execution to ensure that we stall the
>>> pipeline until that read - and therefore the preceding write has
>>> completed.
>>
>> so the function comment should read something like "make sure the gpio
>> state has actually changed before returning to the higher nand layers"
>
> The patch with (hopefully) clearer gpio_nand_dosync() comments.

looks like it, thanks for that

> +static int gpio_nand_remove(struct platform_device *dev)

should have __devexit markings

> +       if (gpio_is_valid(gpiomtd->plat.gpio_nwp))
> +               gpio_set_value(gpiomtd->plat.gpio_nwp, 0);
> +       gpio_set_value(gpiomtd->plat.gpio_nce, 1);
> +
> +       gpio_free(gpiomtd->plat.gpio_cle);
> +       gpio_free(gpiomtd->plat.gpio_ale);
> +       gpio_free(gpiomtd->plat.gpio_nce);
> +       if (gpio_is_valid(gpiomtd->plat.gpio_nwp))
> +               gpio_free(gpiomtd->plat.gpio_nwp);

why do you bother setting the value before releasing ?  when you
release, the pin tends to go to the hardware default and on the
Blackfin, that tends to be "tristate".  are you just banking on the
idea that the pin will stay the way it was last set before it gets
freed ?

> +static int gpio_nand_probe(struct platform_device *dev)

should have __devinit markings

> +err_wp:
> +       if (gpio_is_valid(gpiomtd->plat.gpio_nwp))
> +               gpio_set_value(gpiomtd->plat.gpio_nwp, 0);
> +       gpio_free(gpiomtd->plat.gpio_rdy);
> +err_rdy:
> +       gpio_free(gpiomtd->plat.gpio_cle);
> +err_cle:
> +       gpio_free(gpiomtd->plat.gpio_ale);
> +err_ale:
> +       if (gpio_is_valid(gpiomtd->plat.gpio_nwp))
> +               gpio_free(gpiomtd->plat.gpio_nwp);
> +err_nwp:
> +       gpio_free(gpiomtd->plat.gpio_nce);
> +err_nce:
> +       iounmap(gpiomtd->io_sync);
> +       if (res1)
> +               release_mem_region(res1->start, res1->end - res1->start + 1);
> +err_sync:
> +       iounmap(gpiomtd->nand_chip.IO_ADDR_R);
> +       release_mem_region(res0->start, res0->end - res0->start + 1);
> +err_map:
> +       kfree(gpiomtd);
> +       return -ENOMEM;

you really should be returning "ret" rather than "-ENOMEM" as many
(most?) of the ways you'd get here will not be due to out of memory
conditions.  some error paths above will probably require you to
manually set "ret" to something relevant ...
-mike