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

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

[Umang Jain]

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__);
 		goto error;
 	}
 
@@ -1025,15 +1026,15 @@ static int imx219_identify_module(struct imx219 *imx219)
 
 	ret = cci_read(imx219->regmap, IMX219_REG_CHIP_ID, &val, NULL);
 	if (ret) {
-		dev_err(&client->dev, "failed to read chip id %x\n",
-			IMX219_CHIP_ID);
-		return ret;
+		return dev_err_probe(&client->dev, ret,
+				     "failed to read chip id %x\n",
+				     IMX219_CHIP_ID);
 	}
 
 	if (val != IMX219_CHIP_ID) {
-		dev_err(&client->dev, "chip id mismatch: %x!=%llx\n",
-			IMX219_CHIP_ID, val);
-		return -EIO;
+		return dev_err_probe(&client->dev, -EIO,
+				     "chip id mismatch: %x!=%llx\n",
+				     IMX219_CHIP_ID, val);
 	}
 
 	return 0;
@@ -1048,35 +1049,36 @@ static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
 	int ret = -EINVAL;
 
 	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
-	if (!endpoint) {
-		dev_err(dev, "endpoint node not found\n");
-		return -EINVAL;
-	}
+	if (!endpoint)
+		return dev_err_probe(dev, -EINVAL, "endpoint node not found\n");
 
 	if (v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep_cfg)) {
-		dev_err(dev, "could not parse endpoint\n");
+		dev_err_probe(dev, -EINVAL, "could not parse endpoint\n");
 		goto error_out;
 	}
 
 	/* Check the number of MIPI CSI2 data lanes */
 	if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2 &&
 	    ep_cfg.bus.mipi_csi2.num_data_lanes != 4) {
-		dev_err(dev, "only 2 or 4 data lanes are currently supported\n");
+		dev_err_probe(dev, -EINVAL,
+			      "only 2 or 4 data lanes are currently supported\n");
 		goto error_out;
 	}
 	imx219->lanes = ep_cfg.bus.mipi_csi2.num_data_lanes;
 
 	/* Check the link frequency set in device tree */
 	if (!ep_cfg.nr_of_link_frequencies) {
-		dev_err(dev, "link-frequency property not found in DT\n");
+		dev_err_probe(dev, -EINVAL,
+			      "link-frequency property not found in DT\n");
 		goto error_out;
 	}
 
 	if (ep_cfg.nr_of_link_frequencies != 1 ||
 	   (ep_cfg.link_frequencies[0] != ((imx219->lanes == 2) ?
 	    IMX219_DEFAULT_LINK_FREQ : IMX219_DEFAULT_LINK_FREQ_4LANE))) {
-		dev_err(dev, "Link frequency not supported: %lld\n",
-			ep_cfg.link_frequencies[0]);
+		dev_err_probe(dev, -EINVAL,
+			      "Link frequency not supported: %lld\n",
+			      ep_cfg.link_frequencies[0]);
 		goto error_out;
 	}
 
@@ -1108,30 +1110,27 @@ static int imx219_probe(struct i2c_client *client)
 
 	imx219->regmap = devm_cci_regmap_init_i2c(client, 16);
 	if (IS_ERR(imx219->regmap)) {
-		ret = PTR_ERR(imx219->regmap);
-		dev_err(dev, "failed to initialize CCI: %d\n", ret);
-		return ret;
+		return dev_err_probe(dev, PTR_ERR(imx219->regmap),
+				     "failed to initialize CCI\n");
 	}
 
 	/* Get system clock (xclk) */
 	imx219->xclk = devm_clk_get(dev, NULL);
 	if (IS_ERR(imx219->xclk)) {
-		dev_err(dev, "failed to get xclk\n");
-		return PTR_ERR(imx219->xclk);
+		return dev_err_probe(dev, PTR_ERR(imx219->xclk),
+				     "failed to get xclk\n");
 	}
 
 	imx219->xclk_freq = clk_get_rate(imx219->xclk);
 	if (imx219->xclk_freq != IMX219_XCLK_FREQ) {
-		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);
 	}
 
 	ret = imx219_get_regulators(imx219);
-	if (ret) {
-		dev_err(dev, "failed to get regulators\n");
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to get regulators\n");
 
 	/* Request optional enable pin */
 	imx219->reset_gpio = devm_gpiod_get_optional(dev, "reset",
@@ -1183,20 +1182,21 @@ static int imx219_probe(struct i2c_client *client)
 
 	ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad);
 	if (ret) {
-		dev_err(dev, "failed to init entity pads: %d\n", ret);
+		dev_err_probe(dev, ret, "failed to init entity pads\n");
 		goto error_handler_free;
 	}
 
 	imx219->sd.state_lock = imx219->ctrl_handler.lock;
 	ret = v4l2_subdev_init_finalize(&imx219->sd);
 	if (ret < 0) {
-		dev_err(dev, "subdev init error: %d\n", ret);
+		dev_err_probe(dev, ret, "subdev init error\n");
 		goto error_media_entity;
 	}
 
 	ret = v4l2_async_register_subdev_sensor(&imx219->sd);
 	if (ret < 0) {
-		dev_err(dev, "failed to register sensor sub-device: %d\n", ret);
+		dev_err_probe(dev, ret,
+			      "failed to register sensor sub-device\n");
 		goto error_subdev_cleanup;
 	}
 
-- 
2.43.0

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

[Tommaso Merciai]

Hi Umang,
Thanks for the patch.

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__);
>  		goto error;
>  	}
>  
> @@ -1025,15 +1026,15 @@ static int imx219_identify_module(struct imx219 *imx219)
>  
>  	ret = cci_read(imx219->regmap, IMX219_REG_CHIP_ID, &val, NULL);
>  	if (ret) {
> -		dev_err(&client->dev, "failed to read chip id %x\n",
> -			IMX219_CHIP_ID);
> -		return ret;
> +		return dev_err_probe(&client->dev, ret,
> +				     "failed to read chip id %x\n",
> +				     IMX219_CHIP_ID);
>  	}

