On 07/18/2016 02:52 AM, Brian Norris wrote:
Hi,
Hi,
Some trivial comments. I have some proposed diffs, and if y'all are OK
with them, I might apply this with some fixups.
[...]
+static int cqspi_wait_for_bit(void __iomem *reg, const u32 mask, bool clear)
+{
+ unsigned long end = jiffies + msecs_to_jiffies(CQSPI_TIMEOUT_MS);
+ u32 val;
+
+ while (1) {
+ val = readl(reg);
+ if (clear)
+ val = ~val;
+ val &= mask;
+
+ if (val == mask)
+ return 0;
+
+ if (time_after(jiffies, end))
+ return -ETIMEDOUT;
+ }
This could all be replaced with readl_poll_timeout(), couldn't it?
Oh nice, I didn't know we had generic implementation. Yes, let's use it.
Minor nit to it inline.
Like
the following diff (I won't apply this one now; if anything, maybe I'll
send a follow-up patch for review):
diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
index d403ba7b8f43..87586baaae9e 100644
--- a/drivers/mtd/spi-nor/cadence-quadspi.c
+++ b/drivers/mtd/spi-nor/cadence-quadspi.c
@@ -22,6 +22,7 @@
#include <linux/errno.h>
#include <linux/interrupt.h>
#include <linux/io.h>
+#include <linux/iopoll.h>
#include <linux/jiffies.h>
#include <linux/kernel.h>
#include <linux/module.h>
@@ -226,21 +227,11 @@ struct cqspi_st {
static int cqspi_wait_for_bit(void __iomem *reg, const u32 mask, bool clear)
{
- unsigned long end = jiffies + msecs_to_jiffies(CQSPI_TIMEOUT_MS);
u32 val;
- while (1) {
- val = readl(reg);
- if (clear)
- val = ~val;
- val &= mask;
-
- if (val == mask)
- return 0;
-
- if (time_after(jiffies, end))
- return -ETIMEDOUT;
- }
+ return readl_poll_timeout(reg, val,
+ (val & mask) == (clear ? ~mask : mask),
You probably mean "((clear ? ~val : val) & mask) == mask" here.
+ 0, CQSPI_TIMEOUT_MS * 1000);
}
[...]
+static int cqspi_setup_flash(struct cqspi_st *cqspi, struct device_node *np)
+{
+ struct platform_device *pdev = cqspi->pdev;
+ struct device *dev = &pdev->dev;
+ struct cqspi_flash_pdata *f_pdata;
+ struct spi_nor *nor;
+ struct mtd_info *mtd;
+ unsigned int cs;
+ int i, ret;
+
+ /* Get flash device data */
+ for_each_available_child_of_node(dev->of_node, np) {
+ if (of_property_read_u32(np, "reg", &cs)) {
+ dev_err(dev, "Couldn't determine chip select.\n");
+ goto err;
+ }
+
+ if (cs > CQSPI_MAX_CHIPSELECT) {
+ dev_err(dev, "Chip select %d out of range.\n", cs);
+ goto err;
+ }
+
+ f_pdata = &cqspi->f_pdata[cs];
+ f_pdata->cqspi = cqspi;
+ f_pdata->cs = cs;
+
+ ret = cqspi_of_get_flash_pdata(pdev, f_pdata, np);
+ if (ret)
+ goto err;
+
+ nor = &f_pdata->nor;
+ mtd = &nor->mtd;
+
+ mtd->priv = nor;
+
+ nor->dev = dev;
+ spi_nor_set_flash_node(nor, np);
+ nor->priv = f_pdata;
+
+ nor->read_reg = cqspi_read_reg;
+ nor->write_reg = cqspi_write_reg;
+ nor->read = cqspi_read;
+ nor->write = cqspi_write;
+ nor->erase = cqspi_erase;
+ nor->prepare = cqspi_prep;
+ nor->unprepare = cqspi_unprep;
+
+ mtd->name = kasprintf(GFP_KERNEL, "%s.%d", dev_name(dev), cs);
Might be easier to just use devm_kasprintf()?
Yes, I think that'd work.
+ if (!mtd->name) {
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD);
+ if (ret)
+ goto err;
+
+ ret = mtd_device_register(mtd, NULL, 0);
+ if (ret)
+ goto err;
+ }
+
+ return 0;
+
+err:
+ for (i = 0; i < CQSPI_MAX_CHIPSELECT; i++)
+ if (cqspi->f_pdata[i].nor.mtd.name) {
Chekcing for 'name' doens't really handle all error paths right. If
spi_nor_scan() or mtd_device_register() fails, then you'll be trying to
unregister an unregistered MTD.
Oh, snap. OK.
+ mtd_device_unregister(&cqspi->f_pdata[i].nor.mtd);
+ kfree(cqspi->f_pdata[i].nor.mtd.name);
+ }
+ return ret;
+}
+
+static int cqspi_probe(struct platform_device *pdev)
+{
+ struct device_node *np = pdev->dev.of_node;
+ struct device *dev = &pdev->dev;
+ struct cqspi_st *cqspi;
+ struct resource *res;
+ struct resource *res_ahb;
+ int ret;
+ int irq;
+
+ cqspi = devm_kzalloc(dev, sizeof(*cqspi), GFP_KERNEL);
+ if (!cqspi)
+ return -ENOMEM;
+
+ mutex_init(&cqspi->bus_mutex);
+ cqspi->pdev = pdev;
+ platform_set_drvdata(pdev, cqspi);
+
+ /* Obtain configuration from OF. */
+ ret = cqspi_of_get_pdata(pdev);
+ if (ret) {
+ dev_err(dev, "Cannot get mandatory OF data.\n");
+ return -ENODEV;
+ }
+
+ /* Obtain QSPI clock. */
+ cqspi->clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(cqspi->clk)) {
+ dev_err(dev, "Cannot claim QSPI clock.\n");
+ return PTR_ERR(cqspi->clk);
+ }
+
+ /* Obtain and remap controller address. */
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ cqspi->iobase = devm_ioremap_resource(dev, res);
+ if (IS_ERR(cqspi->iobase)) {
+ dev_err(dev, "Cannot remap controller address.\n");
+ return PTR_ERR(cqspi->iobase);
+ }
+
+ /* Obtain and remap AHB address. */
+ res_ahb = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ cqspi->ahb_base = devm_ioremap_resource(dev, res_ahb);
+ if (IS_ERR(cqspi->ahb_base)) {
+ dev_err(dev, "Cannot remap AHB address.\n");
+ return PTR_ERR(cqspi->ahb_base);
+ }
+
+ init_completion(&cqspi->transfer_complete);
+
+ /* Obtain IRQ line. */
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0) {
+ dev_err(dev, "Cannot obtain IRQ.\n");
+ return -ENXIO;
+ }
+
+ ret = clk_prepare_enable(cqspi->clk);
+ if (ret) {
+ dev_err(dev, "Cannot enable QSPI clock.\n");
+ return ret;
+ }
+
+ cqspi->master_ref_clk_hz = clk_get_rate(cqspi->clk);
+
+ ret = devm_request_irq(dev, irq, cqspi_irq_handler, 0,
+ pdev->name, cqspi);
+ if (ret) {
+ dev_err(dev, "Cannot request IRQ.\n");
+ goto probe_irq_failed;
+ }
+
+ cqspi_wait_idle(cqspi);
+ cqspi_controller_init(cqspi);
+ cqspi->current_cs = -1;
+ cqspi->sclk = 0;
+
+ ret = cqspi_setup_flash(cqspi, np);
+ if (ret) {
+ dev_err(dev, "Cadence QSPI NOR probe failed %d\n", ret);
+ goto probe_setup_failed;
+ }
+
+ return ret;
+probe_irq_failed:
+ cqspi_controller_enable(cqspi, 0);
+probe_setup_failed:
+ clk_disable_unprepare(cqspi->clk);
+ return ret;
+}
+
+static int cqspi_remove(struct platform_device *pdev)
+{
+ struct cqspi_st *cqspi = platform_get_drvdata(pdev);
+ int i;
+
+ cqspi_controller_enable(cqspi, 0);
I think you want to disable the controller *after* unregistering the
MTDs, no?
That's a bit of a misnomer, the second parameter is "bool enable", so
this disables the controller.
+
+ for (i = 0; i < CQSPI_MAX_CHIPSELECT; i++)
+ if (cqspi->f_pdata[i].nor.mtd.name) {
+ mtd_device_unregister(&cqspi->f_pdata[i].nor.mtd);
+ kfree(cqspi->f_pdata[i].nor.mtd.name);
+ }
+
+ clk_disable_unprepare(cqspi->clk);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int cqspi_suspend(struct device *dev)
+{
+ struct cqspi_st *cqspi = dev_get_drvdata(dev);
+
+ cqspi_controller_enable(cqspi, 0);
+ return 0;
+}
+
+static int cqspi_resume(struct device *dev)
+{
+ struct cqspi_st *cqspi = dev_get_drvdata(dev);
+
+ cqspi_controller_enable(cqspi, 1);
+ return 0;
+}
+
+static const struct dev_pm_ops cqspi__dev_pm_ops = {
+ .suspend = cqspi_suspend,
+ .resume = cqspi_resume,
+};
+
+#define CQSPI_DEV_PM_OPS (&cqspi__dev_pm_ops)
+#else
+#define CQSPI_DEV_PM_OPS NULL
+#endif
+
+static struct of_device_id const cqspi_dt_ids[] = {
+ {.compatible = "cdns,qspi-nor",},
+ { /* end of table */ }
+};
+
+MODULE_DEVICE_TABLE(of, cqspi_dt_ids);
+
+static struct platform_driver cqspi_platform_driver = {
+ .probe = cqspi_probe,
+ .remove = cqspi_remove,
+ .driver = {
+ .name = CQSPI_NAME,
+ .pm = CQSPI_DEV_PM_OPS,
+ .of_match_table = cqspi_dt_ids,
+ },
+};
+
+module_platform_driver(cqspi_platform_driver);
+
+MODULE_DESCRIPTION("Cadence QSPI Controller Driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" CQSPI_NAME);
+MODULE_AUTHOR("Ley Foon Tan <lftan@xxxxxxxxxx>");
+MODULE_AUTHOR("Graham Moore <grmoore@xxxxxxxxxxxxxxxxxxxxx>");
So, I'm thinking of applying this patch with the following diff:
diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
index 2fff31c0b9c3..d403ba7b8f43 100644
--- a/drivers/mtd/spi-nor/cadence-quadspi.c
+++ b/drivers/mtd/spi-nor/cadence-quadspi.c
@@ -53,6 +53,7 @@ struct cqspi_flash_pdata {
u8 addr_width;
u8 data_width;
u8 cs;
+ bool registered;
This feels a bit odd, but I don't have a better solution now.
};
struct cqspi_st {
@@ -1111,7 +1112,8 @@ static int cqspi_setup_flash(struct cqspi_st *cqspi, struct device_node *np)
nor->prepare = cqspi_prep;
nor->unprepare = cqspi_unprep;
- mtd->name = kasprintf(GFP_KERNEL, "%s.%d", dev_name(dev), cs);
+ mtd->name = devm_kasprintf(dev, GFP_KERNEL, "%s.%d",
+ dev_name(dev), cs);
if (!mtd->name) {
ret = -ENOMEM;
goto err;
@@ -1124,16 +1126,16 @@ static int cqspi_setup_flash(struct cqspi_st *cqspi, struct device_node *np)
ret = mtd_device_register(mtd, NULL, 0);
if (ret)
goto err;
+
+ f_pdata->registered = true;
}
return 0;
err:
for (i = 0; i < CQSPI_MAX_CHIPSELECT; i++)
- if (cqspi->f_pdata[i].nor.mtd.name) {
+ if (cqspi->f_pdata[i].registered)
mtd_device_unregister(&cqspi->f_pdata[i].nor.mtd);
- kfree(cqspi->f_pdata[i].nor.mtd.name);
- }
return ret;
}
@@ -1233,13 +1235,11 @@ static int cqspi_remove(struct platform_device *pdev)
struct cqspi_st *cqspi = platform_get_drvdata(pdev);
int i;
- cqspi_controller_enable(cqspi, 0);
-
for (i = 0; i < CQSPI_MAX_CHIPSELECT; i++)
- if (cqspi->f_pdata[i].nor.mtd.name) {
+ if (cqspi->f_pdata[i].registered)
mtd_device_unregister(&cqspi->f_pdata[i].nor.mtd);
- kfree(cqspi->f_pdata[i].nor.mtd.name);
- }
+
+ cqspi_controller_enable(cqspi, 0);
clk_disable_unprepare(cqspi->clk);
Thanks
--
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
