Re: [ANNOUNCE] [PATCH] [MTD] Flex-OneNAND MTD Driver available.

[Adrian Hunter <ext-adrian.hunter@nokia.com>]

Rohit wrote:
> Adrian Hunter wrote:
>>> +		this->command(mtd, FLEXONENAND_CMD_PI_ACCESS, die, 0);
>>> +		this->wait(mtd, FL_SYNCING);
>> Why is the error return not checked?
>>
>>> +
>>> +		this->command(mtd, ONENAND_CMD_READ, die, 0);
>>> +		ret = this->wait(mtd, FL_READING);
>> Why is the error return not checked?
>>
>>> +
>>> +		bdry = this->read_word(this->base + ONENAND_DATARAM);
>>> +		locked = bdry >> FLEXONENAND_PI_UNLOCK_SHIFT;
>>> +		locked = (locked == 0x3) ? 0 : 1;
>>> +		this->boundary[die] = bdry & FLEXONENAND_PI_MASK;
>>> +		this->boundary_locked[die] = locked;
>>> +		this->command(mtd, ONENAND_CMD_RESET, 0, 0);
>>> +		ret = this->wait(mtd, FL_RESETING);
>> Why is the error return not checked?
> 
> As per datasheet PI_ACCESS, PI_UPDATE, PI_READ and RESET always succeed.
> PI_ERASE and PI_WRITE can fail. Added error check for PI_ERASE.

So if the device is behaving correctly and the driver has no bugs and there
are no other related hardware errors, then 'ret' will always be zero.  Well,
I would check it anyway, but that's just me.

>>> +	if (FLEXONENAND(this)) {
>>> +		int boundary[] =
>>> +			{FLEXONENAND_DIE0_BOUNDARY, FLEXONENAND_DIE1_BOUNDARY};
>>> +		int lock[] =
>>> +			{FLEXONENAND_DIE0_ISLOCKED, FLEXONENAND_DIE1_ISLOCKED};
>>> +		unsigned die;
>>> +
>>> +		flexonenand_get_size(mtd);
>>> +
>>> +		/* Change the device boundaries if required */
>>> +		for (die = 0; die < this->dies; die++)
>>> +			if ((!this->boundary_locked[die]) &&
>>> +			   (boundary[die] >= 0) &&
>>> +			   (boundary[die] != this->boundary[die]))
>>> +				flexonenand_set_boundary(mtd, die,
>>> +						 boundary[die], lock[die]);
>> I am not sure how you intend FLEXONENAND_DIEx_BOUNDARY and FLEXONENAND_DIEx_ISLOCKED
>> to be used.  Obviously this code presently does nothing.  If you expect the values
>> to be configured for the device, then shouldn't they be config options?
> 
> Ok. Added config option for boundary and lock settings.
> Configuration of boundary / lock is optional. Default value in config is -1
> which will not change existing boundary of Flex-OneNAND.
> 
>> Also, ideally there should be a way for the platform driver to override them.
>>
> 
> Do you mean runtime configuration of boundary. Then we need to add some ioctl.
> 

No I mean a onenand driver (e.g. omap2.c) that reads the values from platform data
and wants to set them.  It is not important.  It can be done later by whomever needs it.

> diff --git a/include/linux/mtd/onenand.h b/include/linux/mtd/onenand.h
> index 9aa2a91..d23eeef 100644
> --- a/include/linux/mtd/onenand.h
> +++ b/include/linux/mtd/onenand.h
> @@ -17,8 +17,23 @@
>  #include <linux/mtd/onenand_regs.h>
>  #include <linux/mtd/bbm.h>
>  
> +#define MAX_DIES		2
>  #define MAX_BUFFERRAM		2
>  
> +#define FLEXONENAND_DIE0_BOUNDARY	CONFIG_MTD_FLEXONENAND_DIE0_BOUNDARY
> +#define FLEXONENAND_DIE1_BOUNDARY	CONFIG_MTD_FLEXONENAND_DIE1_BOUNDARY
> +

CONFIG_MTD_FLEXONENAND_DIEx_BOUNDARY are not necessarily defined i.e. you
need "#if defined ..." there as well.

I briefly tested the driver with an old 'non-Flex' OneNAND and it seemd to work
fine with no apparent effect on performance.  So I have no more issues :-)

Thanks
Adrian