Re: [PATCH v4 4/4] mtd: rawnand: brcmnand: exec_op implementation

[David Regan]

Hi Miquèl,

On Mon, Nov 20, 2023 at 2:28 AM Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
>
> Hello,
>
> dregan@xxxxxxxxxxxx wrote on Mon, 23 Oct 2023 10:14:44 -0700:
>
> > From: David Regan <dregan@xxxxxxxxxxxx>
> >
> > exec_op implementation for Broadcom STB, Broadband and iProc SoC
> > This adds exec_op and removes the legacy interface. Based on changes
> > proposed by Boris Brezillon.
> >
> > https://github.com/bbrezillon/linux/commit/4ec6f8d8d83f5aaca5d1877f02d48da96d41fcba
> > https://github.com/bbrezillon/linux/commit/11b4acffd761c4928652d7028d19fcd6f45e4696
> >
> > Signed-off-by: David Regan <dregan@xxxxxxxxxxxx>
>
> I'm fine with patches 1-3, a few minor nits on this version as well,
> nothing big. I guess I'll let some time to Florian as well to give his
> feedback and perhaps test the ->exec_op() implementation.

Thank you very much for your time, sorry for my missteps.
In the meantime I'll update and everyone can have additional
time to look.

>
> ...
>
> > +static int brcmnand_exec_instr(struct brcmnand_host *host, int i,
> > +                             const struct nand_operation *op)
> > +{
> > +     struct brcmnand_controller *ctrl = host->ctrl;
> > +     const struct nand_op_instr *instr = &op->instrs[i];
> > +     const u8 *out;
> > +     u8 *in;
> > +     int ret = 0;
> > +     bool last_op;
> > +
> > +     /*
> > +      * if we are on the last command in the sequence (not including
> > +      * waitrdy which is not a NAND command) then flag the controller
>
> May I suggest:
>
>         /*
>          * The controller needs to be aware of the last command in the operation
>          * (WAITRDY excepted).
>          */
>

Will change.

> > +      */
> > +     last_op = (((i == (op->ninstrs - 1)) &&
> > +                     (instr->type != NAND_OP_WAITRDY_INSTR)) ||
>
> You can cross the 80 chars boundary. Please use this form:
>
>         last_op = ((i == (op->ninstrs - 1)) && (instr->type != NAND_OP_WAITRDY_INSTR)) ||
>                   ((i == (op->ninstrs - 2)) && (op->instrs[i+1].type == NAND_OP_WAITRDY_INSTR));
>

Will do.

...

> > +static int brcmnand_exec_op_is_status(const struct nand_operation *op)
>
> brcmnand_op_is_status() would make more sense
>
> > +{
> > +     if ((op->ninstrs == 2) &&
> > +             (op->instrs[0].type == NAND_OP_CMD_INSTR) &&
> > +             (op->instrs[0].ctx.cmd.opcode == NAND_CMD_STATUS) &&
> > +             (op->instrs[1].type == NAND_OP_DATA_IN_INSTR))
> > +             return 1;
> > +
> > +     return 0;
> > +}
> > +
> > +static int brcmnand_exec_op_is_reset(const struct nand_operation *op)
>
> same here, please s/exec_//
>

I'll update the names.

> > +{
> > +     if ((op->ninstrs == 1) &&
> > +             (op->instrs[0].type == NAND_OP_CMD_INSTR) &&
> > +             (op->instrs[0].ctx.cmd.opcode == NAND_CMD_RESET))
> > +             return 1;
> > +
> > +     return 0;
> > +}
> > +
> > +static int brcmnand_exec_op(struct nand_chip *chip,
> > +                         const struct nand_operation *op,
> > +                         bool check_only)
> > +{
> > +     struct brcmnand_host *host = nand_get_controller_data(chip);
> > +     struct mtd_info *mtd = nand_to_mtd(chip);
> > +     u8 *status;
> > +     unsigned int i;
> > +     int ret = 0;
> > +
> > +     if (check_only)
> > +             return 0;
> > +
> > +     if (brcmnand_exec_op_is_status(op)) {
> > +             status = op->instrs[1].ctx.data.buf.in;
> > +             *status = brcmnand_status(host);
> > +
> > +             return 0;
> > +     }
>
> I would add the below chunk here:
>
>         } else if (brcmnand_exec_op_is_reset(op)) {
>                 ...
>
>                 return ...
>         }

Good idea will do.

...

>
> Thanks,
> Miquèl

Thanks!

-Dave