Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 5 Jun 2023 10:22:08 +0200
From:   Miquel Raynal <miquel.raynal@bootlin.com>
To:     andy.shevchenko@gmail.com
Cc:     Nikita Shubin <nikita.shubin@maquefel.me>,
        Alexander Sverdlin <alexander.sverdlin@gmail.com>,
        Arnd Bergmann <arnd@arndb.de>,
        Linus Walleij <linus.walleij@linaro.org>,
        Richard Weinberger <richard@nod.at>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Jonathan Cameron <Jonathan.Cameron@huawei.com>,
        Stephen Boyd <sboyd@kernel.org>,
        Christophe Kerello <christophe.kerello@foss.st.com>,
        Jean Delvare <jdelvare@suse.de>,
        Liang Yang <liang.yang@amlogic.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Michael Peters <mpeters@embeddedts.com>,
        Kris Bahnsen <kris@embeddedts.com>,
        linux-kernel@vger.kernel.org, linux-mtd@lists.infradead.org
Subject: Re: [PATCH v1 24/43] mtd: nand: add support for ts72xx
Message-ID: <20230605102208.4e6d26f3@xps-13>
In-Reply-To: <ZHugqa697eGqHnr4@surfacebook>
References: <20230424123522.18302-1-nikita.shubin@maquefel.me>
        <20230601054549.10843-6-nikita.shubin@maquefel.me>
        <ZHugqa697eGqHnr4@surfacebook>
Organization: Bootlin
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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

Yeah, I made the same comment.

> To me it looks like
>=20
> 	if (err) {
> 		nand_cleanup(&data->chip);
> 		return err;
> 	}
>=20
> 	return 0;
>=20
> has to be here.
>=20
> > +} =20
>=20
> ...
>=20
> > +	ret =3D mtd_device_unregister(nand_to_mtd(chip));
> > +	WARN_ON(ret); =20
>=20
> 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.

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


Thanks,
Miqu=C3=A8l