I think you can remove also here the curve brakets we don't need that
anymore.

>  
>  	if (val != IMX219_CHIP_ID) {
> -		dev_err(&client->dev, "chip id mismatch: %x!=%llx\n",
> -			IMX219_CHIP_ID, val);
> -		return -EIO;
> +		return dev_err_probe(&client->dev, -EIO,
> +				     "chip id mismatch: %x!=%llx\n",
> +				     IMX219_CHIP_ID, val);
>  	}

ditto

>  
>  	return 0;
> @@ -1048,35 +1049,36 @@ static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
>  	int ret = -EINVAL;
>  
>  	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
> -	if (!endpoint) {
> -		dev_err(dev, "endpoint node not found\n");
> -		return -EINVAL;
> -	}
> +	if (!endpoint)
> +		return dev_err_probe(dev, -EINVAL, "endpoint node not found\n");
>  
>  	if (v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep_cfg)) {
> -		dev_err(dev, "could not parse endpoint\n");
> +		dev_err_probe(dev, -EINVAL, "could not parse endpoint\n");
>  		goto error_out;
>  	}
>  
>  	/* Check the number of MIPI CSI2 data lanes */
>  	if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2 &&
>  	    ep_cfg.bus.mipi_csi2.num_data_lanes != 4) {
> -		dev_err(dev, "only 2 or 4 data lanes are currently supported\n");
> +		dev_err_probe(dev, -EINVAL,
> +			      "only 2 or 4 data lanes are currently supported\n");
>  		goto error_out;
>  	}
>  	imx219->lanes = ep_cfg.bus.mipi_csi2.num_data_lanes;
>  
>  	/* Check the link frequency set in device tree */
>  	if (!ep_cfg.nr_of_link_frequencies) {
> -		dev_err(dev, "link-frequency property not found in DT\n");
> +		dev_err_probe(dev, -EINVAL,
> +			      "link-frequency property not found in DT\n");
>  		goto error_out;
>  	}
>  
>  	if (ep_cfg.nr_of_link_frequencies != 1 ||
>  	   (ep_cfg.link_frequencies[0] != ((imx219->lanes == 2) ?
>  	    IMX219_DEFAULT_LINK_FREQ : IMX219_DEFAULT_LINK_FREQ_4LANE))) {
> -		dev_err(dev, "Link frequency not supported: %lld\n",
> -			ep_cfg.link_frequencies[0]);
> +		dev_err_probe(dev, -EINVAL,
> +			      "Link frequency not supported: %lld\n",
> +			      ep_cfg.link_frequencies[0]);
>  		goto error_out;
>  	}
>  
> @@ -1108,30 +1110,27 @@ static int imx219_probe(struct i2c_client *client)
>  
>  	imx219->regmap = devm_cci_regmap_init_i2c(client, 16);
>  	if (IS_ERR(imx219->regmap)) {
> -		ret = PTR_ERR(imx219->regmap);
> -		dev_err(dev, "failed to initialize CCI: %d\n", ret);
> -		return ret;
> +		return dev_err_probe(dev, PTR_ERR(imx219->regmap),
> +				     "failed to initialize CCI\n");
>  	}

ditto

>  
>  	/* Get system clock (xclk) */
>  	imx219->xclk = devm_clk_get(dev, NULL);
>  	if (IS_ERR(imx219->xclk)) {
> -		dev_err(dev, "failed to get xclk\n");
> -		return PTR_ERR(imx219->xclk);
> +		return dev_err_probe(dev, PTR_ERR(imx219->xclk),
> +				     "failed to get xclk\n");
>  	}

ditto

>  
>  	imx219->xclk_freq = clk_get_rate(imx219->xclk);
>  	if (imx219->xclk_freq != IMX219_XCLK_FREQ) {
> -		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);
>  	}

ditto

>  
>  	ret = imx219_get_regulators(imx219);
> -	if (ret) {
> -		dev_err(dev, "failed to get regulators\n");
> -		return ret;
> -	}
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to get regulators\n");
>  
>  	/* Request optional enable pin */
>  	imx219->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> @@ -1183,20 +1182,21 @@ static int imx219_probe(struct i2c_client *client)
>  
>  	ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad);
>  	if (ret) {
> -		dev_err(dev, "failed to init entity pads: %d\n", ret);
> +		dev_err_probe(dev, ret, "failed to init entity pads\n");
>  		goto error_handler_free;
>  	}
>  
>  	imx219->sd.state_lock = imx219->ctrl_handler.lock;
>  	ret = v4l2_subdev_init_finalize(&imx219->sd);
>  	if (ret < 0) {
> -		dev_err(dev, "subdev init error: %d\n", ret);
> +		dev_err_probe(dev, ret, "subdev init error\n");
>  		goto error_media_entity;
>  	}
>  
>  	ret = v4l2_async_register_subdev_sensor(&imx219->sd);
>  	if (ret < 0) {
> -		dev_err(dev, "failed to register sensor sub-device: %d\n", ret);
> +		dev_err_probe(dev, ret,
> +			      "failed to register sensor sub-device\n");
>  		goto error_subdev_cleanup;
>  	}
>  
> -- 
> 2.43.0

