[PATCH 0/5] media: atomisp: ov2680 work + add testing instructions

[PATCH 0/5] media: atomisp: ov2680 work + add testing instructions

[Hans de Goede]

Hi All,

Here is some more ov2680 sensor driver work. This work is the result
of trying to get the main drivers/media/i2c/ov2680.c driver in a shape
where it is good enough to replace the atomisp specific version.

The plan is to port recent improvements to atomisp-ov2680.c over
to the main driver. While working on this I noticed some issues which
need fixing before copying them over to the "main" driver.

Besides that this also adds a small patch to make testing with
gstreamer easier and this adds testing instruction to the TODO file.

Regards,

Hans


Hans de Goede (5):
  media: atomisp: Stop resetting selected input to 0 between /dev/video#
    opens
  media: atomisp: ov2680: Stop using half pixelclock for binned modes
  media: atomisp: ov2680: Remove unnecessary registers from
    ov2680_global_setting[]
  media: atomisp: ov2680: Rename unknown/0x370a to sensor_ctrl_0a
  media: atomisp: Add testing instructions to TODO file

 drivers/staging/media/atomisp/TODO            |  33 +++++
 .../media/atomisp/i2c/atomisp-ov2680.c        |  15 +--
 drivers/staging/media/atomisp/i2c/ov2680.h    | 118 +++++++++---------
 .../staging/media/atomisp/pci/atomisp_fops.c  |   3 -
 4 files changed, 95 insertions(+), 74 deletions(-)

-- 
2.40.1

[PATCH 1/5] media: atomisp: Stop resetting selected input to 0 between /dev/video# opens

[Hans de Goede]

No other V4L2 driver resets the selected input (front cam or back cam
selected in case of the atomisp).

Stop resetting selected input to 0 between /dev/video# opens.

This allows e.g. using v4l2-ctl -i to switch the input before starting
another app, which is useful since most apps don't support selecting
the input.

Note more in general the whole resetting of a bunch of internal
state from the open fop needs to be removed there to allow multiple
opens of /dev/video# for full v4l2 API compliance.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/pci/atomisp_fops.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_fops.c b/drivers/staging/media/atomisp/pci/atomisp_fops.c
index 36e441dce7d5..54466d2f323a 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_fops.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_fops.c
@@ -474,9 +474,6 @@ static void atomisp_subdev_init_struct(struct atomisp_sub_device *asd)
 	/* s3a grid not enabled for any pipe */
 	asd->params.s3a_enabled_pipe = IA_CSS_PIPE_ID_NUM;
 
-	/* Add for channel */
-	asd->input_curr = 0;
-
 	asd->copy_mode = false;
 
 	asd->stream_prepared = false;
-- 
2.40.1

[PATCH 2/5] media: atomisp: ov2680: Stop using half pixelclock for binned modes

[Hans de Goede]

Stop using half pixelclock for binned modes this fixes:
1. The exposure being twice as high for binned mods (due to the half clk)
2. The framerate being 15 fps instead of 30 fps

The original code with fixed per mode register lists did use half pixel
clk, but this should be combined with using half for the VTS value too.

