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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 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



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux