Re: [PATCH 4/6] mtd: Introduce SPI NAND framework

[Ezequiel Garcia <ezequiel.garcia@imgtec.com>]

Dear Qi Wang,

Thanks for the review,

On 12/22/2014 01:34 AM, Qi Wang 王起 (qiwang) wrote:
> Hi Ezequiel,
> 
>> +/*
>> + * Wait until the status register busy bit is cleared.
>> + * Returns a negatie errno on error or time out, and a non-negative
>> +status
>> + * value if the device is ready.
>> + */
>> +static int spi_nand_wait_till_ready(struct spi_nand *snand) {
>> +	unsigned long deadline = jiffies + msecs_to_jiffies(100);
> 
> 100ms will be applied to all operation, but I think it would be more
> make sense to use different timeout value for different operation, 
> just like Parallel NAND as below:
> "

Yes, indeed. You are right. Now we need to find out the appropriate
value in each case. Any suggestions?

> static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
> {
> 
> 	int status, state = chip->state;
> 	unsigned long timeo = (state == FL_ERASING ? 400 : 20);
> "	
> 
> 
>> +static int spi_nand_write(struct spi_nand *snand) {
>> +	int ret;
>> +
>> +	/* Store the page to cache */
>> +	ret = snand->store_cache(snand, 0, snand->buf_size, snand-
>>> data_buf);
>> +	if (ret < 0) {
>> +		dev_err(snand->dev, "error %d storing page 0x%x to cache\n",
>> +			ret, snand->page_addr);
>> +		return ret;
>> +	}
>> +
>> +	ret = spi_nand_wait_till_ready(snand);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = snand->write_enable(snand);
>> +	if (ret < 0) {
>> +		dev_err(snand->dev, "write enable command failed\n");
>> +		return ret;
>> +	}
>> +
>> +	/* Get page from the device cache into our internal buffer */
>> +	ret = snand->write_page(snand, snand->page_addr);
> 
> 
> The page program sequence in datasheet is write_enable->program_load->
> program_execute->read_status. Seems different with the procedure in 
> your code.
> 	

Right. I thought calling write_enable() before actually programming the
page would be enough. Do you think we should do it before the cache
programming instead?

And what happens if it fails, will we need to call write_disable explicitly?

-- 
Ezequiel