[PATCH] OneNAND MTD support

[Kyungmin Park]

Hi

It's new patch as your request

and I append some bufferram function to reduce unnecessary read access.

It's similay page buf in NAND.

Regards

Kyungmin Park

> 
> Some remarks:
> 
> Kconfig and Makefile need references in drivers/mtd/Kconfig and
> drivers/mtd/Makefile !!!

Yes, I'm missing

>+menu "OneNAND Flash Device Drivers (EXPERIMENTAL)"
>+	depends on MTD != n && EXPERIMENTAL
>+
>+config MTD_ONENAND
>+	tristate "OneNAND Device Support"
>	depends on MTD
>	select MTD_ONENAND_INFO

>Why do you need a seperate config option, which is just a mirror of
MTD_ONENAND ?

I don't understood. What's meaning?

> 
> +
> +        /* try the first address */
> +	omap_onenand_flash_base = 
> ioremap(OMAP_ONENAND_FLASH_START1, SZ_128K);
> 
> Why a seperate static variable ? Please use the base member of the
> private structure.

OK. I done.

> +static int onenand_dataram(struct mtd_info *mtd, int write,
> +		unsigned char *buffer, int offset, int count)
> +{
> +	struct onenand_chip *this = mtd->priv;
> +	void __iomem *src, *dest;
> +	void __iomem *dataram;
> +
> +	dataram = this->base + ONENAND_DATARAM;
> +
> +	if (ONENAND_CURRENT_BUFFERRAM(this))
> +		dataram += mtd->oobblock;
> +
> +	if (write) {
> +		src = buffer;
> +		dest = dataram + offset;
> +	} else {
> +		src = dataram + offset;
> +		dest = buffer;
> +	}
> +
> +	memcpy(dest, src, count);
> 
> 
> Can the read/write functions be seperate please ?
> Only the hardware access should be replaceable by the board driver.
> Calculations which are valid for every implementation should 
> be done in
> generic code. This reduces the code size and the error sources for
> hardware specific implementations

As your request, I create read/write_bufferram function instread of dataram
and spareram
 
> 
> +static int onenand_unlock(struct mtd_info *mtd, loff_t ofs, 
> size_t len)
> <SNIP>
> +		/* There's no return value */
> +		this->wait(mtd, FL_UNLOCKING);
> 
> Why not ?

There's no status check in locking mechanism

> 
> +		/* Sanity check */
> +		while (onenand_readw(this->base + 
> ONENAND_REG_CTRL_STATUS)
> +		    & ONENAND_CTRL_ONGO)
> +			continue;
> 
> What happens if the condition is not met ?

I think it's board bug. For safety I check it. 

> 
> +/**
> + * onenand_scan - [OneNAND Interface] Probe the OneNAND device
> + * @param mtd		MTD device structure
> + *
> + * OneNAND detection method:
> + *   Compare the the values from command with ones from register
> + */
> +static int onenand_probe(struct mtd_info *mtd)
> 
> Please make comment and function name consistent.

OK. fix it

> +/*
> + *  linux/drivers/mtd/onenand/onenand_info.c
> 
> <SNIP>
> 
> +EXPORT_SYMBOL_GPL(onenand_print_device_info);
> +EXPORT_SYMBOL_GPL(onenand_print_maf_info);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Kyungmin Park <kyungmin.park at samsung.com>");
> +MODULE_DESCRIPTION("Display OneNAND device and manufacturer ID's");
> 
> 
> Please link this with onenand_base.c. It's overkill to have those
> exports and a seperate module. The seperate nand_ids file in
> drivers/mtd/nand is solely there to make it reusable by the legacy
> DiskOnChip drivers. Once those are gone the id table will be linked to
> nand_base.c unconditionally.

I merged it 

> 
> 
> +	FL_ERASING,
> +	FL_SYNCING,
> +	FL_UNLOCKING,
> +} onenand_state_t;
> 
> Shouldnt there be a LOCK counterpart to UNLOCK ?

OK.

> 
> 
> +/*
> + * Functions to access the OneNAND IO region
> + */
> +#define onenand_readw(a)		readw(a)
> +#define onenand_writew(v,a)		writew(v, a)
> 
> Please make onenand_readw/writew functions accessible by function
> pointers along with default implementations, so the board driver can
> override them if necessary.
> 

It's just memory access function. I think no need to make a function.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: onenand-mtd.patch
Type: application/octet-stream
Size: 56990 bytes
Desc: not available
Url : http://lists.infradead.org/pipermail/linux-mtd/attachments/20050709/29aa851f/attachment.obj