[<prev] [next>] [<thread-prev] [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 :)
>
Powered by blists - more mailing lists