diff mbox series

[1/2] media: i2c: ov5670: Use dev_err_probe in probe function

Message ID 20230210-ov5670-single-lane-v1-1-71835d39c1ce@z3ntu.xyz (mailing list archive)
State New, archived
Headers show
Series Single-lane CSI-2 operation on OmniVision OV5670 | expand

Commit Message

Luca Weiss Feb. 10, 2023, 8:33 p.m. UTC 
Replace the unusual const char *err_msg usage with dev_err_probe which
also handles -EPROBE_DEFER better by not printing the message to kmsg.

Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
 drivers/media/i2c/ov5670.c | 37 ++++++++++++-------------------------
 1 file changed, 12 insertions(+), 25 deletions(-)

Comments

Jacopo Mondi Feb. 13, 2023, 6:04 p.m. UTC | #1 
Hi Luca

On Fri, Feb 10, 2023 at 09:33:17PM +0100, Luca Weiss wrote:
> Replace the unusual const char *err_msg usage with dev_err_probe which
> also handles -EPROBE_DEFER better by not printing the message to kmsg.
>
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>

Thanks! The *err_msg thing was weird indeed

> ---
>  drivers/media/i2c/ov5670.c | 37 ++++++++++++-------------------------
>  1 file changed, 12 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> index f79d908f4531..189920571df2 100644
> --- a/drivers/media/i2c/ov5670.c
> +++ b/drivers/media/i2c/ov5670.c
> @@ -2648,17 +2648,13 @@ static int ov5670_gpio_probe(struct ov5670 *ov5670)
>  static int ov5670_probe(struct i2c_client *client)
>  {
>  	struct ov5670 *ov5670;
> -	const char *err_msg;
>  	u32 input_clk = 0;
>  	bool full_power;
>  	int ret;
>
>  	ov5670 = devm_kzalloc(&client->dev, sizeof(*ov5670), GFP_KERNEL);
> -	if (!ov5670) {
> -		ret = -ENOMEM;
> -		err_msg = "devm_kzalloc() error";
> -		goto error_print;
> -	}
> +	if (!ov5670)
> +		return -ENOMEM;
>
>  	ov5670->xvclk = devm_clk_get(&client->dev, NULL);
>  	if (!IS_ERR_OR_NULL(ov5670->xvclk))
> @@ -2680,29 +2676,23 @@ static int ov5670_probe(struct i2c_client *client)
>  	v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops);
>
>  	ret = ov5670_regulators_probe(ov5670);
> -	if (ret) {
> -		err_msg = "Regulators probe failed";
> -		goto error_print;
> -	}
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret, "Regulators probe failed\n");
>
>  	ret = ov5670_gpio_probe(ov5670);
> -	if (ret) {
> -		err_msg = "GPIO probe failed";
> -		goto error_print;
> -	}
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret, "GPIO probe failed\n");

From now on, I don't think there are functions that can return
-EPROBE_DEFER and you could

        if (ret) {
                dev_err(...)
                goto ...;
        }

But if others are fine with what you have and consider using
dev_err_probe() better regardless if the called function can
return -EPROBE_DEFER or not, I'm fine with what you have here.

