Re: [PATCH v6 4/4] media: i2c: ov5675: add .get_selection supporty

[Tommaso Merciai]

Hi,

On Wed, Jun 08, 2022 at 03:05:29PM +0200, Quentin Schulz wrote:
> Jacopo, Tommaso,
> 
> On 6/8/22 08:42, Jacopo Mondi wrote:
> > Hi
> > 
> > On Wed, Jun 08, 2022 at 12:04:05AM +0200, Tommaso Merciai wrote:
> > > 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@xxxxxxxxxxxxxxxxxxxxx>
> > > > > 
> > > > > 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@xxxxxxxxxxxxxxxxxxxxx>
> > > > > ---
> > > > > 
> > > > > 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 ?
> > > 
> 
> I wouldn't say there's some misunderstanding, it's just super hard to figure
> out how to match what the datasheet says to what the kernel wants. Yay to
> obscure/confusing datasheets \o/

I know your feels :)

> 
> I just did things too quickly, nothing changed. Sorry, will send a v7.
> 
> > > 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;
> > > 
> 
> They are hardcoded today but actually depend on what;s set in the registers
> too, which might differ if we add more modes in the future? It's anyway
> auto-magic and it's the only place it's used, so not sure it brings much
> especially since the variable names on the left hand side of the operator
> are pretty self-explanatory (not talking about V4L2_SEL_TGT_CROP_* :p)? Not
> that I'm against it.

Thanks for the explaination.
You are right.

Regards,
Tommaso

> 
> Cheers,
> Quentin

-- 
Tommaso Merciai
Embedded Linux Engineer
tommaso.merciai@xxxxxxxxxxxxxxxxxxxx
__________________________________

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