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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <525551ae-fab0-40df-9190-81701f3480e8@wanadoo.fr>
Date: Mon, 7 Apr 2025 21:59:31 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Benjamin Mugnier <benjamin.mugnier@...s.st.com>
Cc: conor+dt@...nel.org, devicetree@...r.kernel.org, krzk+dt@...nel.org,
 linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
 mchehab@...nel.org, robh@...nel.org, sakari.ailus@...ux.intel.com,
 sylvain.petinot@...s.st.com
Subject: Re: [PATCH v5 2/2] media: i2c: Add driver for ST VD55G1 camera sensor

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