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

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Boris,

Thanks for the conversion!

Boris Brezillon <boris.brezillon@collabora.com> 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).

> +
> +	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.

> +	NAND_OP_PARSER_PATTERN(atmel_hsmc_exec_rw,
> +		NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, UINT_MAX)),

You probably meant DATA_OUT here?

> +	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@bootlin.com>


Thanks,
Miquèl

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