>
>  	full_power = acpi_dev_state_d0(&client->dev);
>  	if (full_power) {
>  		ret = ov5670_runtime_resume(&client->dev);
> -		if (ret) {
> -			err_msg = "Power up failed";
> -			goto error_print;
> -		}
> +		if (ret)
> +			return dev_err_probe(&client->dev, ret, "Power up failed\n");
>
>  		/* Check module identity */
>  		ret = ov5670_identify_module(ov5670);
>  		if (ret) {
> -			err_msg = "ov5670_identify_module() error";
> +			dev_err_probe(&client->dev, ret, "ov5670_identify_module() error\n");
>  			goto error_power_off;
>  		}
>  	}
> @@ -2714,7 +2704,7 @@ static int ov5670_probe(struct i2c_client *client)
>
>  	ret = ov5670_init_controls(ov5670);
>  	if (ret) {
> -		err_msg = "ov5670_init_controls() error";
> +		dev_err_probe(&client->dev, ret, "ov5670_init_controls() error\n");
>  		goto error_mutex_destroy;
>  	}
>
> @@ -2727,7 +2717,7 @@ static int ov5670_probe(struct i2c_client *client)
>  	ov5670->pad.flags = MEDIA_PAD_FL_SOURCE;
>  	ret = media_entity_pads_init(&ov5670->sd.entity, 1, &ov5670->pad);
>  	if (ret) {
> -		err_msg = "media_entity_pads_init() error";
> +		dev_err_probe(&client->dev, ret, "media_entity_pads_init() error\n");
>  		goto error_handler_free;
>  	}
>
> @@ -2741,7 +2731,7 @@ static int ov5670_probe(struct i2c_client *client)
>  	/* Async register for subdev */
>  	ret = v4l2_async_register_subdev_sensor(&ov5670->sd);
>  	if (ret < 0) {
> -		err_msg = "v4l2_async_register_subdev() error";
> +		dev_err_probe(&client->dev, ret, "v4l2_async_register_subdev() error\n");
>  		goto error_pm_disable;
>  	}
>
> @@ -2764,9 +2754,6 @@ static int ov5670_probe(struct i2c_client *client)
>  	if (full_power)
>  		ov5670_runtime_suspend(&client->dev);
>
> -error_print:
> -	dev_err(&client->dev, "%s: %s %d\n", __func__, err_msg, ret);
> -
>  	return ret;
>  }
>
>
> --
> 2.39.1
>
Luca Weiss Feb. 15, 2023, 7:12 p.m. UTC | #2 
On Montag, 13. Februar 2023 19:04:29 CET Jacopo Mondi wrote:
> Hi Luca
> 
> On Fri, Feb 10, 2023 at 09:33:17PM +0100, Luca Weiss wrote:
> > Replace the unusual const char *err_msg usage with dev_err_probe which
> > also handles -EPROBE_DEFER better by not printing the message to kmsg.
> > 
> > Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> 
> Thanks! The *err_msg thing was weird indeed
> 
> > ---
> > 
> >  drivers/media/i2c/ov5670.c | 37 ++++++++++++-------------------------
> >  1 file changed, 12 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> > index f79d908f4531..189920571df2 100644
> > --- a/drivers/media/i2c/ov5670.c
> > +++ b/drivers/media/i2c/ov5670.c
> > @@ -2648,17 +2648,13 @@ static int ov5670_gpio_probe(struct ov5670
> > *ov5670)
> > 
> >  static int ov5670_probe(struct i2c_client *client)
> >  {
> >  
> >  	struct ov5670 *ov5670;
> > 
> > -	const char *err_msg;
> > 
> >  	u32 input_clk = 0;
> >  	bool full_power;
> >  	int ret;
> >  	
> >  	ov5670 = devm_kzalloc(&client->dev, sizeof(*ov5670), GFP_KERNEL);
> > 
> > -	if (!ov5670) {
> > -		ret = -ENOMEM;
> > -		err_msg = "devm_kzalloc() error";
> > -		goto error_print;
> > -	}
> > +	if (!ov5670)
> > +		return -ENOMEM;
> > 
> >  	ov5670->xvclk = devm_clk_get(&client->dev, NULL);
> >  	if (!IS_ERR_OR_NULL(ov5670->xvclk))
> > 
> > @@ -2680,29 +2676,23 @@ static int ov5670_probe(struct i2c_client *client)
> > 
> >  	v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops);
> >  	
> >  	ret = ov5670_regulators_probe(ov5670);
> > 
> > -	if (ret) {
> > -		err_msg = "Regulators probe failed";
> > -		goto error_print;
> > -	}
> > +	if (ret)
> > +		return dev_err_probe(&client->dev, ret, "Regulators 
probe failed\n");
> > 
> >  	ret = ov5670_gpio_probe(ov5670);
> > 
> > -	if (ret) {
> > -		err_msg = "GPIO probe failed";
> > -		goto error_print;
> > -	}
> > +	if (ret)
> > +		return dev_err_probe(&client->dev, ret, "GPIO probe 
failed\n");
> 
> From now on, I don't think there are functions that can return
> -EPROBE_DEFER and you could
> 
>         if (ret) {
>                 dev_err(...)
>                 goto ...;
>         }
> 
> But if others are fine with what you have and consider using
> dev_err_probe() better regardless if the called function can
> return -EPROBE_DEFER or not, I'm fine with what you have here.

I like using dev_err_probe more or less everywhere in _probe so if the 
maintainers don't mind, I'd keep it that way :)

Regards
Luca

> 
> >  	full_power = acpi_dev_state_d0(&client->dev);
> >  	if (full_power) {
> >  	
> >  		ret = ov5670_runtime_resume(&client->dev);
> > 
> > -		if (ret) {
> > -			err_msg = "Power up failed";
> > -			goto error_print;
> > -		}
> > +		if (ret)
> > +			return dev_err_probe(&client->dev, ret, 
"Power up failed\n");
> > 
> >  		/* Check module identity */
> >  		ret = ov5670_identify_module(ov5670);
> >  		if (ret) {
> > 
> > -			err_msg = "ov5670_identify_module() error";
> > +			dev_err_probe(&client->dev, ret, 
"ov5670_identify_module() error\n");
> > 
> >  			goto error_power_off;
> >  		
> >  		}
> >  	
> >  	}
> > 
> > @@ -2714,7 +2704,7 @@ static int ov5670_probe(struct i2c_client *client)
> > 
> >  	ret = ov5670_init_controls(ov5670);
> >  	if (ret) {
> > 
> > -		err_msg = "ov5670_init_controls() error";
> > +		dev_err_probe(&client->dev, ret, 
"ov5670_init_controls() error\n");
> > 
> >  		goto error_mutex_destroy;
> >  	
> >  	}
> > 
> > @@ -2727,7 +2717,7 @@ static int ov5670_probe(struct i2c_client *client)
> > 
> >  	ov5670->pad.flags = MEDIA_PAD_FL_SOURCE;
> >  	ret = media_entity_pads_init(&ov5670->sd.entity, 1, &ov5670->pad);
> >  	if (ret) {
> > 
> > -		err_msg = "media_entity_pads_init() error";
> > +		dev_err_probe(&client->dev, ret, 
"media_entity_pads_init() error\n");
> > 
> >  		goto error_handler_free;
> >  	
> >  	}
> > 
> > @@ -2741,7 +2731,7 @@ static int ov5670_probe(struct i2c_client *client)
> > 
> >  	/* Async register for subdev */
> >  	ret = v4l2_async_register_subdev_sensor(&ov5670->sd);
> >  	if (ret < 0) {
> > 
> > -		err_msg = "v4l2_async_register_subdev() error";
> > +		dev_err_probe(&client->dev, ret, 
"v4l2_async_register_subdev()
> > error\n");> 
> >  		goto error_pm_disable;
> >  	
> >  	}
> > 
> > @@ -2764,9 +2754,6 @@ static int ov5670_probe(struct i2c_client *client)
> > 
> >  	if (full_power)
> >  	
> >  		ov5670_runtime_suspend(&client->dev);
> > 
> > -error_print:
> > -	dev_err(&client->dev, "%s: %s %d\n", __func__, err_msg, ret);
> > -
> > 
> >  	return ret;
> >  
> >  }
> > 
> > --
> > 2.39.1
diff mbox series

Patch

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index f79d908f4531..189920571df2 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -2648,17 +2648,13 @@  static int ov5670_gpio_probe(struct ov5670 *ov5670)
 static int ov5670_probe(struct i2c_client *client)
 {
 	struct ov5670 *ov5670;
-	const char *err_msg;
 	u32 input_clk = 0;
 	bool full_power;
 	int ret;
 
 	ov5670 = devm_kzalloc(&client->dev, sizeof(*ov5670), GFP_KERNEL);
-	if (!ov5670) {
-		ret = -ENOMEM;
-		err_msg = "devm_kzalloc() error";
-		goto error_print;
-	}
+	if (!ov5670)
+		return -ENOMEM;
 
 	ov5670->xvclk = devm_clk_get(&client->dev, NULL);
 	if (!IS_ERR_OR_NULL(ov5670->xvclk))
@@ -2680,29 +2676,23 @@  static int ov5670_probe(struct i2c_client *client)
 	v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops);
 
 	ret = ov5670_regulators_probe(ov5670);
-	if (ret) {
-		err_msg = "Regulators probe failed";
-		goto error_print;
-	}
+	if (ret)
+		return dev_err_probe(&client->dev, ret, "Regulators probe failed\n");
 
 	ret = ov5670_gpio_probe(ov5670);
-	if (ret) {
-		err_msg = "GPIO probe failed";
-		goto error_print;
-	}
+	if (ret)
+		return dev_err_probe(&client->dev, ret, "GPIO probe failed\n");
 
 	full_power = acpi_dev_state_d0(&client->dev);
 	if (full_power) {
 		ret = ov5670_runtime_resume(&client->dev);
-		if (ret) {
-			err_msg = "Power up failed";
-			goto error_print;
-		}
+		if (ret)
+			return dev_err_probe(&client->dev, ret, "Power up failed\n");
 
 		/* Check module identity */
 		ret = ov5670_identify_module(ov5670);
 		if (ret) {
-			err_msg = "ov5670_identify_module() error";
+			dev_err_probe(&client->dev, ret, "ov5670_identify_module() error\n");
 			goto error_power_off;
 		}
 	}
@@ -2714,7 +2704,7 @@  static int ov5670_probe(struct i2c_client *client)
 
 	ret = ov5670_init_controls(ov5670);
 	if (ret) {
-		err_msg = "ov5670_init_controls() error";
+		dev_err_probe(&client->dev, ret, "ov5670_init_controls() error\n");
 		goto error_mutex_destroy;
 	}
 
@@ -2727,7 +2717,7 @@  static int ov5670_probe(struct i2c_client *client)
 	ov5670->pad.flags = MEDIA_PAD_FL_SOURCE;
 	ret = media_entity_pads_init(&ov5670->sd.entity, 1, &ov5670->pad);
 	if (ret) {
-		err_msg = "media_entity_pads_init() error";
+		dev_err_probe(&client->dev, ret, "media_entity_pads_init() error\n");
 		goto error_handler_free;
 	}
 
@@ -2741,7 +2731,7 @@  static int ov5670_probe(struct i2c_client *client)
 	/* Async register for subdev */
 	ret = v4l2_async_register_subdev_sensor(&ov5670->sd);
 	if (ret < 0) {
-		err_msg = "v4l2_async_register_subdev() error";
+		dev_err_probe(&client->dev, ret, "v4l2_async_register_subdev() error\n");
 		goto error_pm_disable;
 	}
 
@@ -2764,9 +2754,6 @@  static int ov5670_probe(struct i2c_client *client)
 	if (full_power)
 		ov5670_runtime_suspend(&client->dev);
 
-error_print:
-	dev_err(&client->dev, "%s: %s %d\n", __func__, err_msg, ret);
-
 	return ret;
 }