Messages in this thread | | | From | David Regan <> | Date | Mon, 20 Nov 2023 17:55:51 -0800 | Subject | Re: [PATCH v4 4/4] mtd: rawnand: brcmnand: exec_op implementation |
| |
Hi Miquèl,
On Mon, Nov 20, 2023 at 2:28 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hello, > > dregan@broadcom.com wrote on Mon, 23 Oct 2023 10:14:44 -0700: > > > From: David Regan <dregan@broadcom.com> > > > > 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@broadcom.com> > > 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
| |