Hi Miquel,
On Fri, Nov 19, 2021 at 10:23 AM Miquel Raynal
<miquel.raynal@xxxxxxxxxxx> wrote:
> geert@xxxxxxxxxxxxxx wrote on Fri, 19 Nov 2021 09:55:53 +0100:
> > On Thu, Nov 18, 2021 at 12:19 PM Miquel Raynal
> > <miquel.raynal@xxxxxxxxxxx> wrote:
> > > Introduce Renesas RZ/N1x NAND controller driver which supports:
> > > - All ONFI timing modes
> > > - Different configurations of its internal ECC controller
> > > - On-die (not tested) and software ECC support
> > > - Several chips (not tested)
> > > - Subpage accesses
> > > - DMA and PIO
> > >
> > > Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
> > > --- /dev/null
> > > +++ b/drivers/mtd/nand/raw/rzn1-nand-controller.c
> >
> > > +static int rzn1_read_subpage_hw_ecc(struct nand_chip *chip, u32 req_offset,
> > > + u32 req_len, u8 *bufpoi, int page)
> > > +{
> > > + struct rzn1_nfc *nfc = to_rzn1_nfc(chip->controller);
> > > + struct mtd_info *mtd = nand_to_mtd(chip);
> > > + struct rzn1_nand_chip *rzn1_nand = to_rzn1_nand(chip);
> > > + unsigned int cs = to_nfc_cs(rzn1_nand);
> > > + unsigned int page_off = round_down(req_offset, chip->ecc.size);
> > > + unsigned int real_len = round_up(req_offset + req_len - page_off,
> > > + chip->ecc.size);
> > > + unsigned int start_chunk = page_off / chip->ecc.size;
> > > + unsigned int nchunks = real_len / chip->ecc.size;
> > > + unsigned int ecc_off = 2 + (start_chunk * chip->ecc.bytes);
> > > + struct rzn1_op rop = {
> > > + .command = COMMAND_INPUT_SEL_AHBS | COMMAND_0(NAND_CMD_READ0) |
> > > + COMMAND_2(NAND_CMD_READSTART) | COMMAND_FIFO_SEL |
> > > + COMMAND_SEQ_READ_PAGE,
> > > + .addr0_row = page,
> > > + .addr0_col = page_off,
> > > + .len = real_len,
> > > + .ecc_offset = ECC_OFFSET(mtd->writesize + ecc_off),
> > > + };
> > > + unsigned int max_bitflips = 0;
> > > + u32 ecc_stat;
> > > + int bf, ret, i;
> >
> > unsigned int i
>
> Strangely I'm used to always set my loop indexes as signed integers,
> but I'll happily change that everywhere in the driver before
> re-submitting.
It depends. Some of the upper bounds are signed, as dictated by some
field in a struct.
> > > +static int rzn1_nfc_probe(struct platform_device *pdev)
> > > +{
> > > + struct rzn1_nfc *nfc;
> > > + int irq, ret;
> > > +
> > > + nfc = devm_kzalloc(&pdev->dev, sizeof(*nfc), GFP_KERNEL);
> > > + if (!nfc)
> > > + return -ENOMEM;
> > > +
> > > + nfc->dev = &pdev->dev;
> > > + nand_controller_init(&nfc->controller);
> > > + nfc->controller.ops = &rzn1_nfc_ops;
> > > + INIT_LIST_HEAD(&nfc->chips);
> > > + init_completion(&nfc->complete);
> > > +
> > > + nfc->regs = devm_platform_ioremap_resource(pdev, 0);
> > > + if (IS_ERR(nfc->regs))
> > > + return PTR_ERR(nfc->regs);
> > > +
> > > + rzn1_nfc_dis_interrupts(nfc);
> > > + irq = platform_get_irq(pdev, 0);
> >
> > platform_get_irq_optional()
> >
> > > + if (irq < 0) {
> >
> > What if this is a real error, or -EPROBE_DEFER?
>
> If it's a real error I believe we should still fallback to polling? Or
> do you prefer to only use polling on a fixed condition?
It's debatable: in this case, you have the option to fallback to polling if
it is a real error, in other drivers you haven't. If it fails for real here,
it will probably fail for real in other drivers, too.
> > > + {} /* sentinel */
> > > +};
> > > +MODULE_DEVICE_TABLE(of, nfc_id_table);
> > > +
> > > +static struct platform_driver rzn1_nfc_driver = {
> > > + .driver = {
> > > + .name = "renesas-nfc",
> >
> > Perhaps s/nfc/nandc/ everywhere, to avoid confusion with the other nfc?
>
> There are many NAND controller drivers that are abbreviated with nfc
> because it's short and easy to write while still precise, but I have no
> issue rewording nfc into nandc if you prefer.
My main worry is that we ever get a "renesas-nfc" driver for Near
Field Communication. Both drivers will have the same name, which
will still work with DT (board files are dead), but the output from
dev_*() may be confusing.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
