| Message ID | 1359473048-26551-5-git-send-email-florian@openwrt.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Dear Florian Fainelli, On Tue, 29 Jan 2013 16:24:07 +0100, Florian Fainelli wrote: > This patch changes the Marvell MDIO driver to be registered by using > both Device Tree and platform device methods. The driver voluntarily > does not use devm_ioremap() to share the same error path for Device Tree > and non-Device Tree cases. Not sure why you think devm_ioremap() can't be used here. Maybe I'm missing something, but could you explain? If you use devm_ioremap(), then basically you don't need to do anything in the error path regarding to the I/O mapping... since it's the whole purpose of the devm_*() stuff to automagically undo things in the error case, and in the ->remove() code. > - dev->err_interrupt = irq_of_parse_and_map(pdev->dev.of_node, 0); > + if (pdev->dev.of_node) { > + dev->regs = of_iomap(pdev->dev.of_node, 0); > + if (!dev->regs) { > + dev_err(&pdev->dev, "No SMI register address given in DT\n"); > + ret = -ENODEV; > + goto out_free; > + } > + > + dev->err_interrupt = irq_of_parse_and_map(pdev->dev.of_node, 0); > + } else { > + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + > + dev->regs = ioremap(r->start, resource_size(r)); > + if (!dev->regs) { > + dev_err(&pdev->dev, "No SMI register address given\n"); > + ret = -ENODEV; > + goto out_free; > + } > + > + dev->err_interrupt = platform_get_irq(pdev, 0); > + } I think you can do a devm_ioremap() and a platform_get_irq() in both cases here, and therefore keep the code common between the DT case and the !DT case. Thanks, Thomas
On Tue, Jan 29, 2013 at 04:24:07PM +0100, Florian Fainelli wrote: > - dev->err_interrupt = irq_of_parse_and_map(pdev->dev.of_node, 0); > + if (pdev->dev.of_node) { > + dev->regs = of_iomap(pdev->dev.of_node, 0); > + if (!dev->regs) { > + dev_err(&pdev->dev, "No SMI register address given in DT\n"); > + ret = -ENODEV; > + goto out_free; > + } > + > + dev->err_interrupt = irq_of_parse_and_map(pdev->dev.of_node, 0); > + } else { > + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + > + dev->regs = ioremap(r->start, resource_size(r)); > + if (!dev->regs) { > + dev_err(&pdev->dev, "No SMI register address given\n"); > + ret = -ENODEV; > + goto out_free; > + } > + > + dev->err_interrupt = platform_get_irq(pdev, 0); > + } Why do you have these different paths for OF and platform? AFAIK these days when a OF device is automatically converted into a platform device all the struct resources are created too, so you can't you just use platform_get_resource and devm_request_and_ioremap for both flows? Ditto for the interrupt - platform_get_irq should work in both cases? Jason
Le mardi 29 janvier 2013 18:59:12, Jason Gunthorpe a écrit : > On Tue, Jan 29, 2013 at 04:24:07PM +0100, Florian Fainelli wrote: > > - dev->err_interrupt = irq_of_parse_and_map(pdev->dev.of_node, 0); > > + if (pdev->dev.of_node) { > > + dev->regs = of_iomap(pdev->dev.of_node, 0); > > + if (!dev->regs) { > > + dev_err(&pdev->dev, "No SMI register address given in DT\n"); > > + ret = -ENODEV; > > + goto out_free; > > + } > > + > > + dev->err_interrupt = irq_of_parse_and_map(pdev->dev.of_node, 0); > > + } else { > > + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + > > + dev->regs = ioremap(r->start, resource_size(r)); > > + if (!dev->regs) { > > + dev_err(&pdev->dev, "No SMI register address given\n"); > > + ret = -ENODEV; > > + goto out_free; > > + } > > + > > + dev->err_interrupt = platform_get_irq(pdev, 0); > > + } > > Why do you have these different paths for OF and platform? AFAIK these > days when a OF device is automatically converted into a platform > device all the struct resources are created too, so you can't you just > use platform_get_resource and devm_request_and_ioremap for both flows? > > Ditto for the interrupt - platform_get_irq should work in both cases? There was no particular reason and I updated the patchset to do that precisely in version 2.
diff --git a/drivers/net/ethernet/marvell/mvmdio.c b/drivers/net/ethernet/marvell/mvmdio.c index cada794..10c593c 100644 --- a/drivers/net/ethernet/marvell/mvmdio.c +++ b/drivers/net/ethernet/marvell/mvmdio.c @@ -190,6 +190,7 @@ static irqreturn_t orion_mdio_err_irq(int irq, void *dev_id) static int orion_mdio_probe(struct platform_device *pdev) { struct device_node *np = pdev->dev.of_node; + struct resource *r = NULL; struct mii_bus *bus; struct orion_mdio_dev *dev; int i, ret; @@ -219,18 +220,31 @@ static int orion_mdio_probe(struct platform_device *pdev) bus->irq[i] = PHY_POLL; dev = bus->priv; - dev->regs = of_iomap(pdev->dev.of_node, 0); - if (!dev->regs) { - dev_err(&pdev->dev, "No SMI register address given in DT\n"); - kfree(bus->irq); - mdiobus_free(bus); - return -ENODEV; - } - dev->err_interrupt = NO_IRQ; init_waitqueue_head(&dev->smi_busy_wait); - dev->err_interrupt = irq_of_parse_and_map(pdev->dev.of_node, 0); + if (pdev->dev.of_node) { + dev->regs = of_iomap(pdev->dev.of_node, 0); + if (!dev->regs) { + dev_err(&pdev->dev, "No SMI register address given in DT\n"); + ret = -ENODEV; + goto out_free; + } + + dev->err_interrupt = irq_of_parse_and_map(pdev->dev.of_node, 0); + } else { + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); + + dev->regs = ioremap(r->start, resource_size(r)); + if (!dev->regs) { + dev_err(&pdev->dev, "No SMI register address given\n"); + ret = -ENODEV; + goto out_free; + } + + dev->err_interrupt = platform_get_irq(pdev, 0); + } + if (dev->err_interrupt != NO_IRQ) { ret = devm_request_irq(&pdev->dev, dev->err_interrupt, orion_mdio_err_irq, @@ -242,18 +256,24 @@ static int orion_mdio_probe(struct platform_device *pdev) mutex_init(&dev->lock); - ret = of_mdiobus_register(bus, np); + if (np) + ret = of_mdiobus_register(bus, np); + else + ret = mdiobus_register(bus); if (ret < 0) { dev_err(&pdev->dev, "Cannot register MDIO bus (%d)\n", ret); iounmap(dev->regs); - kfree(bus->irq); - mdiobus_free(bus); - return ret; + goto out_free; } platform_set_drvdata(pdev, bus); return 0; + +out_free: + kfree(bus->irq); + mdiobus_free(bus); + return ret; } static int orion_mdio_remove(struct platform_device *pdev)
This patch changes the Marvell MDIO driver to be registered by using both Device Tree and platform device methods. The driver voluntarily does not use devm_ioremap() to share the same error path for Device Tree and non-Device Tree cases. Signed-off-by: Florian Fainelli <florian@openwrt.org> --- drivers/net/ethernet/marvell/mvmdio.c | 46 +++++++++++++++++++++++---------- 1 file changed, 33 insertions(+), 13 deletions(-)