Using half VTS fixes the framerate issue, but this has the undesired
side-effect of change the exposure ctrl range (half the range, double
the amount of exposure per step).

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/i2c/atomisp-ov2680.c | 8 +-------
 drivers/staging/media/atomisp/i2c/ov2680.h         | 1 +
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
index 3ec0421b90a5..1db2eb9f9e25 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
@@ -244,27 +244,21 @@ static void ov2680_calc_mode(struct ov2680_dev *sensor)
 static int ov2680_set_mode(struct ov2680_dev *sensor)
 {
 	struct i2c_client *client = sensor->client;
-	u8 pll_div, unknown, inc, fmt1, fmt2;
+	u8 unknown, inc, fmt1, fmt2;
 	int ret;
 
 	if (sensor->mode.binning) {
-		pll_div = 1;
 		unknown = 0x23;
 		inc = 0x31;
 		fmt1 = 0xc2;
 		fmt2 = 0x01;
 	} else {
-		pll_div = 0;
 		unknown = 0x21;
 		inc = 0x11;
 		fmt1 = 0xc0;
 		fmt2 = 0x00;
 	}
 
-	ret = ov_write_reg8(client, 0x3086, pll_div);
-	if (ret)
-		return ret;
-
 	ret = ov_write_reg8(client, 0x370a, unknown);
 	if (ret)
 		return ret;
diff --git a/drivers/staging/media/atomisp/i2c/ov2680.h b/drivers/staging/media/atomisp/i2c/ov2680.h
index fd9c7485f8c1..b6c0ef591c69 100644
--- a/drivers/staging/media/atomisp/i2c/ov2680.h
+++ b/drivers/staging/media/atomisp/i2c/ov2680.h
@@ -181,6 +181,7 @@ static struct ov2680_reg const ov2680_global_setting[] = {
 	{0x3082, 0x45},
 	{0x3084, 0x09},
 	{0x3085, 0x04},
+	{0x3086, 0x00},
 	{0x3503, 0x03},
 	{0x350b, 0x36},
 	{0x3600, 0xb4},
-- 
2.40.1

[PATCH 3/5] media: atomisp: ov2680: Remove unnecessary registers from ov2680_global_setting[]

[Hans de Goede]

Many of the values in ov2680_global_setting[] match the default/reset
register value for the ov2680 sensor (verified with both datasheet
and actual hw) so they are no-ops.

And there are also a coupe of others which are later overwritten
by ctrls or by ov2680_set_mode().

Remove all the unnecessary entries and add annotations to
the remaining entries documenting what they change
(in sofar as things are documented in the datasheet).

This also removes the double writing of OV2680_REG_SOFT_RESET in
ov2680_init_registers() (one direct write, one in ov2680_global_setting[])
instead add a short sleep after the first write to give the sensor
time to reset.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../media/atomisp/i2c/atomisp-ov2680.c        |   1 +
 drivers/staging/media/atomisp/i2c/ov2680.h    | 117 ++++++++----------
 2 files changed, 56 insertions(+), 62 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
index 1db2eb9f9e25..dcc06c725544 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
@@ -175,6 +175,7 @@ static int ov2680_init_registers(struct v4l2_subdev *sd)
 	int ret;
 
 	ret = ov_write_reg8(client, OV2680_SW_RESET, 0x01);
+	usleep_range(1000, 2000);
 	ret |= ov2680_write_reg_array(client, ov2680_global_setting);
 
 	return ret;
diff --git a/drivers/staging/media/atomisp/i2c/ov2680.h b/drivers/staging/media/atomisp/i2c/ov2680.h
index b6c0ef591c69..6a71de55600b 100644
--- a/drivers/staging/media/atomisp/i2c/ov2680.h
+++ b/drivers/staging/media/atomisp/i2c/ov2680.h
@@ -172,82 +172,75 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
 }
 
 static struct ov2680_reg const ov2680_global_setting[] = {
-	{0x0103, 0x01},
-	{0x3002, 0x00},
+	/* MIPI PHY, 0x10 -> 0x1c enable bp_c_hs_en_lat and bp_d_hs_en_lat */
 	{0x3016, 0x1c},
-	{0x3018, 0x44},
-	{0x3020, 0x00},
-	{0x3080, 0x02},
+
+	/* PLL MULT bits 0-7, datasheet default 0x37 for 24MHz extclk, use 0x45 for 19.2 Mhz extclk */
 	{0x3082, 0x45},
-	{0x3084, 0x09},
-	{0x3085, 0x04},
-	{0x3086, 0x00},
+
+	/* R MANUAL set exposure (0x01) and gain (0x02) to manual (hw does not do auto) */
 	{0x3503, 0x03},
-	{0x350b, 0x36},
-	{0x3600, 0xb4},
-	{0x3603, 0x39},
-	{0x3604, 0x24},
-	{0x3605, 0x00},
-	{0x3620, 0x26},
-	{0x3621, 0x37},
-	{0x3622, 0x04},
-	{0x3628, 0x00},
-	{0x3705, 0x3c},
-	{0x370c, 0x50},
-	{0x370d, 0xc0},
-	{0x3718, 0x88},
-	{0x3720, 0x00},
-	{0x3721, 0x00},
-	{0x3722, 0x00},
-	{0x3723, 0x00},
-	{0x3738, 0x00},
-	{0x3717, 0x58},
-	{0x3781, 0x80},
-	{0x3789, 0x60},
-	{0x3800, 0x00},
-	{0x3819, 0x04},
+
+	/* Analog control register tweaks */
+	{0x3603, 0x39}, /* Reset value 0x99 */
+	{0x3604, 0x24}, /* Reset value 0x74 */
+	{0x3621, 0x37}, /* Reset value 0x44 */
+
+	/* Sensor control register tweaks */
+	{0x3701, 0x64}, /* Reset value 0x61 */
+	{0x3705, 0x3c}, /* Reset value 0x21 */
+	{0x370c, 0x50}, /* Reset value 0x10 */
+	{0x370d, 0xc0}, /* Reset value 0x00 */
+	{0x3718, 0x88}, /* Reset value 0x80 */
+
+	/* PSRAM tweaks */
+	{0x3781, 0x80}, /* Reset value 0x00 */
+	{0x3784, 0x0c}, /* Reset value 0x00, based on OV2680_R1A_AM10.ovt */
+	{0x3789, 0x60}, /* Reset value 0x50 */
+
+	/* BLC CTRL00 0x01 -> 0x81 set avg_weight to 8 */
 	{0x4000, 0x81},
-	{0x4001, 0x40},
+
+	/* Set black level compensation range to 0 - 3 (default 0 - 11) */
 	{0x4008, 0x00},
 	{0x4009, 0x03},
+
+	/* VFIFO R2 0x00 -> 0x02 set Frame reset enable */
 	{0x4602, 0x02},
+
+	/* MIPI ctrl CLK PREPARE MIN change from 0x26 (38) -> 0x36 (54) */
 	{0x481f, 0x36},
+
+	/* MIPI ctrl CLK LPX P MIN change from 0x32 (50) -> 0x36 (54) */
 	{0x4825, 0x36},
-	{0x4837, 0x18},
+
+	/* R ISP CTRL2 0x20 -> 0x30, set sof_sel bit */
 	{0x5002, 0x30},
-	{0x5004, 0x04},//manual awb 1x
-	{0x5005, 0x00},
-	{0x5006, 0x04},
-	{0x5007, 0x00},
-	{0x5008, 0x04},
-	{0x5009, 0x00},
-	{0x5080, 0x00},
-	{0x5081, 0x41},
-	{0x5708, 0x01},  /* add for full size flip off and mirror off 2014/09/11 */
-	{0x3701, 0x64},  //add on 14/05/13
-	{0x3784, 0x0c},  //based OV2680_R1A_AM10.ovt add on 14/06/13
-	{0x5780, 0x3e},  //based OV2680_R1A_AM10.ovt,Adjust DPC setting (57xx) on 14/06/13
-	{0x5781, 0x0f},
-	{0x5782, 0x04},
-	{0x5783, 0x02},
-	{0x5784, 0x01},
-	{0x5785, 0x01},
-	{0x5786, 0x00},
-	{0x5787, 0x04},
+
+	/*
+	 * Window CONTROL 0x00 -> 0x01, enable manual window control,
+	 * this is necessary for full size flip and mirror support.
+	 */
+	{0x5708, 0x01},
+
+	/*
+	 * DPC CTRL0 0x14 -> 0x3e, set enable_tail, enable_3x3_cluster
+	 * and enable_general_tail bits based OV2680_R1A_AM10.ovt.
+	 */
+	{0x5780, 0x3e},
+
+	/* DPC MORE CONNECTION CASE THRE 0x0c (12) -> 0x02 (2) */
 	{0x5788, 0x02},
-	{0x5789, 0x00},
-	{0x578a, 0x01},
-	{0x578b, 0x02},
-	{0x578c, 0x03},
-	{0x578d, 0x03},
+
+	/* DPC GAIN LIST1 0x0f (15) -> 0x08 (8) */
 	{0x578e, 0x08},
+
+	/* DPC GAIN LIST2 0x3f (63) -> 0x0c (12) */
 	{0x578f, 0x0c},
-	{0x5790, 0x08},
-	{0x5791, 0x04},
+
+	/* DPC THRE RATIO 0x04 (4) -> 0x00 (0) */
 	{0x5792, 0x00},
-	{0x5793, 0x00},
-	{0x5794, 0x03}, //based OV2680_R1A_AM10.ovt,Adjust DPC setting (57xx) on 14/06/13
-	{0x0100, 0x00},	//stream off
+
 	{}
 };
 
-- 
2.40.1

[PATCH 4/5] media: atomisp: ov2680: Rename unknown/0x370a to sensor_ctrl_0a

[Hans de Goede]

The data sheets say the registers at offset 0x3700 - 0x373f are
"sensor control" registers rename the unknown variable in
ov2680_set_mode() to sensor_ctrl_0a and add
a OV2680_REG_SENSOR_CTRL_0A define.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/i2c/atomisp-ov2680.c | 8 ++++----
 drivers/staging/media/atomisp/i2c/ov2680.h         | 2 ++
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
index dcc06c725544..7a5ad8ccd81f 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
@@ -245,22 +245,22 @@ static void ov2680_calc_mode(struct ov2680_dev *sensor)
 static int ov2680_set_mode(struct ov2680_dev *sensor)
 {
 	struct i2c_client *client = sensor->client;
-	u8 unknown, inc, fmt1, fmt2;
+	u8 sensor_ctrl_0a, inc, fmt1, fmt2;
 	int ret;
 
 	if (sensor->mode.binning) {
-		unknown = 0x23;
+		sensor_ctrl_0a = 0x23;
 		inc = 0x31;
 		fmt1 = 0xc2;
 		fmt2 = 0x01;
 	} else {
-		unknown = 0x21;
+		sensor_ctrl_0a = 0x21;
 		inc = 0x11;
 		fmt1 = 0xc0;
 		fmt2 = 0x00;
 	}
 
-	ret = ov_write_reg8(client, 0x370a, unknown);
+	ret = ov_write_reg8(client, OV2680_REG_SENSOR_CTRL_0A, sensor_ctrl_0a);
 	if (ret)
 		return ret;
 
diff --git a/drivers/staging/media/atomisp/i2c/ov2680.h b/drivers/staging/media/atomisp/i2c/ov2680.h
index 6a71de55600b..d032af245674 100644
--- a/drivers/staging/media/atomisp/i2c/ov2680.h
+++ b/drivers/staging/media/atomisp/i2c/ov2680.h
@@ -72,6 +72,8 @@
 #define OV2680_REG_EXPOSURE_PK_HIGH		0x3500
 #define OV2680_REG_GAIN_PK			0x350a
 
+#define OV2680_REG_SENSOR_CTRL_0A		0x370a
+
 #define OV2680_HORIZONTAL_START_H		0x3800 /* Bit[11:8] */
 #define OV2680_HORIZONTAL_START_L		0x3801 /* Bit[7:0]  */
 #define OV2680_VERTICAL_START_H			0x3802 /* Bit[11:8] */
-- 
2.40.1

[PATCH 5/5] media: atomisp: Add testing instructions to TODO file

[Hans de Goede]

Testing the atomisp can be a bit tricky. The BYT/CHT CPUs are not
very powerful so some apps like camorama cannot run at full FPS.

Add instructions for how to test with gstreamer which does
runs at full FPS without issues.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/TODO | 33 ++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/staging/media/atomisp/TODO b/drivers/staging/media/atomisp/TODO
index b7a09eb20d0d..ecf8ba67b7af 100644
--- a/drivers/staging/media/atomisp/TODO
+++ b/drivers/staging/media/atomisp/TODO
@@ -78,3 +78,36 @@ TODO
   the firmware files
 
 * The atomisp code still has a lot of cruft which needs cleaning up
+
+
+Testing
+=======
+
+Since libcamera support is not available yet, the easiest way to test for
+now is using v4l2-ctl to select the input and gstreamer for streaming.
+
+To select the input run:
+
+v4l2-ctl -i <input>
+
+Where <input> is 0 (front cam) or 1 (back cam).
+
+The simplest gstreamer pipeline for testing running the sensor
+at its max resolution is:
+
+gst-launch-1.0 v4l2src ! videoconvert ! xvimagesink sync=false
+
+To select e.g 640x480 as resolution use:
+
+gst-launch-1.0 v4l2src ! video/x-raw,format=YV12,width=640,height=480 ! \
+               videoconvert ! xvimagesink sync=false
+
+And to show fps use:
+
+gst-launch-1.0 v4l2src ! video/x-raw,format=YV12,width=640,height=480 ! \
+               videoconvert ! fpsdisplaysink video-sink=xvimagesink sync=false
+
+Often the image will be over / under exposed. This can be fixed by using
+v4l2-ctl on the sensor subdev to tweak the exposure ctrl; or by using a GUI
+app for v4l2-controls which also supports subdev such as the Fedora patched
+gtk-v4l tool.
-- 
2.40.1

Re: [PATCH 3/5] media: atomisp: ov2680: Remove unnecessary registers from ov2680_global_setting[]

[Andy Shevchenko]

On Sun, Jun 4, 2023 at 7:14 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Many of the values in ov2680_global_setting[] match the default/reset
> register value for the ov2680 sensor (verified with both datasheet
> and actual hw) so they are no-ops.
>
> And there are also a coupe of others which are later overwritten
> by ctrls or by ov2680_set_mode().
>
> Remove all the unnecessary entries and add annotations to
> the remaining entries documenting what they change
> (in sofar as things are documented in the datasheet).

My spell checker suggests either "in so far as" or "insofar as".

> This also removes the double writing of OV2680_REG_SOFT_RESET in
> ov2680_init_registers() (one direct write, one in ov2680_global_setting[])
> instead add a short sleep after the first write to give the sensor
> time to reset.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  .../media/atomisp/i2c/atomisp-ov2680.c        |   1 +
>  drivers/staging/media/atomisp/i2c/ov2680.h    | 117 ++++++++----------
>  2 files changed, 56 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> index 1db2eb9f9e25..dcc06c725544 100644
> --- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> +++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> @@ -175,6 +175,7 @@ static int ov2680_init_registers(struct v4l2_subdev *sd)
>         int ret;
>
>         ret = ov_write_reg8(client, OV2680_SW_RESET, 0x01);
> +       usleep_range(1000, 2000);

Perhaps surround this with blank lines and s little comment?
Also does this sleep is anyhow required by a datasheet, or is it
purely empirical?

>         ret |= ov2680_write_reg_array(client, ov2680_global_setting);

Side note: this is weird for an int returned variable.

>         return ret;
> diff --git a/drivers/staging/media/atomisp/i2c/ov2680.h b/drivers/staging/media/atomisp/i2c/ov2680.h
> index b6c0ef591c69..6a71de55600b 100644
> --- a/drivers/staging/media/atomisp/i2c/ov2680.h
> +++ b/drivers/staging/media/atomisp/i2c/ov2680.h
> @@ -172,82 +172,75 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
>  }
>
>  static struct ov2680_reg const ov2680_global_setting[] = {
> -       {0x0103, 0x01},
> -       {0x3002, 0x00},
> +       /* MIPI PHY, 0x10 -> 0x1c enable bp_c_hs_en_lat and bp_d_hs_en_lat */
>         {0x3016, 0x1c},
> -       {0x3018, 0x44},
> -       {0x3020, 0x00},
> -       {0x3080, 0x02},
> +
> +       /* PLL MULT bits 0-7, datasheet default 0x37 for 24MHz extclk, use 0x45 for 19.2 Mhz extclk */
>         {0x3082, 0x45},
> -       {0x3084, 0x09},
> -       {0x3085, 0x04},
> -       {0x3086, 0x00},
> +
> +       /* R MANUAL set exposure (0x01) and gain (0x02) to manual (hw does not do auto) */
>         {0x3503, 0x03},
> -       {0x350b, 0x36},
> -       {0x3600, 0xb4},
> -       {0x3603, 0x39},
> -       {0x3604, 0x24},
> -       {0x3605, 0x00},
> -       {0x3620, 0x26},
> -       {0x3621, 0x37},
> -       {0x3622, 0x04},
> -       {0x3628, 0x00},
> -       {0x3705, 0x3c},
> -       {0x370c, 0x50},
> -       {0x370d, 0xc0},
> -       {0x3718, 0x88},
> -       {0x3720, 0x00},
> -       {0x3721, 0x00},
> -       {0x3722, 0x00},
> -       {0x3723, 0x00},
> -       {0x3738, 0x00},
> -       {0x3717, 0x58},
> -       {0x3781, 0x80},
> -       {0x3789, 0x60},
> -       {0x3800, 0x00},
> -       {0x3819, 0x04},
> +
> +       /* Analog control register tweaks */
> +       {0x3603, 0x39}, /* Reset value 0x99 */
> +       {0x3604, 0x24}, /* Reset value 0x74 */
> +       {0x3621, 0x37}, /* Reset value 0x44 */
> +
> +       /* Sensor control register tweaks */
> +       {0x3701, 0x64}, /* Reset value 0x61 */
> +       {0x3705, 0x3c}, /* Reset value 0x21 */
> +       {0x370c, 0x50}, /* Reset value 0x10 */
> +       {0x370d, 0xc0}, /* Reset value 0x00 */
> +       {0x3718, 0x88}, /* Reset value 0x80 */
> +
> +       /* PSRAM tweaks */
> +       {0x3781, 0x80}, /* Reset value 0x00 */
> +       {0x3784, 0x0c}, /* Reset value 0x00, based on OV2680_R1A_AM10.ovt */
> +       {0x3789, 0x60}, /* Reset value 0x50 */
> +
> +       /* BLC CTRL00 0x01 -> 0x81 set avg_weight to 8 */
>         {0x4000, 0x81},
> -       {0x4001, 0x40},
> +
> +       /* Set black level compensation range to 0 - 3 (default 0 - 11) */
>         {0x4008, 0x00},
>         {0x4009, 0x03},
> +
> +       /* VFIFO R2 0x00 -> 0x02 set Frame reset enable */
>         {0x4602, 0x02},
> +
> +       /* MIPI ctrl CLK PREPARE MIN change from 0x26 (38) -> 0x36 (54) */
>         {0x481f, 0x36},
> +
> +       /* MIPI ctrl CLK LPX P MIN change from 0x32 (50) -> 0x36 (54) */
>         {0x4825, 0x36},
> -       {0x4837, 0x18},
> +
> +       /* R ISP CTRL2 0x20 -> 0x30, set sof_sel bit */
>         {0x5002, 0x30},
> -       {0x5004, 0x04},//manual awb 1x
> -       {0x5005, 0x00},
> -       {0x5006, 0x04},
> -       {0x5007, 0x00},
> -       {0x5008, 0x04},
> -       {0x5009, 0x00},
> -       {0x5080, 0x00},
> -       {0x5081, 0x41},
> -       {0x5708, 0x01},  /* add for full size flip off and mirror off 2014/09/11 */
> -       {0x3701, 0x64},  //add on 14/05/13
> -       {0x3784, 0x0c},  //based OV2680_R1A_AM10.ovt add on 14/06/13
> -       {0x5780, 0x3e},  //based OV2680_R1A_AM10.ovt,Adjust DPC setting (57xx) on 14/06/13
> -       {0x5781, 0x0f},
> -       {0x5782, 0x04},
> -       {0x5783, 0x02},
> -       {0x5784, 0x01},
> -       {0x5785, 0x01},
> -       {0x5786, 0x00},
> -       {0x5787, 0x04},
> +
> +       /*
> +        * Window CONTROL 0x00 -> 0x01, enable manual window control,
> +        * this is necessary for full size flip and mirror support.
> +        */
> +       {0x5708, 0x01},
> +
> +       /*
> +        * DPC CTRL0 0x14 -> 0x3e, set enable_tail, enable_3x3_cluster
> +        * and enable_general_tail bits based OV2680_R1A_AM10.ovt.
> +        */
> +       {0x5780, 0x3e},
> +
> +       /* DPC MORE CONNECTION CASE THRE 0x0c (12) -> 0x02 (2) */
>         {0x5788, 0x02},
> -       {0x5789, 0x00},
> -       {0x578a, 0x01},
> -       {0x578b, 0x02},
> -       {0x578c, 0x03},
> -       {0x578d, 0x03},
> +
> +       /* DPC GAIN LIST1 0x0f (15) -> 0x08 (8) */
>         {0x578e, 0x08},
> +
> +       /* DPC GAIN LIST2 0x3f (63) -> 0x0c (12) */
>         {0x578f, 0x0c},
> -       {0x5790, 0x08},
> -       {0x5791, 0x04},
> +
> +       /* DPC THRE RATIO 0x04 (4) -> 0x00 (0) */
>         {0x5792, 0x00},
> -       {0x5793, 0x00},
> -       {0x5794, 0x03}, //based OV2680_R1A_AM10.ovt,Adjust DPC setting (57xx) on 14/06/13
> -       {0x0100, 0x00}, //stream off
> +
>         {}
>  };
>
> --
> 2.40.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/5] media: atomisp: ov2680 work + add testing instructions

