Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
MIME-Version: 1.0
References: <20231023171444.322311-1-dregan@broadcom.com> <20231023171444.322311-4-dregan@broadcom.com>
 <20231120112816.15a19e05@xps-13>
In-Reply-To: <20231120112816.15a19e05@xps-13>
From:   David Regan <dregan@broadcom.com>
Date:   Mon, 20 Nov 2023 17:55:51 -0800
Message-ID: <CAA_RMS4H3qxHehN5uZdeV9KKXEZqONAsyJHrBjgt57+2kQVqsg@mail.gmail.com>
Subject: Re: [PATCH v4 4/4] mtd: rawnand: brcmnand: exec_op implementation
To:     Miquel Raynal <miquel.raynal@bootlin.com>
Cc:     David Regan <dregan@broadcom.com>,
        bcm-kernel-feedback-list@broadcom.com,
        linux-mtd@lists.infradead.org, f.fainelli@gmail.com,
        rafal@milecki.pl, Joel Peshkin <joel.peshkin@broadcom.com>,
        computersforpeace@gmail.com,
        Dan Beygelman <dan.beygelman@broadcom.com>,
        William Zhang <william.zhang@broadcom.com>,
        frieder.schrempf@kontron.de, linux-kernel@vger.kernel.org,
        vigneshr@ti.com, richard@nod.at, bbrezillon@kernel.org,
        kdasu.kdev@gmail.com, JaimeLiao <jaimeliao.tw@gmail.com>,
        Adam Borowski <kilobyte@angband.pl>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Miqu=C3=A8l,

On Mon, Nov 20, 2023 at 2:28=E2=80=AFAM Miquel Raynal <miquel.raynal@bootli=
n.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/4ec6f8d8d83f5aaca5d1877f02d4=
8da96d41fcba
> > https://github.com/bbrezillon/linux/commit/11b4acffd761c4928652d7028d19=
fcd6f45e4696
> >
> > 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 =3D host->ctrl;
> > +     const struct nand_op_instr *instr =3D &op->instrs[i];
> > +     const u8 *out;
> > +     u8 *in;
> > +     int ret =3D 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 op=
eration
>          * (WAITRDY excepted).
>          */
>

Will change.

> > +      */
> > +     last_op =3D (((i =3D=3D (op->ninstrs - 1)) &&
> > +                     (instr->type !=3D NAND_OP_WAITRDY_INSTR)) ||
>
> You can cross the 80 chars boundary. Please use this form:
>
>         last_op =3D ((i =3D=3D (op->ninstrs - 1)) && (instr->type !=3D NA=
ND_OP_WAITRDY_INSTR)) ||
>                   ((i =3D=3D (op->ninstrs - 2)) && (op->instrs[i+1].type =
=3D=3D 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 =3D=3D 2) &&
> > +             (op->instrs[0].type =3D=3D NAND_OP_CMD_INSTR) &&
> > +             (op->instrs[0].ctx.cmd.opcode =3D=3D NAND_CMD_STATUS) &&
> > +             (op->instrs[1].type =3D=3D 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 =3D=3D 1) &&
> > +             (op->instrs[0].type =3D=3D NAND_OP_CMD_INSTR) &&
> > +             (op->instrs[0].ctx.cmd.opcode =3D=3D 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 =3D nand_get_controller_data(chip);
> > +     struct mtd_info *mtd =3D nand_to_mtd(chip);
> > +     u8 *status;
> > +     unsigned int i;
> > +     int ret =3D 0;
> > +
> > +     if (check_only)
> > +             return 0;
> > +
> > +     if (brcmnand_exec_op_is_status(op)) {
> > +             status =3D op->instrs[1].ctx.data.buf.in;
> > +             *status =3D 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=C3=A8l

Thanks!

-Dave