On Thu, Jul 20, 2023 at 02:29:24PM +0300, Nikita Shubin via B4 Relay wrote:
From: Nikita Shubin nikita.shubin@maquefel.me
Technologic Systems has it's own nand controller implementation in CPLD.
...
+ bits.h
+#include <linux/err.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/slab.h>
...
+static int ts72xx_nand_attach_chip(struct nand_chip *chip) +{
- switch (chip->ecc.engine_type) {
- case NAND_ECC_ENGINE_TYPE_SOFT:
if (chip->ecc.algo == NAND_ECC_ALGO_UNKNOWN)chip->ecc.algo = NAND_ECC_ALGO_HAMMING;break;- case NAND_ECC_ENGINE_TYPE_ON_HOST:
return -EINVAL;- default:
break;
Here it will return 0, is it a problem?
- }
- return 0;
+}
...
+static int ts72xx_nand_probe(struct platform_device *pdev) +{
- struct ts72xx_nand_data *data;
- struct device_node *child;
- struct mtd_info *mtd;
- int err;
- /* Allocate memory for the device structure (and zero it) */
Useless comment.
- data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
- if (!data)
return -ENOMEM;- data->controller.ops = &ts72xx_nand_ops;
- nand_controller_init(&data->controller);
- data->chip.controller = &data->controller;
- data->io_base = devm_platform_ioremap_resource(pdev, 0);
- if (IS_ERR(data->io_base))
return PTR_ERR(data->io_base);- child = of_get_next_child(pdev->dev.of_node, NULL);
Why not using device property API from day 1?
fwnode_get_next_child_node()
- if (!child)
return dev_err_probe(&pdev->dev, -ENXIO,"ts72xx controller node should have exactly one child\n");
From now on you leak the reference count in error path.
- nand_set_flash_node(&data->chip, child);
- mtd = nand_to_mtd(&data->chip);
- mtd->dev.parent = &pdev->dev;
- data->chip.legacy.IO_ADDR_R = data->io_base;
- data->chip.legacy.IO_ADDR_W = data->io_base;
- data->chip.legacy.cmd_ctrl = ts72xx_nand_hwcontrol;
- data->chip.legacy.dev_ready = ts72xx_nand_device_ready;
- platform_set_drvdata(pdev, data);
- /*
* This driver assumes that the default ECC engine should be TYPE_SOFT.* Set ->engine_type before registering the NAND devices in order to* provide a driver specific default value.*/- data->chip.ecc.engine_type = NAND_ECC_ENGINE_TYPE_SOFT;
- /* Scan to find existence of the device */
- err = nand_scan(&data->chip, 1);
- if (err)
return err;- err = mtd_device_parse_register(mtd, NULL, NULL, NULL, 0);
- if (err) {
nand_cleanup(&data->chip);
return err;- }
- return 0;
These 4 lines can be simply
return err;
but see above.
+}
...
+static void ts72xx_nand_remove(struct platform_device *pdev) +{
- struct ts72xx_nand_data *data = platform_get_drvdata(pdev);
- struct nand_chip *chip = &data->chip;
- int ret;
- ret = mtd_device_unregister(nand_to_mtd(chip));
- WARN_ON(ret);
Why?! Is it like this in other MTD drivers?
- nand_cleanup(chip);
+}