[Andy Shevchenko]

On Sun, Jun 4, 2023 at 7:14 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi All,
>
> Here is some more ov2680 sensor driver work. This work is the result
> of trying to get the main drivers/media/i2c/ov2680.c driver in a shape
> where it is good enough to replace the atomisp specific version.
>
> The plan is to port recent improvements to atomisp-ov2680.c over
> to the main driver. While working on this I noticed some issues which
> need fixing before copying them over to the "main" driver.
>
> Besides that this also adds a small patch to make testing with
> gstreamer easier and this adds testing instruction to the TODO file.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
(a nit-pick in one patch commented separately)

Thank you!

> Regards,
>
> Hans
>
>
> Hans de Goede (5):
>   media: atomisp: Stop resetting selected input to 0 between /dev/video#
>     opens
>   media: atomisp: ov2680: Stop using half pixelclock for binned modes
>   media: atomisp: ov2680: Remove unnecessary registers from
>     ov2680_global_setting[]
>   media: atomisp: ov2680: Rename unknown/0x370a to sensor_ctrl_0a
>   media: atomisp: Add testing instructions to TODO file
>
>  drivers/staging/media/atomisp/TODO            |  33 +++++
>  .../media/atomisp/i2c/atomisp-ov2680.c        |  15 +--
>  drivers/staging/media/atomisp/i2c/ov2680.h    | 118 +++++++++---------
>  .../staging/media/atomisp/pci/atomisp_fops.c  |   3 -
>  4 files changed, 95 insertions(+), 74 deletions(-)
>
> --
> 2.40.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/5] media: atomisp: ov2680: Remove unnecessary registers from ov2680_global_setting[]

