[PATCH 00/11] media: atomisp-ov2680: Cleanups and exposure + gain fixes

[PATCH 00/11] media: atomisp-ov2680: Cleanups and exposure + gain fixes

[Hans de Goede]

Hi All,

I've finally made some time to look into running the atomisp2 driver
from staging on an Asus T101HA. Thanks to the great work from
Tsuchiya and Mauro I actually got a working picture in camorama
now, which is awesome!

But I noticed that exposure / gain setting was not working, not
even if I first set it using the --exposure option to v4l2n before
running camorama.

There were a number of issues, which this series all fixes,
after this series you can set an exposure + gain with v4l2n
and the run camorama and actually see the difference.

Note despite the cleanups in here the atomisp-ov2680 code still
is far from great, but it works :)

Regards,

Hans

p.s.

I've also given the current media-staging code a real quick
test on a T100HA, fwiw it does not work there atm. I did
not investigate this further (not yet anyways).


Hans de Goede (11):
  media: atomisp-ov2680: Remove a bunch of unused vars from
    ov2680_device
  media: atomisp-ov2680: Turn on power only once
  media: atomisp-ov2680: Push the input_lock taking up into
    ov2680_s_power()
  media: atomisp-ov2680: Remove the ov2680_res and N_RES global
    variables
  media: atomisp-ov2680: Move ov2680_init_registers() call to power_up()
  media: atomisp-ov2680: Save/restore exposure and gain over sensor
    power-down
  media: atomisp-ov2680: Make ov2680_read_reg() support 24 bit registers
  media: atomisp-ov2680: Fix and simplify ov2680_q_exposure()
  media: atomisp-ov2680: Fix ov2680_write_reg() always writing 0 to 16
    bit registers
  media: atomisp-ov2680: Fix ov2680_set_fmt() clobbering the exposure
  media: atomisp-ov2680: Fix ov2680_set_fmt() messing up high exposure
    settings

 .../media/atomisp/i2c/atomisp-ov2680.c        | 167 +++++++-----------
 drivers/staging/media/atomisp/i2c/ov2680.h    |  59 +------
 2 files changed, 71 insertions(+), 155 deletions(-)

-- 
2.31.1

[PATCH 01/11] media: atomisp-ov2680: Remove a bunch of unused vars from ov2680_device

[Hans de Goede]