Thanks & Regards,
Tommaso

> 
> 

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

[Umang Jain]

Hi Tommaso,

On 11/03/24 5:05 pm, Tommaso Merciai wrote:
> Hi Umang,
> Thanks for the patch.
>
> 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__);
>>   		goto error;
>>   	}
>>   
>> @@ -1025,15 +1026,15 @@ static int imx219_identify_module(struct imx219 *imx219)
>>   
>>   	ret = cci_read(imx219->regmap, IMX219_REG_CHIP_ID, &val, NULL);
>>   	if (ret) {
>> -		dev_err(&client->dev, "failed to read chip id %x\n",
>> -			IMX219_CHIP_ID);
>> -		return ret;
>> +		return dev_err_probe(&client->dev, ret,
>> +				     "failed to read chip id %x\n",
>> +				     IMX219_CHIP_ID);
>>   	}
> I think you can remove also here the curve brakets we don't need that
> anymore.

I think multi-line single statement like this one, is better with { ... 
} and is actually preferred?

I actually got a review-comment about this long ago(don't remember when) 
in a non-related, kernel patch series.

I'll leaving this upto maintainers probably
>
>>   
>>   	if (val != IMX219_CHIP_ID) {
>> -		dev_err(&client->dev, "chip id mismatch: %x!=%llx\n",
>> -			IMX219_CHIP_ID, val);
>> -		return -EIO;
>> +		return dev_err_probe(&client->dev, -EIO,
>> +				     "chip id mismatch: %x!=%llx\n",
>> +				     IMX219_CHIP_ID, val);
>>   	}
> ditto
>
>>   
>>   	return 0;
>> @@ -1048,35 +1049,36 @@ static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
>>   	int ret = -EINVAL;
>>   
>>   	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
>> -	if (!endpoint) {
>> -		dev_err(dev, "endpoint node not found\n");
>> -		return -EINVAL;
>> -	}
>> +	if (!endpoint)
>> +		return dev_err_probe(dev, -EINVAL, "endpoint node not found\n");
>>   
>>   	if (v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep_cfg)) {
>> -		dev_err(dev, "could not parse endpoint\n");
>> +		dev_err_probe(dev, -EINVAL, "could not parse endpoint\n");
>>   		goto error_out;
>>   	}
>>   
>>   	/* Check the number of MIPI CSI2 data lanes */
>>   	if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2 &&
>>   	    ep_cfg.bus.mipi_csi2.num_data_lanes != 4) {
>> -		dev_err(dev, "only 2 or 4 data lanes are currently supported\n");
>> +		dev_err_probe(dev, -EINVAL,
>> +			      "only 2 or 4 data lanes are currently supported\n");
>>   		goto error_out;
>>   	}
>>   	imx219->lanes = ep_cfg.bus.mipi_csi2.num_data_lanes;
>>   
>>   	/* Check the link frequency set in device tree */
>>   	if (!ep_cfg.nr_of_link_frequencies) {
>> -		dev_err(dev, "link-frequency property not found in DT\n");
>> +		dev_err_probe(dev, -EINVAL,
>> +			      "link-frequency property not found in DT\n");
>>   		goto error_out;
>>   	}
>>   
>>   	if (ep_cfg.nr_of_link_frequencies != 1 ||
>>   	   (ep_cfg.link_frequencies[0] != ((imx219->lanes == 2) ?
>>   	    IMX219_DEFAULT_LINK_FREQ : IMX219_DEFAULT_LINK_FREQ_4LANE))) {
>> -		dev_err(dev, "Link frequency not supported: %lld\n",
>> -			ep_cfg.link_frequencies[0]);
>> +		dev_err_probe(dev, -EINVAL,
>> +			      "Link frequency not supported: %lld\n",
>> +			      ep_cfg.link_frequencies[0]);
>>   		goto error_out;
>>   	}
>>   
>> @@ -1108,30 +1110,27 @@ static int imx219_probe(struct i2c_client *client)
>>   
>>   	imx219->regmap = devm_cci_regmap_init_i2c(client, 16);
>>   	if (IS_ERR(imx219->regmap)) {
>> -		ret = PTR_ERR(imx219->regmap);
>> -		dev_err(dev, "failed to initialize CCI: %d\n", ret);
>> -		return ret;
>> +		return dev_err_probe(dev, PTR_ERR(imx219->regmap),
>> +				     "failed to initialize CCI\n");
>>   	}
> ditto
>
>>   
>>   	/* Get system clock (xclk) */
>>   	imx219->xclk = devm_clk_get(dev, NULL);
>>   	if (IS_ERR(imx219->xclk)) {
>> -		dev_err(dev, "failed to get xclk\n");
>> -		return PTR_ERR(imx219->xclk);
>> +		return dev_err_probe(dev, PTR_ERR(imx219->xclk),
>> +				     "failed to get xclk\n");
>>   	}
> ditto
>
>>   
>>   	imx219->xclk_freq = clk_get_rate(imx219->xclk);
>>   	if (imx219->xclk_freq != IMX219_XCLK_FREQ) {
>> -		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);
>>   	}
> ditto
>
>>   
>>   	ret = imx219_get_regulators(imx219);
>> -	if (ret) {
>> -		dev_err(dev, "failed to get regulators\n");
>> -		return ret;
>> -	}
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "failed to get regulators\n");
>>   
>>   	/* Request optional enable pin */
>>   	imx219->reset_gpio = devm_gpiod_get_optional(dev, "reset",
>> @@ -1183,20 +1182,21 @@ static int imx219_probe(struct i2c_client *client)
>>   
>>   	ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad);
>>   	if (ret) {
>> -		dev_err(dev, "failed to init entity pads: %d\n", ret);
>> +		dev_err_probe(dev, ret, "failed to init entity pads\n");
>>   		goto error_handler_free;
>>   	}
>>   
>>   	imx219->sd.state_lock = imx219->ctrl_handler.lock;
>>   	ret = v4l2_subdev_init_finalize(&imx219->sd);
>>   	if (ret < 0) {
>> -		dev_err(dev, "subdev init error: %d\n", ret);
>> +		dev_err_probe(dev, ret, "subdev init error\n");
>>   		goto error_media_entity;
>>   	}
>>   
>>   	ret = v4l2_async_register_subdev_sensor(&imx219->sd);
>>   	if (ret < 0) {
>> -		dev_err(dev, "failed to register sensor sub-device: %d\n", ret);
>> +		dev_err_probe(dev, ret,
>> +			      "failed to register sensor sub-device\n");
>>   		goto error_subdev_cleanup;
>>   	}
>>   
>> -- 
>> 2.43.0
> Thanks & Regards,
> Tommaso
>
>>

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

