Re: [PATCH v4 8/9] nand: spi: Add generic SPI controller support

Marek Vasut
2017-03-23T14:17:11Z

On 03/23/2017 10:43 AM, Peter Pan wrote:
> This commit supports to use generic spi controller
> as spi nand controller.
>
> Signed-off-by: Peter Pan <peterpandong@micron.com>
[...]
> +static int gen_spi_spinand_probe(struct spi_device *spi)
> +{
> + struct spinand_device *chip;
> + struct gen_spi_spinand_controller *controller;
> + struct spinand_controller *spinand_controller;
> + int ret;
> + u32 max_speed_hz = spi->max_speed_hz;
> +
> + chip = spinand_alloc(&spi->dev);
> + if (IS_ERR(chip)) {
> + ret = PTR_ERR(chip);
> + goto err1;
> + }
> + controller = kzalloc(sizeof(*controller), GFP_KERNEL);
> + if (!controller) {
> + ret = -ENOMEM;
> + goto err2;
> + }
> + controller->spi = spi;
> + spinand_controller = &controller->ctrl;
> + spinand_controller->ops = &gen_spi_spinand_ops;
> + spinand_controller->caps = SPINAND_CAP_RD_X1 | SPINAND_CAP_WR_X1;
> + if (spi->mode & SPI_RX_QUAD)
> + spinand_controller->caps |= SPINAND_CAP_RD_QUAD |
> + SPINAND_CAP_RD_X4;
> + if (spi->mode & SPI_RX_DUAL)
> + spinand_controller->caps |= SPINAND_CAP_RD_DUAL |
> + SPINAND_CAP_RD_X2;
> + if (spi->mode & SPI_TX_QUAD)
> + spinand_controller->caps |= SPINAND_CAP_WR_QUAD |
> + SPINAND_CAP_WR_X4;
> + if (spi->mode & SPI_TX_DUAL)
> + spinand_controller->caps |= SPINAND_CAP_WR_DUAL |
> + SPINAND_CAP_WR_X2;
> + chip->controller.controller = spinand_controller;
> + spi_set_drvdata(spi, chip);
> + spi->max_speed_hz = min_t(int, 25000000, max_speed_hz);

Please avoid hard-coding random numbers, make the 25 MHz into a macro
and add a comment explaining why you use 25 MHz

> +
> + ret = spinand_register(chip);
> + if (ret)
> + goto err3;
> +
> + spi->max_speed_hz = max_speed_hz;
> +
> + return 0;
> +
> +err3:
> + kfree(controller);
> +err2:
> + spinand_free(chip);
> +err1:
> + return ret;
> +}
> +
> +static int gen_spi_spinand_remove(struct spi_device *spi)
> +{
> + struct spinand_device *chip = spi_get_drvdata(spi);
> +
> + spinand_unregister(chip);
> + kfree(to_gen_spi_spinand_controller(chip->controller.controller));

This looks pretty awful, introduce a variable maybe ?

> + spinand_free(chip);
> +
> + return 0;
> +}
> +
> +static struct spi_driver gen_spi_spinand_driver = {
> + .driver = {
> + .name = "generic_spinand",
> + .owner = THIS_MODULE,
> + },
> + .probe = gen_spi_spinand_probe,
> + .remove = gen_spi_spinand_remove,
> +};
> +module_spi_driver(gen_spi_spinand_driver);
> +
> +MODULE_DESCRIPTION("Generic SPI controller to support SPI NAND");
> +MODULE_AUTHOR("Peter Pan<peterpandong@micron.com>");
> +MODULE_LICENSE("GPL v2");
>

--
Best regards,
Marek Vasut