Re: [PATCH v1 24/43] mtd: nand: add support for ts72xx

From: Miquel Raynal
Date: Mon Jun 05 2023 - 04:22:26 EST


Hi andy.shevchenko@xxxxxxxxx,

andy.shevchenko@xxxxxxxxx wrote on Sat, 3 Jun 2023 23:20:57 +0300:

> Thu, Jun 01, 2023 at 08:45:29AM +0300, Nikita Shubin kirjoitti:
> > Technologic Systems has it's own nand controller implementation in CPLD.
> >
> > This patch adds support for TS-72XX boards family.
>
> Use imperative mode, this is documented in the Submitting Patches,
>
> ...
>
> > +/*
> > + * Technologic Systems TS72xx NAND controller driver
> > + *
> > + * Copyright (C) 2023 Nikita Shubin <nikita.shubin@xxxxxxxxxxx>
> > + *
> > + * derived: plat_nand.c
>
> Derived from:
>
> > + * Author: Vitaly Wool <vitalywool@xxxxxxxxx>
> > + */
>
> ...
>
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
>
> + Blank line?
>
> > +#include <linux/mtd/mtd.h>
> > +#include <linux/mtd/platnand.h>
>
> ...
>
> > + bits = readb(addr) & ~0x07;
>
> GENMASK()?
>
> ...
>
> > + addr += (1 << TS72XX_NAND_BUSY_ADDR_LINE);
>
> BIT() ?
>
> ...
>
> > + return !!(readb(addr) & 0x20);
>
> BIT() ?
>
> ...
>
> > + struct ts72xx_nand_data *data;
> > + struct mtd_info *mtd;
> > + int err = 0;
>
> Redundant assignment.
>
> > + /* Allocate memory for the device structure (and zero it) */
> > + data = devm_kzalloc(&pdev->dev, sizeof(struct ts72xx_nand_data),
>
> sizeof(*data) and make it a single line.
>
> > + GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
>
> ...
>
> > + nand_set_flash_node(&data->chip, pdev->dev.of_node);
>
> Hmm... wondering why this uses OF node instead of fwnode... But okay, this is
> question to the subsystem maintaners.
>
>
> > + err = mtd_device_parse_register(mtd, NULL, NULL,
> > + NULL, 0);
>
> There is plenty of space on the previous line.
>
> > +
>
> Redundant blank line.
>
> > + if (!err)
> > + return err;
> > +
> > + nand_cleanup(&data->chip);
> > +
> > + return 0;
>
> This seems at least weird and rather broken.

Yeah, I made the same comment.

> To me it looks like
>
> if (err) {
> nand_cleanup(&data->chip);
> return err;
> }
>
> return 0;
>
> has to be here.
>
> > +}
>
> ...
>
> > + ret = mtd_device_unregister(nand_to_mtd(chip));
> > + WARN_ON(ret);
>
> WARN_ON()?! Why?

This is actually something that is expected for now, the device
unregistration should not fail and the return value should not be used
to skip other cleanups. I cannot find the original discussion anymore
but we decided to use that construction. We might actually switch that
one to void someday.

>
> > + nand_cleanup(chip);
> > +}
>


Thanks,
Miquèl