Re: [PATCH -next v2] media: Switch to use dev_err_probe() helper

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Sep 19, 2022 at 08:25:07PM +0200, Ricardo Ribalda wrote:
> Hi Yang
> 
> On Mon, 19 Sept 2022 at 18:02, Yang Yingliang <yangyingliang@xxxxxxxxxx> wrote:
> >
> > In the probe path, dev_err() can be replaced with dev_err_probe()
> > which will check if error code is -EPROBE_DEFER.
> >
> > Reviewed-by: Sean Young <sean@xxxxxxxx>
> > Reviewed-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> FWIW: I originally reviewed only uvc

I liked the split-patches version better too, but that's no reason to
submit a v3 just for that.

> > Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Yang Yingliang <yangyingliang@xxxxxxxxxx>
> > ---
> > v2:
> >   - Merge the patches in one patch.
> >   - s/replace/replaced in commit message.
> >   - Add '\n' in xilinx-csi2rxss.c and imx274.c
> >   - Add missing return value in media-dev.c
> > ---
> >  drivers/media/cec/platform/stm32/stm32-cec.c  |  9 +++----
> >  drivers/media/i2c/ad5820.c                    | 18 +++++--------
> >  drivers/media/i2c/imx274.c                    |  5 ++--
> >  drivers/media/i2c/tc358743.c                  |  9 +++----
> >  .../platform/mediatek/mdp/mtk_mdp_comp.c      |  5 ++--
> >  .../platform/samsung/exynos4-is/media-dev.c   |  4 +--
> >  drivers/media/platform/st/stm32/stm32-dcmi.c  | 27 +++++++------------
> >  drivers/media/platform/ti/omap3isp/isp.c      |  3 +--
> >  .../media/platform/xilinx/xilinx-csi2rxss.c   |  8 +++---
> >  drivers/media/rc/gpio-ir-recv.c               | 10 +++----
> >  drivers/media/rc/gpio-ir-tx.c                 |  9 +++----
> >  drivers/media/rc/ir-rx51.c                    |  9 ++-----
> >  drivers/media/usb/uvc/uvc_driver.c            |  9 +++----
> >  13 files changed, 41 insertions(+), 84 deletions(-)
> >
> > diff --git a/drivers/media/cec/platform/stm32/stm32-cec.c b/drivers/media/cec/platform/stm32/stm32-cec.c
> > index 40db7911b437..7b2db46a5722 100644
> > --- a/drivers/media/cec/platform/stm32/stm32-cec.c
> > +++ b/drivers/media/cec/platform/stm32/stm32-cec.c
> > @@ -288,12 +288,9 @@ static int stm32_cec_probe(struct platform_device *pdev)
> >                 return ret;
> >
> >         cec->clk_cec = devm_clk_get(&pdev->dev, "cec");
> > -       if (IS_ERR(cec->clk_cec)) {
> > -               if (PTR_ERR(cec->clk_cec) != -EPROBE_DEFER)
> > -                       dev_err(&pdev->dev, "Cannot get cec clock\n");
> > -
> > -               return PTR_ERR(cec->clk_cec);
> > -       }
> > +       if (IS_ERR(cec->clk_cec))
> > +               return dev_err_probe(&pdev->dev, PTR_ERR(cec->clk_cec),
> > +                                    "Cannot get cec clock\n");
> >
> >         ret = clk_prepare(cec->clk_cec);
> >         if (ret) {
> > diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c
> > index 516de278cc49..6a7f8ef9db05 100644
> > --- a/drivers/media/i2c/ad5820.c
> > +++ b/drivers/media/i2c/ad5820.c
> > @@ -301,21 +301,15 @@ static int ad5820_probe(struct i2c_client *client,
> >                 return -ENOMEM;
> >
> >         coil->vana = devm_regulator_get(&client->dev, "VANA");
> > -       if (IS_ERR(coil->vana)) {
> > -               ret = PTR_ERR(coil->vana);
> > -               if (ret != -EPROBE_DEFER)
> > -                       dev_err(&client->dev, "could not get regulator for vana\n");
> > -               return ret;
> > -       }
> > +       if (IS_ERR(coil->vana))
> > +               return dev_err_probe(&client->dev, PTR_ERR(coil->vana),
> > +                                    "could not get regulator for vana\n");
> >
> >         coil->enable_gpio = devm_gpiod_get_optional(&client->dev, "enable",
> >                                                     GPIOD_OUT_LOW);
> > -       if (IS_ERR(coil->enable_gpio)) {
> > -               ret = PTR_ERR(coil->enable_gpio);
> > -               if (ret != -EPROBE_DEFER)
> > -                       dev_err(&client->dev, "could not get enable gpio\n");
> > -               return ret;
> > -       }
> > +       if (IS_ERR(coil->enable_gpio))
> > +               return dev_err_probe(&client->dev, PTR_ERR(coil->enable_gpio),
> > +                                    "could not get enable gpio\n");
> >
> >         mutex_init(&coil->power_lock);
> >
> > diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
> > index a00761b1e18c..9219f3c9594b 100644
> > --- a/drivers/media/i2c/imx274.c
> > +++ b/drivers/media/i2c/imx274.c
> > @@ -2060,9 +2060,8 @@ static int imx274_probe(struct i2c_client *client)
> >         imx274->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> >                                                      GPIOD_OUT_HIGH);
> >         if (IS_ERR(imx274->reset_gpio)) {
> > -               if (PTR_ERR(imx274->reset_gpio) != -EPROBE_DEFER)
> > -                       dev_err(dev, "Reset GPIO not setup in DT");
> > -               ret = PTR_ERR(imx274->reset_gpio);
> > +               ret = dev_err_probe(dev, PTR_ERR(imx274->reset_gpio),
> > +                                   "Reset GPIO not setup in DT\n");
> >                 goto err_me;
> 
> Not sure I like the use of dev_err_probe here. We only save one line.
> Same goes for all the other cases where there is no "return dev_err_probe..."

It's not just about saving a line, dev_err_probe() also records the
cause of probe deferral (without printing the message) and makes it
available through debugfs.

> >         }
> >
> > diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
> > index 200841c1f5cf..9197fa0b1bc2 100644
> > --- a/drivers/media/i2c/tc358743.c
> > +++ b/drivers/media/i2c/tc358743.c
> > @@ -1891,12 +1891,9 @@ static int tc358743_probe_of(struct tc358743_state *state)
> >         int ret;
> >
> >         refclk = devm_clk_get(dev, "refclk");
> > -       if (IS_ERR(refclk)) {
> > -               if (PTR_ERR(refclk) != -EPROBE_DEFER)
> > -                       dev_err(dev, "failed to get refclk: %ld\n",
> > -                               PTR_ERR(refclk));
> > -               return PTR_ERR(refclk);
> > -       }
> > +       if (IS_ERR(refclk))
> > +               return dev_err_probe(dev, PTR_ERR(refclk),
> > +                                    "failed to get refclk\n");
> >
> >         ep = of_graph_get_next_endpoint(dev->of_node, NULL);
> >         if (!ep) {
> > diff --git a/drivers/media/platform/mediatek/mdp/mtk_mdp_comp.c b/drivers/media/platform/mediatek/mdp/mtk_mdp_comp.c
> > index 1e3833f1c9ae..ad5fab2d8bfa 100644
> > --- a/drivers/media/platform/mediatek/mdp/mtk_mdp_comp.c
> > +++ b/drivers/media/platform/mediatek/mdp/mtk_mdp_comp.c
> > @@ -52,9 +52,8 @@ int mtk_mdp_comp_init(struct device *dev, struct device_node *node,
> >         for (i = 0; i < ARRAY_SIZE(comp->clk); i++) {
> >                 comp->clk[i] = of_clk_get(node, i);
> >                 if (IS_ERR(comp->clk[i])) {
> > -                       if (PTR_ERR(comp->clk[i]) != -EPROBE_DEFER)
> > -                               dev_err(dev, "Failed to get clock\n");
> > -                       ret = PTR_ERR(comp->clk[i]);
> > +                       ret = dev_err_probe(dev, PTR_ERR(comp->clk[i]),
> > +                                           "Failed to get clock\n");
> >                         goto put_dev;
> >                 }
> >
> > diff --git a/drivers/media/platform/samsung/exynos4-is/media-dev.c b/drivers/media/platform/samsung/exynos4-is/media-dev.c
> > index 52b43ea04030..7a9d51b8bb4c 100644
> > --- a/drivers/media/platform/samsung/exynos4-is/media-dev.c
> > +++ b/drivers/media/platform/samsung/exynos4-is/media-dev.c
> > @@ -1473,9 +1473,7 @@ static int fimc_md_probe(struct platform_device *pdev)
> >
> >         pinctrl = devm_pinctrl_get(dev);
> >         if (IS_ERR(pinctrl)) {
> > -               ret = PTR_ERR(pinctrl);
> > -               if (ret != EPROBE_DEFER)
> > -                       dev_err(dev, "Failed to get pinctrl: %d\n", ret);
> > +               ret = dev_err_probe(dev, PTR_ERR(pinctrl), "Failed to get pinctrl\n");
> >                 goto err_clk;
> >         }
> >
> > diff --git a/drivers/media/platform/st/stm32/stm32-dcmi.c b/drivers/media/platform/st/stm32/stm32-dcmi.c
> > index 2ca95ab2b0fe..ec138843d090 100644
> > --- a/drivers/media/platform/st/stm32/stm32-dcmi.c
> > +++ b/drivers/media/platform/st/stm32/stm32-dcmi.c
> > @@ -1946,12 +1946,9 @@ static int dcmi_probe(struct platform_device *pdev)
> >                 return -ENOMEM;
> >
> >         dcmi->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> > -       if (IS_ERR(dcmi->rstc)) {
> > -               if (PTR_ERR(dcmi->rstc) != -EPROBE_DEFER)
> > -                       dev_err(&pdev->dev, "Could not get reset control\n");
> > -
> > -               return PTR_ERR(dcmi->rstc);
> > -       }
> > +       if (IS_ERR(dcmi->rstc))
> > +               return dev_err_probe(&pdev->dev, PTR_ERR(dcmi->rstc),
> > +                                    "Could not get reset control\n");
> >
> >         /* Get bus characteristics from devicetree */
> >         np = of_graph_get_next_endpoint(np, NULL);
> > @@ -2003,20 +2000,14 @@ static int dcmi_probe(struct platform_device *pdev)
> >         }
> >
> >         mclk = devm_clk_get(&pdev->dev, "mclk");
> > -       if (IS_ERR(mclk)) {
> > -               if (PTR_ERR(mclk) != -EPROBE_DEFER)
> > -                       dev_err(&pdev->dev, "Unable to get mclk\n");
> > -               return PTR_ERR(mclk);
> > -       }
> > +       if (IS_ERR(mclk))
> > +               return dev_err_probe(&pdev->dev, PTR_ERR(mclk),
> > +                                    "Unable to get mclk\n");
> >
> >         chan = dma_request_chan(&pdev->dev, "tx");
> > -       if (IS_ERR(chan)) {
> > -               ret = PTR_ERR(chan);
> > -               if (ret != -EPROBE_DEFER)
> > -                       dev_err(&pdev->dev,
> > -                               "Failed to request DMA channel: %d\n", ret);
> > -               return ret;
> > -       }
> > +       if (IS_ERR(chan))
> > +               return dev_err_probe(&pdev->dev, PTR_ERR(chan),
> > +                                    "Failed to request DMA channel\n");
> >
> >         dcmi->dma_max_burst = UINT_MAX;
> >         ret = dma_get_slave_caps(chan, &caps);
> > diff --git a/drivers/media/platform/ti/omap3isp/isp.c b/drivers/media/platform/ti/omap3isp/isp.c
> > index a6052df9bb19..5d6867b8f197 100644
> > --- a/drivers/media/platform/ti/omap3isp/isp.c
> > +++ b/drivers/media/platform/ti/omap3isp/isp.c
> > @@ -1886,8 +1886,7 @@ static int isp_initialize_modules(struct isp_device *isp)
> >
> >         ret = omap3isp_ccp2_init(isp);
> >         if (ret < 0) {
> > -               if (ret != -EPROBE_DEFER)
> > -                       dev_err(isp->dev, "CCP2 initialization failed\n");
> > +               dev_err_probe(isp->dev, ret, "CCP2 initialization failed\n");
> >                 goto error_ccp2;
> >         }
> >
> > diff --git a/drivers/media/platform/xilinx/xilinx-csi2rxss.c b/drivers/media/platform/xilinx/xilinx-csi2rxss.c
> > index 29b53febc2e7..d8a23f18cfbc 100644
> > --- a/drivers/media/platform/xilinx/xilinx-csi2rxss.c
> > +++ b/drivers/media/platform/xilinx/xilinx-csi2rxss.c
> > @@ -976,11 +976,9 @@ static int xcsi2rxss_probe(struct platform_device *pdev)
> >         /* Reset GPIO */
> >         xcsi2rxss->rst_gpio = devm_gpiod_get_optional(dev, "video-reset",
> >                                                       GPIOD_OUT_HIGH);
> > -       if (IS_ERR(xcsi2rxss->rst_gpio)) {
> > -               if (PTR_ERR(xcsi2rxss->rst_gpio) != -EPROBE_DEFER)
> > -                       dev_err(dev, "Video Reset GPIO not setup in DT");
> > -               return PTR_ERR(xcsi2rxss->rst_gpio);
> > -       }
> > +       if (IS_ERR(xcsi2rxss->rst_gpio))
> > +               return dev_err_probe(dev, PTR_ERR(xcsi2rxss->rst_gpio),
> > +                                    "Video Reset GPIO not setup in DT\n");
> >
> >         ret = xcsi2rxss_parse_of(xcsi2rxss);
> >         if (ret < 0)
> > diff --git a/drivers/media/rc/gpio-ir-recv.c b/drivers/media/rc/gpio-ir-recv.c
> > index 22e524b69806..8f1fff7af6c9 100644
> > --- a/drivers/media/rc/gpio-ir-recv.c
> > +++ b/drivers/media/rc/gpio-ir-recv.c
> > @@ -74,13 +74,9 @@ static int gpio_ir_recv_probe(struct platform_device *pdev)
> >                 return -ENOMEM;
> >
> >         gpio_dev->gpiod = devm_gpiod_get(dev, NULL, GPIOD_IN);
> > -       if (IS_ERR(gpio_dev->gpiod)) {
> > -               rc = PTR_ERR(gpio_dev->gpiod);
> > -               /* Just try again if this happens */
> > -               if (rc != -EPROBE_DEFER)
> > -                       dev_err(dev, "error getting gpio (%d)\n", rc);
> > -               return rc;
> > -       }
> > +       if (IS_ERR(gpio_dev->gpiod))
> > +               return dev_err_probe(dev, PTR_ERR(gpio_dev->gpiod),
> > +                                    "error getting gpio\n");
> >         gpio_dev->irq = gpiod_to_irq(gpio_dev->gpiod);
> >         if (gpio_dev->irq < 0)
> >                 return gpio_dev->irq;
> > diff --git a/drivers/media/rc/gpio-ir-tx.c b/drivers/media/rc/gpio-ir-tx.c
> > index d3063ddb472e..2b829c146db1 100644
> > --- a/drivers/media/rc/gpio-ir-tx.c
> > +++ b/drivers/media/rc/gpio-ir-tx.c
> > @@ -174,12 +174,9 @@ static int gpio_ir_tx_probe(struct platform_device *pdev)
> >                 return -ENOMEM;
> >
> >         gpio_ir->gpio = devm_gpiod_get(&pdev->dev, NULL, GPIOD_OUT_LOW);
> > -       if (IS_ERR(gpio_ir->gpio)) {
> > -               if (PTR_ERR(gpio_ir->gpio) != -EPROBE_DEFER)
> > -                       dev_err(&pdev->dev, "Failed to get gpio (%ld)\n",
> > -                               PTR_ERR(gpio_ir->gpio));
> > -               return PTR_ERR(gpio_ir->gpio);
> > -       }
> > +       if (IS_ERR(gpio_ir->gpio))
> > +               return dev_err_probe(&pdev->dev, PTR_ERR(gpio_ir->gpio),
> > +                                    "Failed to get gpio\n");
> >
> >         rcdev->priv = gpio_ir;
> >         rcdev->driver_name = DRIVER_NAME;
> > diff --git a/drivers/media/rc/ir-rx51.c b/drivers/media/rc/ir-rx51.c
> > index a3b145183260..85080c3d2053 100644
> > --- a/drivers/media/rc/ir-rx51.c
> > +++ b/drivers/media/rc/ir-rx51.c
> > @@ -231,13 +231,8 @@ static int ir_rx51_probe(struct platform_device *dev)
> >         struct rc_dev *rcdev;
> >
> >         pwm = pwm_get(&dev->dev, NULL);
> > -       if (IS_ERR(pwm)) {
> > -               int err = PTR_ERR(pwm);
> > -
> > -               if (err != -EPROBE_DEFER)
> > -                       dev_err(&dev->dev, "pwm_get failed: %d\n", err);
> > -               return err;
> > -       }
> > +       if (IS_ERR(pwm))
> > +               return dev_err_probe(&dev->dev, PTR_ERR(pwm), "pwm_get failed\n");
> >
> >         /* Use default, in case userspace does not set the carrier */
> >         ir_rx51.freq = DIV_ROUND_CLOSEST_ULL(pwm_get_period(pwm), NSEC_PER_SEC);
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index 744051b52e12..94f84c3c4712 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -1554,12 +1554,9 @@ static int uvc_gpio_parse(struct uvc_device *dev)
> >                 return PTR_ERR_OR_ZERO(gpio_privacy);
> >
> >         irq = gpiod_to_irq(gpio_privacy);
> > -       if (irq < 0) {
> > -               if (irq != EPROBE_DEFER)
> > -                       dev_err(&dev->udev->dev,
> > -                               "No IRQ for privacy GPIO (%d)\n", irq);
> > -               return irq;
> > -       }
> > +       if (irq < 0)
> > +               return dev_err_probe(&dev->udev->dev, irq,
> > +                                    "No IRQ for privacy GPIO\n");
> >
> >         unit = uvc_alloc_entity(UVC_EXT_GPIO_UNIT, UVC_EXT_GPIO_UNIT_ID, 0, 1);
> >         if (!unit)

-- 
Regards,

Laurent Pinchart



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux