Messages in this thread | | | Date | Mon, 5 Jun 2023 10:22:08 +0200 | From | Miquel Raynal <> | Subject | Re: [PATCH v1 24/43] mtd: nand: add support for ts72xx |
| |
Hi andy.shevchenko@gmail.com,
andy.shevchenko@gmail.com 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@maquefel.me> > > + * > > + * derived: plat_nand.c > > Derived from: > > > + * Author: Vitaly Wool <vitalywool@gmail.com> > > + */ > > ... > > > +#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
| |