Remove a couple of variables which where either completely unused,
or only ever got a value assigned to them but were never read.

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

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
index ef439937717b..56f95e44316d 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
@@ -166,8 +166,6 @@ static int ov2680_get_intg_factor(struct i2c_client *client,
 				  struct camera_mipi_info *info,
 				  const struct ov2680_resolution *res)
 {
-	struct v4l2_subdev *sd = i2c_get_clientdata(client);
-	struct ov2680_device *dev = to_ov2680_sensor(sd);
 	struct atomisp_sensor_mode_data *buf = &info->data;
 	unsigned int pix_clk_freq_hz;
 	u16 reg_val;
@@ -180,7 +178,6 @@ static int ov2680_get_intg_factor(struct i2c_client *client,
 	/* pixel clock */
 	pix_clk_freq_hz = res->pix_clk_freq * 1000000;
 
-	dev->vt_pix_clk_freq_mhz = pix_clk_freq_hz;
 	buf->vt_pix_clk_freq_mhz = pix_clk_freq_hz;
 
 	/* get integration time */
@@ -434,24 +431,8 @@ static int ov2680_q_exposure(struct v4l2_subdev *sd, s32 *value)
 	return ret;
 }
 
-static u32 ov2680_translate_bayer_order(enum atomisp_bayer_order code)
-{
-	switch (code) {
-	case atomisp_bayer_order_rggb:
-		return MEDIA_BUS_FMT_SRGGB10_1X10;
-	case atomisp_bayer_order_grbg:
-		return MEDIA_BUS_FMT_SGRBG10_1X10;
-	case atomisp_bayer_order_bggr:
-		return MEDIA_BUS_FMT_SBGGR10_1X10;
-	case atomisp_bayer_order_gbrg:
-		return MEDIA_BUS_FMT_SGBRG10_1X10;
-	}
-	return 0;
-}
-
 static int ov2680_v_flip(struct v4l2_subdev *sd, s32 value)
 {
-	struct ov2680_device *dev = to_ov2680_sensor(sd);
 	struct camera_mipi_info *ov2680_info = NULL;
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	int ret;
@@ -476,15 +457,12 @@ static int ov2680_v_flip(struct v4l2_subdev *sd, s32 value)
 	ov2680_info = v4l2_get_subdev_hostdata(sd);
 	if (ov2680_info) {
 		ov2680_info->raw_bayer_order = ov2680_bayer_order_mapping[index];
-		dev->format.code = ov2680_translate_bayer_order(
-				       ov2680_info->raw_bayer_order);
 	}
 	return ret;
 }
 
 static int ov2680_h_flip(struct v4l2_subdev *sd, s32 value)
 {
-	struct ov2680_device *dev = to_ov2680_sensor(sd);
 	struct camera_mipi_info *ov2680_info = NULL;
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	int ret;
@@ -510,8 +488,6 @@ static int ov2680_h_flip(struct v4l2_subdev *sd, s32 value)
 	ov2680_info = v4l2_get_subdev_hostdata(sd);
 	if (ov2680_info) {
 		ov2680_info->raw_bayer_order = ov2680_bayer_order_mapping[index];
-		dev->format.code = ov2680_translate_bayer_order(
-				       ov2680_info->raw_bayer_order);
 	}
 	return ret;
 }
@@ -1199,7 +1175,6 @@ static int ov2680_probe(struct i2c_client *client)
 
 	dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 	dev->pad.flags = MEDIA_PAD_FL_SOURCE;
-	dev->format.code = MEDIA_BUS_FMT_SBGGR10_1X10;
 	dev->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
 	ret =
 	    v4l2_ctrl_handler_init(&dev->ctrl_handler,
diff --git a/drivers/staging/media/atomisp/i2c/ov2680.h b/drivers/staging/media/atomisp/i2c/ov2680.h
index 535440ed14d7..9f9f6695eed9 100644
--- a/drivers/staging/media/atomisp/i2c/ov2680.h
+++ b/drivers/staging/media/atomisp/i2c/ov2680.h
@@ -169,14 +169,10 @@ struct ov2680_format {
 struct ov2680_device {
 	struct v4l2_subdev sd;
 	struct media_pad pad;
-	struct v4l2_mbus_framefmt format;
 	struct mutex input_lock;
 	struct v4l2_ctrl_handler ctrl_handler;
 	struct ov2680_resolution *res;
 	struct camera_sensor_platform_data *platform_data;
-	int vt_pix_clk_freq_mhz;
-	int run_mode;
-	u8 type;
 };
 
 /**
-- 
2.31.1

[PATCH 02/11] media: atomisp-ov2680: Turn on power only once

[Hans de Goede]

Add a power_on bool to track if the power is on, and make
power_up() a no-op if the power is already on.

This also removes a power_down() call from ov2680_s_config() since
that is a no-op now, this is ok because s_config() is only called
once on probe and the sensor is off at boot.

Besides avoiding to the work in power_up() multiple times this patch
is also a preparation for switching to the clk and regulator frameworks
which keep an enable count, so there we must call enable() and
disable() only once per power-cycle.

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

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
index 56f95e44316d..7b7cf7a68823 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
@@ -736,6 +736,9 @@ static int power_up(struct v4l2_subdev *sd)
 		return -ENODEV;
 	}
 
+	if (dev->power_on)
+		return 0; /* Already on */
+
 	/* power control */
 	ret = power_ctrl(sd, 1);
 	if (ret)
@@ -760,6 +763,7 @@ static int power_up(struct v4l2_subdev *sd)
 	/* according to DS, 20ms is needed between PWDN and i2c access */
 	msleep(20);
 
+	dev->power_on = true;
 	return 0;
 
 fail_clk:
@@ -785,6 +789,9 @@ static int power_down(struct v4l2_subdev *sd)
 		return -ENODEV;
 	}
 
+	if (!dev->power_on)
+		return 0; /* Already off */
+
 	ret = dev->platform_data->flisclk_ctrl(sd, 0);
 	if (ret)
 		dev_err(&client->dev, "flisclk failed\n");
@@ -799,10 +806,13 @@ static int power_down(struct v4l2_subdev *sd)
 
 	/* power control */
 	ret = power_ctrl(sd, 0);
-	if (ret)
+	if (ret) {
 		dev_err(&client->dev, "vprog failed.\n");
+		return ret;
+	}
 
-	return ret;
+	dev->power_on = false;
+	return 0;
 }
 
 static int ov2680_s_power(struct v4l2_subdev *sd, int on)
@@ -866,7 +876,7 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
 	dev_dbg(&client->dev, "%s: %dx%d\n",
 		__func__, fmt->width, fmt->height);
 
-	// IS IT NEEDED?
+	/* s_power has not been called yet for std v4l2 clients (camorama) */
 	power_up(sd);
 	ret = ov2680_write_reg_array(client, dev->res->regs);
 	if (ret)
@@ -998,16 +1008,6 @@ static int ov2680_s_config(struct v4l2_subdev *sd,
 	    (struct camera_sensor_platform_data *)platform_data;
 
 	mutex_lock(&dev->input_lock);
-	/*
-	 * power off the module, then power on it in future
-	 * as first power on by board may not fulfill the
-	 * power on sequqence needed by the module
-	 */
-	ret = power_down(sd);
-	if (ret) {
-		dev_err(&client->dev, "ov2680 power-off err.\n");
-		goto fail_power_off;
-	}
 
 	ret = power_up(sd);
 	if (ret) {
@@ -1041,7 +1041,6 @@ static int ov2680_s_config(struct v4l2_subdev *sd,
 fail_power_on:
 	power_down(sd);
 	dev_err(&client->dev, "sensor power-gating failed\n");
-fail_power_off:
 	mutex_unlock(&dev->input_lock);
 	return ret;
 }
diff --git a/drivers/staging/media/atomisp/i2c/ov2680.h b/drivers/staging/media/atomisp/i2c/ov2680.h
index 9f9f6695eed9..edd87bb8563f 100644
--- a/drivers/staging/media/atomisp/i2c/ov2680.h
+++ b/drivers/staging/media/atomisp/i2c/ov2680.h
@@ -173,6 +173,7 @@ struct ov2680_device {
 	struct v4l2_ctrl_handler ctrl_handler;
 	struct ov2680_resolution *res;
 	struct camera_sensor_platform_data *platform_data;
+	bool power_on;
 };
 
 /**
-- 
2.31.1

[PATCH 03/11] media: atomisp-ov2680: Push the input_lock taking up into ov2680_s_power()

[Hans de Goede]

ov2680_s_power() is the only caller of ov2680_init(), push the input_lock
taking from ov2680_init() up into ov2680_s_power(), this way the new
power_on bool is also protected by it.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../media/atomisp/i2c/atomisp-ov2680.c        | 20 ++++++++-----------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
index 7b7cf7a68823..2721223ebcde 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
@@ -655,21 +655,11 @@ static int ov2680_init_registers(struct v4l2_subdev *sd)
 
 static int ov2680_init(struct v4l2_subdev *sd)
 {
-	struct ov2680_device *dev = to_ov2680_sensor(sd);
-
-	int ret;
-
-	mutex_lock(&dev->input_lock);
-
 	/* restore settings */
 	ov2680_res = ov2680_res_preview;
 	N_RES = N_RES_PREVIEW;
 
-	ret = ov2680_init_registers(sd);
-
-	mutex_unlock(&dev->input_lock);
-
-	return ret;
+	return ov2680_init_registers(sd);
 }
 
 static int power_ctrl(struct v4l2_subdev *sd, bool flag)
@@ -817,15 +807,21 @@ static int power_down(struct v4l2_subdev *sd)
 
 static int ov2680_s_power(struct v4l2_subdev *sd, int on)
 {
+	struct ov2680_device *dev = to_ov2680_sensor(sd);
 	int ret;
 
+	mutex_lock(&dev->input_lock);
+
 	if (on == 0) {
 		ret = power_down(sd);
 	} else {
 		ret = power_up(sd);
 		if (!ret)
-			return ov2680_init(sd);
+			ret = ov2680_init(sd);
 	}
+
+	mutex_unlock(&dev->input_lock);
+
 	return ret;
 }
 
-- 
2.31.1

[PATCH 04/11] media: atomisp-ov2680: Remove the ov2680_res and N_RES global variables

[Hans de Goede]

The ov2680_res and N_RES global variables are just hardcoded as aliases
for ov2680_res_preview and N_RES_PREVIEW, remove them.

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

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
index 2721223ebcde..b6927f9be022 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
@@ -653,15 +653,6 @@ static int ov2680_init_registers(struct v4l2_subdev *sd)
 	return ret;
 }
 
-static int ov2680_init(struct v4l2_subdev *sd)
-{
-	/* restore settings */
-	ov2680_res = ov2680_res_preview;
-	N_RES = N_RES_PREVIEW;
-
-	return ov2680_init_registers(sd);
-}
-
 static int power_ctrl(struct v4l2_subdev *sd, bool flag)
 {
 	int ret = 0;
@@ -817,7 +808,7 @@ static int ov2680_s_power(struct v4l2_subdev *sd, int on)
 	} else {
 		ret = power_up(sd);
 		if (!ret)
-			ret = ov2680_init(sd);
+			ret = ov2680_init_registers(sd);
 	}
 
 	mutex_unlock(&dev->input_lock);
@@ -857,7 +848,7 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
 				     ARRAY_SIZE(ov2680_res_preview), width,
 				     height, fmt->width, fmt->height);
 	if (!res)
