Re: [PATCH v3 3/3] media: i2c: os02g10: implement crop handling with set_selection

[Tarang Raval]

Hi Vladimir,

Thank you for the review.

Since I authored this patch, I will try to address the comments below.

> On 4/24/26 12:25, Elgin Perumbilly wrote:
> > From: Tarang Raval <tarang.raval@xxxxxxxxxxxxxxxxx>
> >
> > Add crop support to os02g10 by implementing .set_selection() and
> > storing the crop rectangle in subdev state.
> >
> > Initialize the default crop to the active area, make set_fmt() use the
> > current crop, and update the output format when the crop size changes.
> > Also program the sensor window from the active crop/format state instead
> > of using the fixed supported_modes entry.
> >
> > This allows userspace to configure the sensor crop window explicitly.
> >
> > Signed-off-by: Tarang Raval <tarang.raval@xxxxxxxxxxxxxxxxx>
> > Signed-off-by: Elgin Perumbilly <elgin.perumbilly@xxxxxxxxxxxxxxxxx>
> > ---
> >   drivers/media/i2c/os02g10.c | 166 ++++++++++++++++++++++--------------
> >   1 file changed, 103 insertions(+), 63 deletions(-)
> >
> > diff --git a/drivers/media/i2c/os02g10.c b/drivers/media/i2c/os02g10.c
> > index fad2dd0ad7aa..9bf8f5d1caea 100644
> > --- a/drivers/media/i2c/os02g10.c
> > +++ b/drivers/media/i2c/os02g10.c
> > @@ -112,6 +112,11 @@
> >   #define OS02G10_ORIENTATION_BAYER_FIX               0x32
> >
> >   #define OS02G10_LINK_FREQ_720MHZ            (720 * HZ_PER_MHZ)
> > +#define OS02G10_WINDOW_WIDTH_MIN             2
> > +#define OS02G10_WINDOW_HEIGHT_MIN            2
>
> Add a blank line before the new group of macro.
>
> > +#define OS02G10_VBLANK_DEF                   166
>
> This one is computable, and it can be dropped.


Can you explain how this value can be computed ?


> > +#define OS02G10_VBLANK_MIN                   25
>
> This macro shall be added to the group of OS02G10_REG_FRAME_LENGTH
> register, and it should be included into the previous change.
>
> > +#define OS02G10_EXPOSURE_DEF                 1100
>
> This macro shall be added to the group of OS02G10_REG_LONG_EXPOSURE
> register, and it should be included into the previous change.


I would prefer to introduce these macros here only, as they are related to
this patch. There is no use of these macros in the previous patch, so moving
them there would not provide any benefit.

I will group them in the appropriate place within this patch as per your
suggestion.


> >
> >   /* OS02G10 native and active pixel array size */
> >   static const struct v4l2_rect os02g10_native_area = {
> > @@ -152,15 +157,6 @@ struct os02g10 {
> >       struct v4l2_ctrl *hflip;
> >   };
> >
> > -struct os02g10_mode {
> > -     u32 width;
> > -     u32 height;
> > -     u32 vts_def;
> > -     u32 exp_def;
> > -     u32 x_start;
> > -     u32 y_start;
> > -};
> > -
> >   static const struct cci_reg_sequence os02g10_common_regs[] = {
> >       { OS02G10_REG_PLL_DIV_CTRL,             0x0a},
> >       { OS02G10_REG_PLL_DCTL_BIAS_CTRL,       0x04},

...

> >   static const struct v4l2_subdev_video_ops os02g10_video_ops = {
> > @@ -645,6 +684,7 @@ static const struct v4l2_subdev_pad_ops os02g10_pad_ops = {
> >       .get_fmt = v4l2_subdev_get_fmt,
> >       .set_fmt = os02g10_set_pad_format,
> >       .get_selection = os02g10_get_selection,
> > +     .set_selection = os02g10_set_selection,
> >       .enum_frame_size = os02g10_enum_frame_size,
> >       .enable_streams = os02g10_enable_streams,
> >       .disable_streams = os02g10_disable_streams,
>
> I understand that this change is written by another person, and likely
> it is not squashed with the previous one to preserve authorship, however
> it significantly rewrites the change already found in the series.

I don't think this patch significantly rewrites the previous changes. Its
main purpose is to introduce a crop rectangle and implement set_selection(),
allowing userspace to stream arbitrary resolutions within the sensor limits
instead of being restricted to 1920x1080.

> I don't see information about the maximum supported frame height/width
> or default VTS setting etc. anymore, for me it's hard to say, if
> this kind of information can be dropped with no consequences in runtime.

The maximum supported width and height are still defined and enforced in
os02g10_set_selection(). The default VTS handling is implemented in
os02g10_set_pad_format(), where VBLANK is adjusted to maintain 30 fps for
the selected resolution. So I don't believe any information has been dropped.

> Probably this 3/3 change will break a quick inclusion of the sensor
> driver, you may consider to exlcude it from the series now, and publish
> it afterwards.

Regarding whether patch 3/3 should be included in the current series, I am
happy to leave that decision to Sakari. If needed, this patch can be merged
separately once the new raw sensor model is finalized.

This patch was created based on Laurent's suggestion.

Link: https://lore.kernel.org/linux-media/20260414084952.217215-1-elgin.perumbilly@xxxxxxxxxxxxxxxxx/T/#t

Best Regards,
Tarang