[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <927dc606-ef77-4435-81f1-f36b951be25b@redhat.com>
Date: Thu, 27 Mar 2025 10:10:20 +0100
From: Hans de Goede <hdegoede@...hat.com>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>,
Bryan O'Donoghue <bryan.odonoghue@...aro.org>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Bryan O'Donoghue <bod@...nel.org>,
Hans de Goede <hansg@...nel.org>, Jingjing Xiong <jingjing.xiong@...el.com>,
Hao Yao <hao.yao@...el.com>, Jim Lai <jim.lai@...el.com>,
You-Sheng Yang <vicamo.yang@...onical.com>,
Alan Stern <stern@...land.harvard.edu>, linux-kernel@...r.kernel.org,
linux-media@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v2 2/2] media: i2c: ov02e10: add OV02E10 image sensor
driver
Hi,
On 27-Mar-25 08:41, Sakari Ailus wrote:
> A few more comments.
>
> On Tue, Mar 25, 2025 at 02:49:29PM +0000, Bryan O'Donoghue wrote:
>> +static int ov02e10_set_format(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *sd_state,
>> + struct v4l2_subdev_format *fmt)
>> +{
>> + struct ov02e10 *ov02e10 = to_ov02e10(sd);
>> + const struct ov02e10_mode *mode;
>> + s32 vblank_def, h_blank;
>> +
>> + mode = v4l2_find_nearest_size(supported_modes,
>> + ARRAY_SIZE(supported_modes),
>> + width, height, fmt->format.width,
>> + fmt->format.height);
>> +
>> + mutex_lock(&ov02e10->mutex);
>> + ov02e10_update_pad_format(mode, &fmt->format);
>> + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
>> + *v4l2_subdev_state_get_format(sd_state, fmt->pad) =
>> + fmt->format;
>> + } else {
>> + ov02e10->cur_mode = mode;
>> + __v4l2_ctrl_s_ctrl(ov02e10->link_freq, mode->link_freq_index);
>> + __v4l2_ctrl_s_ctrl_int64(ov02e10->pixel_rate,
>> + to_pixel_rate(mode->link_freq_index));
>> +
>> + /* Update limits and set FPS to default */
>> + vblank_def = mode->vts_def - mode->height;
>> + __v4l2_ctrl_modify_range(ov02e10->vblank,
>> + mode->vts_min - mode->height,
>> + OV02E10_VTS_MAX - mode->height, 1,
>> + vblank_def);
>
> Note that this can fail.
>
>> + __v4l2_ctrl_s_ctrl(ov02e10->vblank, vblank_def);
>
> As well as this one.
>
>> + h_blank = to_pixels_per_line(mode->hts, mode->link_freq_index) -
>> + mode->width;
>> + __v4l2_ctrl_modify_range(ov02e10->hblank, h_blank, h_blank, 1,
>> + h_blank);
>> + }
>> + mutex_unlock(&ov02e10->mutex);
>> +
>> + return 0;
>> +}
>
> Please rely on sub-device state and the state lock instead, see e.g. imx219
> driver for an example.
Or see the conversion to sub-device state in my incremental v8 posting
of the ov02c10 series. Converting this driver should be alsmost
identical.
>> +
>> +static int ov02e10_get_format(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *sd_state,
>> + struct v4l2_subdev_format *fmt)
>> +{
>> + struct ov02e10 *ov02e10 = to_ov02e10(sd);
>> +
>> + mutex_lock(&ov02e10->mutex);
>> + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
>> + fmt->format = *v4l2_subdev_state_get_format(sd_state, fmt->pad);
>> + else
>> + ov02e10_update_pad_format(ov02e10->cur_mode, &fmt->format);
>> +
>> + mutex_unlock(&ov02e10->mutex);
>> +
>> + return 0;
>> +}
>
> And you won't need this with sub-device state.
Also see my incremental v8 posting of the ov02c10 series.
>
>> +
>> +static int ov02e10_enum_mbus_code(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *sd_state,
>> + struct v4l2_subdev_mbus_code_enum *code)
>> +{
>> + if (code->index > 0)
>> + return -EINVAL;
>> +
>> + code->code = MEDIA_BUS_FMT_SGRBG10_1X10;
>> +
>> + return 0;
>> +}
>> +
>> +static int ov02e10_enum_frame_size(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *sd_state,
>> + struct v4l2_subdev_frame_size_enum *fse)
>> +{
>> + if (fse->index >= ARRAY_SIZE(supported_modes))
>> + return -EINVAL;
>> +
>> + if (fse->code != MEDIA_BUS_FMT_SGRBG10_1X10)
>> + return -EINVAL;
>> +
>> + fse->min_width = supported_modes[fse->index].width;
>> + fse->max_width = fse->min_width;
>> + fse->min_height = supported_modes[fse->index].height;
>> + fse->max_height = fse->min_height;
>> +
>> + return 0;
>> +}
>> +
>> +static int ov02e10_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>> +{
>> + struct ov02e10 *ov02e10 = to_ov02e10(sd);
>> +
>> + mutex_lock(&ov02e10->mutex);
>> + ov02e10_update_pad_format(&supported_modes[0],
>> + v4l2_subdev_state_get_format(fh->state, 0));
>
> Please rely on init_cfg pad op instead.
>
>> + mutex_unlock(&ov02e10->mutex);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct v4l2_subdev_video_ops ov02e10_video_ops = {
>> + .s_stream = ov02e10_set_stream,
>
> Please use {enable,disable}_streams instead. See e.g. imx283 driver for an
> example.
Also see my incremental v8 posting of the ov02c10 series.
Regards,
Hans
Powered by blists - more mailing lists