Hi Yifeng,
Johan Jonker <jbx6244@xxxxxxxxx> 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
