| Message ID | 20210610091154.4141911-1-yangyingliang@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: mdio: mscc-miim: Use devm_platform_get_and_ioremap_resource() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 2 maintainers not CCed: hkallweit1@gmail.com linux@armlinux.org.uk |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 34 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
> - dev->regs = devm_ioremap_resource(&pdev->dev, res); > + dev->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res); > if (IS_ERR(dev->regs)) { Here, only dev->regs is considered. > dev_err(&pdev->dev, "Unable to map MIIM registers\n"); > return PTR_ERR(dev->regs); > } > + dev->phy_regs = devm_platform_get_and_ioremap_resource(pdev, 1, &res); > + if (res && IS_ERR(dev->phy_regs)) { Here you look at both res and dev->phy_regs. This seems inconsistent. Can devm_platform_get_and_ioremap_resource() return success despite res being NULL? Andrew
Hi, On 2021/6/11 0:01, Andrew Lunn wrote: >> - dev->regs = devm_ioremap_resource(&pdev->dev, res); >> + dev->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res); >> if (IS_ERR(dev->regs)) { > Here, only dev->regs is considered. > >> dev_err(&pdev->dev, "Unable to map MIIM registers\n"); >> return PTR_ERR(dev->regs); >> } > > >> + dev->phy_regs = devm_platform_get_and_ioremap_resource(pdev, 1, &res); >> + if (res && IS_ERR(dev->phy_regs)) { > Here you look at both res and dev->phy_regs. > > This seems inconsistent. Can devm_platform_get_and_ioremap_resource() > return success despite res being NULL? No, if res is NULL, devm_platform_get_and_ioremap_resource() returns failed. But, before this patch, if the internal phy res is NULL, it doesn't return error code, so I checked the res to make sure it doesn't change the origin code logic. Thanks, Yang > > Andrew > .
On Fri, Jun 11, 2021 at 09:35:04AM +0800, Yang Yingliang wrote: > Hi, > > On 2021/6/11 0:01, Andrew Lunn wrote: > > > - dev->regs = devm_ioremap_resource(&pdev->dev, res); > > > + dev->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res); > > > if (IS_ERR(dev->regs)) { > > Here, only dev->regs is considered. > > > > > dev_err(&pdev->dev, "Unable to map MIIM registers\n"); > > > return PTR_ERR(dev->regs); > > > } > > > > > > > + dev->phy_regs = devm_platform_get_and_ioremap_resource(pdev, 1, &res); > > > + if (res && IS_ERR(dev->phy_regs)) { > > Here you look at both res and dev->phy_regs. > > > > This seems inconsistent. Can devm_platform_get_and_ioremap_resource() > > return success despite res being NULL? > No, if res is NULL, devm_platform_get_and_ioremap_resource() returns failed. > But, before this patch, if the internal phy res is NULL, it doesn't return > error > code, so I checked the res to make sure it doesn't change the origin code > logic. O.K, so IORESOURCE_MEM, 1 is optional. By making this change, i think you have made this less clear. So i would say it is O.K. to change the first platform_get_resource(pdev, IORESOURCE_MEM, 0) and devm_ioremap_resource(&pdev->dev, res) to one call, but i would leave the second pair alone. Andrew
diff --git a/drivers/net/mdio/mdio-mscc-miim.c b/drivers/net/mdio/mdio-mscc-miim.c index b36e5ea04ddf..b8491efbd330 100644 --- a/drivers/net/mdio/mdio-mscc-miim.c +++ b/drivers/net/mdio/mdio-mscc-miim.c @@ -139,10 +139,6 @@ static int mscc_miim_probe(struct platform_device *pdev) struct mscc_miim_dev *dev; int ret; - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - if (!res) - return -ENODEV; - bus = devm_mdiobus_alloc_size(&pdev->dev, sizeof(*dev)); if (!bus) return -ENOMEM; @@ -155,19 +151,16 @@ static int mscc_miim_probe(struct platform_device *pdev) bus->parent = &pdev->dev; dev = bus->priv; - dev->regs = devm_ioremap_resource(&pdev->dev, res); + dev->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res); if (IS_ERR(dev->regs)) { dev_err(&pdev->dev, "Unable to map MIIM registers\n"); return PTR_ERR(dev->regs); } - res = platform_get_resource(pdev, IORESOURCE_MEM, 1); - if (res) { - dev->phy_regs = devm_ioremap_resource(&pdev->dev, res); - if (IS_ERR(dev->phy_regs)) { - dev_err(&pdev->dev, "Unable to map internal phy registers\n"); - return PTR_ERR(dev->phy_regs); - } + dev->phy_regs = devm_platform_get_and_ioremap_resource(pdev, 1, &res); + if (res && IS_ERR(dev->phy_regs)) { + dev_err(&pdev->dev, "Unable to map internal phy registers\n"); + return PTR_ERR(dev->phy_regs); } ret = of_mdiobus_register(bus, pdev->dev.of_node);
Use devm_platform_get_and_ioremap_resource() to simplify code. Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> --- drivers/net/mdio/mdio-mscc-miim.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)