[Hans de Goede]

Hi,

On 6/4/23 21:23, Andy Shevchenko wrote:
> On Sun, Jun 4, 2023 at 7:14 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Many of the values in ov2680_global_setting[] match the default/reset
>> register value for the ov2680 sensor (verified with both datasheet
>> and actual hw) so they are no-ops.
>>
>> And there are also a coupe of others which are later overwritten
>> by ctrls or by ov2680_set_mode().
>>
>> Remove all the unnecessary entries and add annotations to
>> the remaining entries documenting what they change
>> (in sofar as things are documented in the datasheet).
> 
> My spell checker suggests either "in so far as" or "insofar as".
> 
>> This also removes the double writing of OV2680_REG_SOFT_RESET in
>> ov2680_init_registers() (one direct write, one in ov2680_global_setting[])
>> instead add a short sleep after the first write to give the sensor
>> time to reset.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  .../media/atomisp/i2c/atomisp-ov2680.c        |   1 +
>>  drivers/staging/media/atomisp/i2c/ov2680.h    | 117 ++++++++----------
>>  2 files changed, 56 insertions(+), 62 deletions(-)
>>
>> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
>> index 1db2eb9f9e25..dcc06c725544 100644
>> --- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
>> +++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
>> @@ -175,6 +175,7 @@ static int ov2680_init_registers(struct v4l2_subdev *sd)
>>         int ret;
>>
>>         ret = ov_write_reg8(client, OV2680_SW_RESET, 0x01);
>> +       usleep_range(1000, 2000);
> 
> Perhaps surround this with blank lines and s little comment?
> Also does this sleep is anyhow required by a datasheet, or is it
> purely empirical?

