Re: [PATCH 08/10] media: i2c: ov9282: harmonize dev_err_probe usage

[Richard Leitner]

Hi Dave,

thanks for the review!

On Tue, Sep 15, 2026 at 03:55:01PM +0100, Dave Stevenson wrote:
> Hi Richard
> 
> On Mon, 14 Sept 2026 at 20:21, Richard Leitner
> <richard.leitner@xxxxxxxxx> wrote:
> >
> > Use dev_err_probe() for all error messages during probing. This ensures
> > there's a common "look-and-feel" in the drivers source code as well as
> > the system log.
> >
> > Signed-off-by: Richard Leitner <richard.leitner@xxxxxxxxx>
> > ---
> >  drivers/media/i2c/ov9282.c | 38 ++++++++++++++++++--------------------
> >  1 file changed, 18 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index 28f8b05b4c09e..f728709fcf0a6 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -1109,26 +1109,25 @@ static int ov9282_parse_hw_config(struct ov9282 *ov9282)
> >         ov9282->reset_gpio = devm_gpiod_get_optional(ov9282->dev, "reset",
> >                                                      GPIOD_OUT_LOW);
> >         if (IS_ERR(ov9282->reset_gpio)) {
> > -               dev_err(ov9282->dev, "failed to get reset gpio %pe",
> > -                       ov9282->reset_gpio);
> > -               return PTR_ERR(ov9282->reset_gpio);
> > +               return dev_err_probe(ov9282->dev, PTR_ERR(ov9282->reset_gpio),
> > +                                    "failed to get reset gpio");
> >         }
> >
> >         /* Get sensor input clock */
> >         ov9282->inclk = devm_v4l2_sensor_clk_get(ov9282->dev, NULL);
> >         if (IS_ERR(ov9282->inclk))
> >                 return dev_err_probe(ov9282->dev, PTR_ERR(ov9282->inclk),
> > -                                    "could not get inclk\n");
> > +                                    "could not get inclk");
> 
> My understanding is that dev_err_probe should always have the \n on
> the end of the log text.
> Admittedly "failed to get reset gpio %pe" above is missing it, but
> removing it off all the other instances seems to be the wrong fix.

Thanks for the pointer. I wasn't aware of this convention. Also the
documentation of dev_err_probe mentions nothing. Nonetheless when
grepping through the kernel source I totally agree. "\n" is definitely
preferred.

I will adapt this for v2.

regards;rl

> 
> Otherwise the patch looks fine.
> 
>   Dave
> 
> >
> >         ret = ov9282_configure_regulators(ov9282);
> >         if (ret)
> >                 return dev_err_probe(ov9282->dev, ret,
> > -                                    "Failed to get power regulators\n");
> > +                                    "Failed to get power regulators");
> >
> >         rate = clk_get_rate(ov9282->inclk);
> >         if (rate != OV9282_INCLK_RATE) {
> > -               dev_err(ov9282->dev, "inclk frequency mismatch");
> > -               return -EINVAL;
> > +               return dev_err_probe(ov9282->dev, -EINVAL,
> > +                                    "inclk frequency mismatch");
> >         }
> >
> >         ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
> > @@ -1144,16 +1143,15 @@ static int ov9282_parse_hw_config(struct ov9282 *ov9282)
> >                 bus_cfg.bus.mipi_csi2.flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK;
> >
> >         if (bus_cfg.bus.mipi_csi2.num_data_lanes != OV9282_NUM_DATA_LANES) {
> > -               dev_err(ov9282->dev,
> > -                       "number of CSI2 data lanes %d is not supported",
> > -                       bus_cfg.bus.mipi_csi2.num_data_lanes);
> > -               ret = -EINVAL;
> > +               ret = dev_err_probe(ov9282->dev, -EINVAL,
> > +                                   "number of CSI2 data lanes %d is not supported",
> > +                                   bus_cfg.bus.mipi_csi2.num_data_lanes);
> >                 goto done_endpoint_free;
> >         }
> >
> >         if (!bus_cfg.nr_of_link_frequencies) {
> > -               dev_err(ov9282->dev, "no link frequencies defined");
> > -               ret = -EINVAL;
> > +               ret = dev_err_probe(ov9282->dev, -EINVAL,
> > +                                   "no link frequencies defined");
> >                 goto done_endpoint_free;
> >         }
> >
> > @@ -1382,14 +1380,14 @@ static int ov9282_probe(struct i2c_client *client)
> >
> >         ret = ov9282_parse_hw_config(ov9282);
> >         if (ret) {
> > -               dev_err(ov9282->dev, "HW configuration is not supported");
> > -               return ret;
> > +               return dev_err_probe(ov9282->dev, ret,
> > +                                    "HW configuration is not supported");
> >         }
> >
> >         ov9282->regmap = devm_cci_regmap_init_i2c(client, 16);
> >         if (IS_ERR(ov9282->regmap))
> >                 return dev_err_probe(ov9282->dev, PTR_ERR(ov9282->regmap),
> > -                                    "Failed to init CCI\n");
> > +                                    "Failed to init CCI");
> >
> >         ret = ov9282_power_on(ov9282->dev);
> >         if (ret)
> > @@ -1399,7 +1397,7 @@ static int ov9282_probe(struct i2c_client *client)
> >         /* Check module identity */
> >         ret = ov9282_detect(ov9282);
> >         if (ret) {
> > -               dev_err(ov9282->dev, "failed to find sensor: %d", ret);
> > +               dev_err_probe(ov9282->dev, ret, "failed to find sensor");
> >                 goto error_power_off;
> >         }
> >
> > @@ -1409,7 +1407,7 @@ static int ov9282_probe(struct i2c_client *client)
> >
> >         ret = ov9282_init_controls(ov9282);
> >         if (ret) {
> > -               dev_err(ov9282->dev, "failed to init controls: %d", ret);
> > +               dev_err_probe(ov9282->dev, ret, "failed to init controls");
> >                 goto error_power_off;
> >         }
> >
> > @@ -1422,14 +1420,14 @@ static int ov9282_probe(struct i2c_client *client)
> >         ov9282->pad.flags = MEDIA_PAD_FL_SOURCE;
> >         ret = media_entity_pads_init(&ov9282->sd.entity, 1, &ov9282->pad);
> >         if (ret) {
> > -               dev_err(ov9282->dev, "failed to init entity pads: %d", ret);
> > +               dev_err_probe(ov9282->dev, ret, "failed to init entity pads");
> >                 goto error_handler_free;
> >         }
> >
> >         ov9282->sd.state_lock = ov9282->ctrl_handler.lock;
> >         ret = v4l2_subdev_init_finalize(&ov9282->sd);
> >         if (ret < 0) {
> > -               dev_err_probe(ov9282->dev, ret, "failed to init subdev\n");
> > +               dev_err_probe(ov9282->dev, ret, "failed to init subdev");
> >                 goto error_media_entity;
> >         }
> >
> >
> > --
> > 2.53.0
> >
> >