[PATCH v6 10/15] nand: spi: add basic blocks for infrastructure

[Peter Pan 潘栋 (peterpandong)]

Hi Boris,

> On 30 May 2017, at 05:51, Boris Brezillon <boris.brezillon at free-electrons.com> wrote:
> 
> On Wed, 24 May 2017 15:07:06 +0800
> Peter Pan <peterpandong at micron.com> wrote:
> 
>> This is the first commit for spi nand framkework.
> 
>                    ^ framework
> 
>> This commit is to add add basic building blocks
> 
>          is adding basic ...
> 
>> for the SPI NAND infrastructure.
>> 
>> Signed-off-by: Peter Pan <peterpandong at micron.com>
>> ---
> 
> [...]
> 
>> +
>> +/**
>> + * devm_spinand_alloc - [SPI NAND Interface] allocate SPI NAND device instance
> 
> Let's drop those [SPI NAND Interface] specifier. It's pretty obvious
> that this is part of the spi-nand API, since those symbols are exported.

Okay 

> 
>> + * @dev: pointer to device model structure
>> + */
>> +struct spinand_device *devm_spinand_alloc(struct device *dev)
> 
> You can pass a pointer to the nand_controller object driving the
> nand_device here.
> 
>> +{
>> +    struct spinand_device *spinand;
>> +    struct mtd_info *mtd;
>> +
>> +    spinand = devm_kzalloc(dev, sizeof(*spinand), GFP_KERNEL);
>> +    if (!spinand)
>> +        return ERR_PTR(-ENOMEM);
>> +
>> +    spinand_set_of_node(spinand, dev->of_node);
>> +    mutex_init(&spinand->lock);
>> +    spinand->dev = dev;
> 
> Hm, I don't think this is correct. The device here is likely to
> represent the controller not the SPI NAND device. For the generic spi
> nand controller, I agree, it's the same, but, in case you have one
> controller that is attached several spi devices, it's not.

Yes, you are right. I only thought about generic spi bus when I was
writing the code.

> 
> How about putting the struct device pointer in nand_controller and then
> pass the controller to this spinand_alloc() function.

Good solution

> 
>> +    mtd = spinand_to_mtd(spinand);
>> +    mtd->dev.parent = dev;
>> +
>> +    return spinand;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_spinand_alloc);
>> +
>> +/**
>> + * spinand_init - [SPI NAND Interface] initialize the SPI NAND device
>> + * @spinand: SPI NAND device structure
>> + */
>> +int spinand_init(struct spinand_device *spinand)
>> +{
>> +    struct mtd_info *mtd = spinand_to_mtd(spinand);
>> +    struct nand_device *nand = mtd_to_nand(mtd);
>> +    int ret;
>> +
>> +    ret = spinand_detect(spinand);
>> +    if (ret) {
>> +        dev_err(spinand->dev,
>> +            "Detect SPI NAND failed with error %d.\n", ret);
>> +        goto err_out;
> 
>        return ret;
> 
> 
>> +    }
> 
> I'd still prefer to move the detection step out of this _init()
> function even if this implies duplicating the _detect()+_init()
> sequence in all drivers. Maybe you can provide a wrapper called
> spinand_detect_and_init() to do both in one go.

Okay for me 
> 
>> +
>> +    spinand_set_rd_wr_op(spinand);
>> +
>> +    /*
>> +     * Use kzalloc() instead of devm_kzalloc() here, beacause some drivers
>> +     * may use this buffer for DMA access.
>> +     * Memory allocated by devm_ does not guarantee DMA-safe alignment.
>> +     */
>> +    spinand->buf = kzalloc(nand_page_size(nand) +
>> +                   nand_per_page_oobsize(nand),
>> +                   GFP_KERNEL);
>> +    if (!spinand->buf) {
>> +        ret = -ENOMEM;
>> +        goto err_out;
> 
>        return -ENOMEM;
> 
>> +    }
>> +
>> +    spinand->oobbuf = spinand->buf + nand_page_size(nand);
>> +
>> +    ret = spinand_manufacturer_init(spinand);
>> +    if (ret) {
>> +        dev_err(spinand->dev,
>> +            "Manufacurer init SPI NAND failed with err %d.\n",
>> +            ret);
>> +        goto err_free_buf;
>> +    }
>> +
>> +    mtd->name = spinand->name;
>> +    mtd->size = nand_size(nand);
>> +    mtd->erasesize = nand_eraseblock_size(nand);
>> +    mtd->writesize = nand_page_size(nand);
>> +    mtd->writebufsize = mtd->writesize;
>> +    mtd->owner = THIS_MODULE;
>> +    mtd->type = MTD_NANDFLASH;
>> +    mtd->flags = MTD_CAP_NANDFLASH;
>> +    mtd->oobsize = nand_per_page_oobsize(nand);
>> +    /*
>> +     * Right now, we don't support ECC, so let the whole oob
>> +     * area is available for user.
>> +     */
>> +    mtd->oobavail = mtd->oobsize;
>> +
>> +    /* After power up, all blocks are locked, so unlock it here. */
>> +    spinand_lock_block(spinand, BL_ALL_UNLOCKED);
>> +
>> +    return 0;
>> +
>> +err_free_buf:
>> +    kfree(spinand->buf);
>> +err_out:
> 
> You can get rid of err_out.
> 

Okay

Thanks 
Peter Pan 
>> +    return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(spinand_init);
>> +
>