| Message ID | 20220607153335.875956-4-foss+kernel@0leil.net |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v6,1/4] media: dt-bindings: ov5675: document YAML binding | expand |
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@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 ? >> 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 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. Cheers, Quentin
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: + 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, };