The sleep is taken from the drivers/media/i2c/ov2680.c version
of the driver, to replace writing the reset reg twice.

The datasheet says the first i2c-transaction after a reset
needs to be delayed by 8192 cycles of the provided external clk,
which with a 19.2MHz clk is roughly 0.5 ms.  But that is for
a reset through the XSHUTDOWN pin of the sensor. How long
a sw initiated reset takes is not specified.

Regards,

Hans



> 
>>         ret |= ov2680_write_reg_array(client, ov2680_global_setting);
> 
> Side note: this is weird for an int returned variable.
> 
>>         return ret;
>> diff --git a/drivers/staging/media/atomisp/i2c/ov2680.h b/drivers/staging/media/atomisp/i2c/ov2680.h
>> index b6c0ef591c69..6a71de55600b 100644
>> --- a/drivers/staging/media/atomisp/i2c/ov2680.h
>> +++ b/drivers/staging/media/atomisp/i2c/ov2680.h
>> @@ -172,82 +172,75 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
>>  }
>>
>>  static struct ov2680_reg const ov2680_global_setting[] = {
>> -       {0x0103, 0x01},
>> -       {0x3002, 0x00},
>> +       /* MIPI PHY, 0x10 -> 0x1c enable bp_c_hs_en_lat and bp_d_hs_en_lat */
>>         {0x3016, 0x1c},
>> -       {0x3018, 0x44},
>> -       {0x3020, 0x00},
>> -       {0x3080, 0x02},
>> +
>> +       /* PLL MULT bits 0-7, datasheet default 0x37 for 24MHz extclk, use 0x45 for 19.2 Mhz extclk */
>>         {0x3082, 0x45},
>> -       {0x3084, 0x09},
>> -       {0x3085, 0x04},
>> -       {0x3086, 0x00},
>> +
>> +       /* R MANUAL set exposure (0x01) and gain (0x02) to manual (hw does not do auto) */
>>         {0x3503, 0x03},
>> -       {0x350b, 0x36},
>> -       {0x3600, 0xb4},
>> -       {0x3603, 0x39},
>> -       {0x3604, 0x24},
>> -       {0x3605, 0x00},
>> -       {0x3620, 0x26},
>> -       {0x3621, 0x37},
>> -       {0x3622, 0x04},
>> -       {0x3628, 0x00},
>> -       {0x3705, 0x3c},
>> -       {0x370c, 0x50},
>> -       {0x370d, 0xc0},
>> -       {0x3718, 0x88},
>> -       {0x3720, 0x00},
>> -       {0x3721, 0x00},
>> -       {0x3722, 0x00},
>> -       {0x3723, 0x00},
>> -       {0x3738, 0x00},
>> -       {0x3717, 0x58},
>> -       {0x3781, 0x80},
>> -       {0x3789, 0x60},
>> -       {0x3800, 0x00},
>> -       {0x3819, 0x04},
>> +
>> +       /* Analog control register tweaks */
>> +       {0x3603, 0x39}, /* Reset value 0x99 */
>> +       {0x3604, 0x24}, /* Reset value 0x74 */
>> +       {0x3621, 0x37}, /* Reset value 0x44 */
>> +
>> +       /* Sensor control register tweaks */
>> +       {0x3701, 0x64}, /* Reset value 0x61 */
>> +       {0x3705, 0x3c}, /* Reset value 0x21 */
>> +       {0x370c, 0x50}, /* Reset value 0x10 */
>> +       {0x370d, 0xc0}, /* Reset value 0x00 */
>> +       {0x3718, 0x88}, /* Reset value 0x80 */
>> +
>> +       /* PSRAM tweaks */
>> +       {0x3781, 0x80}, /* Reset value 0x00 */
>> +       {0x3784, 0x0c}, /* Reset value 0x00, based on OV2680_R1A_AM10.ovt */
>> +       {0x3789, 0x60}, /* Reset value 0x50 */
>> +
>> +       /* BLC CTRL00 0x01 -> 0x81 set avg_weight to 8 */
>>         {0x4000, 0x81},
>> -       {0x4001, 0x40},
>> +
>> +       /* Set black level compensation range to 0 - 3 (default 0 - 11) */
>>         {0x4008, 0x00},
>>         {0x4009, 0x03},
>> +
>> +       /* VFIFO R2 0x00 -> 0x02 set Frame reset enable */
>>         {0x4602, 0x02},
>> +
>> +       /* MIPI ctrl CLK PREPARE MIN change from 0x26 (38) -> 0x36 (54) */
>>         {0x481f, 0x36},
>> +
>> +       /* MIPI ctrl CLK LPX P MIN change from 0x32 (50) -> 0x36 (54) */
>>         {0x4825, 0x36},
>> -       {0x4837, 0x18},
>> +
>> +       /* R ISP CTRL2 0x20 -> 0x30, set sof_sel bit */
>>         {0x5002, 0x30},
>> -       {0x5004, 0x04},//manual awb 1x
>> -       {0x5005, 0x00},
>> -       {0x5006, 0x04},
>> -       {0x5007, 0x00},
>> -       {0x5008, 0x04},
>> -       {0x5009, 0x00},
>> -       {0x5080, 0x00},
>> -       {0x5081, 0x41},
>> -       {0x5708, 0x01},  /* add for full size flip off and mirror off 2014/09/11 */
>> -       {0x3701, 0x64},  //add on 14/05/13
>> -       {0x3784, 0x0c},  //based OV2680_R1A_AM10.ovt add on 14/06/13
>> -       {0x5780, 0x3e},  //based OV2680_R1A_AM10.ovt,Adjust DPC setting (57xx) on 14/06/13
>> -       {0x5781, 0x0f},
>> -       {0x5782, 0x04},
>> -       {0x5783, 0x02},
>> -       {0x5784, 0x01},
>> -       {0x5785, 0x01},
>> -       {0x5786, 0x00},
>> -       {0x5787, 0x04},
>> +
>> +       /*
>> +        * Window CONTROL 0x00 -> 0x01, enable manual window control,
>> +        * this is necessary for full size flip and mirror support.
>> +        */
>> +       {0x5708, 0x01},
>> +
>> +       /*
>> +        * DPC CTRL0 0x14 -> 0x3e, set enable_tail, enable_3x3_cluster
>> +        * and enable_general_tail bits based OV2680_R1A_AM10.ovt.
>> +        */
>> +       {0x5780, 0x3e},
>> +
>> +       /* DPC MORE CONNECTION CASE THRE 0x0c (12) -> 0x02 (2) */
>>         {0x5788, 0x02},
>> -       {0x5789, 0x00},
>> -       {0x578a, 0x01},
>> -       {0x578b, 0x02},
>> -       {0x578c, 0x03},
>> -       {0x578d, 0x03},
>> +
>> +       /* DPC GAIN LIST1 0x0f (15) -> 0x08 (8) */
>>         {0x578e, 0x08},
>> +
>> +       /* DPC GAIN LIST2 0x3f (63) -> 0x0c (12) */
>>         {0x578f, 0x0c},
>> -       {0x5790, 0x08},
>> -       {0x5791, 0x04},
>> +
>> +       /* DPC THRE RATIO 0x04 (4) -> 0x00 (0) */
>>         {0x5792, 0x00},
>> -       {0x5793, 0x00},
>> -       {0x5794, 0x03}, //based OV2680_R1A_AM10.ovt,Adjust DPC setting (57xx) on 14/06/13
>> -       {0x0100, 0x00}, //stream off
>> +
>>         {}
>>  };
>>
>> --
>> 2.40.1
>>
> 
> 

