Re: [PATCH v5 2/7] mtd: rawnand: rockchip: NFC drivers for RK3308, RK3188 and others

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Yifeng,

Johan Jonker <jbx6244@gmail.com> wrote on Wed, 29 Apr 2020 17:55:56
+0200:

> Hi Yifeng,
> 
> A few more comments below for now (part 2).
> 
> On 4/26/20 12:02 PM, Yifeng Zhao wrote:
> 
> [..]
> 
> > +#define	THIS_NAME		"rk-nand"  
> 
> > +static int rk_nfc_nand_chip_init(struct device *dev, struct rk_nfc *nfc,
> > +				 struct device_node *np)
> > +{
> > +	struct rk_nfc_nand_chip *nand;
> > +	struct nand_chip *chip;
> > +	struct mtd_info *mtd;
> > +	int nsels;
> > +	u32 tmp;
> > +	int ret;
> > +	int i;
> > +
> > +	if (!of_get_property(np, "reg", &nsels))
> > +		return -ENODEV;
> > +	nsels /= sizeof(u32);
> > +	if (!nsels || nsels > NFC_MAX_NSELS) {
> > +		dev_err(dev, "invalid reg property size %d\n", nsels);
> > +		return -EINVAL;
> > +	}
> > +
> > +	nand = devm_kzalloc(dev, sizeof(*nand) + nsels * sizeof(u8),
> > +			    GFP_KERNEL);
> > +	if (!nand)
> > +		return -ENOMEM;
> > +
> > +	nand->nsels = nsels;
> > +	for (i = 0; i < nsels; i++) {
> > +		ret = of_property_read_u32_index(np, "reg", i, &tmp);
> > +		if (ret) {
> > +			dev_err(dev, "reg property failure : %d\n", ret);
> > +			return ret;
> > +		}
> > +
> > +		if (tmp >= NFC_MAX_NSELS) {
> > +			dev_err(dev, "invalid CS: %u\n", tmp);
> > +			return -EINVAL;
> > +		}
> > +
> > +		if (test_and_set_bit(tmp, &nfc->assigned_cs)) {
> > +			dev_err(dev, "CS %u already assigned\n", tmp);
> > +			return -EINVAL;
> > +		}
> > +
> > +		nand->sels[i] = tmp;
> > +	}
> > +
> > +	chip = &nand->chip;
> > +	chip->controller = &nfc->controller;
> > +
> > +	nand_set_flash_node(chip, np);
> > +	nand_set_controller_data(chip, nfc);
> > +
> > +	chip->options |= NAND_USE_BOUNCE_BUFFER | NAND_NO_SUBPAGE_WRITE;
> > +	chip->bbt_options = NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
> > +
> > +	/* set default mode in case dt entry is missing */
> > +	chip->ecc.mode = NAND_ECC_HW;
> > +
> > +	mtd = nand_to_mtd(chip);
> > +	mtd->owner = THIS_MODULE;
> > +	mtd->dev.parent = dev;  
> 
> > +	mtd->name = THIS_NAME;  
> 
> The 'mtd->name' shows up somewhere in file tree.

Good catch.

> The rk3288 has 2 nfc's. In theory 2 probes and also 2 device names, so I
> think that we shouldn't use a fixed define for 'mtd->name'.
> Maybe use something like this:

Yifeng, please use the NAND chip "label" DT property, which is parsed
by the core automatically and will give you meaningful names for every
chip:

nand_set_flash_node(chip, np);
if (!mtd->name) {
	dev_err(nfc->dev, "NAND label property is mandatory\n");
	return -EINVAL;
}


Thanks,
Miquèl