Re: [PATCH v3 7/8] nand: spi: Add generic SPI controller support

[Arnaud Mouiche <arnaud.mouiche@gmail.com>]

On 17/03/2017 15:20, Boris Brezillon wrote:
> [...]
> Should be done before calling spinand_detect(), just in case some DT
> props need to be parsed before the detection step.
>
> This being said, I'm not sure we should let the spinand controller
> driver do the registration step. I'm currently trying to rework the
> parallel NAND framework to act like all other busses, and I think SPI
> NAND controllers should also follow this road:
>
> 1/ spinand controller driver register a controller to the spinand core
>    (spinand_controller_register())
> 2/ the core parses the DT (or board info if DT is not available) and
>     creates N spinand devices (the N value depends on the number of SPI
>     nands connected to the controller)
> 3/ for each spinand device the detection/initialization takes place
>     directly in the core (the spinand_controller_ops should contain
>     anything required to do the detection)
> 4/ for each spinand dev the spinand_controller_ops->add() is called, to
>     let the controller driver attach private data to the device (if
>     required), and/or let it use it's own ECC engine (optional as well).
> 5/ underlying MTD device registered by spinand core code and spinand
>     dev added to the list of devices controlled by this controller (done
>     in the core)
>
> When removing a device, you just have the reverse steps:
>
> 1/ remove from list and unregister the MTD dev
> 2/ call spinand_controller_ops->remove()
> 3/ core/manufacturer cleanup
>
> Not sure how feasible this is, especially for the generic SPI NAND
> controller case where the SPI NAND controller does not have a node in
> the DT, but that would avoid all this boiler-plate duplication we have
> in the // NAND framework.

Since the probe for generic spi devices is generally triggered by the 
SPI layer, it will not match easily in the way you would like the 
registration done.
Can we let this registration question not be a showstopper for Peter's 
effort ?

Regards,
Arnaud
>> +
>> +	ret = mtd_device_register(mtd, NULL, 0);
>> +	if (!ret)
>> +		return 0;
>> +
>> +err5:
>> +	spinand_cleanup(chip);
>> +err4:
>> +	kfree(ecc_engine);
>> +err3:
>> +	kfree(controller);
>> +err2:
>> +	kfree(chip);
>> +err1:
>> +	return ret;
>> +}
>> +
>> +static int generic_spinand_remove(struct spi_device *spi)
>> +{
>> +	struct spinand_device *chip = spi_get_drvdata(spi);
>> +	struct mtd_info *mtd = spinand_to_mtd(chip);
>> +
>> +	mtd_device_unregister(mtd);
>> +	spinand_cleanup(chip);
>> +	kfree(chip->ecc.engine);
>> +	kfree(chip->controller);
>> +	kfree(chip);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct spi_driver generic_spinand_driver = {
>> +	.driver = {
>> +		.name	= "generic_spinand",
>> +		.owner	= THIS_MODULE,
>> +	},
>> +	.probe	= generic_spinand_probe,
>> +	.remove	= generic_spinand_remove,
>> +};
>> +module_spi_driver(generic_spinand_driver);
>> +
>> +MODULE_DESCRIPTION("Generic SPI controller to support SPI NAND");
>> +MODULE_AUTHOR("Peter Pan<peterpandong@micron.com>");
>> +MODULE_LICENSE("GPL v2");