Re: [PATCH 0/5] media: atomisp: ov2680 work + add testing instructions

[Hans de Goede]

Hi Andy,

On 6/4/23 21:28, Andy Shevchenko wrote:
> On Sun, Jun 4, 2023 at 7:14 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi All,
>>
>> Here is some more ov2680 sensor driver work. This work is the result
>> of trying to get the main drivers/media/i2c/ov2680.c driver in a shape
>> where it is good enough to replace the atomisp specific version.
>>
>> The plan is to port recent improvements to atomisp-ov2680.c over
>> to the main driver. While working on this I noticed some issues which
>> need fixing before copying them over to the "main" driver.
>>
>> Besides that this also adds a small patch to make testing with
>> gstreamer easier and this adds testing instruction to the TODO file.
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> (a nit-pick in one patch commented separately)
> 
> Thank you!

Thank you for the reviews. I've pushed this series (with the nitpick
addressed) as well as the previous 3 fixes you reviewed to my
media-atomisp branch now.

Regards,

Hans




>> Hans de Goede (5):
>>   media: atomisp: Stop resetting selected input to 0 between /dev/video#
>>     opens
>>   media: atomisp: ov2680: Stop using half pixelclock for binned modes
>>   media: atomisp: ov2680: Remove unnecessary registers from
>>     ov2680_global_setting[]
>>   media: atomisp: ov2680: Rename unknown/0x370a to sensor_ctrl_0a
>>   media: atomisp: Add testing instructions to TODO file
>>
>>  drivers/staging/media/atomisp/TODO            |  33 +++++
>>  .../media/atomisp/i2c/atomisp-ov2680.c        |  15 +--
>>  drivers/staging/media/atomisp/i2c/ov2680.h    | 118 +++++++++---------
>>  .../staging/media/atomisp/pci/atomisp_fops.c  |   3 -
>>  4 files changed, 95 insertions(+), 74 deletions(-)
>>
>> --
>> 2.40.1
>>
> 
>