Re: [PATCHv2 net-next] net: modernize ioremap in probe
From: Sergey Shtylyov <hidden>
Date: 2024-11-17 17:14:21
Also in:
linux-can, linux-renesas-soc, lkml
On 11/11/24 11:02 PM, Rosen Penev wrote:
I changed resource acquisition to be performed in a single step. Possible because devm is used here.
Have you tested it? Asking because switching to devm_platform_ioremap_resource_byname() and devm_platform_get_and_ioremap_resource() seems to add devm_request_mem_region() call into the picture... I'm also not sure the single patch per drivers/net/ would be enough, but that's for the maintainers to decide...
Signed-off-by: Rosen Penev <redacted>
[...]
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/dsa/hirschmann/hellcreek.c b/drivers/net/dsa/hirschmann/hellcreek.c index 283ec5a6e23c..940c4fa6a924 100644 --- a/drivers/net/dsa/hirschmann/hellcreek.c +++ b/drivers/net/dsa/hirschmann/hellcreek.c
[...]
quoted hunk ↗ jump to hunk
@@ -1982,23 +1981,12 @@ static int hellcreek_probe(struct platform_device *pdev) hellcreek->dev = dev; - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "tsn"); - if (!res) { - dev_err(dev, "No memory region provided!\n"); - return -ENODEV; - } - - hellcreek->base = devm_ioremap_resource(dev, res); + hellcreek->base = devm_platform_ioremap_resource_byname(pdev, "tsn");
The new code here should behave equivalently to the old, so seems OK.
if (IS_ERR(hellcreek->base))
return PTR_ERR(hellcreek->base);
- res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ptp");
- if (!res) {
- dev_err(dev, "No PTP memory region provided!\n");
- return -ENODEV;
- }
-
- hellcreek->ptp_base = devm_ioremap_resource(dev, res);
+ hellcreek->ptp_base =
+ devm_platform_ioremap_resource_byname(pdev, "ptp");Here as well... [...]
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c index 571631a30320..faf853edc0db 100644 --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c@@ -7425,21 +7425,17 @@ static int mvpp2_init(struct platform_device *pdev, struct mvpp2 *priv) static int mvpp2_get_sram(struct platform_device *pdev, struct mvpp2 *priv) { - struct resource *res; void __iomem *base; - res = platform_get_resource(pdev, IORESOURCE_MEM, 2); - if (!res) { + base = devm_platform_ioremap_resource(pdev, 2); + if (IS_ERR(base)) { if (has_acpi_companion(&pdev->dev)) dev_warn(&pdev->dev, "ACPI is too old, Flow control not supported\n"); else - dev_warn(&pdev->dev, "DT is too old, Flow control not supported\n"); - return 0; - } - - base = devm_ioremap_resource(&pdev->dev, res); - if (IS_ERR(base)) + dev_warn(&pdev->dev, + "DT is too old, Flow control not supported\n"); return PTR_ERR(base); + } priv->cm3_base = base; return 0;
This change also seems to look sane... [...]
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c index 8d18dae4d8fb..8ef52fc46a01 100644 --- a/drivers/net/ethernet/renesas/rswitch.c +++ b/drivers/net/ethernet/renesas/rswitch.c
[...]
quoted hunk ↗ jump to hunk
@@ -2074,7 +2067,7 @@ static int renesas_eth_sw_probe(struct platform_device *pdev) platform_set_drvdata(pdev, priv); priv->pdev = pdev; - priv->addr = devm_ioremap_resource(&pdev->dev, res); + priv->addr = devm_platform_ioremap_resource_byname(pdev, "secure_base"); if (IS_ERR(priv->addr)) return PTR_ERR(priv->addr);
This one looks OKish too...
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/renesas/rtsn.c b/drivers/net/ethernet/renesas/rtsn.c index 6b3f7fca8d15..bfe08facc707 100644 --- a/drivers/net/ethernet/renesas/rtsn.c +++ b/drivers/net/ethernet/renesas/rtsn.c@@ -1297,14 +1297,8 @@ static int rtsn_probe(struct platform_device *pdev) ndev->netdev_ops = &rtsn_netdev_ops; ndev->ethtool_ops = &rtsn_ethtool_ops; - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "gptp"); - if (!res) { - dev_err(&pdev->dev, "Can't find gptp resource\n"); - ret = -EINVAL; - goto error_free; - } - - priv->ptp_priv->addr = devm_ioremap_resource(&pdev->dev, res); + priv->ptp_priv->addr = + devm_platform_ioremap_resource_byname(pdev, "gptp"); if (IS_ERR(priv->ptp_priv->addr)) { ret = PTR_ERR(priv->ptp_priv->addr); goto error_free;
Looks OKish too...
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c index d072f394eecb..07d1f1504a97 100644 --- a/drivers/net/ethernet/renesas/sh_eth.c +++ b/drivers/net/ethernet/renesas/sh_eth.c@@ -3351,31 +3351,12 @@ static int sh_eth_drv_probe(struct platform_device *pdev) if (mdp->cd->tsu) { int port = pdev->id < 0 ? 0 : pdev->id % 2; - struct resource *rtsu; - rtsu = platform_get_resource(pdev, IORESOURCE_MEM, 1); - if (!rtsu) { - dev_err(&pdev->dev, "no TSU resource\n"); - ret = -ENODEV; - goto out_release; - } - /* We can only request the TSU region for the first port - * of the two sharing this TSU for the probe to succeed... - */ - if (port == 0 && - !devm_request_mem_region(&pdev->dev, rtsu->start, - resource_size(rtsu), - dev_name(&pdev->dev))) { - dev_err(&pdev->dev, "can't request TSU resource.\n"); - ret = -EBUSY; - goto out_release; - } /* ioremap the TSU registers */ - mdp->tsu_addr = devm_ioremap(&pdev->dev, rtsu->start, - resource_size(rtsu)); - if (!mdp->tsu_addr) { + mdp->tsu_addr = devm_platform_ioremap_resource(pdev, 1); + if (IS_ERR(mdp->tsu_addr)) { dev_err(&pdev->dev, "TSU region ioremap() failed.\n"); - ret = -ENOMEM; + ret = PTR_ERR(mdp->tsu_addr); goto out_release; } mdp->port = port;
No, this one won't fly since you're removing the port == 0 check... :-/ This code looks so strange on purpose... :-) [...]
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c index dd3ed2d6430b..725e5c13d212 100644 --- a/drivers/net/mdio/mdio-ipq4019.c +++ b/drivers/net/mdio/mdio-ipq4019.c@@ -256,7 +256,7 @@ static int ipq_mdio_reset(struct mii_bus *bus) /* To indicate CMN_PLL that ethernet_ldo has been ready if platform resource 1 * is specified in the device tree. */ - if (priv->eth_ldo_rdy) { + if (!IS_ERR(priv->eth_ldo_rdy)) {
What's that? :-/ Ah, devm_ioremap_resource() returns error ptr too, so this looks like a fix for the existing code?
val = readl(priv->eth_ldo_rdy); val |= BIT(0); writel(val, priv->eth_ldo_rdy);
[...]
quoted hunk ↗ jump to hunk
@@ -351,9 +350,7 @@ static int ipq4019_mdio_probe(struct platform_device *pdev) /* The platform resource is provided on the chipset IPQ5018 */ /* This resource is optional */ - res = platform_get_resource(pdev, IORESOURCE_MEM, 1); - if (res) - priv->eth_ldo_rdy = devm_ioremap_resource(&pdev->dev, res); + priv->eth_ldo_rdy = devm_platform_ioremap_resource(pdev, 1); bus->name = "ipq4019_mdio"; bus->read = ipq4019_mdio_read_c22;
Other than that looks OKish... [...]
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/mdio/mdio-octeon.c b/drivers/net/mdio/mdio-octeon.c index 2beb83154d39..cb53dccbde1a 100644 --- a/drivers/net/mdio/mdio-octeon.c +++ b/drivers/net/mdio/mdio-octeon.c@@ -17,37 +17,20 @@ static int octeon_mdiobus_probe(struct platform_device *pdev) { struct cavium_mdiobus *bus; struct mii_bus *mii_bus; - struct resource *res_mem; - resource_size_t mdio_phys; - resource_size_t regsize; union cvmx_smix_en smi_en; - int err = -ENOENT; + int err; mii_bus = devm_mdiobus_alloc_size(&pdev->dev, sizeof(*bus)); if (!mii_bus) return -ENOMEM; - res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); - if (res_mem == NULL) { - dev_err(&pdev->dev, "found no memory resource\n"); - return -ENXIO; - } - bus = mii_bus->priv; bus->mii_bus = mii_bus; - mdio_phys = res_mem->start; - regsize = resource_size(res_mem); - if (!devm_request_mem_region(&pdev->dev, mdio_phys, regsize, - res_mem->name)) { - dev_err(&pdev->dev, "request_mem_region failed\n"); - return -ENXIO; - } - - bus->register_base = devm_ioremap(&pdev->dev, mdio_phys, regsize); - if (!bus->register_base) { + bus->register_base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(bus->register_base)) { dev_err(&pdev->dev, "dev_ioremap failed\n"); - return -ENOMEM; + return PTR_ERR(bus->register_base); } smi_en.u64 = 0;
Seems OKish too... MBR, Sergey