[Tommaso Merciai]

Hi Umang,

On Thu, Mar 14, 2024 at 06:51:04PM +0530, Umang Jain wrote:
> Hi Tommaso,
> 
> On 11/03/24 5:05 pm, Tommaso Merciai wrote:
> > Hi Umang,
> > Thanks for the patch.
> > 
> > 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__);
> > >   		goto error;
> > >   	}
> > > @@ -1025,15 +1026,15 @@ static int imx219_identify_module(struct imx219 *imx219)
> > >   	ret = cci_read(imx219->regmap, IMX219_REG_CHIP_ID, &val, NULL);
> > >   	if (ret) {
> > > -		dev_err(&client->dev, "failed to read chip id %x\n",
> > > -			IMX219_CHIP_ID);
> > > -		return ret;
> > > +		return dev_err_probe(&client->dev, ret,
> > > +				     "failed to read chip id %x\n",
> > > +				     IMX219_CHIP_ID);
> > >   	}
> > I think you can remove also here the curve brakets we don't need that
> > anymore.
> 
> I think multi-line single statement like this one, is better with { ... }
> and is actually preferred?

Yep, some driver is using your pattern some other not.
I'm curious about the truth :) (for my education :))

> 
> I actually got a review-comment about this long ago(don't remember when) in
> a non-related, kernel patch series.
> 
> I'll leaving this upto maintainers probably

Oks, thanks for sharing!

Regards,
Tommaso

> > 
> > >   	if (val != IMX219_CHIP_ID) {
> > > -		dev_err(&client->dev, "chip id mismatch: %x!=%llx\n",
> > > -			IMX219_CHIP_ID, val);
> > > -		return -EIO;
> > > +		return dev_err_probe(&client->dev, -EIO,
> > > +				     "chip id mismatch: %x!=%llx\n",
> > > +				     IMX219_CHIP_ID, val);
> > >   	}
> > ditto
> > 
> > >   	return 0;
> > > @@ -1048,35 +1049,36 @@ static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
> > >   	int ret = -EINVAL;
> > >   	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
> > > -	if (!endpoint) {
> > > -		dev_err(dev, "endpoint node not found\n");
> > > -		return -EINVAL;
> > > -	}
> > > +	if (!endpoint)
> > > +		return dev_err_probe(dev, -EINVAL, "endpoint node not found\n");
> > >   	if (v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep_cfg)) {
> > > -		dev_err(dev, "could not parse endpoint\n");
> > > +		dev_err_probe(dev, -EINVAL, "could not parse endpoint\n");
> > >   		goto error_out;
> > >   	}
> > >   	/* Check the number of MIPI CSI2 data lanes */
> > >   	if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2 &&
> > >   	    ep_cfg.bus.mipi_csi2.num_data_lanes != 4) {
> > > -		dev_err(dev, "only 2 or 4 data lanes are currently supported\n");
> > > +		dev_err_probe(dev, -EINVAL,
> > > +			      "only 2 or 4 data lanes are currently supported\n");
> > >   		goto error_out;
> > >   	}
> > >   	imx219->lanes = ep_cfg.bus.mipi_csi2.num_data_lanes;
> > >   	/* Check the link frequency set in device tree */
> > >   	if (!ep_cfg.nr_of_link_frequencies) {
> > > -		dev_err(dev, "link-frequency property not found in DT\n");
> > > +		dev_err_probe(dev, -EINVAL,
> > > +			      "link-frequency property not found in DT\n");
> > >   		goto error_out;
> > >   	}
> > >   	if (ep_cfg.nr_of_link_frequencies != 1 ||
> > >   	   (ep_cfg.link_frequencies[0] != ((imx219->lanes == 2) ?
> > >   	    IMX219_DEFAULT_LINK_FREQ : IMX219_DEFAULT_LINK_FREQ_4LANE))) {
> > > -		dev_err(dev, "Link frequency not supported: %lld\n",
> > > -			ep_cfg.link_frequencies[0]);
> > > +		dev_err_probe(dev, -EINVAL,
> > > +			      "Link frequency not supported: %lld\n",
> > > +			      ep_cfg.link_frequencies[0]);
> > >   		goto error_out;
> > >   	}
> > > @@ -1108,30 +1110,27 @@ static int imx219_probe(struct i2c_client *client)
> > >   	imx219->regmap = devm_cci_regmap_init_i2c(client, 16);
> > >   	if (IS_ERR(imx219->regmap)) {
> > > -		ret = PTR_ERR(imx219->regmap);
> > > -		dev_err(dev, "failed to initialize CCI: %d\n", ret);
> > > -		return ret;
> > > +		return dev_err_probe(dev, PTR_ERR(imx219->regmap),
> > > +				     "failed to initialize CCI\n");
> > >   	}
> > ditto
> > 
> > >   	/* Get system clock (xclk) */
> > >   	imx219->xclk = devm_clk_get(dev, NULL);
> > >   	if (IS_ERR(imx219->xclk)) {
> > > -		dev_err(dev, "failed to get xclk\n");
> > > -		return PTR_ERR(imx219->xclk);
> > > +		return dev_err_probe(dev, PTR_ERR(imx219->xclk),
> > > +				     "failed to get xclk\n");
> > >   	}
> > ditto
> > 
> > >   	imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > >   	if (imx219->xclk_freq != IMX219_XCLK_FREQ) {
> > > -		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);
> > >   	}
> > ditto
> > 
> > >   	ret = imx219_get_regulators(imx219);
> > > -	if (ret) {
> > > -		dev_err(dev, "failed to get regulators\n");
> > > -		return ret;
> > > -	}
> > > +	if (ret)
> > > +		return dev_err_probe(dev, ret, "failed to get regulators\n");
> > >   	/* Request optional enable pin */
> > >   	imx219->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> > > @@ -1183,20 +1182,21 @@ static int imx219_probe(struct i2c_client *client)
> > >   	ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad);
> > >   	if (ret) {
> > > -		dev_err(dev, "failed to init entity pads: %d\n", ret);
> > > +		dev_err_probe(dev, ret, "failed to init entity pads\n");
> > >   		goto error_handler_free;
> > >   	}
> > >   	imx219->sd.state_lock = imx219->ctrl_handler.lock;
> > >   	ret = v4l2_subdev_init_finalize(&imx219->sd);
> > >   	if (ret < 0) {
> > > -		dev_err(dev, "subdev init error: %d\n", ret);
> > > +		dev_err_probe(dev, ret, "subdev init error\n");
> > >   		goto error_media_entity;
> > >   	}
> > >   	ret = v4l2_async_register_subdev_sensor(&imx219->sd);
> > >   	if (ret < 0) {
> > > -		dev_err(dev, "failed to register sensor sub-device: %d\n", ret);
> > > +		dev_err_probe(dev, ret,
> > > +			      "failed to register sensor sub-device\n");
> > >   		goto error_subdev_cleanup;
> > >   	}
> > > -- 
> > > 2.43.0
> > Thanks & Regards,
> > Tommaso
> > 
> > > 
> 

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

