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

[Miquel Raynal]

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