Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Wed, 8 Jun 2022 00:04:05 +0200
From:   Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
To:     Jacopo Mondi <jacopo@jmondi.org>
Cc:     Quentin Schulz <foss+kernel@0leil.net>, shawnx.tu@intel.com,
        mchehab@kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, linux-media@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        Quentin Schulz <quentin.schulz@theobroma-systems.com>
Subject: Re: [PATCH v6 4/4] media: i2c: ov5675: add .get_selection support
Message-ID: <20220607220405.GB821506@tom-ThinkPad-T14s-Gen-2i>
References: <20220607153335.875956-1-foss+kernel@0leil.net>
 <20220607153335.875956-4-foss+kernel@0leil.net>
 <20220607165136.bmriu2n7yorc7fx6@uno.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220607165136.bmriu2n7yorc7fx6@uno.localdomain>
Precedence: bulk

Hi Quentin/Jacopo,

On Tue, Jun 07, 2022 at 06:51:36PM +0200, Jacopo Mondi wrote:
> Hi Quentin,
> 
> On Tue, Jun 07, 2022 at 05:33:35PM +0200, Quentin Schulz wrote:
> > From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> >
> > The sensor has 2592*1944 active pixels, surrounded by 16 active dummy
> > pixels and there are an additional 24 black rows "at the bottom".
> >
> >                      [2624]
> >         +-----+------------------+-----+
> >         |     |     16 dummy     |     |
> >         +-----+------------------+-----+
> >         |     |                  |     |
> >         |     |     [2592]       |     |
> >         |     |                  |     |
> >         |16   |      valid       | 16  |[2000]
> >         |dummy|                  |dummy|
> >         |     |            [1944]|     |
> >         |     |                  |     |
> >         +-----+------------------+-----+
> >         |     |     16 dummy     |     |
> >         +-----+------------------+-----+
> >         |     |  24 black lines  |     |
> >         +-----+------------------+-----+
> >
> > The top-left coordinate is gotten from the registers specified in the
> > modes which are identical for both currently supported modes.
> >
> > There are currently two modes supported by this driver: 2592*1944 and
> > 1296*972. The second mode is obtained thanks to subsampling while
> > keeping the same field of view (FoV). No cropping involved, hence the
> > harcoded values.
> >
> > Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > ---
> >
> > v6:
> >  - explicit a bit more the commit log around subsampling for lower
> >  resolution modes,
> >  - (again) fixed reporting for V4L2_SEL_TGT_CROP_* thanks to Jacopo's help,
> >
> > v4:
> >  - explicit a bit more the commit log,
> >  - added drawing in the commit log,
> >  - fixed reporting for V4L2_SEL_TGT_CROP_* thanks to Jacopo's help,
> >
> > added in v3
> >
> >  drivers/media/i2c/ov5675.c | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
> > index 80840ad7bbb0..2230ff47ef49 100644
> > --- a/drivers/media/i2c/ov5675.c
> > +++ b/drivers/media/i2c/ov5675.c
> > @@ -1121,6 +1121,26 @@ static int ov5675_get_format(struct v4l2_subdev *sd,
> >  	return 0;
> >  }
> >
> > +static int ov5675_get_selection(struct v4l2_subdev *sd,
> > +				struct v4l2_subdev_state *state,
> > +				struct v4l2_subdev_selection *sel)
> > +{
> > +	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> > +		return -EINVAL;
> > +
> > +	switch (sel->target) {
> > +	case V4L2_SEL_TGT_CROP:
> > +	case V4L2_SEL_TGT_CROP_BOUNDS:
> 
> Seem like we have trouble understanding each other, or better, I have
> troubles explaining myself most probably :)
> 
> If the dummy/black area is readable, this should just be (0, 0, 2624,
> 2000) like it was in your previous version. What has changed that I
> have missed ?

Taking as reference drivers/media/i2c/ov5693.c and others,
seems ok what Quentin have done from my side.

Just one thing: maybe is better to avoid magic numbers with more
explicit defines like:

 + case V4L2_SEL_TGT_CROP_DEFAULT:
 +           sel->r.top = OV5675_ACTIVE_START_TOP;
 +           sel->r.left = OV5693_ACTIVE_START_LEFT;
 +           sel->r.width = OV5693_ACTIVE_WIDTH;
 +           sel->r.height = OV5693_ACTIVE_HEIGHT;
 
What do you think about?

Thanks,
Tommaso


> 
> Thanks
>   j
> 
> 
> > +	case V4L2_SEL_TGT_CROP_DEFAULT:
> > +		sel->r.top = 16;
> > +		sel->r.left = 16;
> > +		sel->r.width = 2592;
> > +		sel->r.height = 1944;
> > +		return 0;
> > +	}
> > +	return -EINVAL;
> > +}
> > +
> >  static int ov5675_enum_mbus_code(struct v4l2_subdev *sd,
> >  				 struct v4l2_subdev_state *sd_state,
> >  				 struct v4l2_subdev_mbus_code_enum *code)
> > @@ -1170,6 +1190,7 @@ static const struct v4l2_subdev_video_ops ov5675_video_ops = {
> >  static const struct v4l2_subdev_pad_ops ov5675_pad_ops = {
> >  	.set_fmt = ov5675_set_format,
> >  	.get_fmt = ov5675_get_format,
> > +	.get_selection = ov5675_get_selection,
> >  	.enum_mbus_code = ov5675_enum_mbus_code,
> >  	.enum_frame_size = ov5675_enum_frame_size,
> >  };
> > --
> > 2.36.1
> >

-- 
Tommaso Merciai
Embedded Linux Engineer
tommaso.merciai@amarulasolutions.com
__________________________________

Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
info@amarulasolutions.com
www.amarulasolutions.com