[Laurent Pinchart]

Hi Umang,

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 ?

> >>   		goto error;
> >>   	}
> >>   
> >> @@ -1025,15 +1026,15 @@ static int imx219_identify_module(struct imx219 *imx219)
> >>   
> >>   	ret = cci_read(imx219->regmap, IMX219_REG_CHIP_ID, &val, NULL);
> >>   	if (ret) {
> >> -		dev_err(&client->dev, "failed to read chip id %x\n",
> >> -			IMX219_CHIP_ID);
> >> -		return ret;
> >> +		return dev_err_probe(&client->dev, ret,
> >> +				     "failed to read chip id %x\n",
> >> +				     IMX219_CHIP_ID);
> >>   	}
> >
> > I think you can remove also here the curve brakets we don't need that
> > anymore.
> 
> I think multi-line single statement like this one, is better with { ... 
> } and is actually preferred?
> 
> I actually got a review-comment about this long ago(don't remember when) 
> in a non-related, kernel patch series.
> 
> I'll leaving this upto maintainers probably

I think the preferred coding style in the media subsystem is to leave
the curly brackets out for all single statements, even if the statement
spans multiple lines.

> >>   
> >>   	if (val != IMX219_CHIP_ID) {
> >> -		dev_err(&client->dev, "chip id mismatch: %x!=%llx\n",
> >> -			IMX219_CHIP_ID, val);
> >> -		return -EIO;
> >> +		return dev_err_probe(&client->dev, -EIO,
> >> +				     "chip id mismatch: %x!=%llx\n",
> >> +				     IMX219_CHIP_ID, val);
> >>   	}
> >
> > ditto
> >
> >>   
> >>   	return 0;
> >> @@ -1048,35 +1049,36 @@ static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
> >>   	int ret = -EINVAL;
> >>   
> >>   	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
> >> -	if (!endpoint) {
> >> -		dev_err(dev, "endpoint node not found\n");
> >> -		return -EINVAL;
> >> -	}
> >> +	if (!endpoint)
> >> +		return dev_err_probe(dev, -EINVAL, "endpoint node not found\n");

Same here, what's the advantage of using dev_err_probe() when you
hardcode the error value to something different than -EPROBE_DEFER ?
Same below.

> >>   
> >>   	if (v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep_cfg)) {
> >> -		dev_err(dev, "could not parse endpoint\n");
> >> +		dev_err_probe(dev, -EINVAL, "could not parse endpoint\n");

This I like even less. -EINVAL is meaningless here, it's not even
guaranteed to be the error code that the function will return.

> >>   		goto error_out;
> >>   	}
> >>   
> >>   	/* Check the number of MIPI CSI2 data lanes */
> >>   	if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2 &&
> >>   	    ep_cfg.bus.mipi_csi2.num_data_lanes != 4) {
> >> -		dev_err(dev, "only 2 or 4 data lanes are currently supported\n");
> >> +		dev_err_probe(dev, -EINVAL,
> >> +			      "only 2 or 4 data lanes are currently supported\n");
> >>   		goto error_out;
> >>   	}
> >>   	imx219->lanes = ep_cfg.bus.mipi_csi2.num_data_lanes;
> >>   
> >>   	/* Check the link frequency set in device tree */
> >>   	if (!ep_cfg.nr_of_link_frequencies) {
> >> -		dev_err(dev, "link-frequency property not found in DT\n");
> >> +		dev_err_probe(dev, -EINVAL,
> >> +			      "link-frequency property not found in DT\n");
> >>   		goto error_out;
> >>   	}
> >>   
> >>   	if (ep_cfg.nr_of_link_frequencies != 1 ||
> >>   	   (ep_cfg.link_frequencies[0] != ((imx219->lanes == 2) ?
> >>   	    IMX219_DEFAULT_LINK_FREQ : IMX219_DEFAULT_LINK_FREQ_4LANE))) {
> >> -		dev_err(dev, "Link frequency not supported: %lld\n",
> >> -			ep_cfg.link_frequencies[0]);
> >> +		dev_err_probe(dev, -EINVAL,
> >> +			      "Link frequency not supported: %lld\n",
> >> +			      ep_cfg.link_frequencies[0]);
> >>   		goto error_out;
> >>   	}
> >>   
> >> @@ -1108,30 +1110,27 @@ static int imx219_probe(struct i2c_client *client)
> >>   
> >>   	imx219->regmap = devm_cci_regmap_init_i2c(client, 16);
> >>   	if (IS_ERR(imx219->regmap)) {
> >> -		ret = PTR_ERR(imx219->regmap);
> >> -		dev_err(dev, "failed to initialize CCI: %d\n", ret);
> >> -		return ret;
> >> +		return dev_err_probe(dev, PTR_ERR(imx219->regmap),
> >> +				     "failed to initialize CCI\n");
> >>   	}
> >
> > ditto
> >
> >>   
> >>   	/* Get system clock (xclk) */
> >>   	imx219->xclk = devm_clk_get(dev, NULL);
> >>   	if (IS_ERR(imx219->xclk)) {
> >> -		dev_err(dev, "failed to get xclk\n");
> >> -		return PTR_ERR(imx219->xclk);
> >> +		return dev_err_probe(dev, PTR_ERR(imx219->xclk),
> >> +				     "failed to get xclk\n");

This change makes sense, the clock provider may not have probed yet
(even if it's quite unlikely).

> >>   	}
> >
> > ditto
> >
> >>   
> >>   	imx219->xclk_freq = clk_get_rate(imx219->xclk);
> >>   	if (imx219->xclk_freq != IMX219_XCLK_FREQ) {
> >> -		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);
> >>   	}
> >
> > ditto
> >
> >>   
> >>   	ret = imx219_get_regulators(imx219);
> >> -	if (ret) {
> >> -		dev_err(dev, "failed to get regulators\n");
> >> -		return ret;
> >> -	}
> >> +	if (ret)
> >> +		return dev_err_probe(dev, ret, "failed to get regulators\n");

