| Message ID | 20230705090126.26854-4-frank.li@vivo.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/4] phy: marvell: phy-mvebu-cp110-comphy: Use devm_platform_get_and_ioremap_resource() | expand |
Il 05/07/23 11:01, Yangtao Li ha scritto: > Use devm_platform_ioremap_resource() to simplify code. > > Signed-off-by: Yangtao Li <frank.li@vivo.com> > --- > drivers/phy/mediatek/phy-mtk-tphy.c | 10 +++------- > 1 file changed, 3 insertions(+), 7 deletions(-) > > diff --git a/drivers/phy/mediatek/phy-mtk-tphy.c b/drivers/phy/mediatek/phy-mtk-tphy.c > index 0d110e50bbfd..cb7a4e6ea017 100644 > --- a/drivers/phy/mediatek/phy-mtk-tphy.c > +++ b/drivers/phy/mediatek/phy-mtk-tphy.c > @@ -1554,7 +1554,6 @@ static int mtk_tphy_probe(struct platform_device *pdev) > struct device_node *np = dev->of_node; > struct device_node *child_np; > struct phy_provider *provider; > - struct resource *sif_res; > struct mtk_tphy *tphy; > struct resource res; > int port, retval; > @@ -1576,15 +1575,12 @@ static int mtk_tphy_probe(struct platform_device *pdev) > tphy->dev = dev; > platform_set_drvdata(pdev, tphy); > > - sif_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > /* SATA phy of V1 needn't it if not shared with PCIe or USB */ > - if (sif_res && tphy->pdata->version == MTK_PHY_V1) { > + if (tphy->pdata->version == MTK_PHY_V1) { > /* get banks shared by multiple phys */ > - tphy->sif_base = devm_ioremap_resource(dev, sif_res); > - if (IS_ERR(tphy->sif_base)) { > - dev_err(dev, "failed to remap sif regs\n"); > + tphy->sif_base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(tphy->sif_base) && PTR_ERR(tphy->sif_base) != -EINVAL) That's wrong. You want to return any error unconditionally, as the V1 PHY *needs* the sif iospace and there's nothing afterwards retrying this. Please fix. Regards, Angelo > return PTR_ERR(tphy->sif_base); > - } > } > > if (tphy->pdata->version < MTK_PHY_V3) {
HI AngeloGioacchino, On 2023/7/5 17:22, AngeloGioacchino Del Regno wrote: > Il 05/07/23 11:01, Yangtao Li ha scritto: >> Use devm_platform_ioremap_resource() to simplify code. >> >> Signed-off-by: Yangtao Li <frank.li@vivo.com> >> --- >> drivers/phy/mediatek/phy-mtk-tphy.c | 10 +++------- >> 1 file changed, 3 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/phy/mediatek/phy-mtk-tphy.c >> b/drivers/phy/mediatek/phy-mtk-tphy.c >> index 0d110e50bbfd..cb7a4e6ea017 100644 >> --- a/drivers/phy/mediatek/phy-mtk-tphy.c >> +++ b/drivers/phy/mediatek/phy-mtk-tphy.c >> @@ -1554,7 +1554,6 @@ static int mtk_tphy_probe(struct >> platform_device *pdev) >> struct device_node *np = dev->of_node; >> struct device_node *child_np; >> struct phy_provider *provider; >> - struct resource *sif_res; >> struct mtk_tphy *tphy; >> struct resource res; >> int port, retval; >> @@ -1576,15 +1575,12 @@ static int mtk_tphy_probe(struct >> platform_device *pdev) >> tphy->dev = dev; >> platform_set_drvdata(pdev, tphy); >> - sif_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> /* SATA phy of V1 needn't it if not shared with PCIe or USB */ >> - if (sif_res && tphy->pdata->version == MTK_PHY_V1) { >> + if (tphy->pdata->version == MTK_PHY_V1) { >> /* get banks shared by multiple phys */ >> - tphy->sif_base = devm_ioremap_resource(dev, sif_res); >> - if (IS_ERR(tphy->sif_base)) { >> - dev_err(dev, "failed to remap sif regs\n"); >> + tphy->sif_base = devm_platform_ioremap_resource(pdev, 0); >> + if (IS_ERR(tphy->sif_base) && PTR_ERR(tphy->sif_base) != >> -EINVAL) > > That's wrong. You want to return any error unconditionally, as the V1 > PHY *needs* > the sif iospace and there's nothing afterwards retrying this. When sif_res is not configured, an IOMEM_ERR_PTR(-EINVAL) error will be returned in __devm_ioremap_resource. This kind of error is ignored in the newly added code, and the driving probe process will not be interrupted at this time. What did I miss? Thx, Yangtao > > Please fix. > > Regards, > Angelo > >> return PTR_ERR(tphy->sif_base); >> - } >> } >> if (tphy->pdata->version < MTK_PHY_V3) { > >
Il 05/07/23 11:30, Yangtao Li ha scritto: > HI AngeloGioacchino, > > On 2023/7/5 17:22, AngeloGioacchino Del Regno wrote: >> Il 05/07/23 11:01, Yangtao Li ha scritto: >>> Use devm_platform_ioremap_resource() to simplify code. >>> >>> Signed-off-by: Yangtao Li <frank.li@vivo.com> >>> --- >>> drivers/phy/mediatek/phy-mtk-tphy.c | 10 +++------- >>> 1 file changed, 3 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/phy/mediatek/phy-mtk-tphy.c >>> b/drivers/phy/mediatek/phy-mtk-tphy.c >>> index 0d110e50bbfd..cb7a4e6ea017 100644 >>> --- a/drivers/phy/mediatek/phy-mtk-tphy.c >>> +++ b/drivers/phy/mediatek/phy-mtk-tphy.c >>> @@ -1554,7 +1554,6 @@ static int mtk_tphy_probe(struct platform_device *pdev) >>> struct device_node *np = dev->of_node; >>> struct device_node *child_np; >>> struct phy_provider *provider; >>> - struct resource *sif_res; >>> struct mtk_tphy *tphy; >>> struct resource res; >>> int port, retval; >>> @@ -1576,15 +1575,12 @@ static int mtk_tphy_probe(struct platform_device *pdev) >>> tphy->dev = dev; >>> platform_set_drvdata(pdev, tphy); >>> - sif_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>> /* SATA phy of V1 needn't it if not shared with PCIe or USB */ >>> - if (sif_res && tphy->pdata->version == MTK_PHY_V1) { >>> + if (tphy->pdata->version == MTK_PHY_V1) { >>> /* get banks shared by multiple phys */ >>> - tphy->sif_base = devm_ioremap_resource(dev, sif_res); >>> - if (IS_ERR(tphy->sif_base)) { >>> - dev_err(dev, "failed to remap sif regs\n"); >>> + tphy->sif_base = devm_platform_ioremap_resource(pdev, 0); >>> + if (IS_ERR(tphy->sif_base) && PTR_ERR(tphy->sif_base) != -EINVAL) >> >> That's wrong. You want to return any error unconditionally, as the V1 PHY *needs* >> the sif iospace and there's nothing afterwards retrying this. > > > When sif_res is not configured, an IOMEM_ERR_PTR(-EINVAL) error will be returned in > __devm_ioremap_resource. > > This kind of error is ignored in the newly added code, and the driving probe > process will not be interrupted at this time. > > What did I miss? > As I said, MTK_PHY_V1 *needs* sif; devm_platform_ioremap_resource() returns a handle to that iospace, or error. tphy->sif_base = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(tphy->sif_base)) return PTR_ERR(tphy->sif_base); Regards, Angelo > Thx, > > Yangtao > > >> >> Please fix. >> >> Regards, >> Angelo >> >>> return PTR_ERR(tphy->sif_base); >>> - } >>> } >>> if (tphy->pdata->version < MTK_PHY_V3) { >> >>
On 05-07-23, 12:00, AngeloGioacchino Del Regno wrote: > Il 05/07/23 11:30, Yangtao Li ha scritto: > > HI AngeloGioacchino, > > > > On 2023/7/5 17:22, AngeloGioacchino Del Regno wrote: > > > Il 05/07/23 11:01, Yangtao Li ha scritto: > > > > Use devm_platform_ioremap_resource() to simplify code. > > > > > > > > Signed-off-by: Yangtao Li <frank.li@vivo.com> > > > > --- > > > > drivers/phy/mediatek/phy-mtk-tphy.c | 10 +++------- > > > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/drivers/phy/mediatek/phy-mtk-tphy.c > > > > b/drivers/phy/mediatek/phy-mtk-tphy.c > > > > index 0d110e50bbfd..cb7a4e6ea017 100644 > > > > --- a/drivers/phy/mediatek/phy-mtk-tphy.c > > > > +++ b/drivers/phy/mediatek/phy-mtk-tphy.c > > > > @@ -1554,7 +1554,6 @@ static int mtk_tphy_probe(struct platform_device *pdev) > > > > struct device_node *np = dev->of_node; > > > > struct device_node *child_np; > > > > struct phy_provider *provider; > > > > - struct resource *sif_res; > > > > struct mtk_tphy *tphy; > > > > struct resource res; > > > > int port, retval; > > > > @@ -1576,15 +1575,12 @@ static int mtk_tphy_probe(struct platform_device *pdev) > > > > tphy->dev = dev; > > > > platform_set_drvdata(pdev, tphy); > > > > - sif_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > > /* SATA phy of V1 needn't it if not shared with PCIe or USB */ > > > > - if (sif_res && tphy->pdata->version == MTK_PHY_V1) { > > > > + if (tphy->pdata->version == MTK_PHY_V1) { > > > > /* get banks shared by multiple phys */ > > > > - tphy->sif_base = devm_ioremap_resource(dev, sif_res); > > > > - if (IS_ERR(tphy->sif_base)) { > > > > - dev_err(dev, "failed to remap sif regs\n"); > > > > + tphy->sif_base = devm_platform_ioremap_resource(pdev, 0); > > > > + if (IS_ERR(tphy->sif_base) && PTR_ERR(tphy->sif_base) != -EINVAL) > > > > > > That's wrong. You want to return any error unconditionally, as the V1 PHY *needs* > > > the sif iospace and there's nothing afterwards retrying this. > > > > > > When sif_res is not configured, an IOMEM_ERR_PTR(-EINVAL) error will be > > returned in __devm_ioremap_resource. > > > > This kind of error is ignored in the newly added code, and the driving > > probe process will not be interrupted at this time. > > > > What did I miss? > > > > As I said, MTK_PHY_V1 *needs* sif; devm_platform_ioremap_resource() returns a > handle to that iospace, or error. > > tphy->sif_base = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(tphy->sif_base)) > return PTR_ERR(tphy->sif_base); That does sound right to me
On Wed, 2023-07-05 at 12:00 +0200, AngeloGioacchino Del Regno wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > Il 05/07/23 11:30, Yangtao Li ha scritto: > > HI AngeloGioacchino, > > > > On 2023/7/5 17:22, AngeloGioacchino Del Regno wrote: > >> Il 05/07/23 11:01, Yangtao Li ha scritto: > >>> Use devm_platform_ioremap_resource() to simplify code. > >>> > >>> Signed-off-by: Yangtao Li <frank.li@vivo.com> > >>> --- > >>> drivers/phy/mediatek/phy-mtk-tphy.c | 10 +++------- > >>> 1 file changed, 3 insertions(+), 7 deletions(-) > >>> > >>> diff --git a/drivers/phy/mediatek/phy-mtk-tphy.c > >>> b/drivers/phy/mediatek/phy-mtk-tphy.c > >>> index 0d110e50bbfd..cb7a4e6ea017 100644 > >>> --- a/drivers/phy/mediatek/phy-mtk-tphy.c > >>> +++ b/drivers/phy/mediatek/phy-mtk-tphy.c > >>> @@ -1554,7 +1554,6 @@ static int mtk_tphy_probe(struct > platform_device *pdev) > >>> struct device_node *np = dev->of_node; > >>> struct device_node *child_np; > >>> struct phy_provider *provider; > >>> - struct resource *sif_res; > >>> struct mtk_tphy *tphy; > >>> struct resource res; > >>> int port, retval; > >>> @@ -1576,15 +1575,12 @@ static int mtk_tphy_probe(struct > platform_device *pdev) > >>> tphy->dev = dev; > >>> platform_set_drvdata(pdev, tphy); > >>> - sif_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >>> /* SATA phy of V1 needn't it if not shared with PCIe or USB > */ > >>> - if (sif_res && tphy->pdata->version == MTK_PHY_V1) { > >>> + if (tphy->pdata->version == MTK_PHY_V1) { > >>> /* get banks shared by multiple phys */ > >>> - tphy->sif_base = devm_ioremap_resource(dev, sif_res); > >>> - if (IS_ERR(tphy->sif_base)) { > >>> - dev_err(dev, "failed to remap sif regs\n"); > >>> + tphy->sif_base = devm_platform_ioremap_resource(pdev, > 0); > >>> + if (IS_ERR(tphy->sif_base) && PTR_ERR(tphy->sif_base) != > -EINVAL) > >> > >> That's wrong. You want to return any error unconditionally, as the > V1 PHY *needs* > >> the sif iospace and there's nothing afterwards retrying this. > > > > > > When sif_res is not configured, an IOMEM_ERR_PTR(-EINVAL) error > will be returned in > > __devm_ioremap_resource. > > > > This kind of error is ignored in the newly added code, and the > driving probe > > process will not be interrupted at this time. > > > > What did I miss? > > > > As I said, MTK_PHY_V1 *needs* sif; devm_platform_ioremap_resource() > returns a > handle to that iospace, or error. > > tphy->sif_base = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(tphy->sif_base)) > return PTR_ERR(tphy->sif_base); Yes, it's right, thanks > > Regards, > Angelo > > > Thx, > > > > Yangtao > > > > > >> > >> Please fix. > >> > >> Regards, > >> Angelo > >> > >>> return PTR_ERR(tphy->sif_base); > >>> - } > >>> } > >>> if (tphy->pdata->version < MTK_PHY_V3) { > >> > >> > > >
diff --git a/drivers/phy/mediatek/phy-mtk-tphy.c b/drivers/phy/mediatek/phy-mtk-tphy.c index 0d110e50bbfd..cb7a4e6ea017 100644 --- a/drivers/phy/mediatek/phy-mtk-tphy.c +++ b/drivers/phy/mediatek/phy-mtk-tphy.c @@ -1554,7 +1554,6 @@ static int mtk_tphy_probe(struct platform_device *pdev) struct device_node *np = dev->of_node; struct device_node *child_np; struct phy_provider *provider; - struct resource *sif_res; struct mtk_tphy *tphy; struct resource res; int port, retval; @@ -1576,15 +1575,12 @@ static int mtk_tphy_probe(struct platform_device *pdev) tphy->dev = dev; platform_set_drvdata(pdev, tphy); - sif_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); /* SATA phy of V1 needn't it if not shared with PCIe or USB */ - if (sif_res && tphy->pdata->version == MTK_PHY_V1) { + if (tphy->pdata->version == MTK_PHY_V1) { /* get banks shared by multiple phys */ - tphy->sif_base = devm_ioremap_resource(dev, sif_res); - if (IS_ERR(tphy->sif_base)) { - dev_err(dev, "failed to remap sif regs\n"); + tphy->sif_base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(tphy->sif_base) && PTR_ERR(tphy->sif_base) != -EINVAL) return PTR_ERR(tphy->sif_base); - } } if (tphy->pdata->version < MTK_PHY_V3) {
Use devm_platform_ioremap_resource() to simplify code. Signed-off-by: Yangtao Li <frank.li@vivo.com> --- drivers/phy/mediatek/phy-mtk-tphy.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)