Re: [PATCH v5 2/2] media: i2c: Add driver for ST VD55G1 camera sensor

[Christophe JAILLET]

Le 07/04/2025 à 11:07, Benjamin Mugnier a écrit :
> Hi Christophe
>
> Thank you for your review.
>
> On 4/4/25 18:09, Christophe JAILLET wrote:
> > Le 04/04/2025 à 16:50, Benjamin Mugnier a écrit :
> > > The VD55G1 is a monochrome global shutter camera with a 804x704 maximum
> > > resolution with RAW8 and RAW10 bytes per pixel.
> > > The driver supports :
> > > - Auto exposure from the sensor, or manual exposure mode
> > > - HDR subtraction mode, allowing edge detection and background removal
> > > - Auto exposure cold start, using configuration values from last stream
> > > to start the next one
> > > - LED GPIOs for illumination
> > > - Most standard camera sensor features (hblank, vblank, test patterns,
> > > again, dgain, hflip, vflip, auto exposure bias, etc.)
> > > Add driver source code to MAINTAINERS file.
> >
> > Hi, a few nitpicks below, should they make sense.
> >
> > ...
> >
> > > +static int vd55g1_prepare_clock_tree(struct vd55g1 *sensor)
> > > +{
> > > +    struct i2c_client *client = sensor->i2c_client;
> > > +    /* Double data rate */
> > > +    u32 mipi_freq = sensor->link_freq * 2;
> > > +    u32 sys_clk, mipi_div, pixel_div;
> > > +    int ret = 0;
> > > +
> > > +    if (sensor->xclk_freq < 6 * HZ_PER_MHZ ||
> > > +        sensor->xclk_freq > 27 * HZ_PER_MHZ) {
> > > +        dev_err(&client->dev,
> > > +            "Only 6Mhz-27Mhz clock range supported. Provided %lu MHz\n",
> > > +            sensor->xclk_freq / HZ_PER_MHZ);
> > > +        return -EINVAL;
> > > +    }
> > > +
> > > +    if (mipi_freq < 250 * HZ_PER_MHZ ||
> > > +        mipi_freq > 1200 * HZ_PER_MHZ) {
> > > +        dev_err(&client->dev,
> > > +            "Only 250Mhz-1200Mhz link frequency range supported.
> > > Provided %lu MHz\n",
> > > +            mipi_freq / HZ_PER_MHZ);
> > > +        return -EINVAL;
> > > +    }
> > > +
> > > +    if (mipi_freq <= 300 * HZ_PER_MHZ)
> > > +        mipi_div = 4;
> > > +    else if (mipi_freq <= 600 * HZ_PER_MHZ)
> > > +        mipi_div = 2;
> > > +    else
> > > +        mipi_div = 1;
> > > +
> > > +    sys_clk = mipi_freq * mipi_div;
> > > +
> > > +    if (sys_clk <= 780 * HZ_PER_MHZ)
> > > +        pixel_div = 5;
> > > +    else if (sys_clk <= 900 * HZ_PER_MHZ)
> > > +        pixel_div = 6;
> > > +    else
> > > +        pixel_div = 8;
> > > +
> > > +    sensor->pixel_clock = sys_clk / pixel_div;
> > > +    /* Frequency to data rate is 1:1 ratio for MIPI */
> > > +    sensor->data_rate_in_mbps = mipi_freq;
> > > +
> > > +    return ret;
> >
> > Could be return 0, and ret could be removed.
>
> Yes, I replaced all valid return paths by return 0.
>
> > > +}
> >
> > ...
> >
> > > +static int vd55g1_enable_streams(struct v4l2_subdev *sd,
> > > +                 struct v4l2_subdev_state *state, u32 pad,
> > > +                 u64 streams_mask)
> > > +{
> > > +    struct vd55g1 *sensor = to_vd55g1(sd);
> > > +    struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
> > > +    int ret = 0;
> >
> > Un-needed init, it is set just the line after.
>
> I always wonder if it is worth removing the initialization if it is
> redundant. I find myself spending time debugging issues happening
> because I modified the flow of a function and now the return value
> needs to be initialized.

On obvious cases, like this one, personally I prefer omitting the 
initialization, as already done in vd55g1_new_format_change_controls() 
or vd55g1_init_state() below.

But time is more important than these few bytes that will be optimized 
by the compiler anyway.

So if it may save some of your time, I think that consistently 
initializing it is certainly the way to go.

Just my 2c.

CJ

> You're absolutely correct in these initializations being unnecessary
> though, and I removed them for v6, but I'll gladly take your thinking on
> my comment :)