[PATCH V3 2/6] pxa3xx_nand: rework irq logic
Lei Wen
adrian.wenl at gmail.com
Sun Feb 13 10:46:05 EST 2011
Hi Eric,
On Sun, Feb 13, 2011 at 5:46 PM, Eric Miao <eric.y.miao at gmail.com> wrote:
> Hi Lei,
>
> I like this idea very much. There are several places for improvement,
> though, you mind take a look?
>
>> static int pxa3xx_nand_dev_ready(struct mtd_info *mtd)
>> @@ -580,14 +582,12 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command,
>> {
>> struct pxa3xx_nand_info *info = mtd->priv;
>> const struct pxa3xx_nand_cmdset *cmdset = info->cmdset;
>> - int ret;
>> + int ret, exec_cmd = 0;
>>
>> info->use_dma = (use_dma) ? 1 : 0;
>> info->use_ecc = 0;
>> info->data_size = 0;
>> - info->state = STATE_READY;
>> -
>> - init_completion(&info->cmd_complete);
>> + info->state = 0;
>>
>> switch (command) {
>> case NAND_CMD_READOOB:
>> @@ -596,14 +596,12 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command,
>> info->buf_start = mtd->writesize + column;
>> memset(info->data_buff, 0xFF, info->buf_count);
>>
>> - if (prepare_read_prog_cmd(info, cmdset->read1, column, page_addr))
>> - break;
>> -
>> - pxa3xx_nand_do_cmd(info, NDSR_RDDREQ | NDSR_DBERR | NDSR_SBERR);
>> -
>> + prepare_read_prog_cmd(info, cmdset->read1, column, page_addr);
>> /* We only are OOB, so if the data has error, does not matter */
>> if (info->retcode == ERR_DBERR)
>> info->retcode = ERR_NONE;
>
> Note the above statement is executed after a do_cmd() is done, so
> info->retcode contains a valid return code. While the actual code
> to perform the command has been moved to below.
It is my fault not delete those line here...
In my following 0004 patch, I would unify the prepare step into one function.
BTW, for OOB data, I think we also shouldn't omit the DB error.
Like BCH crc algorithm, it also protect the oob area, so it is reasonable to
be aware that area...
>
> For each transaction, there are three steps:
>
> 1. prepare/fill the command
> 2. perform the command
> 3. handle the return status
>
> I actually though of merging steps 2) and 3) together, but step
> 3) could be different, and that the timeout for different type of
> commands might be different, that's why I introduced the do_cmd()
> function.
I agree. :)
That is what I want to do in this patch set, unify the prepare behavior
into one function, and then execute it.
>
>> +
>> + exec_cmd = 1;
>
> Maybe for those cases where commands are not executed, just simply
> return will avoid introducing this exec_cmd variable.
For I unify the prepare process into one function according to different
command, simply return would be unable to notify the execute action...
The exec_cmd would be made as the return value for the prepare function.
>
> .....
>
>> @@ -807,10 +795,7 @@ static int __readid(struct pxa3xx_nand_info *info, uint32_t *id)
>> uint32_t ndcr;
>> uint8_t id_buff[8];
>>
>
> I saw __readid() is replaced by pxa3xx_nand_cmdfunc(NAND_CMD_READID)
> in the code below. Is it possible to completely remove this function
> now??
I made the remove behavior in the patch 0003. :)
Thanks,
Lei
More information about the linux-arm-kernel
mailing list