Same here.

> >>   
> >>   	/* Request optional enable pin */
> >>   	imx219->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> >> @@ -1183,20 +1182,21 @@ static int imx219_probe(struct i2c_client *client)
> >>   
> >>   	ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad);
> >>   	if (ret) {
> >> -		dev_err(dev, "failed to init entity pads: %d\n", ret);
> >> +		dev_err_probe(dev, ret, "failed to init entity pads\n");
> >>   		goto error_handler_free;
> >>   	}
> >>   
> >>   	imx219->sd.state_lock = imx219->ctrl_handler.lock;
> >>   	ret = v4l2_subdev_init_finalize(&imx219->sd);
> >>   	if (ret < 0) {
> >> -		dev_err(dev, "subdev init error: %d\n", ret);
> >> +		dev_err_probe(dev, ret, "subdev init error\n");
> >>   		goto error_media_entity;
> >>   	}
> >>   
> >>   	ret = v4l2_async_register_subdev_sensor(&imx219->sd);
> >>   	if (ret < 0) {
> >> -		dev_err(dev, "failed to register sensor sub-device: %d\n", ret);
> >> +		dev_err_probe(dev, ret,
> >> +			      "failed to register sensor sub-device\n");
> >>   		goto error_subdev_cleanup;
> >>   	}
> >>   

