Re: [PATCH v5] mtd: rawnand: brcmnand: legacy exec_op implementation

Miquel Raynal
2025-05-23T15:09:47Z

On 21/05/2025 at 10:03:25 +02, Álvaro Fernández Rojas <noltari@gmail.com> wrote:

> Commit 3c8260ce7663 ("mtd: rawnand: brcmnand: exec_op implementation")
> removed legacy interface functions, breaking < v5.0 controllers support.
> In order to fix older controllers we need to add an alternative exec_op
> implementation which doesn't rely on low level registers.
>
> Fixes: 3c8260ce7663 ("mtd: rawnand: brcmnand: exec_op implementation")
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> Reviewed-by: David Regan <dregan@broadcom.com>
> ---
> drivers/mtd/nand/raw/brcmnand/brcmnand.c | 222 ++++++++++++++++++++++-
> 1 file changed, 215 insertions(+), 7 deletions(-)
>
> v5: add changes requested by Miquèl Raynal:
> - Mention and explain legacy in native_cmd_conv.
> - EOPNOTSUPP instead of EINVAL for instr->type else.
> - Implement missing check_only functionality.
>
> v4: add changes requested by Jonas Gorski:
> - Add missing breaks in brcmnand_exec_instructions_legacy.
> - Restore missing ret assignment in brcmnand_exec_op.
>
> v3: add changes requested by Florian and other improvements:
> - Add associative array for native command conversion.
> - Add function pointer to brcmnand_controller for exec_instr
> functionality.
> - Fix CMD_BLOCK_ERASE address.
> - Drop NAND_CMD_READOOB support.
>
> v2: multiple improvements:
> - Use proper native commands for checks.
> - Fix NAND_CMD_PARAM/NAND_CMD_RNDOUT addr calculation.
> - Remove host->last_addr usage.
> - Remove sector_size_1k since it only applies to v5.0+ controllers.
> - Remove brcmnand_wp since it doesn't exist for < v5.0 controllers.
> - Use j instead of i for flash_cache loop.
>

...

> +static int brcmnand_check_instructions_legacy(struct nand_chip *chip,
> + const struct nand_operation *op)
> +{
> + const struct nand_op_instr *instr;
> + unsigned int i;
> + u8 cmd;
> +
> + for (i = 0; i < op->ninstrs; i++) {
> + instr = &op->instrs[i];
> +
> + switch (instr->type) {
> + case NAND_OP_CMD_INSTR:
> + cmd = native_cmd_conv[instr->ctx.cmd.opcode];
> + if (cmd == CMD_NOT_SUPPORTED)
> + return -EOPNOTSUPP;
> + break;
> + case NAND_OP_ADDR_INSTR:
> + case NAND_OP_DATA_IN_INSTR:

No NAND_OP_DATA_OUT_INSTR?

> + case NAND_OP_WAITRDY_INSTR:
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> + }
> +
> + return 0;
> +}

Rest lgtm.

Thanks,
Miquèl