| Message ID | 20200910115607.11392-1-tangbin@cmss.chinamobile.com |
|---|---|
| State | Deferred |
| Headers | show |
| Series | usb: phy: tegra: Use IS_ERR() to check and simplify code | expand |
Hello! On 10.09.2020 14:56, Tang Bin wrote: > Use IS_ERR() and PTR_ERR() instead of PTR_ERR_OR_ZERO() to > simplify code, avoid redundant judgements. > > Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> > Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com> > --- > drivers/usb/phy/phy-tegra-usb.c | 25 ++++++++++--------------- > 1 file changed, 10 insertions(+), 15 deletions(-) > > diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c > index 6153cc35a..3b901429d 100644 > --- a/drivers/usb/phy/phy-tegra-usb.c > +++ b/drivers/usb/phy/phy-tegra-usb.c > @@ -1121,10 +1121,9 @@ static int tegra_usb_phy_probe(struct platform_device *pdev) > return PTR_ERR(tegra_phy->vbus); > > tegra_phy->pll_u = devm_clk_get(&pdev->dev, "pll_u"); > - err = PTR_ERR_OR_ZERO(tegra_phy->pll_u); > - if (err) { > + if (IS_ERR(tegra_phy->pll_u)) { > dev_err(&pdev->dev, "Failed to get pll_u clock: %d\n", err); 'err' should be changed here too... > - return err; > + return PTR_ERR(tegra_phy->pll_u); > } > > phy_type = of_usb_get_phy_mode(np); > @@ -1135,20 +1134,18 @@ static int tegra_usb_phy_probe(struct platform_device *pdev) > return err; > > tegra_phy->pad_clk = devm_clk_get(&pdev->dev, "utmi-pads"); > - err = PTR_ERR_OR_ZERO(tegra_phy->pad_clk); > - if (err) { > + if (IS_ERR(tegra_phy->pad_clk)) { > dev_err(&pdev->dev, > "Failed to get UTMIP pad clock: %d\n", err); Same here. > - return err; > + return PTR_ERR(tegra_phy->pad_clk); > } > > reset = devm_reset_control_get_optional_shared(&pdev->dev, > "utmi-pads"); > - err = PTR_ERR_OR_ZERO(reset); > - if (err) { > + if (IS_ERR(reset)) { > dev_err(&pdev->dev, > "Failed to get UTMI-pads reset: %d\n", err); And here. > - return err; > + return PTR_ERR(reset); > } > tegra_phy->pad_rst = reset; > break; > @@ -1157,22 +1154,20 @@ static int tegra_usb_phy_probe(struct platform_device *pdev) > tegra_phy->is_ulpi_phy = true; > > tegra_phy->clk = devm_clk_get(&pdev->dev, "ulpi-link"); > - err = PTR_ERR_OR_ZERO(tegra_phy->clk); > - if (err) { > + if (IS_ERR(tegra_phy->clk)) { > dev_err(&pdev->dev, > "Failed to get ULPI clock: %d\n", err); And here. > - return err; > + return PTR_ERR(tegra_phy->clk); > } > > gpiod = devm_gpiod_get_from_of_node(&pdev->dev, np, > "nvidia,phy-reset-gpio", > 0, GPIOD_OUT_HIGH, > "ulpi_phy_reset_b"); > - err = PTR_ERR_OR_ZERO(gpiod); > - if (err) { > + if (IS_ERR(gpiod)) { > dev_err(&pdev->dev, > "Request failed for reset GPIO: %d\n", err); And here. > - return err; > + return PTR_ERR(gpiod); > } > tegra_phy->reset_gpio = gpiod; > Overall, this patch is broken and not even worth redoing -- the current code seems good... MBR, Sergei
Tang Bin <tangbin@cmss.chinamobile.com> writes: > Use IS_ERR() and PTR_ERR() instead of PTR_ERR_OR_ZERO() to > simplify code, avoid redundant judgements. > > Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> > Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com> Applied for next merge window. Make sure to get this driver out of drivers/usb/phy and moved into drivers/phy ASAP.
On Thu, Sep 24, 2020 at 10:26:15AM +0300, Felipe Balbi wrote: > Tang Bin <tangbin@cmss.chinamobile.com> writes: > > > Use IS_ERR() and PTR_ERR() instead of PTR_ERR_OR_ZERO() to > > simplify code, avoid redundant judgements. > > > > Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> > > Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com> > > Applied for next merge window. Make sure to get this driver out of > drivers/usb/phy and moved into drivers/phy ASAP. Sergei had commented on this patch with valid concerns, see here in case you don't have his reply in your inbox: http://patchwork.ozlabs.org/project/linux-tegra/patch/20200910115607.11392-1-tangbin@cmss.chinamobile.com/#2526208 I agree with those concerns. This patch is broken because it will output the wrong error code on failure. I don't fully agree with Sergei's point that this patch isn't worth redoing. I do like the idiomatic error handling better, but I think we shouldn't be breaking the error messages like this. Thierry
Thierry Reding <thierry.reding@gmail.com> writes: > On Thu, Sep 24, 2020 at 10:26:15AM +0300, Felipe Balbi wrote: >> Tang Bin <tangbin@cmss.chinamobile.com> writes: >> >> > Use IS_ERR() and PTR_ERR() instead of PTR_ERR_OR_ZERO() to >> > simplify code, avoid redundant judgements. >> > >> > Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> >> > Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com> >> >> Applied for next merge window. Make sure to get this driver out of >> drivers/usb/phy and moved into drivers/phy ASAP. > > Sergei had commented on this patch with valid concerns, see here in case > you don't have his reply in your inbox: > > http://patchwork.ozlabs.org/project/linux-tegra/patch/20200910115607.11392-1-tangbin@cmss.chinamobile.com/#2526208 > > I agree with those concerns. This patch is broken because it will output > the wrong error code on failure. I don't fully agree with Sergei's point > that this patch isn't worth redoing. I do like the idiomatic error > handling better, but I think we shouldn't be breaking the error messages > like this. Sure thing, dropped for now.
Hi all: 在 2020/9/24 18:37, Felipe Balbi 写道: > Thierry Reding <thierry.reding@gmail.com> writes: > >> On Thu, Sep 24, 2020 at 10:26:15AM +0300, Felipe Balbi wrote: >>> Tang Bin <tangbin@cmss.chinamobile.com> writes: >>> >>>> Use IS_ERR() and PTR_ERR() instead of PTR_ERR_OR_ZERO() to >>>> simplify code, avoid redundant judgements. >>>> >>>> Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> >>>> Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com> >>> Applied for next merge window. Make sure to get this driver out of >>> drivers/usb/phy and moved into drivers/phy ASAP. >> Sergei had commented on this patch with valid concerns, see here in case >> you don't have his reply in your inbox: >> >> http://patchwork.ozlabs.org/project/linux-tegra/patch/20200910115607.11392-1-tangbin@cmss.chinamobile.com/#2526208 >> >> I agree with those concerns. This patch is broken because it will output >> the wrong error code on failure. I don't fully agree with Sergei's point >> that this patch isn't worth redoing. I do like the idiomatic error >> handling better, but I think we shouldn't be breaking the error messages >> like this. > Sure thing, dropped for now. Sorry, it's my fault. Thanks Tang Bin >
diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c index 6153cc35a..3b901429d 100644 --- a/drivers/usb/phy/phy-tegra-usb.c +++ b/drivers/usb/phy/phy-tegra-usb.c @@ -1121,10 +1121,9 @@ static int tegra_usb_phy_probe(struct platform_device *pdev) return PTR_ERR(tegra_phy->vbus); tegra_phy->pll_u = devm_clk_get(&pdev->dev, "pll_u"); - err = PTR_ERR_OR_ZERO(tegra_phy->pll_u); - if (err) { + if (IS_ERR(tegra_phy->pll_u)) { dev_err(&pdev->dev, "Failed to get pll_u clock: %d\n", err); - return err; + return PTR_ERR(tegra_phy->pll_u); } phy_type = of_usb_get_phy_mode(np); @@ -1135,20 +1134,18 @@ static int tegra_usb_phy_probe(struct platform_device *pdev) return err; tegra_phy->pad_clk = devm_clk_get(&pdev->dev, "utmi-pads"); - err = PTR_ERR_OR_ZERO(tegra_phy->pad_clk); - if (err) { + if (IS_ERR(tegra_phy->pad_clk)) { dev_err(&pdev->dev, "Failed to get UTMIP pad clock: %d\n", err); - return err; + return PTR_ERR(tegra_phy->pad_clk); } reset = devm_reset_control_get_optional_shared(&pdev->dev, "utmi-pads"); - err = PTR_ERR_OR_ZERO(reset); - if (err) { + if (IS_ERR(reset)) { dev_err(&pdev->dev, "Failed to get UTMI-pads reset: %d\n", err); - return err; + return PTR_ERR(reset); } tegra_phy->pad_rst = reset; break; @@ -1157,22 +1154,20 @@ static int tegra_usb_phy_probe(struct platform_device *pdev) tegra_phy->is_ulpi_phy = true; tegra_phy->clk = devm_clk_get(&pdev->dev, "ulpi-link"); - err = PTR_ERR_OR_ZERO(tegra_phy->clk); - if (err) { + if (IS_ERR(tegra_phy->clk)) { dev_err(&pdev->dev, "Failed to get ULPI clock: %d\n", err); - return err; + return PTR_ERR(tegra_phy->clk); } gpiod = devm_gpiod_get_from_of_node(&pdev->dev, np, "nvidia,phy-reset-gpio", 0, GPIOD_OUT_HIGH, "ulpi_phy_reset_b"); - err = PTR_ERR_OR_ZERO(gpiod); - if (err) { + if (IS_ERR(gpiod)) { dev_err(&pdev->dev, "Request failed for reset GPIO: %d\n", err); - return err; + return PTR_ERR(gpiod); } tegra_phy->reset_gpio = gpiod;