Re: [PATCH] media: i2c: imx219: Use dev_err_probe on probe

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Umang,

On Fri, Mar 15, 2024 at 12:13:15PM +0530, Umang Jain wrote:
> On 14/03/24 8:51 pm, Laurent Pinchart wrote:
> > On Thu, Mar 14, 2024 at 06:51:04PM +0530, Umang Jain wrote:
> >> On 11/03/24 5:05 pm, Tommaso Merciai wrote:
> >>> On Mon, Mar 11, 2024 at 02:30:42PM +0530, Umang Jain wrote:
> >>>> Drop dev_err() and use the dev_err_probe() helper on probe path.
> >>>>
> >>>> No functional changes intended.
> >>>>
> >>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> >>>> ---
> >>>>    drivers/media/i2c/imx219.c | 64 +++++++++++++++++++-------------------
> >>>>    1 file changed, 32 insertions(+), 32 deletions(-)
> >>>>
> >>>> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> >>>> index e17ef2e9d9d0..acd27e2ef849 100644
> >>>> --- a/drivers/media/i2c/imx219.c
> >>>> +++ b/drivers/media/i2c/imx219.c
> >>>> @@ -551,8 +551,9 @@ static int imx219_init_controls(struct imx219 *imx219)
> >>>>    
> >>>>    	if (ctrl_hdlr->error) {
> >>>>    		ret = ctrl_hdlr->error;
> >>>> -		dev_err(&client->dev, "%s control init failed (%d)\n",
> >>>> -			__func__, ret);
> >>>> +		dev_err_probe(&client->dev, ret,
> >>>> +			      "%s control init failed\n",
> >>>> +			      __func__);
> >
> > ctrl_hdlr->error can never be -EPROBE_DEFER, is dev_err_probe() really
> > useful here ?
> 
> is dev_err_probe() really /only/ about -EPROBE_DEFER ?  or all probe() 
> errors?
> 
> The documentation is explicitly stating for dev_Err_probe()
> 
> ```
>   * Note that it is deemed acceptable to use this function for error
>   * prints during probe even if the @err is known to never be -EPROBE_DEFER.
>   * The benefit compared to a normal dev_err() is the standardized format
>   * of the error code and the fact that the error code is returned.
>   *
> 
> ```

As in so many cases, it's partly a matter of taste :-) When it comes to
changes such as

-		dev_err(dev, "xclk frequency not supported: %d Hz\n",
-			imx219->xclk_freq);
-		return -EINVAL;
+		return dev_err_probe(dev, -EINVAL,
+				     "xclk frequency not supported: %d Hz\n",
+				     imx219->xclk_freq);

I find the resulting less readable. The indentation is higher, you have
to look at the parameters to dev_err_probe() to see what is returned
(compared to reading "return -EINVAL"), and adding the error code to the
printed message also makes the kernel log less as the error code really
doesn't need to be printed in this case.

> >>>>    		goto error;
> >>>>    	}

[snip]

-- 
Regards,

Laurent Pinchart