Hello Adrian,
On Fri, 1 Aug 2008, Adrian Hunter wrote:
> Update OneNAND support for OMAP3.
a few quick comments.
> + reg =
> omap2_onenand_readw(onenand_base+ONENAND_REG_VERSION_ID);
Just a minor nit - please use spaces around binary & ternary operators per
CodingStyle.
> + (sync_write?GPMC_CONFIG1_WRITEMULTIPLE_SUPP:0) |
> + (sync_write?GPMC_CONFIG1_WRITETYPE_SYNC:0) |
as above.
> diff --git a/drivers/mtd/onenand/omap2.c b/drivers/mtd/onenand/omap2.c
> index ba83900..378ee17 100644
> --- a/drivers/mtd/onenand/omap2.c
> +++ b/drivers/mtd/onenand/omap2.c
> @@ -223,6 +227,155 @@ static inline int omap2_onenand_bufferram_offset(struct
> mtd_info *mtd, int area)
> return 0;
> }
>
> +#if defined(CONFIG_ARCH_OMAP3)
> +
> +static int omap3_onenand_read_bufferram(struct mtd_info *mtd, int area,
> + unsigned char *buffer, int offset,
> + size_t count)
> +{
> + struct omap2_onenand *info = container_of(mtd, struct omap2_onenand,
> mtd);
> + struct onenand_chip *this = mtd->priv;
> + dma_addr_t dma_src, dma_dst;
> + int bram_offset;
> + unsigned long timeout;
> + void *buf = (void *)buffer;
> + size_t xtra;
> + volatile unsigned *done;
The way this volatile is used doesn't look right...
> + INIT_COMPLETION(info->dma_done);
> + omap_start_dma(info->dma_channel);
> +
> + timeout = jiffies + msecs_to_jiffies(20);
> + done = &info->dma_done.done;
So the volatile here appears to apply to the address of 'done', but this
address does not change, correct? Only the value of 'done' itself
changes.
> + while (time_before(jiffies, timeout))
> + if (*done)
> + break;
Can this be replaced with wait_for_completion_timeout() or something
similar?
> + dma_unmap_single(&info->pdev->dev, dma_dst, count, DMA_FROM_DEVICE);
> +
> + if (!*done) {
> + dev_err(&info->pdev->dev, "timeout waiting for DMA\n");
> + goto out_copy;
> + }
...
> +static int omap3_onenand_write_bufferram(struct mtd_info *mtd, int area,
> + const unsigned char *buffer, int
> offset,
> + size_t count)
> +{
> + struct omap2_onenand *info = container_of(mtd, struct omap2_onenand,
> mtd);
> + struct onenand_chip *this = mtd->priv;
> + dma_addr_t dma_src, dma_dst;
> + int bram_offset;
> + unsigned long timeout;
> + void *buf = (void *)buffer;
> + volatile unsigned *done;
Same comments in this function per volatile.
- Paul
--
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
