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@xxxxxxxxxxxxxxxx>
> >>>> ---
> >>>> 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