-- 
Regards,

Laurent Pinchart

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

[Sakari Ailus]

On Thu, Mar 14, 2024 at 03:58:44PM +0100, Tommaso Merciai wrote:
> > > > @@ -1025,15 +1026,15 @@ static int imx219_identify_module(struct imx219 *imx219)
> > > >   	ret = cci_read(imx219->regmap, IMX219_REG_CHIP_ID, &val, NULL);
> > > >   	if (ret) {
> > > > -		dev_err(&client->dev, "failed to read chip id %x\n",
> > > > -			IMX219_CHIP_ID);
> > > > -		return ret;
> > > > +		return dev_err_probe(&client->dev, ret,
> > > > +				     "failed to read chip id %x\n",
> > > > +				     IMX219_CHIP_ID);
> > > >   	}
> > > I think you can remove also here the curve brakets we don't need that
> > > anymore.
> > 
> > I think multi-line single statement like this one, is better with { ... }
> > and is actually preferred?
> 
> Yep, some driver is using your pattern some other not.
> I'm curious about the truth :) (for my education :))

I'd prefer it without braces.

See the end of section 3 in Documentation/process/coding-style.rst .

-- 
Sakari Ailus

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

[Umang Jain]

HI Laurent,

On 14/03/24 8:51 pm, Laurent Pinchart wrote:
> Hi Umang,
>
> 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.
  *

