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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Laurent,

On Thu, Mar 28, 2024 at 01:09:13PM +0200, Laurent Pinchart wrote:
> Sakari, there's a question for you below.
> 
> On Thu, Mar 28, 2024 at 04:29:41PM +0530, Umang Jain wrote:
> > On 28/03/24 4:23 pm, Laurent Pinchart wrote:
> > > 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.
> > 
> > Indeed, a matter of taste ...
> > 
> > On IMX283 driver, I got the feedback that all probe errors should go via 
> > dev_err_probe()
> > 
> > "Make all messages in ->probe() stage to use the same pattern." [1]
> > 
> > For IMX219, (since it's the most appropriate reference driver from 
> > framework PoV), I saw that it is not logging through dev_err_probe(), 
> > and in such cases hence tried to align with [1]
> 
> I'd say we should have common guidelines for all sensor drivers. As
> Sakari is the maintainer here, he can be the judge too :-)

I don't really have much of an opinion. If there's a chance you might have
-EPROBE_DEFER, then you should use it, I guess, but it may well be
reasonable also otherwise.

-- 
Regards,

Sakari Ailus