Re: [PATCH 00/17] mtd: rawnand: cafe: Convert to exec_op() (and more)

[Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>]

On Sat,  2 May 2020 13:27:32 +0200
Lubomir Rintel <lkundrak@xxxxx> wrote:

> Boris Brezillon wrote:
> > Hello,
> > 
> > A bit of context to explain the motivation behind those conversions
> > I've been sending for the last couple of weeks. The raw NAND subsystem
> > carries a lot of history which makes any rework not only painful, but
> > also subject to regressions which we only detect when someone dares to
> > update its kernel on one of those ancient HW. While carrying drivers
> > for old HW is not a problem per se, carrying ancient and unmaintained
> > drivers that are not converted to new APIs is a maintenance burden,
> > hence this massive conversion attempt I'm conducting here.
> > 
> > So here it is, a series converting the CAFE NAND controller driver to
> > exec_op(), plus a bunch of minor improvements done along the way.
> > I hope I'll find someone to test those changes, but if there's no one
> > still owning OLPC HW or no interest in keeping it supported in recent
> > kernel versions, we should definitely consider removing the driver
> > instead.
> > 
> > Regards,
> > 
> > Boris
> > 
> > Boris Brezillon (17):
> >   mtd: rawnand: cafe: Get rid of an inaccurate kernel doc header
> >   mtd: rawnand: cafe: Rename cafe_nand_write_page_lowlevel()
> >   mtd: rawnand: cafe: Use a correct ECC mode and pass the ECC alg
> >   mtd: rawnand: cafe: Include linux/io.h instead of asm/io.h
> >   mtd: rawnand: cafe: Demistify register fields
> >   mtd: rawnand: cafe: Factor out the controller initialization logic
> >   mtd: rawnand: cafe: Get rid of the debug module param
> >   mtd: rawnand: cafe: Use devm_kzalloc and devm_request_irq()
> >   mtd: rawnand: cafe: Get rid of a useless label
> >   mtd: rawnand: cafe: Explicitly inherit from nand_controller
> >   mtd: rawnand: cafe: Don't leave ECC enabled in the write path
> >   mtd: rawnand: cafe: Don't split things when reading/writing a page
> >   mtd: rawnand: cafe: Add exec_op() support
> >   mtd: rawnand: cafe: Get rid of the legacy interface implementation
> >   mtd: rawnand: cafe: Adjust the cafe_{read,write}_buf() prototypes
> >   mtd: rawnand: cafe: Handle non-32bit aligned reads/writes
> >   mtd: rawnand: cafe: s/uint{8,16,32}_t/u{8,16,32}/
> > 
> >  drivers/mtd/nand/raw/cafe_nand.c | 805 ++++++++++++++++---------------
> >  1 file changed, 423 insertions(+), 382 deletions(-)  
> 
> Thanks for doing this. With a couple of changes I've indicated in responses to
> some of the patches this has been:
> 
> Tested-by: Lubomir Rintel <lkundrak@xxxxx>

Thanks a lot for testing *and debugging* my changes. I must admit that
was unexpected, and I'm amazed by how fast I got feedback on that one
:-). Kudos to Thomas as well for noticing the email and getting us in
touch.

> 
> Other than that, I have a couple of suggestions (I'm not really in a position 
> to demand them, but they would've done the review easier for me):
> 
> 1.) I'm wondering if we could remove these:
> 
>   /* Make it easier to switch to PIO if we need to */
>   #define cafe_readl(cafe, addr)                  readl((cafe)->mmio + CAFE_##addr)
>   #define cafe_writel(cafe, datum, addr)          writel(datum, (cafe)->mmio + CAFE_##addr)
> 
> Or at least don't add new calls to them in our patches and call 
> readl()/writel() directly. The string pasting makes it impossible to grep
> for the register names.
> 
> It's not like a switch to PIO is ever going to happen and with an instance
> of readl_poll_timeout() added it's not like it would be a matter of
> rewriting those macros.

I can certainly do that.

> 
> 2.) When the block after a conditional is multiple lines, could you please
> include the curly braces? That is:
> 
>         if (ctrl1 & CAFE_NAND_CTRL1_HAS_DATA_IN) {
>                 cafe_read_buf(chip,
>                               subop->instrs[data_instr].ctx.data.buf.in +
>                               nand_subop_get_data_start_off(subop, data_instr),
>                               nand_subop_get_data_len(subop, data_instr));
>         }
> 
> Instead of:
> 
>         if (ctrl1 & CAFE_NAND_CTRL1_HAS_DATA_IN)
>                 cafe_read_buf(chip,
>                               subop->instrs[data_instr].ctx.data.buf.in +
>                               nand_subop_get_data_start_off(subop, data_instr),
>                               nand_subop_get_data_len(subop, data_instr));
> 
> This makes things significantly easier to read for me, not to mention that it
> comes handy to have the braces around for printf debugging.

I do prefer the version without brackets, but given you debugged it,
I'd be okay changing that one ;-) (assuming Miquel is okay with that
too, of course).

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