```
>
>>>>    		goto error;
>>>>    	}
>>>>    
>>>> @@ -1025,15 +1026,15 @@ static int imx219_identify_module(struct imx219 *imx219)
>>>>    
>>>>    	ret = cci_read(imx219->regmap, IMX219_REG_CHIP_ID, &val, NULL);
>>>>    	if (ret) {
>>>> -		dev_err(&client->dev, "failed to read chip id %x\n",
>>>> -			IMX219_CHIP_ID);
>>>> -		return ret;
>>>> +		return dev_err_probe(&client->dev, ret,
>>>> +				     "failed to read chip id %x\n",
>>>> +				     IMX219_CHIP_ID);
>>>>    	}
>>> I think you can remove also here the curve brakets we don't need that
>>> anymore.
>> I think multi-line single statement like this one, is better with { ...
>> } and is actually preferred?
>>
>> I actually got a review-comment about this long ago(don't remember when)
>> in a non-related, kernel patch series.
>>
>> I'll leaving this upto maintainers probably
> I think the preferred coding style in the media subsystem is to leave
> the curly brackets out for all single statements, even if the statement
> spans multiple lines.

ack
>
>>>>    
>>>>    	if (val != IMX219_CHIP_ID) {
>>>> -		dev_err(&client->dev, "chip id mismatch: %x!=%llx\n",
>>>> -			IMX219_CHIP_ID, val);
>>>> -		return -EIO;
>>>> +		return dev_err_probe(&client->dev, -EIO,
>>>> +				     "chip id mismatch: %x!=%llx\n",
>>>> +				     IMX219_CHIP_ID, val);
>>>>    	}
>>> ditto
>>>
>>>>    
>>>>    	return 0;
>>>> @@ -1048,35 +1049,36 @@ static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
>>>>    	int ret = -EINVAL;
>>>>    
>>>>    	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
>>>> -	if (!endpoint) {
>>>> -		dev_err(dev, "endpoint node not found\n");
>>>> -		return -EINVAL;
>>>> -	}
>>>> +	if (!endpoint)
>>>> +		return dev_err_probe(dev, -EINVAL, "endpoint node not found\n");
> Same here, what's the advantage of using dev_err_probe() when you
> hardcode the error value to something different than -EPROBE_DEFER ?
> Same below.
>
>>>>    
>>>>    	if (v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep_cfg)) {
>>>> -		dev_err(dev, "could not parse endpoint\n");
>>>> +		dev_err_probe(dev, -EINVAL, "could not parse endpoint\n");
> This I like even less. -EINVAL is meaningless here, it's not even
> guaranteed to be the error code that the function will return.
>
>>>>    		goto error_out;
>>>>    	}
>>>>    
>>>>    	/* Check the number of MIPI CSI2 data lanes */
>>>>    	if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2 &&
>>>>    	    ep_cfg.bus.mipi_csi2.num_data_lanes != 4) {
>>>> -		dev_err(dev, "only 2 or 4 data lanes are currently supported\n");
>>>> +		dev_err_probe(dev, -EINVAL,
>>>> +			      "only 2 or 4 data lanes are currently supported\n");
>>>>    		goto error_out;
>>>>    	}
>>>>    	imx219->lanes = ep_cfg.bus.mipi_csi2.num_data_lanes;
>>>>    
>>>>    	/* Check the link frequency set in device tree */
>>>>    	if (!ep_cfg.nr_of_link_frequencies) {
>>>> -		dev_err(dev, "link-frequency property not found in DT\n");
>>>> +		dev_err_probe(dev, -EINVAL,
>>>> +			      "link-frequency property not found in DT\n");
>>>>    		goto error_out;
>>>>    	}
>>>>    
>>>>    	if (ep_cfg.nr_of_link_frequencies != 1 ||
>>>>    	   (ep_cfg.link_frequencies[0] != ((imx219->lanes == 2) ?
>>>>    	    IMX219_DEFAULT_LINK_FREQ : IMX219_DEFAULT_LINK_FREQ_4LANE))) {
>>>> -		dev_err(dev, "Link frequency not supported: %lld\n",
>>>> -			ep_cfg.link_frequencies[0]);
>>>> +		dev_err_probe(dev, -EINVAL,
>>>> +			      "Link frequency not supported: %lld\n",
>>>> +			      ep_cfg.link_frequencies[0]);
>>>>    		goto error_out;
>>>>    	}
>>>>    
>>>> @@ -1108,30 +1110,27 @@ static int imx219_probe(struct i2c_client *client)
>>>>    
>>>>    	imx219->regmap = devm_cci_regmap_init_i2c(client, 16);
>>>>    	if (IS_ERR(imx219->regmap)) {
>>>> -		ret = PTR_ERR(imx219->regmap);
>>>> -		dev_err(dev, "failed to initialize CCI: %d\n", ret);
>>>> -		return ret;
>>>> +		return dev_err_probe(dev, PTR_ERR(imx219->regmap),
>>>> +				     "failed to initialize CCI\n");
>>>>    	}
>>> ditto
>>>
>>>>    
>>>>    	/* Get system clock (xclk) */
>>>>    	imx219->xclk = devm_clk_get(dev, NULL);
>>>>    	if (IS_ERR(imx219->xclk)) {
>>>> -		dev_err(dev, "failed to get xclk\n");
>>>> -		return PTR_ERR(imx219->xclk);
>>>> +		return dev_err_probe(dev, PTR_ERR(imx219->xclk),
>>>> +				     "failed to get xclk\n");
> This change makes sense, the clock provider may not have probed yet
> (even if it's quite unlikely).
>
>>>>    	}
>>> ditto
>>>
>>>>    
>>>>    	imx219->xclk_freq = clk_get_rate(imx219->xclk);
>>>>    	if (imx219->xclk_freq != IMX219_XCLK_FREQ) {
>>>> -		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);
>>>>    	}
>>> ditto
>>>
>>>>    
>>>>    	ret = imx219_get_regulators(imx219);
>>>> -	if (ret) {
>>>> -		dev_err(dev, "failed to get regulators\n");
>>>> -		return ret;
>>>> -	}
>>>> +	if (ret)
>>>> +		return dev_err_probe(dev, ret, "failed to get regulators\n");
> Same here.
>
>>>>    
>>>>    	/* Request optional enable pin */
>>>>    	imx219->reset_gpio = devm_gpiod_get_optional(dev, "reset",
>>>> @@ -1183,20 +1182,21 @@ static int imx219_probe(struct i2c_client *client)
>>>>    
>>>>    	ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad);
>>>>    	if (ret) {
>>>> -		dev_err(dev, "failed to init entity pads: %d\n", ret);
>>>> +		dev_err_probe(dev, ret, "failed to init entity pads\n");
>>>>    		goto error_handler_free;
>>>>    	}
>>>>    
>>>>    	imx219->sd.state_lock = imx219->ctrl_handler.lock;
>>>>    	ret = v4l2_subdev_init_finalize(&imx219->sd);
>>>>    	if (ret < 0) {
>>>> -		dev_err(dev, "subdev init error: %d\n", ret);
>>>> +		dev_err_probe(dev, ret, "subdev init error\n");
>>>>    		goto error_media_entity;
>>>>    	}
>>>>    
>>>>    	ret = v4l2_async_register_subdev_sensor(&imx219->sd);
>>>>    	if (ret < 0) {
>>>> -		dev_err(dev, "failed to register sensor sub-device: %d\n", ret);
>>>> +		dev_err_probe(dev, ret,
>>>> +			      "failed to register sensor sub-device\n");
>>>>    		goto error_subdev_cleanup;
>>>>    	}
>>>>    

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

[Laurent Pinchart]

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

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

[Umang Jain]

Hi Laurent,

On 28/03/24 4:23 pm, Laurent Pinchart wrote:
> 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.

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]

[1] 
https://lore.kernel.org/all/CAHp75Vcvvadd6EeTWk2ZDrmtCQzWBV8rOoxNCotzYRRPwecaEA@mail.gmail.com/
>
>>>>>>     		goto error;
>>>>>>     	}
> [snip]
>

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

[Laurent Pinchart]

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 :-)

> [1] https://lore.kernel.org/all/CAHp75Vcvvadd6EeTWk2ZDrmtCQzWBV8rOoxNCotzYRRPwecaEA@mail.gmail.com/
>
> >>>>>>     		goto error;
> >>>>>>     	}
> >
> > [snip]

-- 
Regards,

Laurent Pinchart

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

[Umang Jain]

hello

On 28/03/24 4:39 pm, 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 :-)

There is also a v2 in case someone has missed it:
https://patchwork.kernel.org/project/linux-media/patch/20240320070027.77194-1-umang.jain@ideasonboard.com/
>
>> [1] https://lore.kernel.org/all/CAHp75Vcvvadd6EeTWk2ZDrmtCQzWBV8rOoxNCotzYRRPwecaEA@mail.gmail.com/
>>
>>>>>>>>      		goto error;
>>>>>>>>      	}
>>> [snip]

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

[Sakari Ailus]

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