Re: [PATCH 2/2] mtd: rawnand: atmel: Convert the driver to exec_op()

[Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>]

On Mon, 27 Apr 2020 17:17:19 +0200
Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:

> Hi Boris,
> 
> Thanks for the conversion!
> 
> Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote on Sat, 18 Apr
> 2020 21:49:59 +0200:
> 
> [...]
> 
> >  
> > -static void atmel_nand_cmd_ctrl(struct nand_chip *chip, int cmd,
> > -				unsigned int ctrl)
> > +static int atmel_hsmc_exec_rw(struct nand_chip *chip,
> > +			      const struct nand_subop *subop)
> >  {
> > +	const struct nand_op_instr *instr = subop->instrs;
> >  	struct atmel_nand *nand = to_atmel_nand(chip);
> > -	struct atmel_nand_controller *nc;
> >  
> > -	nc = to_nand_controller(chip->controller);
> > +	if (WARN_ON_ONCE(subop->ninstrs != 1 ||
> > +			 (instr->type != NAND_OP_DATA_IN_INSTR &&
> > +			  instr->type != NAND_OP_DATA_OUT_INSTR)))
> > +		return -EINVAL;
> >  
> > -	if ((ctrl & NAND_CTRL_CHANGE) && nand->activecs->csgpio) {
> > -		if (ctrl & NAND_NCE)
> > -			gpiod_set_value(nand->activecs->csgpio, 0);
> > -		else
> > -			gpiod_set_value(nand->activecs->csgpio, 1);
> > -	}
> > +	if (instr->type == NAND_OP_DATA_IN_INSTR)
> > +		atmel_nand_read_buf(nand, instr->ctx.data.buf.in,
> > +				    instr->ctx.data.len,
> > +				    instr->ctx.data.force_8bit);
> > +	else
> > +		atmel_nand_write_buf(nand, instr->ctx.data.buf.out,
> > +				     instr->ctx.data.len,
> > +				     instr->ctx.data.force_8bit);
> >  
> > -	if (ctrl & NAND_ALE)
> > -		writeb(cmd, nand->activecs->io.virt + nc->caps->ale_offs);
> > -	else if (ctrl & NAND_CLE)
> > -		writeb(cmd, nand->activecs->io.virt + nc->caps->cle_offs);
> > +	return 0;
> > +}
> > +
> > +static int atmel_hsmc_exec_waitrdy(struct nand_chip *chip,
> > +				   const struct nand_subop *subop)
> > +{
> > +	const struct nand_op_instr *instr = subop->instrs;
> > +	struct atmel_nand *nand = to_atmel_nand(chip);
> > +
> > +	if (WARN_ON_ONCE(subop->ninstrs != 1 ||
> > +			 instr->type != NAND_OP_WAITRDY_INSTR))
> > +		return -EINVAL;  
> 
> How could this happen? I would drop this extra check which IMHO is not
> useful (same for all the occurrences of similar conditions).

Yes, I guess I was overcautious here to detect core bugs, but you're
right, this shouldn't be checked at this level.

> 
> > +
> > +	return atmel_hsmc_nand_waitrdy(nand, instr->ctx.waitrdy.timeout_ms);
> > +}
> > +
> > +static const struct nand_op_parser atmel_hsmc_op_parser = NAND_OP_PARSER(
> > +	NAND_OP_PARSER_PATTERN(atmel_hsmc_exec_cmd_addr,
> > +		NAND_OP_PARSER_PAT_CMD_ELEM(true),
> > +		NAND_OP_PARSER_PAT_ADDR_ELEM(true, 5),
> > +		NAND_OP_PARSER_PAT_CMD_ELEM(true)),
> > +	NAND_OP_PARSER_PATTERN(atmel_hsmc_exec_rw,
> > +		NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, UINT_MAX)),  
> 
> I find more meaningful to use 0 than UINT_MAX as the core will ignore
> any boundary in this case.

Oh, you're right, I had forgotten that 0 meant 'no-limit'.

> 
> > +	NAND_OP_PARSER_PATTERN(atmel_hsmc_exec_rw,
> > +		NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, UINT_MAX)),  
> 
> You probably meant DATA_OUT here?

Absolutely.

> 
> > +	NAND_OP_PARSER_PATTERN(atmel_hsmc_exec_waitrdy,
> > +		NAND_OP_PARSER_PAT_WAITRDY_ELEM(false)),
> > +);
> > +
> > +static int atmel_hsmc_nand_exec_op(struct atmel_nand *nand,
> > +				   const struct nand_operation *op,
> > +				   bool check_only)
> > +{
> > +	int ret;
> > +
> > +	if (check_only)
> > +		return nand_op_parser_exec_op(&nand->base,
> > +					      &atmel_hsmc_op_parser, op, true);
> > +
> > +	atmel_hsmc_nand_select_die(nand, op->cs);
> > +	ret = nand_op_parser_exec_op(&nand->base, &atmel_hsmc_op_parser, op,
> > +				     false);
> > +	atmel_hsmc_nand_unselect_die(nand);
> > +
> > +	return ret;
> >  }
> >    
> 
> With the above fixed, please add my
> 
> Reviewed-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>

Thanks for the review.

Boris

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/