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