-		res = &ov2680_res[N_RES - 1];
+		res = &ov2680_res_preview[N_RES_PREVIEW - 1];
 
 	fmt->width = res->width;
 	fmt->height = res->height;
@@ -975,11 +966,6 @@ static int ov2680_s_stream(struct v4l2_subdev *sd, int enable)
 	ret = ov2680_write_reg(client, 1, OV2680_SW_STREAM,
 			       enable ? OV2680_START_STREAMING :
 			       OV2680_STOP_STREAMING);
-#if 0
-	/* restore settings */
-	ov2680_res = ov2680_res_preview;
-	N_RES = N_RES_PREVIEW;
-#endif
 
 	//otp valid at stream on state
 	//if(!dev->otp_data)
@@ -1069,13 +1055,13 @@ static int ov2680_enum_frame_size(struct v4l2_subdev *sd,
 {
 	int index = fse->index;
 
-	if (index >= N_RES)
+	if (index >= N_RES_PREVIEW)
 		return -EINVAL;
 
-	fse->min_width = ov2680_res[index].width;
-	fse->min_height = ov2680_res[index].height;
-	fse->max_width = ov2680_res[index].width;
-	fse->max_height = ov2680_res[index].height;
+	fse->min_width = ov2680_res_preview[index].width;
+	fse->min_height = ov2680_res_preview[index].height;
+	fse->max_width = ov2680_res_preview[index].width;
+	fse->max_height = ov2680_res_preview[index].height;
 
 	return 0;
 }
diff --git a/drivers/staging/media/atomisp/i2c/ov2680.h b/drivers/staging/media/atomisp/i2c/ov2680.h
index edd87bb8563f..c1998c9132a2 100644
--- a/drivers/staging/media/atomisp/i2c/ov2680.h
+++ b/drivers/staging/media/atomisp/i2c/ov2680.h
@@ -838,7 +838,4 @@ static struct ov2680_resolution ov2680_res_preview[] = {
 
 #define N_RES_PREVIEW (ARRAY_SIZE(ov2680_res_preview))
 
-static struct ov2680_resolution *ov2680_res = ov2680_res_preview;
-static unsigned long N_RES = N_RES_PREVIEW;
-
 #endif
-- 
2.31.1

[PATCH 05/11] media: atomisp-ov2680: Move ov2680_init_registers() call to power_up()

[Hans de Goede]

Move ov2680_init_registers() call to power_up(), so that we also
init the registers on code-paths which do not call ov2680_s_power()
like running camorama.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Note we should really look into makeing sure that s_power() is always
called when it should and remove the power_up() call from
ov2680_set_fmt() when that is done.

Even then we still need the power_on bool though since power_down()
gets called on every runtime-suspend, even if power_up() was never
called.
---
 drivers/staging/media/atomisp/i2c/atomisp-ov2680.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
index b6927f9be022..83608ba4e70a 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
@@ -744,9 +744,15 @@ static int power_up(struct v4l2_subdev *sd)
 	/* according to DS, 20ms is needed between PWDN and i2c access */
 	msleep(20);
 
+	ret = ov2680_init_registers(sd);
+	if (ret)
+		goto fail_init_registers;
+
 	dev->power_on = true;
 	return 0;
 
+fail_init_registers:
+	dev->platform_data->flisclk_ctrl(sd, 0);
 fail_clk:
 	gpio_ctrl(sd, 0);
 fail_power:
@@ -807,8 +813,6 @@ static int ov2680_s_power(struct v4l2_subdev *sd, int on)
 		ret = power_down(sd);
 	} else {
 		ret = power_up(sd);
-		if (!ret)
-			ret = ov2680_init_registers(sd);
 	}
 
 	mutex_unlock(&dev->input_lock);
-- 
2.31.1

[PATCH 06/11] media: atomisp-ov2680: Save/restore exposure and gain over sensor power-down

[Hans de Goede]

Save/restore exposure and gain over sensor power-down and don't write them
to the sensor when ov2680_set_exposure() is called while the sensor is off.

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

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
index 83608ba4e70a..5b192218ba91 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
@@ -359,7 +359,14 @@ static int ov2680_set_exposure(struct v4l2_subdev *sd, int exposure,
 	int ret;
 
 	mutex_lock(&dev->input_lock);
-	ret = __ov2680_set_exposure(sd, exposure, gain, digitgain);
+
+	dev->exposure = exposure;
+	dev->gain = gain;
+	dev->digitgain = digitgain;
+
+	if (dev->power_on)
+		ret = __ov2680_set_exposure(sd, exposure, gain, digitgain);
+
 	mutex_unlock(&dev->input_lock);
 
 	return ret;
@@ -748,6 +755,10 @@ static int power_up(struct v4l2_subdev *sd)
 	if (ret)
 		goto fail_init_registers;
 
+	ret = __ov2680_set_exposure(sd, dev->exposure, dev->gain, dev->digitgain);
+	if (ret)
+		goto fail_init_registers;
+
 	dev->power_on = true;
 	return 0;
 
@@ -1140,6 +1151,8 @@ static int ov2680_probe(struct i2c_client *client)
 	mutex_init(&dev->input_lock);
 
 	dev->res = &ov2680_res_preview[0];
+	dev->exposure = dev->res->lines_per_frame - OV2680_INTEGRATION_TIME_MARGIN;
+	dev->gain = 250; /* 0-2047 */
 	v4l2_i2c_subdev_init(&dev->sd, client, &ov2680_ops);
 
 	pdata = gmin_camera_platform_data(&dev->sd,
diff --git a/drivers/staging/media/atomisp/i2c/ov2680.h b/drivers/staging/media/atomisp/i2c/ov2680.h
index c1998c9132a2..ca20ce5aa285 100644
--- a/drivers/staging/media/atomisp/i2c/ov2680.h
+++ b/drivers/staging/media/atomisp/i2c/ov2680.h
@@ -174,6 +174,9 @@ struct ov2680_device {
 	struct ov2680_resolution *res;
 	struct camera_sensor_platform_data *platform_data;
 	bool power_on;
+	u16 exposure;
+	u16 gain;
+	u16 digitgain;
 };
 
 /**
-- 
2.31.1

[PATCH 07/11] media: atomisp-ov2680: Make ov2680_read_reg() support 24 bit registers

[Hans de Goede]

Some ov2680 registers like exposure are 24 bit,
ov2680_read_reg() already mostly supports this, we just
need to change the return type from u16 to u32.

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

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
index 5b192218ba91..7e49f4eb0410 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
@@ -48,7 +48,7 @@ static enum atomisp_bayer_order ov2680_bayer_order_mapping[] = {
 
 /* i2c read/write stuff */
 static int ov2680_read_reg(struct i2c_client *client,
-			   int len, u16 reg, u16 *val)
+			   int len, u16 reg, u32 *val)
 {
 	struct i2c_msg msgs[2];
 	u8 addr_buf[2] = { reg >> 8, reg & 0xff };
@@ -168,7 +168,7 @@ static int ov2680_get_intg_factor(struct i2c_client *client,
 {
 	struct atomisp_sensor_mode_data *buf = &info->data;
 	unsigned int pix_clk_freq_hz;
-	u16 reg_val;
+	u32 reg_val;
 	int ret;
 
 	dev_dbg(&client->dev,  "++++ov2680_get_intg_factor\n");
@@ -410,7 +410,7 @@ static long ov2680_ioctl(struct v4l2_subdev *sd, unsigned int cmd, void *arg)
 static int ov2680_q_exposure(struct v4l2_subdev *sd, s32 *value)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
-	u16 reg_v, reg_v2;
+	u32 reg_v, reg_v2;
 	int ret;
 
 	/* get exposure */
@@ -433,7 +433,7 @@ static int ov2680_q_exposure(struct v4l2_subdev *sd, s32 *value)
 	if (ret)
 		goto err;
 
-	*value = reg_v + (((u32)reg_v2 << 16));
+	*value = reg_v + (reg_v2 << 16);
 err:
 	return ret;
 }
@@ -443,7 +443,7 @@ static int ov2680_v_flip(struct v4l2_subdev *sd, s32 value)
 	struct camera_mipi_info *ov2680_info = NULL;
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	int ret;
-	u16 val;
+	u32 val;
 	u8 index;
 
 	dev_dbg(&client->dev, "@%s: value:%d\n", __func__, value);
@@ -473,7 +473,7 @@ static int ov2680_h_flip(struct v4l2_subdev *sd, s32 value)
 	struct camera_mipi_info *ov2680_info = NULL;
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	int ret;
-	u16 val;
+	u32 val;
 	u8 index;
 
 	dev_dbg(&client->dev, "@%s: value:%d\n", __func__, value);
@@ -933,7 +933,7 @@ static int ov2680_get_fmt(struct v4l2_subdev *sd,
 static int ov2680_detect(struct i2c_client *client)
 {
 	struct i2c_adapter *adapter = client->adapter;
-	u16 high, low;
+	u32 high, low;
 	int ret;
 	u16 id;
 	u8 revision;
-- 
2.31.1

[PATCH 08/11] media: atomisp-ov2680: Fix and simplify ov2680_q_exposure()

[Hans de Goede]

Switch to ov2680_read_reg() to read all 24 bits in one go;
and the exposure value sits in bits 4-19 of the 24 bit exposure
register, so we need to shift the read value by 4 to report the
correct value.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../media/atomisp/i2c/atomisp-ov2680.c        | 27 +++++--------------
 1 file changed, 6 insertions(+), 21 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
index 7e49f4eb0410..d6a5f75fdd66 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
@@ -410,32 +410,17 @@ static long ov2680_ioctl(struct v4l2_subdev *sd, unsigned int cmd, void *arg)
 static int ov2680_q_exposure(struct v4l2_subdev *sd, s32 *value)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
-	u32 reg_v, reg_v2;
+	u32 reg_val;
 	int ret;
 
 	/* get exposure */
-	ret = ov2680_read_reg(client, 1,
-			      OV2680_EXPOSURE_L,
-			      &reg_v);
-	if (ret)
-		goto err;
-
-	ret = ov2680_read_reg(client, 1,
-			      OV2680_EXPOSURE_M,
-			      &reg_v2);
+	ret = ov2680_read_reg(client, 3, OV2680_EXPOSURE_H, &reg_val);
 	if (ret)
-		goto err;
-
-	reg_v += reg_v2 << 8;
-	ret = ov2680_read_reg(client, 1,
-			      OV2680_EXPOSURE_H,
-			      &reg_v2);
-	if (ret)
-		goto err;
+		return ret;
 
-	*value = reg_v + (reg_v2 << 16);
-err:
-	return ret;
+	/* Lower four bits are not part of the exposure val (always 0) */
+	*value = reg_val >> 4;
+	return 0;
 }
 
 static int ov2680_v_flip(struct v4l2_subdev *sd, s32 value)
-- 
2.31.1

[PATCH 09/11] media: atomisp-ov2680: Fix ov2680_write_reg() always writing 0 to 16 bit registers

[Hans de Goede]

The shift << 16 of the value in the code path for 16 bit values is
bogus, put_unaligned_be16() takes the lower 16 bits which will not
always be 0.

This was causing __ov2680_set_exposure() to always set the
OV2680_AGC and OV2680_TIMING_VTS registers to 0.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/i2c/atomisp-ov2680.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
index d6a5f75fdd66..1092d1c2993f 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
@@ -86,7 +86,7 @@ static int ov2680_write_reg(struct i2c_client *client, unsigned int len,
 	int ret;
 
 	if (len == 2)
-		put_unaligned_be16(val << (8 * (4 - len)), buf + 2);
+		put_unaligned_be16(val, buf + 2);
 	else if (len == 1)
 		buf[2] = val;
 	else
-- 
2.31.1

[PATCH 10/11] media: atomisp-ov2680: Fix ov2680_set_fmt() clobbering the exposure

[Hans de Goede]

Now that we restore the default or last user set exposure setting on
power_up() there is no need for the registers written by ov2680_set_fmt()
to write to the exposure register.

Not doing so fixes the exposure always being reset to the value from
the res->regs array after a set_fmt().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/i2c/ov2680.h | 24 ----------------------
 1 file changed, 24 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/ov2680.h b/drivers/staging/media/atomisp/i2c/ov2680.h
index ca20ce5aa285..d52b7fbe3c06 100644
--- a/drivers/staging/media/atomisp/i2c/ov2680.h
+++ b/drivers/staging/media/atomisp/i2c/ov2680.h
@@ -288,8 +288,6 @@ static struct ov2680_reg const ov2680_global_setting[] = {
  */
 static struct ov2680_reg const ov2680_QCIF_30fps[] = {
 	{0x3086, 0x01},
-	{0x3501, 0x24},
-	{0x3502, 0x40},
 	{0x370a, 0x23},
 	{0x3801, 0xa0},
 	{0x3802, 0x00},
@@ -333,8 +331,6 @@ static struct ov2680_reg const ov2680_QCIF_30fps[] = {
  */
 static struct ov2680_reg const ov2680_CIF_30fps[] = {
 	{0x3086, 0x01},
-	{0x3501, 0x24},
-	{0x3502, 0x40},
 	{0x370a, 0x23},
 	{0x3801, 0xa0},
 	{0x3802, 0x00},
@@ -376,8 +372,6 @@ static struct ov2680_reg const ov2680_CIF_30fps[] = {
  */
 static struct ov2680_reg const ov2680_QVGA_30fps[] = {
 	{0x3086, 0x01},
-	{0x3501, 0x24},
-	{0x3502, 0x40},
 	{0x370a, 0x23},
 	{0x3801, 0xa0},
 	{0x3802, 0x00},
@@ -419,8 +413,6 @@ static struct ov2680_reg const ov2680_QVGA_30fps[] = {
  */
 static struct ov2680_reg const ov2680_656x496_30fps[] = {
 	{0x3086, 0x01},
-	{0x3501, 0x24},
-	{0x3502, 0x40},
 	{0x370a, 0x23},
 	{0x3801, 0xa0},
 	{0x3802, 0x00},
@@ -462,8 +454,6 @@ static struct ov2680_reg const ov2680_656x496_30fps[] = {
  */
 static struct ov2680_reg const ov2680_720x592_30fps[] = {
 	{0x3086, 0x01},
-	{0x3501, 0x26},
-	{0x3502, 0x40},
 	{0x370a, 0x23},
 	{0x3801, 0x00}, // X_ADDR_START;
 	{0x3802, 0x00},
@@ -507,8 +497,6 @@ static struct ov2680_reg const ov2680_720x592_30fps[] = {
  */
 static struct ov2680_reg const ov2680_800x600_30fps[] = {
 	{0x3086, 0x01},
-	{0x3501, 0x26},
-	{0x3502, 0x40},
 	{0x370a, 0x23},
 	{0x3801, 0x00},
 	{0x3802, 0x00},
@@ -550,8 +538,6 @@ static struct ov2680_reg const ov2680_800x600_30fps[] = {
  */
 static struct ov2680_reg const ov2680_720p_30fps[] = {
 	{0x3086, 0x00},
-	{0x3501, 0x48},
-	{0x3502, 0xe0},
 	{0x370a, 0x21},
 	{0x3801, 0xa0},
 	{0x3802, 0x00},
@@ -593,8 +579,6 @@ static struct ov2680_reg const ov2680_720p_30fps[] = {
  */
 static struct ov2680_reg const ov2680_1296x976_30fps[] = {
 	{0x3086, 0x00},
-	{0x3501, 0x48},
-	{0x3502, 0xe0},
 	{0x370a, 0x21},
 	{0x3801, 0xa0},
 	{0x3802, 0x00},
@@ -636,8 +620,6 @@ static struct ov2680_reg const ov2680_1296x976_30fps[] = {
  */
 static struct ov2680_reg const ov2680_1456x1096_30fps[] = {
 	{0x3086, 0x00},
-	{0x3501, 0x48},
-	{0x3502, 0xe0},
 	{0x370a, 0x21},
 	{0x3801, 0x90},
 	{0x3802, 0x00},
@@ -681,8 +663,6 @@ static struct ov2680_reg const ov2680_1456x1096_30fps[] = {
 
 static struct ov2680_reg const ov2680_1616x916_30fps[] = {
 	{0x3086, 0x00},
-	{0x3501, 0x48},
-	{0x3502, 0xe0},
 	{0x370a, 0x21},
 	{0x3801, 0x00},
 	{0x3802, 0x00},
@@ -725,8 +705,6 @@ static struct ov2680_reg const ov2680_1616x916_30fps[] = {
 #if 0
 static struct ov2680_reg const ov2680_1616x1082_30fps[] = {
 	{0x3086, 0x00},
-	{0x3501, 0x48},
-	{0x3502, 0xe0},
 	{0x370a, 0x21},
 	{0x3801, 0x00},
 	{0x3802, 0x00},
@@ -768,8 +746,6 @@ static struct ov2680_reg const ov2680_1616x1082_30fps[] = {
  */
 static struct ov2680_reg const ov2680_1616x1216_30fps[] = {
 	{0x3086, 0x00},
-	{0x3501, 0x48},
-	{0x3502, 0xe0},
 	{0x370a, 0x21},
 	{0x3801, 0x00},
 	{0x3802, 0x00},
-- 
2.31.1

[PATCH 11/11] media: atomisp-ov2680: Fix ov2680_set_fmt() messing up high exposure settings

[Hans de Goede]

For exposure settings > (lines_per_frame - vts_margin) the VTS register
needs to be programmed to (exposure + vts_margin) rather then being
set to lines_per_frame.

The res->regs register array was clobbering this higher setting causing
high exposure settings to not work. Fix this by letting ov2680_set_fmt()
calculate the vts value, instead of hardcoding it.

This is the last in a series of fixes which fixes exposure and gain
settings not working, with this everything works, so drop the comment
that it does not work.

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

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
index 1092d1c2993f..34d008236bd9 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
@@ -387,7 +387,6 @@ static long ov2680_s_exposure(struct v4l2_subdev *sd,
 		return -EINVAL;
 	}
 
-	// EXPOSURE CONTROL DISABLED FOR INITIAL CHECKIN, TUNING DOESN'T WORK
 	return ov2680_set_exposure(sd, coarse_itg, analog_gain, digital_gain);
 }
 
@@ -825,7 +824,7 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	struct camera_mipi_info *ov2680_info = NULL;
 	struct ov2680_resolution *res;
-	int ret = 0;
+	int vts, ret = 0;
 
 	dev_dbg(&client->dev, "%s: %s: pad: %d, fmt: %p\n",
 		__func__,
@@ -870,6 +869,16 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
 		dev_err(&client->dev,
 			"ov2680 write resolution register err: %d\n", ret);
 
+	/* If necessary increase the VTS to match exposure + MARGIN */
+	if (dev->exposure > vts - OV2680_INTEGRATION_TIME_MARGIN)
+		vts = dev->exposure + OV2680_INTEGRATION_TIME_MARGIN;
+	else
+		vts = dev->res->lines_per_frame;
+
+	ret = ov2680_write_reg(client, 2, OV2680_TIMING_VTS_H, vts);
+	if (ret)
+		dev_err(&client->dev, "ov2680 write vts err: %d\n", ret);
+
 	ret = ov2680_get_intg_factor(client, ov2680_info, res);
 	if (ret) {
 		dev_err(&client->dev, "failed to get integration factor\n");
diff --git a/drivers/staging/media/atomisp/i2c/ov2680.h b/drivers/staging/media/atomisp/i2c/ov2680.h
index d52b7fbe3c06..e53be612a1e4 100644
--- a/drivers/staging/media/atomisp/i2c/ov2680.h
+++ b/drivers/staging/media/atomisp/i2c/ov2680.h
@@ -302,8 +302,6 @@ static struct ov2680_reg const ov2680_QCIF_30fps[] = {
 	{0x380b, 0xa0},
 	{0x380c, 0x06},
 	{0x380d, 0xb0},
-	{0x380e, 0x02},
-	{0x380f, 0x84},
 	{0x3810, 0x00},
 	{0x3811, 0x04},
 	{0x3812, 0x00},
@@ -345,8 +343,6 @@ static struct ov2680_reg const ov2680_CIF_30fps[] = {
 	{0x380b, 0x30},
 	{0x380c, 0x06},
 	{0x380d, 0xb0},
-	{0x380e, 0x02},
-	{0x380f, 0x84},
 	{0x3810, 0x00},
 	{0x3811, 0x04},
 	{0x3812, 0x00},
@@ -386,8 +382,6 @@ static struct ov2680_reg const ov2680_QVGA_30fps[] = {
 	{0x380b, 0x00},
 	{0x380c, 0x06},
 	{0x380d, 0xb0},
-	{0x380e, 0x02},
-	{0x380f, 0x84},
 	{0x3810, 0x00},
 	{0x3811, 0x04},
 	{0x3812, 0x00},
@@ -427,8 +421,6 @@ static struct ov2680_reg const ov2680_656x496_30fps[] = {
 	{0x380b, 0xf0},
 	{0x380c, 0x06},
 	{0x380d, 0xb0},
-	{0x380e, 0x02},
-	{0x380f, 0x84},
 	{0x3810, 0x00},
 	{0x3811, 0x04},
 	{0x3812, 0x00},
@@ -468,8 +460,6 @@ static struct ov2680_reg const ov2680_720x592_30fps[] = {
 	{0x380b, 0x50}, // Y_OUTPUT_SIZE;
 	{0x380c, 0x06},
 	{0x380d, 0xac}, // HTS;
-	{0x380e, 0x02},
-	{0x380f, 0x84}, // VTS;
 	{0x3810, 0x00},
 	{0x3811, 0x00},
 	{0x3812, 0x00},
@@ -511,8 +501,6 @@ static struct ov2680_reg const ov2680_800x600_30fps[] = {
 	{0x380b, 0x58},
 	{0x380c, 0x06},
 	{0x380d, 0xac},
-	{0x380e, 0x02},
-	{0x380f, 0x84},
 	{0x3810, 0x00},
 	{0x3811, 0x00},
 	{0x3812, 0x00},
@@ -552,8 +540,6 @@ static struct ov2680_reg const ov2680_720p_30fps[] = {
 	{0x380b, 0xe0},
 	{0x380c, 0x06},
 	{0x380d, 0xa8},
-	{0x380e, 0x05},
-	{0x380f, 0x0e},
 	{0x3810, 0x00},
 	{0x3811, 0x08},
 	{0x3812, 0x00},
@@ -593,8 +579,6 @@ static struct ov2680_reg const ov2680_1296x976_30fps[] = {
 	{0x380b, 0xd0},
 	{0x380c, 0x06},
 	{0x380d, 0xa8},
-	{0x380e, 0x05},
-	{0x380f, 0x0e},
 	{0x3810, 0x00},
 	{0x3811, 0x08},
 	{0x3812, 0x00},
@@ -634,8 +618,6 @@ static struct ov2680_reg const ov2680_1456x1096_30fps[] = {
 	{0x380b, 0x48},
 	{0x380c, 0x06},
 	{0x380d, 0xa8},
-	{0x380e, 0x05},
-	{0x380f, 0x0e},
 	{0x3810, 0x00},
 	{0x3811, 0x08},
 	{0x3812, 0x00},
@@ -677,8 +659,6 @@ static struct ov2680_reg const ov2680_1616x916_30fps[] = {
 	{0x380b, 0x94},
 	{0x380c, 0x06},
 	{0x380d, 0xa8},
-	{0x380e, 0x05},
-	{0x380f, 0x0e},
 	{0x3810, 0x00},
 	{0x3811, 0x00},
 	{0x3812, 0x00},
@@ -719,8 +699,6 @@ static struct ov2680_reg const ov2680_1616x1082_30fps[] = {
 	{0x380b, 0x3a},
 	{0x380c, 0x06},
 	{0x380d, 0xa8},
-	{0x380e, 0x05},
-	{0x380f, 0x0e},
 	{0x3810, 0x00},
 	{0x3811, 0x00},
 	{0x3812, 0x00},
@@ -760,8 +738,6 @@ static struct ov2680_reg const ov2680_1616x1216_30fps[] = {
 	{0x380b, 0xc0},//c0},
 	{0x380c, 0x06},
 	{0x380d, 0xa8},
-	{0x380e, 0x05},
-	{0x380f, 0x0e},
 	{0x3810, 0x00},
 	{0x3811, 0x00},
 	{0x3812, 0x00},
-- 
2.31.1