[PATCH v5 00/32] media: ov2680: Bugfixes + ACPI + selection(crop-tgt) API support

[PATCH v5 00/32] media: ov2680: Bugfixes + ACPI + selection(crop-tgt) API support

[Hans de Goede]

Hi All,

Unforunately v4 still had an issue with do_div() usage, so here is v5.

This is also available in this branch:
https://git.kernel.org/pub/scm/linux/kernel/git/hansg/linux.git/log/?h=media-ov2680

Changes in v5:
- Add some notes to the ACPI support commit message about the OVTI2680 HID
- Make pixel_rate u64 instead of s64 for do_div() to fix:
  drivers/media/i2c/ov2680.c:1092:2: warning: comparison of distinct pointer types ('typeof ((sensor->pixel_rate)) *' (aka 'long long *') and 'uint64_t *' (aka 'unsigned long long *')) [-Wcompare-distinct-pointer-types]

Changes in v4:
- Rebase on top of latest sailus/media_tree/master (69142f89b61e)
- Do not set v4l2_subdev.fwnode (let v4l2-core do endpoint matching)
- Make enum_frame_interval() input checks more strict
- Make enum_frame_size() returned full-size + binned-/quarter-size
- Use V4L2_CID_ANALOGUE_GAIN for gain
- 3 new patches:
  "media: ov2680: Add bus-cfg / endpoint property verification"
  "media: ipu-bridge: Add link-frequency to OV2680 ipu_supported_sensors[] entry"
  "media: atomisp: Drop atomisp-ov2680 sensor driver"

Changes in v3:
- Add Rui Miguel Silva's Ack for the series
- 2 small fixes for remarks from Andy
- Add a new patch adding me as co-maintainer in MAINTAINERS

Changes in v2
- Drop "media: Add MIPI CCI register access helper functions"
  (being reviewed in its own thread / patch-submission)
- Drop "media: ov2680: Add g_skip_frames op support"
- Add "media: ov2680: Fix regulators being left enabled on
  ov2680_power_on() errors"
- Add "media: ov2680: Add link-freq and pixel-rate controls"
  with this the driver now works on IPU3 with ipu3-capture.sh
  (libcamera support requires adding a couple more controls)
- Limit line length to 80 chars everywhere
- Address various small remarks from Andy

During all the work done on the atomisp driver I have mostly been testing
on devices with an ov2680 sensor. As such I have also done a lot of work
on the atomisp-ov2680.c atomisp specific sensor driver.

With the latest atomisp code from:
https://git.kernel.org/pub/scm/linux/kernel/git/hansg/linux.git/tag/?h=media-atomisp-6.5-1

The atomisp code can now work with standard v4l2 sensor drivers using
the selections (crop-tgt) api and v4l2-async sensor driver registration.

This patch series modifies the main drivers/media/i2c/ov2680.c driver
to add bugfixes, ACPI enumeration, selection API support and further
improvments. After this the driver can be used with the atomisp driver
and atomisp-ov2680.c can be dropped.

This also gets the driver much closer to having everything needed for
use with IPU3 / libcamera. I have a Lenovo Miix 510 now with an IPU3 +
ov2680 sensor and with this series raw-capture using the ipu3-capture.sh
script works. I plan to work on libcamera support for this in the near
future.

Regards,

Hans


Hans de Goede (32):
  media: ov2680: Remove auto-gain and auto-exposure controls
  media: ov2680: Fix ov2680_bayer_order()
  media: ov2680: Fix vflip / hflip set functions
  media: ov2680: Remove VIDEO_V4L2_SUBDEV_API ifdef-s
  media: ov2680: Don't take the lock for try_fmt calls
  media: ov2680: Add ov2680_fill_format() helper function
  media: ov2680: Fix ov2680_set_fmt() which == V4L2_SUBDEV_FORMAT_TRY
    not working
  media: ov2680: Fix regulators being left enabled on ov2680_power_on()
    errors
  media: ov2680: Convert to new CCI register access helpers
  media: ov2680: Store dev instead of i2c_client in ov2680_dev
  media: ov2680: Add runtime-pm support
  media: ov2680: Check for "powerdown" GPIO con-id before checking for
    "reset" GPIO con-id
  media: ov2680: Drop is_enabled flag
  media: ov2680: Add support for more clk setups
  media: ov2680: Add support for 19.2 MHz clock
  media: ov2680: Wait for endpoint fwnode before continuing with probe()
  media: ov2680: Add support for ACPI enumeration
  media: ov2680: Fix ov2680_enum_frame_interval()
  media: ov2680: Annotate the per mode register setting lists
  media: ov2680: Add ov2680_mode struct
  media: ov2680: Make setting the mode algorithm based
  media: ov2680: Add an __ov2680_get_pad_format() helper function
  media: ov2680: Implement selection support
  media: ov2680: Fix exposure and gain ctrls range and default value
  media: ov2680: Add a bunch of register tweaks
  media: ov2680: Drop unnecessary pad checks
  media: ov2680: Read and log sensor revision during probe
  media: ov2680: Add link-freq and pixel-rate controls
  media: ov2680: Add bus-cfg / endpoint property verification
  MAINTAINERS: Add Hans de Goede as OV2680 sensor driver maintainer
  media: ipu-bridge: Add link-frequency to OV2680
    ipu_supported_sensors[] entry
  media: atomisp: Drop atomisp-ov2680 sensor driver

 MAINTAINERS                                   |    1 +
 drivers/media/i2c/Kconfig                     |    1 +
 drivers/media/i2c/ov2680.c                    | 1380 +++++++++--------
 drivers/media/pci/intel/ipu-bridge.c          |    2 +-
 drivers/staging/media/atomisp/i2c/Kconfig     |   13 -
 drivers/staging/media/atomisp/i2c/Makefile    |    1 -
 .../media/atomisp/i2c/atomisp-ov2680.c        |  828 ----------
 drivers/staging/media/atomisp/i2c/ov2680.h    |  173 ---
 8 files changed, 752 insertions(+), 1647 deletions(-)
 delete mode 100644 drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
 delete mode 100644 drivers/staging/media/atomisp/i2c/ov2680.h

-- 
2.41.0

[PATCH v5 01/32] media: ov2680: Remove auto-gain and auto-exposure controls

[Hans de Goede]

Quoting the OV2680 datasheet:

"3.2 exposure and gain control

In the OV2680, the exposure time and gain are set manually from an external
controller. The OV2680 supports manual gain and exposure control only for
normal applications, no auto mode."

And indeed testing with the atomisp_ov2680 fork of ov2680.c has shown that
auto-exposure and auto-gain do not work.

Note that the code setting the auto-exposure flag was broken, callers
of ov2680_exposure_set() were directly passing !!ctrls->auto_exp->val as
"bool auto_exp" value, but ctrls->auto_exp is a menu control with:

enum  v4l2_exposure_auto_type {
        V4L2_EXPOSURE_AUTO = 0,
        V4L2_EXPOSURE_MANUAL = 1,
	...

So instead of passing !!ctrls->auto_exp->val they should have been passing
ctrls->auto_exp->val == V4L2_EXPOSURE_AUTO, iow the passed value was
inverted of what it should have been.

Also remove ov2680_g_volatile_ctrl() since without auto support the gain
and exposure controls are not volatile.

This also fixes the control values not being properly applied in
ov2680_mode_set(). The 800x600 mode register-list also sets gain,
exposure and vflip overriding the last set ctrl values.

ov2680_mode_set() does call ov2680_gain_set() and ov2680_exposure_set()
but did this before writing the mode register-list, so these values
would still be overridden by the mode register-list.

Add a v4l2_ctrl_handler_setup() call after writing the mode register-list
to restore all ctrl values. Also remove the ctrls->gain->is_new check from
ov2680_gain_set() so that the gain always gets restored properly.

Last since ov2680_mode_set() now calls v4l2_ctrl_handler_setup(), remove
the v4l2_ctrl_handler_setup() call after ov2680_mode_restore() since
ov2680_mode_restore() calls ov2680_mode_set().

Fixes: 3ee47cad3e69 ("media: ov2680: Add Omnivision OV2680 sensor driver")
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/ov2680.c | 163 ++++---------------------------------
 1 file changed, 18 insertions(+), 145 deletions(-)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index 0541b7c8e77b..3a737a1607a4 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -85,15 +85,8 @@ struct ov2680_mode_info {
 
 struct ov2680_ctrls {
 	struct v4l2_ctrl_handler handler;
-	struct {
-		struct v4l2_ctrl *auto_exp;
-		struct v4l2_ctrl *exposure;
-	};
-	struct {
-		struct v4l2_ctrl *auto_gain;
-		struct v4l2_ctrl *gain;
-	};
-
+	struct v4l2_ctrl *exposure;
+	struct v4l2_ctrl *gain;
 	struct v4l2_ctrl *hflip;
 	struct v4l2_ctrl *vflip;
 	struct v4l2_ctrl *test_pattern;
@@ -143,6 +136,7 @@ static const struct reg_value ov2680_setting_30fps_QUXGA_800_600[] = {
 	{0x380e, 0x02}, {0x380f, 0x84}, {0x3811, 0x04}, {0x3813, 0x04},
 	{0x3814, 0x31}, {0x3815, 0x31}, {0x3820, 0xc0}, {0x4008, 0x00},
 	{0x4009, 0x03}, {0x4837, 0x1e}, {0x3501, 0x4e}, {0x3502, 0xe0},
+	{0x3503, 0x03},
 };
 
 static const struct reg_value ov2680_setting_30fps_720P_1280_720[] = {
@@ -405,69 +399,15 @@ static int ov2680_test_pattern_set(struct ov2680_dev *sensor, int value)
 	return 0;
 }
 
-static int ov2680_gain_set(struct ov2680_dev *sensor, bool auto_gain)
+static int ov2680_gain_set(struct ov2680_dev *sensor, u32 gain)
 {
-	struct ov2680_ctrls *ctrls = &sensor->ctrls;
-	u32 gain;
-	int ret;
-
-	ret = ov2680_mod_reg(sensor, OV2680_REG_R_MANUAL, BIT(1),
-			     auto_gain ? 0 : BIT(1));
-	if (ret < 0)
-		return ret;
-
-	if (auto_gain || !ctrls->gain->is_new)
-		return 0;
-
-	gain = ctrls->gain->val;
-
-	ret = ov2680_write_reg16(sensor, OV2680_REG_GAIN_PK, gain);
-
-	return 0;
+	return ov2680_write_reg16(sensor, OV2680_REG_GAIN_PK, gain);
 }
 
-static int ov2680_gain_get(struct ov2680_dev *sensor)
+static int ov2680_exposure_set(struct ov2680_dev *sensor, u32 exp)
 {
-	u32 gain;
-	int ret;
-
-	ret = ov2680_read_reg16(sensor, OV2680_REG_GAIN_PK, &gain);
-	if (ret)
-		return ret;
-
-	return gain;
-}
-
-static int ov2680_exposure_set(struct ov2680_dev *sensor, bool auto_exp)
-{
-	struct ov2680_ctrls *ctrls = &sensor->ctrls;
-	u32 exp;
-	int ret;
-
-	ret = ov2680_mod_reg(sensor, OV2680_REG_R_MANUAL, BIT(0),
-			     auto_exp ? 0 : BIT(0));
-	if (ret < 0)
-		return ret;
-
-	if (auto_exp || !ctrls->exposure->is_new)
-		return 0;
-
-	exp = (u32)ctrls->exposure->val;
-	exp <<= 4;
-
-	return ov2680_write_reg24(sensor, OV2680_REG_EXPOSURE_PK_HIGH, exp);
-}
-
-static int ov2680_exposure_get(struct ov2680_dev *sensor)
-{
-	int ret;
-	u32 exp;
-
-	ret = ov2680_read_reg24(sensor, OV2680_REG_EXPOSURE_PK_HIGH, &exp);
-	if (ret)
-		return ret;
-
-	return exp >> 4;
+	return ov2680_write_reg24(sensor, OV2680_REG_EXPOSURE_PK_HIGH,
+				  exp << 4);
 }
 
 static int ov2680_stream_enable(struct ov2680_dev *sensor)
@@ -482,32 +422,16 @@ static int ov2680_stream_disable(struct ov2680_dev *sensor)
 
 static int ov2680_mode_set(struct ov2680_dev *sensor)
 {
-	struct ov2680_ctrls *ctrls = &sensor->ctrls;
 	int ret;
 
-	ret = ov2680_gain_set(sensor, false);
-	if (ret < 0)
-		return ret;
-
-	ret = ov2680_exposure_set(sensor, false);
-	if (ret < 0)
-		return ret;
-
 	ret = ov2680_load_regs(sensor, sensor->current_mode);
 	if (ret < 0)
 		return ret;
 
-	if (ctrls->auto_gain->val) {
-		ret = ov2680_gain_set(sensor, true);
-		if (ret < 0)
-			return ret;
-	}
-
-	if (ctrls->auto_exp->val == V4L2_EXPOSURE_AUTO) {
-		ret = ov2680_exposure_set(sensor, true);
-		if (ret < 0)
-			return ret;
-	}
+	/* Restore value of all ctrls */
+	ret = __v4l2_ctrl_handler_setup(&sensor->ctrls.handler);
+	if (ret < 0)
+		return ret;
 
 	sensor->mode_pending_changes = false;
 
@@ -590,15 +514,10 @@ static int ov2680_s_power(struct v4l2_subdev *sd, int on)
 	else
 		ret = ov2680_power_off(sensor);
 
-	mutex_unlock(&sensor->lock);
-
-	if (on && ret == 0) {
-		ret = v4l2_ctrl_handler_setup(&sensor->ctrls.handler);
-		if (ret < 0)
-			return ret;
-
+	if (on && ret == 0)
 		ret = ov2680_mode_restore(sensor);
-	}
+
+	mutex_unlock(&sensor->lock);
 
 	return ret;
 }
@@ -794,52 +713,19 @@ static int ov2680_enum_frame_interval(struct v4l2_subdev *sd,
 	return 0;
 }
 
-static int ov2680_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
-{
-	struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
-	struct ov2680_dev *sensor = to_ov2680_dev(sd);
-	struct ov2680_ctrls *ctrls = &sensor->ctrls;
-	int val;
-
-	if (!sensor->is_enabled)
-		return 0;
-
-	switch (ctrl->id) {
-	case V4L2_CID_GAIN:
-		val = ov2680_gain_get(sensor);
-		if (val < 0)
-			return val;
-		ctrls->gain->val = val;
-		break;
-	case V4L2_CID_EXPOSURE:
-		val = ov2680_exposure_get(sensor);
-		if (val < 0)
-			return val;
-		ctrls->exposure->val = val;
-		break;
-	}
-
-	return 0;
-}
-
 static int ov2680_s_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
 	struct ov2680_dev *sensor = to_ov2680_dev(sd);
-	struct ov2680_ctrls *ctrls = &sensor->ctrls;
 
 	if (!sensor->is_enabled)
 		return 0;
 
 	switch (ctrl->id) {
-	case V4L2_CID_AUTOGAIN:
-		return ov2680_gain_set(sensor, !!ctrl->val);
 	case V4L2_CID_GAIN:
-		return ov2680_gain_set(sensor, !!ctrls->auto_gain->val);
-	case V4L2_CID_EXPOSURE_AUTO:
-		return ov2680_exposure_set(sensor, !!ctrl->val);
+		return ov2680_gain_set(sensor, ctrl->val);
 	case V4L2_CID_EXPOSURE:
-		return ov2680_exposure_set(sensor, !!ctrls->auto_exp->val);
+		return ov2680_exposure_set(sensor, ctrl->val);
 	case V4L2_CID_VFLIP:
 		if (sensor->is_streaming)
 			return -EBUSY;
@@ -864,7 +750,6 @@ static int ov2680_s_ctrl(struct v4l2_ctrl *ctrl)
 }
 
 static const struct v4l2_ctrl_ops ov2680_ctrl_ops = {
-	.g_volatile_ctrl = ov2680_g_volatile_ctrl,
 	.s_ctrl = ov2680_s_ctrl,
 };
 
@@ -936,7 +821,7 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor)
 	if (ret < 0)
 		return ret;
 
-	v4l2_ctrl_handler_init(hdl, 7);
+	v4l2_ctrl_handler_init(hdl, 5);
 
 	hdl->lock = &sensor->lock;
 
@@ -948,16 +833,9 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor)
 					ARRAY_SIZE(test_pattern_menu) - 1,
 					0, 0, test_pattern_menu);
 
-	ctrls->auto_exp = v4l2_ctrl_new_std_menu(hdl, ops,
-						 V4L2_CID_EXPOSURE_AUTO,
-						 V4L2_EXPOSURE_MANUAL, 0,
-						 V4L2_EXPOSURE_AUTO);
-
 	ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE,
 					    0, 32767, 1, 0);
 
-	ctrls->auto_gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_AUTOGAIN,
-					     0, 1, 1, 1);
 	ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN, 0, 2047, 1, 0);
 
 	if (hdl->error) {
@@ -965,14 +843,9 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor)
 		goto cleanup_entity;
 	}
 
-	ctrls->gain->flags |= V4L2_CTRL_FLAG_VOLATILE;
-	ctrls->exposure->flags |= V4L2_CTRL_FLAG_VOLATILE;
 	ctrls->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
 	ctrls->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
 
-	v4l2_ctrl_auto_cluster(2, &ctrls->auto_gain, 0, true);
-	v4l2_ctrl_auto_cluster(2, &ctrls->auto_exp, 1, true);
-
 	sensor->sd.ctrl_handler = hdl;
 
 	ret = v4l2_async_register_subdev(&sensor->sd);
-- 
2.41.0

[PATCH v5 02/32] media: ov2680: Fix ov2680_bayer_order()

[Hans de Goede]

The index into ov2680_hv_flip_bayer_order[] should be 0-3, but
ov2680_bayer_order() was using 0 + BIT(2) + (BIT(2) << 1) as
max index, while the intention was to use: 0 + 1 + 2 as max index.

Fix the index calculation in ov2680_bayer_order(), while at it
also just use the ctrl values rather then reading them back using
a slow i2c-read transaction.

This also allows making the function void, since there now are
no more i2c-reads to error check.

Note the check for the ctrls being NULL is there to allow
adding an ov2680_fill_format() helper later, which will call
ov2680_set_bayer_order() during probe() before the ctrls are created.

Fixes: 3ee47cad3e69 ("media: ov2680: Add Omnivision OV2680 sensor driver")
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/ov2680.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index 3a737a1607a4..621144f16fdc 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -315,26 +315,17 @@ static void ov2680_power_down(struct ov2680_dev *sensor)
 	usleep_range(5000, 10000);
 }
 
-static int ov2680_bayer_order(struct ov2680_dev *sensor)
+static void ov2680_set_bayer_order(struct ov2680_dev *sensor)
 {
-	u32 format1;
-	u32 format2;
-	u32 hv_flip;
-	int ret;
+	int hv_flip = 0;
 
-	ret = ov2680_read_reg(sensor, OV2680_REG_FORMAT1, &format1);
-	if (ret < 0)
-		return ret;
+	if (sensor->ctrls.vflip && sensor->ctrls.vflip->val)
+		hv_flip += 1;
 
-	ret = ov2680_read_reg(sensor, OV2680_REG_FORMAT2, &format2);
-	if (ret < 0)
-		return ret;
-
-	hv_flip = (format2 & BIT(2)  << 1) | (format1 & BIT(2));
+	if (sensor->ctrls.hflip && sensor->ctrls.hflip->val)
+		hv_flip += 2;
 
 	sensor->fmt.code = ov2680_hv_flip_bayer_order[hv_flip];
-
-	return 0;
 }
 
 static int ov2680_vflip_enable(struct ov2680_dev *sensor)
@@ -345,7 +336,8 @@ static int ov2680_vflip_enable(struct ov2680_dev *sensor)
 	if (ret < 0)
 		return ret;
 
-	return ov2680_bayer_order(sensor);
+	ov2680_set_bayer_order(sensor);
+	return 0;
 }
 
 static int ov2680_vflip_disable(struct ov2680_dev *sensor)
@@ -378,7 +370,8 @@ static int ov2680_hflip_disable(struct ov2680_dev *sensor)
 	if (ret < 0)
 		return ret;
 
-	return ov2680_bayer_order(sensor);
+	ov2680_set_bayer_order(sensor);
+	return 0;
 }
 
 static int ov2680_test_pattern_set(struct ov2680_dev *sensor, int value)
-- 
2.41.0

[PATCH v5 03/32] media: ov2680: Fix vflip / hflip set functions

[Hans de Goede]

ov2680_vflip_disable() / ov2680_hflip_disable() pass BIT(0) instead of
0 as value to ov2680_mod_reg().

While fixing this also:

1. Stop having separate enable/disable functions for hflip / vflip
2. Move the is_streaming check, which is unique to hflip / vflip
   into the ov2680_set_?flip() functions.

for a nice code cleanup.

Fixes: 3ee47cad3e69 ("media: ov2680: Add Omnivision OV2680 sensor driver")
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/ov2680.c | 50 ++++++++++----------------------------
 1 file changed, 13 insertions(+), 37 deletions(-)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index 621144f16fdc..74024ba968b4 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -328,11 +328,15 @@ static void ov2680_set_bayer_order(struct ov2680_dev *sensor)
 	sensor->fmt.code = ov2680_hv_flip_bayer_order[hv_flip];
 }
 
-static int ov2680_vflip_enable(struct ov2680_dev *sensor)
+static int ov2680_set_vflip(struct ov2680_dev *sensor, s32 val)
 {
 	int ret;
 
-	ret = ov2680_mod_reg(sensor, OV2680_REG_FORMAT1, BIT(2), BIT(2));
+	if (sensor->is_streaming)
+		return -EBUSY;
+
+	ret = ov2680_mod_reg(sensor, OV2680_REG_FORMAT1,
+			     BIT(2), val ? BIT(2) : 0);
 	if (ret < 0)
 		return ret;
 
@@ -340,33 +344,15 @@ static int ov2680_vflip_enable(struct ov2680_dev *sensor)
 	return 0;
 }
 
-static int ov2680_vflip_disable(struct ov2680_dev *sensor)
+static int ov2680_set_hflip(struct ov2680_dev *sensor, s32 val)
 {
 	int ret;
 
-	ret = ov2680_mod_reg(sensor, OV2680_REG_FORMAT1, BIT(2), BIT(0));
-	if (ret < 0)
-		return ret;
+	if (sensor->is_streaming)
+		return -EBUSY;
 
-	return ov2680_bayer_order(sensor);
-}
-
-static int ov2680_hflip_enable(struct ov2680_dev *sensor)
-{
-	int ret;
-
-	ret = ov2680_mod_reg(sensor, OV2680_REG_FORMAT2, BIT(2), BIT(2));
-	if (ret < 0)
-		return ret;
-
-	return ov2680_bayer_order(sensor);
-}
-
-static int ov2680_hflip_disable(struct ov2680_dev *sensor)
-{
-	int ret;
-
-	ret = ov2680_mod_reg(sensor, OV2680_REG_FORMAT2, BIT(2), BIT(0));
+	ret = ov2680_mod_reg(sensor, OV2680_REG_FORMAT2,
+			     BIT(2), val ? BIT(2) : 0);
 	if (ret < 0)
 		return ret;
 
@@ -720,19 +706,9 @@ static int ov2680_s_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_EXPOSURE:
 		return ov2680_exposure_set(sensor, ctrl->val);
 	case V4L2_CID_VFLIP:
-		if (sensor->is_streaming)
-			return -EBUSY;
-		if (ctrl->val)
-			return ov2680_vflip_enable(sensor);
-		else
-			return ov2680_vflip_disable(sensor);
+		return ov2680_set_vflip(sensor, ctrl->val);
 	case V4L2_CID_HFLIP:
-		if (sensor->is_streaming)
-			return -EBUSY;
-		if (ctrl->val)
-			return ov2680_hflip_enable(sensor);
-		else
-			return ov2680_hflip_disable(sensor);
+		return ov2680_set_hflip(sensor, ctrl->val);
 	case V4L2_CID_TEST_PATTERN:
 		return ov2680_test_pattern_set(sensor, ctrl->val);
 	default:
-- 
2.41.0

[PATCH v5 04/32] media: ov2680: Remove VIDEO_V4L2_SUBDEV_API ifdef-s

[Hans de Goede]

VIDEO_V4L2_SUBDEV_API is now automatically selected in Kconfig
for all sensor drivers. Remove the ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
checks.

This is a preparation patch for fixing ov2680_set_fmt()
which == V4L2_SUBDEV_FORMAT_TRY calls not properly filling in
the passed in v4l2_mbus_framefmt struct.

Fixes: 3ee47cad3e69 ("media: ov2680: Add Omnivision OV2680 sensor driver")
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/ov2680.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index 74024ba968b4..5c1f5dd4824a 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -562,7 +562,6 @@ static int ov2680_get_fmt(struct v4l2_subdev *sd,
 {
 	struct ov2680_dev *sensor = to_ov2680_dev(sd);
 	struct v4l2_mbus_framefmt *fmt = NULL;
-	int ret = 0;
 
 	if (format->pad != 0)
 		return -EINVAL;
@@ -570,22 +569,17 @@ static int ov2680_get_fmt(struct v4l2_subdev *sd,
 	mutex_lock(&sensor->lock);
 
 	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
-#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
 		fmt = v4l2_subdev_get_try_format(&sensor->sd, sd_state,
 						 format->pad);
-#else
-		ret = -EINVAL;
-#endif
 	} else {
 		fmt = &sensor->fmt;
 	}
 
-	if (fmt)
-		format->format = *fmt;
+	format->format = *fmt;
 
 	mutex_unlock(&sensor->lock);
 
-	return ret;
+	return 0;
 }
 
 static int ov2680_set_fmt(struct v4l2_subdev *sd,
@@ -594,9 +588,7 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
 {
 	struct ov2680_dev *sensor = to_ov2680_dev(sd);
 	struct v4l2_mbus_framefmt *fmt = &format->format;
-#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
 	struct v4l2_mbus_framefmt *try_fmt;
-#endif
 	const struct ov2680_mode_info *mode;
 	int ret = 0;
 
@@ -619,10 +611,8 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
 	}
 
 	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
-#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
 		try_fmt = v4l2_subdev_get_try_format(sd, sd_state, 0);
 		format->format = *try_fmt;
-#endif
 		goto unlock;
 	}
 
@@ -780,9 +770,7 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor)
 	v4l2_i2c_subdev_init(&sensor->sd, sensor->i2c_client,
 			     &ov2680_subdev_ops);
 
-#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
 	sensor->sd.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
-#endif
 	sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
 	sensor->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
 
-- 
2.41.0

[PATCH v5 05/32] media: ov2680: Don't take the lock for try_fmt calls

[Hans de Goede]

On ov2680_set_fmt() calls with format->which == V4L2_SUBDEV_FORMAT_TRY,
ov2680_set_fmt() does not talk to the sensor.

So in this case there is no need to lock the sensor->lock mutex or
to check that the sensor is streaming.

Fixes: 3ee47cad3e69 ("media: ov2680: Add Omnivision OV2680 sensor driver")
Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/ov2680.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index 5c1f5dd4824a..e6e14743ba1e 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -595,24 +595,22 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
 	if (format->pad != 0)
 		return -EINVAL;
 
-	mutex_lock(&sensor->lock);
-
-	if (sensor->is_streaming) {
-		ret = -EBUSY;
-		goto unlock;
-	}
-
 	mode = v4l2_find_nearest_size(ov2680_mode_data,
 				      ARRAY_SIZE(ov2680_mode_data), width,
 				      height, fmt->width, fmt->height);
-	if (!mode) {
-		ret = -EINVAL;
-		goto unlock;
-	}
+	if (!mode)
+		return -EINVAL;
 
 	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
 		try_fmt = v4l2_subdev_get_try_format(sd, sd_state, 0);
 		format->format = *try_fmt;
+		return 0;
+	}
+
+	mutex_lock(&sensor->lock);
+
+	if (sensor->is_streaming) {
+		ret = -EBUSY;
 		goto unlock;
 	}
 
-- 
2.41.0

[PATCH v5 06/32] media: ov2680: Add ov2680_fill_format() helper function

[Hans de Goede]

Add a ov2680_fill_format() helper function and use this everywhere were
a v4l2_mbus_framefmt struct needs to be filled in so that the driver always
fills it consistently.

This is a preparation patch for fixing ov2680_set_fmt()
which == V4L2_SUBDEV_FORMAT_TRY calls not properly filling in
the passed in v4l2_mbus_framefmt struct.

Note that for ov2680_init_cfg() this now simply always fills
the try_fmt struct of the passed in sd_state. This is correct because
ov2680_init_cfg() is never called with a NULL sd_state so the old
sd_state check is not necessary.

Fixes: 3ee47cad3e69 ("media: ov2680: Add Omnivision OV2680 sensor driver")
Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/ov2680.c | 49 +++++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 22 deletions(-)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index e6e14743ba1e..f2eb8d85a7e4 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -54,6 +54,9 @@
 #define OV2680_WIDTH_MAX		1600
 #define OV2680_HEIGHT_MAX		1200
 
+#define OV2680_DEFAULT_WIDTH			800
+#define OV2680_DEFAULT_HEIGHT			600
+
 enum ov2680_mode_id {
 	OV2680_MODE_QUXGA_800_600,
 	OV2680_MODE_720P_1280_720,
@@ -315,7 +318,8 @@ static void ov2680_power_down(struct ov2680_dev *sensor)
 	usleep_range(5000, 10000);
 }
 
-static void ov2680_set_bayer_order(struct ov2680_dev *sensor)
+static void ov2680_set_bayer_order(struct ov2680_dev *sensor,
+				   struct v4l2_mbus_framefmt *fmt)
 {
 	int hv_flip = 0;
 
@@ -325,7 +329,19 @@ static void ov2680_set_bayer_order(struct ov2680_dev *sensor)
 	if (sensor->ctrls.hflip && sensor->ctrls.hflip->val)
 		hv_flip += 2;
 
-	sensor->fmt.code = ov2680_hv_flip_bayer_order[hv_flip];
+	fmt->code = ov2680_hv_flip_bayer_order[hv_flip];
+}
+
+static void ov2680_fill_format(struct ov2680_dev *sensor,
+			       struct v4l2_mbus_framefmt *fmt,
+			       unsigned int width, unsigned int height)
+{
+	memset(fmt, 0, sizeof(*fmt));
+	fmt->width = width;
+	fmt->height = height;
+	fmt->field = V4L2_FIELD_NONE;
+	fmt->colorspace = V4L2_COLORSPACE_SRGB;
+	ov2680_set_bayer_order(sensor, fmt);
 }
 
 static int ov2680_set_vflip(struct ov2680_dev *sensor, s32 val)
@@ -340,7 +356,7 @@ static int ov2680_set_vflip(struct ov2680_dev *sensor, s32 val)
 	if (ret < 0)
 		return ret;
 
-	ov2680_set_bayer_order(sensor);
+	ov2680_set_bayer_order(sensor, &sensor->fmt);
 	return 0;
 }
 
@@ -356,7 +372,7 @@ static int ov2680_set_hflip(struct ov2680_dev *sensor, s32 val)
 	if (ret < 0)
 		return ret;
 
-	ov2680_set_bayer_order(sensor);
+	ov2680_set_bayer_order(sensor, &sensor->fmt);
 	return 0;
 }
 
@@ -614,10 +630,7 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
 		goto unlock;
 	}
 
-	fmt->width = mode->width;
-	fmt->height = mode->height;
-	fmt->code = sensor->fmt.code;
-	fmt->colorspace = sensor->fmt.colorspace;
+	ov2680_fill_format(sensor, fmt, mode->width, mode->height);
 
 	sensor->current_mode = mode;
 	sensor->fmt = format->format;
@@ -632,16 +645,11 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
 static int ov2680_init_cfg(struct v4l2_subdev *sd,
 			   struct v4l2_subdev_state *sd_state)
 {
-	struct v4l2_subdev_format fmt = {
-		.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY
-		: V4L2_SUBDEV_FORMAT_ACTIVE,
-		.format = {
-			.width = 800,
-			.height = 600,
-		}
-	};
+	struct ov2680_dev *sensor = to_ov2680_dev(sd);
 
-	return ov2680_set_fmt(sd, sd_state, &fmt);
+	ov2680_fill_format(sensor, &sd_state->pads[0].try_fmt,
+			   OV2680_DEFAULT_WIDTH, OV2680_DEFAULT_HEIGHT);
+	return 0;
 }
 
 static int ov2680_enum_frame_size(struct v4l2_subdev *sd,
@@ -740,11 +748,8 @@ static int ov2680_mode_init(struct ov2680_dev *sensor)
 	const struct ov2680_mode_info *init_mode;
 
 	/* set initial mode */
-	sensor->fmt.code = MEDIA_BUS_FMT_SBGGR10_1X10;
-	sensor->fmt.width = 800;
-	sensor->fmt.height = 600;
-	sensor->fmt.field = V4L2_FIELD_NONE;
-	sensor->fmt.colorspace = V4L2_COLORSPACE_SRGB;
+	ov2680_fill_format(sensor, &sensor->fmt,
+			   OV2680_DEFAULT_WIDTH, OV2680_DEFAULT_HEIGHT);
 
 	sensor->frame_interval.denominator = OV2680_FRAME_RATE;
 	sensor->frame_interval.numerator = 1;
-- 
2.41.0

[PATCH v5 07/32] media: ov2680: Fix ov2680_set_fmt() which == V4L2_SUBDEV_FORMAT_TRY not working

[Hans de Goede]

ov2680_set_fmt() which == V4L2_SUBDEV_FORMAT_TRY was getting
the try_fmt v4l2_mbus_framefmt struct from the passed in sd_state
and then storing the contents of that into the return by reference
format->format struct.

While the right thing to do would be filling format->format based on
the just looked up mode and then store the results of that in
sd_state->pads[0].try_fmt .

Before the previous change introducing ov2680_fill_format() this
resulted in ov2680_set_fmt() which == V4L2_SUBDEV_FORMAT_TRY always
returning the zero-ed out sd_state->pads[0].try_fmt in format->format
breaking callers using this.

After the introduction of ov2680_fill_format() which at least
initializes sd_state->pads[0].try_fmt properly, format->format
is now always being filled with the default 800x600 mode set by
ov2680_init_cfg() independent of the actual requested mode.

Move the filling of format->format with ov2680_fill_format() to
before the if (which == V4L2_SUBDEV_FORMAT_TRY) and then store
the filled in format->format in sd_state->pads[0].try_fmt to
fix this.

Note this removes the fmt local variable because IMHO having a local
variable which points to a sub-struct of one of the function arguments
just leads to confusion when reading the code.

Fixes: 3ee47cad3e69 ("media: ov2680: Add Omnivision OV2680 sensor driver")
Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/ov2680.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index f2eb8d85a7e4..e3652b5394c4 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -603,7 +603,6 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
 			  struct v4l2_subdev_format *format)
 {
 	struct ov2680_dev *sensor = to_ov2680_dev(sd);
-	struct v4l2_mbus_framefmt *fmt = &format->format;
 	struct v4l2_mbus_framefmt *try_fmt;
 	const struct ov2680_mode_info *mode;
 	int ret = 0;
@@ -612,14 +611,18 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
 		return -EINVAL;
 
 	mode = v4l2_find_nearest_size(ov2680_mode_data,
-				      ARRAY_SIZE(ov2680_mode_data), width,
-				      height, fmt->width, fmt->height);
+				      ARRAY_SIZE(ov2680_mode_data),
+				      width, height,
+				      format->format.width,
+				      format->format.height);
 	if (!mode)
 		return -EINVAL;
 
+	ov2680_fill_format(sensor, &format->format, mode->width, mode->height);
+
 	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
 		try_fmt = v4l2_subdev_get_try_format(sd, sd_state, 0);
-		format->format = *try_fmt;
+		*try_fmt = format->format;
 		return 0;
 	}
 
@@ -630,8 +633,6 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
 		goto unlock;
 	}
 
-	ov2680_fill_format(sensor, fmt, mode->width, mode->height);
-
 	sensor->current_mode = mode;
 	sensor->fmt = format->format;
 	sensor->mode_pending_changes = true;
-- 
2.41.0

[PATCH v5 08/32] media: ov2680: Fix regulators being left enabled on ov2680_power_on() errors

[Hans de Goede]

When the ov2680_power_on() "sensor soft reset failed" path is hit during
probe() the WARN() about putting an enabled regulator at
drivers/regulator/core.c:2398 triggers 3 times (once for each regulator),
filling dmesg with backtraces.

Fix this by properly disabling the regulators on ov2680_power_on() errors.

Fixes: 3ee47cad3e69 ("media: ov2680: Add Omnivision OV2680 sensor driver")
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/ov2680.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index e3652b5394c4..1f923acbbc07 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -475,7 +475,7 @@ static int ov2680_power_on(struct ov2680_dev *sensor)
 		ret = ov2680_write_reg(sensor, OV2680_REG_SOFT_RESET, 0x01);
 		if (ret != 0) {
 			dev_err(dev, "sensor soft reset failed\n");
-			return ret;
+			goto err_disable_regulators;
 		}
 		usleep_range(1000, 2000);
 	} else {
@@ -485,7 +485,7 @@ static int ov2680_power_on(struct ov2680_dev *sensor)
 
 	ret = clk_prepare_enable(sensor->xvclk);
 	if (ret < 0)
-		return ret;
+		goto err_disable_regulators;
 
 	sensor->is_enabled = true;
 
@@ -495,6 +495,10 @@ static int ov2680_power_on(struct ov2680_dev *sensor)
 	ov2680_stream_disable(sensor);
 
 	return 0;
+
+err_disable_regulators:
+	regulator_bulk_disable(OV2680_NUM_SUPPLIES, sensor->supplies);
+	return ret;
 }
 
 static int ov2680_s_power(struct v4l2_subdev *sd, int on)
-- 
2.41.0

[PATCH v5 09/32] media: ov2680: Convert to new CCI register access helpers

[Hans de Goede]

Use the new comon CCI register access helpers to replace the private
register access helpers in the ov2680 driver.

Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/Kconfig  |   1 +
 drivers/media/i2c/ov2680.c | 224 ++++++++++---------------------------
 2 files changed, 58 insertions(+), 167 deletions(-)

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index aa55582a2cd0..74ff833ff48c 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -332,6 +332,7 @@ config VIDEO_OV2659
 
 config VIDEO_OV2680
 	tristate "OmniVision OV2680 sensor support"
+	select V4L2_CCI_I2C
 	help
 	  This is a Video4Linux2 sensor driver for the OmniVision
 	  OV2680 camera.
diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index 1f923acbbc07..02ac5b5e6583 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -10,49 +10,45 @@
  *
  */
 
-#include <asm/unaligned.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/err.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/init.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
-#include <linux/gpio/consumer.h>
+#include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 
+#include <media/v4l2-cci.h>
 #include <media/v4l2-common.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-subdev.h>
 
-#define OV2680_XVCLK_VALUE	24000000
+#define OV2680_XVCLK_VALUE			24000000
 
-#define OV2680_CHIP_ID		0x2680
+#define OV2680_CHIP_ID				0x2680
 
-#define OV2680_REG_STREAM_CTRL		0x0100
-#define OV2680_REG_SOFT_RESET		0x0103
+#define OV2680_REG_STREAM_CTRL			CCI_REG8(0x0100)
+#define OV2680_REG_SOFT_RESET			CCI_REG8(0x0103)
 
-#define OV2680_REG_CHIP_ID_HIGH		0x300a
-#define OV2680_REG_CHIP_ID_LOW		0x300b
+#define OV2680_REG_CHIP_ID			CCI_REG16(0x300a)
 
-#define OV2680_REG_R_MANUAL		0x3503
-#define OV2680_REG_GAIN_PK		0x350a
-#define OV2680_REG_EXPOSURE_PK_HIGH	0x3500
-#define OV2680_REG_TIMING_HTS		0x380c
-#define OV2680_REG_TIMING_VTS		0x380e
-#define OV2680_REG_FORMAT1		0x3820
-#define OV2680_REG_FORMAT2		0x3821
+#define OV2680_REG_EXPOSURE_PK			CCI_REG24(0x3500)
+#define OV2680_REG_R_MANUAL			CCI_REG8(0x3503)
+#define OV2680_REG_GAIN_PK			CCI_REG16(0x350a)
+#define OV2680_REG_TIMING_HTS			CCI_REG16(0x380c)
+#define OV2680_REG_TIMING_VTS			CCI_REG16(0x380e)
+#define OV2680_REG_FORMAT1			CCI_REG8(0x3820)
+#define OV2680_REG_FORMAT2			CCI_REG8(0x3821)
 
-#define OV2680_REG_ISP_CTRL00		0x5080
+#define OV2680_REG_ISP_CTRL00			CCI_REG8(0x5080)
 
-#define OV2680_FRAME_RATE		30
+#define OV2680_FRAME_RATE			30
 
-#define OV2680_REG_VALUE_8BIT		1
-#define OV2680_REG_VALUE_16BIT		2
-#define OV2680_REG_VALUE_24BIT		3
-
-#define OV2680_WIDTH_MAX		1600
-#define OV2680_HEIGHT_MAX		1200
+#define OV2680_WIDTH_MAX			1600
+#define OV2680_HEIGHT_MAX			1200
 
 #define OV2680_DEFAULT_WIDTH			800
 #define OV2680_DEFAULT_HEIGHT			600
@@ -64,11 +60,6 @@ enum ov2680_mode_id {
 	OV2680_MODE_MAX,
 };
 
-struct reg_value {
-	u16 reg_addr;
-	u8 val;
-};
-
 static const char * const ov2680_supply_name[] = {
 	"DOVDD",
 	"DVDD",
@@ -82,7 +73,7 @@ struct ov2680_mode_info {
 	enum ov2680_mode_id id;
 	u32 width;
 	u32 height;
-	const struct reg_value *reg_data;
+	const struct reg_sequence *reg_data;
 	u32 reg_data_size;
 };
 
@@ -97,6 +88,7 @@ struct ov2680_ctrls {
 
 struct ov2680_dev {
 	struct i2c_client		*i2c_client;
+	struct regmap			*regmap;
 	struct v4l2_subdev		sd;
 
 	struct media_pad		pad;
@@ -133,7 +125,7 @@ static const int ov2680_hv_flip_bayer_order[] = {
 	MEDIA_BUS_FMT_SRGGB10_1X10,
 };
 
-static const struct reg_value ov2680_setting_30fps_QUXGA_800_600[] = {
+static const struct reg_sequence ov2680_setting_30fps_QUXGA_800_600[] = {
 	{0x3086, 0x01}, {0x370a, 0x23}, {0x3808, 0x03}, {0x3809, 0x20},
 	{0x380a, 0x02}, {0x380b, 0x58}, {0x380c, 0x06}, {0x380d, 0xac},
 	{0x380e, 0x02}, {0x380f, 0x84}, {0x3811, 0x04}, {0x3813, 0x04},
@@ -142,14 +134,14 @@ static const struct reg_value ov2680_setting_30fps_QUXGA_800_600[] = {
 	{0x3503, 0x03},
 };
 
-static const struct reg_value ov2680_setting_30fps_720P_1280_720[] = {
+static const struct reg_sequence ov2680_setting_30fps_720P_1280_720[] = {
 	{0x3086, 0x00}, {0x3808, 0x05}, {0x3809, 0x00}, {0x380a, 0x02},
 	{0x380b, 0xd0}, {0x380c, 0x06}, {0x380d, 0xa8}, {0x380e, 0x05},
 	{0x380f, 0x0e}, {0x3811, 0x08}, {0x3813, 0x06}, {0x3814, 0x11},
 	{0x3815, 0x11}, {0x3820, 0xc0}, {0x4008, 0x00},
 };
 
-static const struct reg_value ov2680_setting_30fps_UXGA_1600_1200[] = {
+static const struct reg_sequence ov2680_setting_30fps_UXGA_1600_1200[] = {
 	{0x3086, 0x00}, {0x3501, 0x4e}, {0x3502, 0xe0}, {0x3808, 0x06},
 	{0x3809, 0x40}, {0x380a, 0x04}, {0x380b, 0xb0}, {0x380c, 0x06},
 	{0x380d, 0xa8}, {0x380e, 0x05}, {0x380f, 0x0e}, {0x3811, 0x00},
@@ -191,115 +183,6 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
 			     ctrls.handler)->sd;
 }
 
-static int __ov2680_write_reg(struct ov2680_dev *sensor, u16 reg,
-			      unsigned int len, u32 val)
-{
-	struct i2c_client *client = sensor->i2c_client;
-	u8 buf[6];
-	int ret;
-
-	if (len > 4)
-		return -EINVAL;
-
-	put_unaligned_be16(reg, buf);
-	put_unaligned_be32(val << (8 * (4 - len)), buf + 2);
-	ret = i2c_master_send(client, buf, len + 2);
-	if (ret != len + 2) {
-		dev_err(&client->dev, "write error: reg=0x%4x: %d\n", reg, ret);
-		return -EIO;
-	}
-
-	return 0;
-}
-
-#define ov2680_write_reg(s, r, v) \
-	__ov2680_write_reg(s, r, OV2680_REG_VALUE_8BIT, v)
-
-#define ov2680_write_reg16(s, r, v) \
-	__ov2680_write_reg(s, r, OV2680_REG_VALUE_16BIT, v)
-
-#define ov2680_write_reg24(s, r, v) \
-	__ov2680_write_reg(s, r, OV2680_REG_VALUE_24BIT, v)
-
-static int __ov2680_read_reg(struct ov2680_dev *sensor, u16 reg,
-			     unsigned int len, u32 *val)
-{
-	struct i2c_client *client = sensor->i2c_client;
-	struct i2c_msg msgs[2];
-	u8 addr_buf[2] = { reg >> 8, reg & 0xff };
-	u8 data_buf[4] = { 0, };
-	int ret;
-
-	if (len > 4)
-		return -EINVAL;
-
-	msgs[0].addr = client->addr;
-	msgs[0].flags = 0;
-	msgs[0].len = ARRAY_SIZE(addr_buf);
-	msgs[0].buf = addr_buf;
-
-	msgs[1].addr = client->addr;
-	msgs[1].flags = I2C_M_RD;
-	msgs[1].len = len;
-	msgs[1].buf = &data_buf[4 - len];
-
-	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
-	if (ret != ARRAY_SIZE(msgs)) {
-		dev_err(&client->dev, "read error: reg=0x%4x: %d\n", reg, ret);
-		return -EIO;
-	}
-
-	*val = get_unaligned_be32(data_buf);
-
-	return 0;
-}
-
-#define ov2680_read_reg(s, r, v) \
-	__ov2680_read_reg(s, r, OV2680_REG_VALUE_8BIT, v)
-
-#define ov2680_read_reg16(s, r, v) \
-	__ov2680_read_reg(s, r, OV2680_REG_VALUE_16BIT, v)
-
-#define ov2680_read_reg24(s, r, v) \
-	__ov2680_read_reg(s, r, OV2680_REG_VALUE_24BIT, v)
-
-static int ov2680_mod_reg(struct ov2680_dev *sensor, u16 reg, u8 mask, u8 val)
-{
-	u32 readval;
-	int ret;
-
-	ret = ov2680_read_reg(sensor, reg, &readval);
-	if (ret < 0)
-		return ret;
-
-	readval &= ~mask;
-	val &= mask;
-	val |= readval;
-
-	return ov2680_write_reg(sensor, reg, val);
-}
-
-static int ov2680_load_regs(struct ov2680_dev *sensor,
-			    const struct ov2680_mode_info *mode)
-{
-	const struct reg_value *regs = mode->reg_data;
-	unsigned int i;
-	int ret = 0;
-	u16 reg_addr;
-	u8 val;
-
-	for (i = 0; i < mode->reg_data_size; ++i, ++regs) {
-		reg_addr = regs->reg_addr;
-		val = regs->val;
-
-		ret = ov2680_write_reg(sensor, reg_addr, val);
-		if (ret)
-			break;
-	}
-
-	return ret;
-}
-
 static void ov2680_power_up(struct ov2680_dev *sensor)
 {
 	if (!sensor->reset_gpio)
@@ -351,8 +234,8 @@ static int ov2680_set_vflip(struct ov2680_dev *sensor, s32 val)
 	if (sensor->is_streaming)
 		return -EBUSY;
 
-	ret = ov2680_mod_reg(sensor, OV2680_REG_FORMAT1,
-			     BIT(2), val ? BIT(2) : 0);
+	ret = cci_update_bits(sensor->regmap, OV2680_REG_FORMAT1,
+			      BIT(2), val ? BIT(2) : 0, NULL);
 	if (ret < 0)
 		return ret;
 
@@ -367,8 +250,8 @@ static int ov2680_set_hflip(struct ov2680_dev *sensor, s32 val)
 	if (sensor->is_streaming)
 		return -EBUSY;
 
-	ret = ov2680_mod_reg(sensor, OV2680_REG_FORMAT2,
-			     BIT(2), val ? BIT(2) : 0);
+	ret = cci_update_bits(sensor->regmap, OV2680_REG_FORMAT2,
+			      BIT(2), val ? BIT(2) : 0, NULL);
 	if (ret < 0)
 		return ret;
 
@@ -378,48 +261,48 @@ static int ov2680_set_hflip(struct ov2680_dev *sensor, s32 val)
 
 static int ov2680_test_pattern_set(struct ov2680_dev *sensor, int value)
 {
-	int ret;
+	int ret = 0;
 
 	if (!value)
-		return ov2680_mod_reg(sensor, OV2680_REG_ISP_CTRL00, BIT(7), 0);
+		return cci_update_bits(sensor->regmap, OV2680_REG_ISP_CTRL00,
+				       BIT(7), 0, NULL);
 
-	ret = ov2680_mod_reg(sensor, OV2680_REG_ISP_CTRL00, 0x03, value - 1);
-	if (ret < 0)
-		return ret;
+	cci_update_bits(sensor->regmap, OV2680_REG_ISP_CTRL00,
+			0x03, value - 1, &ret);
+	cci_update_bits(sensor->regmap, OV2680_REG_ISP_CTRL00,
+			BIT(7), BIT(7), &ret);
 
-	ret = ov2680_mod_reg(sensor, OV2680_REG_ISP_CTRL00, BIT(7), BIT(7));
-	if (ret < 0)
-		return ret;
-
-	return 0;
+	return ret;
 }
 
 static int ov2680_gain_set(struct ov2680_dev *sensor, u32 gain)
 {
-	return ov2680_write_reg16(sensor, OV2680_REG_GAIN_PK, gain);
+	return cci_write(sensor->regmap, OV2680_REG_GAIN_PK, gain, NULL);
 }
 
 static int ov2680_exposure_set(struct ov2680_dev *sensor, u32 exp)
 {
-	return ov2680_write_reg24(sensor, OV2680_REG_EXPOSURE_PK_HIGH,
-				  exp << 4);
+	return cci_write(sensor->regmap, OV2680_REG_EXPOSURE_PK, exp << 4,
+			 NULL);
 }
 
 static int ov2680_stream_enable(struct ov2680_dev *sensor)
 {
-	return ov2680_write_reg(sensor, OV2680_REG_STREAM_CTRL, 1);
+	return cci_write(sensor->regmap, OV2680_REG_STREAM_CTRL, 1, NULL);
 }
 
 static int ov2680_stream_disable(struct ov2680_dev *sensor)
 {
-	return ov2680_write_reg(sensor, OV2680_REG_STREAM_CTRL, 0);
+	return cci_write(sensor->regmap, OV2680_REG_STREAM_CTRL, 0, NULL);
 }
 
 static int ov2680_mode_set(struct ov2680_dev *sensor)
 {
 	int ret;
 
-	ret = ov2680_load_regs(sensor, sensor->current_mode);
+	ret = regmap_multi_reg_write(sensor->regmap,
+				     sensor->current_mode->reg_data,
+				     sensor->current_mode->reg_data_size);
 	if (ret < 0)
 		return ret;
 
@@ -437,7 +320,9 @@ static int ov2680_mode_restore(struct ov2680_dev *sensor)
 {
 	int ret;
 
-	ret = ov2680_load_regs(sensor, &ov2680_mode_init_data);
+	ret = regmap_multi_reg_write(sensor->regmap,
+				     ov2680_mode_init_data.reg_data,
+				     ov2680_mode_init_data.reg_data_size);
 	if (ret < 0)
 		return ret;
 
@@ -472,7 +357,8 @@ static int ov2680_power_on(struct ov2680_dev *sensor)
 	}
 
 	if (!sensor->reset_gpio) {
-		ret = ov2680_write_reg(sensor, OV2680_REG_SOFT_RESET, 0x01);
+		ret = cci_write(sensor->regmap, OV2680_REG_SOFT_RESET, 0x01,
+				NULL);
 		if (ret != 0) {
 			dev_err(dev, "sensor soft reset failed\n");
 			goto err_disable_regulators;
@@ -841,19 +727,19 @@ static int ov2680_get_regulators(struct ov2680_dev *sensor)
 static int ov2680_check_id(struct ov2680_dev *sensor)
 {
 	struct device *dev = ov2680_to_dev(sensor);
-	u32 chip_id;
+	u64 chip_id;
 	int ret;
 
 	ov2680_power_on(sensor);
 
-	ret = ov2680_read_reg16(sensor, OV2680_REG_CHIP_ID_HIGH, &chip_id);
+	ret = cci_read(sensor->regmap, OV2680_REG_CHIP_ID, &chip_id, NULL);
 	if (ret < 0) {
-		dev_err(dev, "failed to read chip id high\n");
+		dev_err(dev, "failed to read chip id\n");
 		return -ENODEV;
 	}
 
 	if (chip_id != OV2680_CHIP_ID) {
-		dev_err(dev, "chip id: 0x%04x does not match expected 0x%04x\n",
+		dev_err(dev, "chip id: 0x%04llx does not match expected 0x%04x\n",
 			chip_id, OV2680_CHIP_ID);
 		return -ENODEV;
 	}
@@ -902,6 +788,10 @@ static int ov2680_probe(struct i2c_client *client)
 
 	sensor->i2c_client = client;
 
+	sensor->regmap = devm_cci_regmap_init_i2c(client, 16);
+	if (IS_ERR(sensor->regmap))
+		return PTR_ERR(sensor->regmap);
+
 	ret = ov2680_parse_dt(sensor);
 	if (ret < 0)
 		return -EINVAL;
-- 
2.41.0

[PATCH v5 10/32] media: ov2680: Store dev instead of i2c_client in ov2680_dev

[Hans de Goede]

Now that the cci_* register access helpers are used access to
the i2c_client after probe() is no longer necessary.

Directly store a struct device *dev pointing to &client->dev inside
ov2680_dev to make the code simpler.

Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Use to_i2c_client(sensor->dev) in ov2680_v4l2_register()
---
 drivers/media/i2c/ov2680.c | 30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index 02ac5b5e6583..9c5f4ac592d8 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -87,7 +87,7 @@ struct ov2680_ctrls {
 };
 
 struct ov2680_dev {
-	struct i2c_client		*i2c_client;
+	struct device			*dev;
 	struct regmap			*regmap;
 	struct v4l2_subdev		sd;
 
@@ -172,11 +172,6 @@ static struct ov2680_dev *to_ov2680_dev(struct v4l2_subdev *sd)
 	return container_of(sd, struct ov2680_dev, sd);
 }
 
-static struct device *ov2680_to_dev(struct ov2680_dev *sensor)
-{
-	return &sensor->i2c_client->dev;
-}
-
 static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
 {
 	return &container_of(ctrl->handler, struct ov2680_dev,
@@ -344,7 +339,6 @@ static int ov2680_power_off(struct ov2680_dev *sensor)
 
 static int ov2680_power_on(struct ov2680_dev *sensor)
 {
-	struct device *dev = ov2680_to_dev(sensor);
 	int ret;
 
 	if (sensor->is_enabled)
@@ -352,7 +346,7 @@ static int ov2680_power_on(struct ov2680_dev *sensor)
 
 	ret = regulator_bulk_enable(OV2680_NUM_SUPPLIES, sensor->supplies);
 	if (ret < 0) {
-		dev_err(dev, "failed to enable regulators: %d\n", ret);
+		dev_err(sensor->dev, "failed to enable regulators: %d\n", ret);
 		return ret;
 	}
 
@@ -360,7 +354,7 @@ static int ov2680_power_on(struct ov2680_dev *sensor)
 		ret = cci_write(sensor->regmap, OV2680_REG_SOFT_RESET, 0x01,
 				NULL);
 		if (ret != 0) {
-			dev_err(dev, "sensor soft reset failed\n");
+			dev_err(sensor->dev, "sensor soft reset failed\n");
 			goto err_disable_regulators;
 		}
 		usleep_range(1000, 2000);
@@ -656,13 +650,13 @@ static int ov2680_mode_init(struct ov2680_dev *sensor)
 
 static int ov2680_v4l2_register(struct ov2680_dev *sensor)
 {
+	struct i2c_client *client = to_i2c_client(sensor->dev);
 	const struct v4l2_ctrl_ops *ops = &ov2680_ctrl_ops;
 	struct ov2680_ctrls *ctrls = &sensor->ctrls;
 	struct v4l2_ctrl_handler *hdl = &ctrls->handler;
 	int ret = 0;
 
-	v4l2_i2c_subdev_init(&sensor->sd, sensor->i2c_client,
-			     &ov2680_subdev_ops);
+	v4l2_i2c_subdev_init(&sensor->sd, client, &ov2680_subdev_ops);
 
 	sensor->sd.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
 	sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
@@ -719,14 +713,12 @@ static int ov2680_get_regulators(struct ov2680_dev *sensor)
 	for (i = 0; i < OV2680_NUM_SUPPLIES; i++)
 		sensor->supplies[i].supply = ov2680_supply_name[i];
 
-	return devm_regulator_bulk_get(&sensor->i2c_client->dev,
-				       OV2680_NUM_SUPPLIES,
-				       sensor->supplies);
+	return devm_regulator_bulk_get(sensor->dev,
+				       OV2680_NUM_SUPPLIES, sensor->supplies);
 }
 
 static int ov2680_check_id(struct ov2680_dev *sensor)
 {
-	struct device *dev = ov2680_to_dev(sensor);
 	u64 chip_id;
 	int ret;
 
@@ -734,12 +726,12 @@ static int ov2680_check_id(struct ov2680_dev *sensor)
 
 	ret = cci_read(sensor->regmap, OV2680_REG_CHIP_ID, &chip_id, NULL);
 	if (ret < 0) {
-		dev_err(dev, "failed to read chip id\n");
+		dev_err(sensor->dev, "failed to read chip id\n");
 		return -ENODEV;
 	}
 
 	if (chip_id != OV2680_CHIP_ID) {
-		dev_err(dev, "chip id: 0x%04llx does not match expected 0x%04x\n",
+		dev_err(sensor->dev, "chip id: 0x%04llx does not match expected 0x%04x\n",
 			chip_id, OV2680_CHIP_ID);
 		return -ENODEV;
 	}
@@ -749,7 +741,7 @@ static int ov2680_check_id(struct ov2680_dev *sensor)
 
 static int ov2680_parse_dt(struct ov2680_dev *sensor)
 {
-	struct device *dev = ov2680_to_dev(sensor);
+	struct device *dev = sensor->dev;
 	int ret;
 
 	sensor->reset_gpio = devm_gpiod_get_optional(dev, "reset",
@@ -786,7 +778,7 @@ static int ov2680_probe(struct i2c_client *client)
 	if (!sensor)
 		return -ENOMEM;
 
-	sensor->i2c_client = client;
+	sensor->dev = &client->dev;
 
 	sensor->regmap = devm_cci_regmap_init_i2c(client, 16);
 	if (IS_ERR(sensor->regmap))
-- 
2.41.0

[PATCH v5 11/32] media: ov2680: Add runtime-pm support

[Hans de Goede]

Remove the obsolete s_power() callback and instead use runtime-pm +
autosuspend, powering-on the sensor on s_stream(1) and releasing
the runtime-pm reference on s_stream(0).

This also removes the need for ov2680_mode_restore() instead
ov2680_stream_enable() now takes care of all sensor initalization
after power-on.

This is a preparation patch for adding ACPI support.

Note this also removes putting the clock lane into LP-11 state from
ov2680_power_on() since now streaming will start immediately after
powering on the sensor there is no need to put the clock lane
in a low power state.

Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/ov2680.c | 134 +++++++++++++++++--------------------
 1 file changed, 61 insertions(+), 73 deletions(-)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index 9c5f4ac592d8..bc0ca2927370 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -18,6 +18,7 @@
 #include <linux/init.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 
@@ -99,7 +100,6 @@ struct ov2680_dev {
 	struct gpio_desc		*reset_gpio;
 	struct mutex			lock; /* protect members */
 
-	bool				mode_pending_changes;
 	bool				is_enabled;
 	bool				is_streaming;
 
@@ -282,19 +282,15 @@ static int ov2680_exposure_set(struct ov2680_dev *sensor, u32 exp)
 }
 
 static int ov2680_stream_enable(struct ov2680_dev *sensor)
-{
-	return cci_write(sensor->regmap, OV2680_REG_STREAM_CTRL, 1, NULL);
-}
-
-static int ov2680_stream_disable(struct ov2680_dev *sensor)
-{
-	return cci_write(sensor->regmap, OV2680_REG_STREAM_CTRL, 0, NULL);
-}
-
-static int ov2680_mode_set(struct ov2680_dev *sensor)
 {
 	int ret;
 
+	ret = regmap_multi_reg_write(sensor->regmap,
+				     ov2680_mode_init_data.reg_data,
+				     ov2680_mode_init_data.reg_data_size);
+	if (ret < 0)
+		return ret;
+
 	ret = regmap_multi_reg_write(sensor->regmap,
 				     sensor->current_mode->reg_data,
 				     sensor->current_mode->reg_data_size);
@@ -306,22 +302,12 @@ static int ov2680_mode_set(struct ov2680_dev *sensor)
 	if (ret < 0)
 		return ret;
 
-	sensor->mode_pending_changes = false;
-
-	return 0;
+	return cci_write(sensor->regmap, OV2680_REG_STREAM_CTRL, 1, NULL);
 }
 
-static int ov2680_mode_restore(struct ov2680_dev *sensor)
+static int ov2680_stream_disable(struct ov2680_dev *sensor)
 {
-	int ret;
-
-	ret = regmap_multi_reg_write(sensor->regmap,
-				     ov2680_mode_init_data.reg_data,
-				     ov2680_mode_init_data.reg_data_size);
-	if (ret < 0)
-		return ret;
-
-	return ov2680_mode_set(sensor);
+	return cci_write(sensor->regmap, OV2680_REG_STREAM_CTRL, 0, NULL);
 }
 
 static int ov2680_power_off(struct ov2680_dev *sensor)
@@ -369,11 +355,6 @@ static int ov2680_power_on(struct ov2680_dev *sensor)
 
 	sensor->is_enabled = true;
 
-	/* Set clock lane into LP-11 state */
-	ov2680_stream_enable(sensor);
-	usleep_range(1000, 2000);
-	ov2680_stream_disable(sensor);
-
 	return 0;
 
 err_disable_regulators:
@@ -381,26 +362,6 @@ static int ov2680_power_on(struct ov2680_dev *sensor)
 	return ret;
 }
 
-static int ov2680_s_power(struct v4l2_subdev *sd, int on)
-{
-	struct ov2680_dev *sensor = to_ov2680_dev(sd);
-	int ret = 0;
-
-	mutex_lock(&sensor->lock);
-
-	if (on)
-		ret = ov2680_power_on(sensor);
-	else
-		ret = ov2680_power_off(sensor);
-
-	if (on && ret == 0)
-		ret = ov2680_mode_restore(sensor);
-
-	mutex_unlock(&sensor->lock);
-
-	return ret;
-}
-
 static int ov2680_s_g_frame_interval(struct v4l2_subdev *sd,
 				     struct v4l2_subdev_frame_interval *fi)
 {
@@ -423,16 +384,20 @@ static int ov2680_s_stream(struct v4l2_subdev *sd, int enable)
 	if (sensor->is_streaming == !!enable)
 		goto unlock;
 
-	if (enable && sensor->mode_pending_changes) {
-		ret = ov2680_mode_set(sensor);
+	if (enable) {
+		ret = pm_runtime_resume_and_get(sensor->sd.dev);
 		if (ret < 0)
 			goto unlock;
-	}
 
-	if (enable)
 		ret = ov2680_stream_enable(sensor);
-	else
+		if (ret < 0) {
+			pm_runtime_put(sensor->sd.dev);
+			goto unlock;
+		}
+	} else {
 		ret = ov2680_stream_disable(sensor);
+		pm_runtime_put(sensor->sd.dev);
+	}
 
 	sensor->is_streaming = !!enable;
 
@@ -519,7 +484,6 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
 
 	sensor->current_mode = mode;
 	sensor->fmt = format->format;
-	sensor->mode_pending_changes = true;
 
 unlock:
 	mutex_unlock(&sensor->lock);
@@ -603,10 +567,6 @@ static const struct v4l2_ctrl_ops ov2680_ctrl_ops = {
 	.s_ctrl = ov2680_s_ctrl,
 };
 
-static const struct v4l2_subdev_core_ops ov2680_core_ops = {
-	.s_power = ov2680_s_power,
-};
-
 static const struct v4l2_subdev_video_ops ov2680_video_ops = {
 	.g_frame_interval	= ov2680_s_g_frame_interval,
 	.s_frame_interval	= ov2680_s_g_frame_interval,
@@ -623,7 +583,6 @@ static const struct v4l2_subdev_pad_ops ov2680_pad_ops = {
 };
 
 static const struct v4l2_subdev_ops ov2680_subdev_ops = {
-	.core	= &ov2680_core_ops,
 	.video	= &ov2680_video_ops,
 	.pad	= &ov2680_pad_ops,
 };
@@ -643,8 +602,6 @@ static int ov2680_mode_init(struct ov2680_dev *sensor)
 
 	sensor->current_mode = init_mode;
 
-	sensor->mode_pending_changes = true;
-
 	return 0;
 }
 
@@ -722,8 +679,6 @@ static int ov2680_check_id(struct ov2680_dev *sensor)
 	u64 chip_id;
 	int ret;
 
-	ov2680_power_on(sensor);
-
 	ret = cci_read(sensor->regmap, OV2680_REG_CHIP_ID, &chip_id, NULL);
 	if (ret < 0) {
 		dev_err(sensor->dev, "failed to read chip id\n");
@@ -800,18 +755,39 @@ static int ov2680_probe(struct i2c_client *client)
 
 	mutex_init(&sensor->lock);
 
-	ret = ov2680_check_id(sensor);
+	/*
+	 * Power up and verify the chip now, so that if runtime pm is
+	 * disabled the chip is left on and streaming will work.
+	 */
+	ret = ov2680_power_on(sensor);
 	if (ret < 0)
 		goto lock_destroy;
 
+	ret = ov2680_check_id(sensor);
+	if (ret < 0)
+		goto err_powerdown;
+
+	pm_runtime_set_active(&client->dev);
+	pm_runtime_get_noresume(&client->dev);
+	pm_runtime_enable(&client->dev);
+
 	ret = ov2680_v4l2_register(sensor);
 	if (ret < 0)
-		goto lock_destroy;
+		goto err_pm_runtime;
+
+	pm_runtime_set_autosuspend_delay(&client->dev, 1000);
+	pm_runtime_use_autosuspend(&client->dev);
+	pm_runtime_put_autosuspend(&client->dev);
 
 	dev_info(dev, "ov2680 init correctly\n");
 
 	return 0;
 
+err_pm_runtime:
+	pm_runtime_disable(&client->dev);
+	pm_runtime_put_noidle(&client->dev);
+err_powerdown:
+	ov2680_power_off(sensor);
 lock_destroy:
 	dev_err(dev, "ov2680 init fail: %d\n", ret);
 	mutex_destroy(&sensor->lock);
@@ -828,9 +804,18 @@ static void ov2680_remove(struct i2c_client *client)
 	mutex_destroy(&sensor->lock);
 	media_entity_cleanup(&sensor->sd.entity);
 	v4l2_ctrl_handler_free(&sensor->ctrls.handler);
+
+	/*
+	 * Disable runtime PM. In case runtime PM is disabled in the kernel,
+	 * make sure to turn power off manually.
+	 */
+	pm_runtime_disable(&client->dev);
+	if (!pm_runtime_status_suspended(&client->dev))
+		ov2680_power_off(sensor);
+	pm_runtime_set_suspended(&client->dev);
 }
 
-static int __maybe_unused ov2680_suspend(struct device *dev)
+static int ov2680_suspend(struct device *dev)
 {
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
 	struct ov2680_dev *sensor = to_ov2680_dev(sd);
@@ -838,15 +823,19 @@ static int __maybe_unused ov2680_suspend(struct device *dev)
 	if (sensor->is_streaming)
 		ov2680_stream_disable(sensor);
 
-	return 0;
+	return ov2680_power_off(sensor);
 }
 
-static int __maybe_unused ov2680_resume(struct device *dev)
+static int ov2680_resume(struct device *dev)
 {
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
 	struct ov2680_dev *sensor = to_ov2680_dev(sd);
 	int ret;
 
+	ret = ov2680_power_on(sensor);
+	if (ret < 0)
+		goto stream_disable;
+
 	if (sensor->is_streaming) {
 		ret = ov2680_stream_enable(sensor);
 		if (ret < 0)
@@ -862,9 +851,8 @@ static int __maybe_unused ov2680_resume(struct device *dev)
 	return ret;
 }
 
-static const struct dev_pm_ops ov2680_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(ov2680_suspend, ov2680_resume)
-};
+static DEFINE_RUNTIME_DEV_PM_OPS(ov2680_pm_ops, ov2680_suspend, ov2680_resume,
+				 NULL);
 
 static const struct of_device_id ov2680_dt_ids[] = {
 	{ .compatible = "ovti,ov2680" },
@@ -875,7 +863,7 @@ MODULE_DEVICE_TABLE(of, ov2680_dt_ids);
 static struct i2c_driver ov2680_i2c_driver = {
 	.driver = {
 		.name  = "ov2680",
-		.pm = &ov2680_pm_ops,
+		.pm = pm_sleep_ptr(&ov2680_pm_ops),
 		.of_match_table	= ov2680_dt_ids,
 	},
 	.probe		= ov2680_probe,
-- 
2.41.0

[PATCH v5 12/32] media: ov2680: Check for "powerdown" GPIO con-id before checking for "reset" GPIO con-id

[Hans de Goede]

The datasheet of the OV2680 labels the single GPIO to put the sensor in
powersaving mode as XSHUTDN aka shutdown, _not_ reset.

This is important because some boards have standardized sensor connectors
which allow connecting various sensor modules. These connectors have both
reset and powerdown signals and the powerdown signal is routed to
the OV2680's XSHUTDN pin.

On x86/ACPI multiple Bay Trail, Cherry Trail, Sky Lake and Kaby Lake models
have an OV2680 connected to the ISP2 / IPU3. On these devices the GPIOS are
not described in DT instead the GPIOs are described with an Intel specific
DSM which labels them as either powerdown or reset. Often this DSM returns
both reset and powerdown pins even though the OV2680 has only 1 such pin.

For the ov2680 driver to work on these devices it must use the GPIO with
"powerdown" as con-id, matching the XSHUTDN name from the datasheet.

As for why "powerdown" vs say "shutdown" the ACPI DSM -> con-id mapping
code is shared, so we must use standardized names and currently all of
the following sensor drivers already use "powerdown":
adv7180, gc0310, isl7998x, ov02a10, ov2659, ov5640, ov5648, ov5670,
ov5693, ov7670, ov772x, ov7740, ov8858, ov8865 and ov9650 .

Where as the hi846 driver is the lonely standout using "shutdown".

Try the "powerdown" con-id first to make things work, falling back to
"reset" to keep existing DT setups working.

Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/ov2680.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index bc0ca2927370..b912ae7a63da 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -97,7 +97,7 @@ struct ov2680_dev {
 	u32				xvclk_freq;
 	struct regulator_bulk_data	supplies[OV2680_NUM_SUPPLIES];
 
-	struct gpio_desc		*reset_gpio;
+	struct gpio_desc		*pwdn_gpio;
 	struct mutex			lock; /* protect members */
 
 	bool				is_enabled;
@@ -180,19 +180,19 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
 
 static void ov2680_power_up(struct ov2680_dev *sensor)
 {
-	if (!sensor->reset_gpio)
+	if (!sensor->pwdn_gpio)
 		return;
 
-	gpiod_set_value(sensor->reset_gpio, 0);
+	gpiod_set_value(sensor->pwdn_gpio, 0);
 	usleep_range(5000, 10000);
 }
 
 static void ov2680_power_down(struct ov2680_dev *sensor)
 {
-	if (!sensor->reset_gpio)
+	if (!sensor->pwdn_gpio)
 		return;
 
-	gpiod_set_value(sensor->reset_gpio, 1);
+	gpiod_set_value(sensor->pwdn_gpio, 1);
 	usleep_range(5000, 10000);
 }
 
@@ -336,7 +336,7 @@ static int ov2680_power_on(struct ov2680_dev *sensor)
 		return ret;
 	}
 
-	if (!sensor->reset_gpio) {
+	if (!sensor->pwdn_gpio) {
 		ret = cci_write(sensor->regmap, OV2680_REG_SOFT_RESET, 0x01,
 				NULL);
 		if (ret != 0) {
@@ -697,16 +697,27 @@ static int ov2680_check_id(struct ov2680_dev *sensor)
 static int ov2680_parse_dt(struct ov2680_dev *sensor)
 {
 	struct device *dev = sensor->dev;
+	struct gpio_desc *gpio;
 	int ret;
 
-	sensor->reset_gpio = devm_gpiod_get_optional(dev, "reset",
-						     GPIOD_OUT_HIGH);
-	ret = PTR_ERR_OR_ZERO(sensor->reset_gpio);
+	/*
+	 * The pin we want is named XSHUTDN in the datasheet. Linux sensor
+	 * drivers have standardized on using "powerdown" as con-id name
+	 * for powerdown or shutdown pins. Older DTB files use "reset",
+	 * so fallback to that if there is no "powerdown" pin.
+	 */
+	gpio = devm_gpiod_get_optional(dev, "powerdown", GPIOD_OUT_HIGH);
+	if (!gpio)
+		gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+
+	ret = PTR_ERR_OR_ZERO(gpio);
 	if (ret < 0) {
 		dev_dbg(dev, "error while getting reset gpio: %d\n", ret);
 		return ret;
 	}
 
+	sensor->pwdn_gpio = gpio;
+
 	sensor->xvclk = devm_clk_get(dev, "xvclk");
 	if (IS_ERR(sensor->xvclk)) {
 		dev_err(dev, "xvclk clock missing or invalid\n");
-- 
2.41.0

[PATCH v5 13/32] media: ov2680: Drop is_enabled flag

[Hans de Goede]

With runtime-pm it is guaranteed that ov2680_power_on() and
ov2680_power_off() will always be called in a balanced way;
and the is_enabled check in ov2680_s_ctrl() can be replaced
by checking the runtime-suspend state.

So there is no more need for the is_enabled flag, remove it.

While at it also make sure that flip control changes while
suspended still lead to the bayer-order getting updated so
that get_fmt returns the correct bayer-order.

Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/ov2680.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index b912ae7a63da..cf84701a6a5a 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -100,7 +100,6 @@ struct ov2680_dev {
 	struct gpio_desc		*pwdn_gpio;
 	struct mutex			lock; /* protect members */
 
-	bool				is_enabled;
 	bool				is_streaming;
 
 	struct ov2680_ctrls		ctrls;
@@ -312,14 +311,9 @@ static int ov2680_stream_disable(struct ov2680_dev *sensor)
 
 static int ov2680_power_off(struct ov2680_dev *sensor)
 {
-	if (!sensor->is_enabled)
-		return 0;
-
 	clk_disable_unprepare(sensor->xvclk);
 	ov2680_power_down(sensor);
 	regulator_bulk_disable(OV2680_NUM_SUPPLIES, sensor->supplies);
-	sensor->is_enabled = false;
-
 	return 0;
 }
 
@@ -327,9 +321,6 @@ static int ov2680_power_on(struct ov2680_dev *sensor)
 {
 	int ret;
 
-	if (sensor->is_enabled)
-		return 0;
-
 	ret = regulator_bulk_enable(OV2680_NUM_SUPPLIES, sensor->supplies);
 	if (ret < 0) {
 		dev_err(sensor->dev, "failed to enable regulators: %d\n", ret);
@@ -353,8 +344,6 @@ static int ov2680_power_on(struct ov2680_dev *sensor)
 	if (ret < 0)
 		goto err_disable_regulators;
 
-	sensor->is_enabled = true;
-
 	return 0;
 
 err_disable_regulators:
@@ -541,26 +530,37 @@ static int ov2680_s_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
 	struct ov2680_dev *sensor = to_ov2680_dev(sd);
+	int ret;
 
-	if (!sensor->is_enabled)
+	/* Only apply changes to the controls if the device is powered up */
+	if (!pm_runtime_get_if_in_use(sensor->sd.dev)) {
+		ov2680_set_bayer_order(sensor, &sensor->fmt);
 		return 0;
+	}
 
 	switch (ctrl->id) {
 	case V4L2_CID_GAIN:
-		return ov2680_gain_set(sensor, ctrl->val);
+		ret = ov2680_gain_set(sensor, ctrl->val);
+		break;
 	case V4L2_CID_EXPOSURE:
-		return ov2680_exposure_set(sensor, ctrl->val);
+		ret = ov2680_exposure_set(sensor, ctrl->val);
+		break;
 	case V4L2_CID_VFLIP:
-		return ov2680_set_vflip(sensor, ctrl->val);
+		ret = ov2680_set_vflip(sensor, ctrl->val);
+		break;
 	case V4L2_CID_HFLIP:
-		return ov2680_set_hflip(sensor, ctrl->val);
+		ret = ov2680_set_hflip(sensor, ctrl->val);
+		break;
 	case V4L2_CID_TEST_PATTERN:
-		return ov2680_test_pattern_set(sensor, ctrl->val);
+		ret = ov2680_test_pattern_set(sensor, ctrl->val);
+		break;
 	default:
+		ret = -EINVAL;
 		break;
 	}
 
-	return -EINVAL;
+	pm_runtime_put(sensor->sd.dev);
+	return ret;
 }
 
 static const struct v4l2_ctrl_ops ov2680_ctrl_ops = {
-- 
2.41.0

[PATCH v5 14/32] media: ov2680: Add support for more clk setups

[Hans de Goede]

On ACPI systems the following 2 scenarios are possible:

1. The xvclk is fully controlled by ACPI powermanagement, so there
   is no "xvclk" for the driver to get (since it is abstracted away).
   In this case there will be a "clock-frequency" device property
   to tell the driver the xvclk rate.

2. There is a xvclk modelled in the clk framework for the driver,
   but the clk-generator may not be set to the right frequency
   yet. In this case there will also be a "clock-frequency" device
   property and the driver is expected to set the rate of the xvclk
   through this frequency through the clk framework.

Handle both these scenarios by switching to devm_clk_get_optional()
and checking for a "clock-frequency" device property.

This is modelled after how the same issue was fixed for the ov8865 in
commit 73dcffeb2ff9 ("media: i2c: Support 19.2MHz input clock in ov8865").

Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Reviewed-by: Tommaso Merciai <tomm.merciai@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/ov2680.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index cf84701a6a5a..42be7b094d5d 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -698,6 +698,7 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor)
 {
 	struct device *dev = sensor->dev;
 	struct gpio_desc *gpio;
+	unsigned int rate = 0;
 	int ret;
 
 	/*
@@ -718,13 +719,34 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor)
 
 	sensor->pwdn_gpio = gpio;
 
-	sensor->xvclk = devm_clk_get(dev, "xvclk");
+	sensor->xvclk = devm_clk_get_optional(dev, "xvclk");
 	if (IS_ERR(sensor->xvclk)) {
 		dev_err(dev, "xvclk clock missing or invalid\n");
 		return PTR_ERR(sensor->xvclk);
 	}
 
-	sensor->xvclk_freq = clk_get_rate(sensor->xvclk);
+	/*
+	 * We could have either a 24MHz or 19.2MHz clock rate from either DT or
+	 * ACPI... but we also need to support the weird IPU3 case which will
+	 * have an external clock AND a clock-frequency property. Check for the
+	 * clock-frequency property and if found, set that rate if we managed
+	 * to acquire a clock. This should cover the ACPI case. If the system
+	 * uses devicetree then the configured rate should already be set, so
+	 * we can just read it.
+	 */
+	ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
+				       &rate);
+	if (ret && !sensor->xvclk)
+		return dev_err_probe(dev, ret, "invalid clock config\n");
+
+	if (!ret && sensor->xvclk) {
+		ret = clk_set_rate(sensor->xvclk, rate);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "failed to set clock rate\n");
+	}
+
+	sensor->xvclk_freq = rate ?: clk_get_rate(sensor->xvclk);
 	if (sensor->xvclk_freq != OV2680_XVCLK_VALUE) {
 		dev_err(dev, "wrong xvclk frequency %d HZ, expected: %d Hz\n",
 			sensor->xvclk_freq, OV2680_XVCLK_VALUE);
-- 
2.41.0

[PATCH v5 15/32] media: ov2680: Add support for 19.2 MHz clock

[Hans de Goede]

Most x86/ACPI boards use the ov2680 with a 19.2 MHz xvclk,
rather then the expected 24MHz, add support for this.

Compensate for the lower clk by setting a higher PLL multiplier
of 69 when using 19.2 MHz vs the default multiplier of 55 for
a 24MHz xvclk.

Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/ov2680.c | 41 +++++++++++++++++++++++++++++++-------
 1 file changed, 34 insertions(+), 7 deletions(-)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index 42be7b094d5d..a8c257f3bcd6 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -27,14 +27,13 @@
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-subdev.h>
 
-#define OV2680_XVCLK_VALUE			24000000
-
 #define OV2680_CHIP_ID				0x2680
 
 #define OV2680_REG_STREAM_CTRL			CCI_REG8(0x0100)
 #define OV2680_REG_SOFT_RESET			CCI_REG8(0x0103)
 
 #define OV2680_REG_CHIP_ID			CCI_REG16(0x300a)
+#define OV2680_REG_PLL_MULTIPLIER		CCI_REG16(0x3081)
 
 #define OV2680_REG_EXPOSURE_PK			CCI_REG24(0x3500)
 #define OV2680_REG_R_MANUAL			CCI_REG8(0x3503)
@@ -69,6 +68,21 @@ static const char * const ov2680_supply_name[] = {
 
 #define OV2680_NUM_SUPPLIES ARRAY_SIZE(ov2680_supply_name)
 
+enum {
+	OV2680_19_2_MHZ,
+	OV2680_24_MHZ,
+};
+
+static const unsigned long ov2680_xvclk_freqs[] = {
+	[OV2680_19_2_MHZ] = 19200000,
+	[OV2680_24_MHZ] = 24000000,
+};
+
+static const u8 ov2680_pll_multipliers[] = {
+	[OV2680_19_2_MHZ] = 69,
+	[OV2680_24_MHZ] = 55,
+};
+
 struct ov2680_mode_info {
 	const char *name;
 	enum ov2680_mode_id id;
@@ -95,6 +109,7 @@ struct ov2680_dev {
 	struct media_pad		pad;
 	struct clk			*xvclk;
 	u32				xvclk_freq;
+	u8				pll_mult;
 	struct regulator_bulk_data	supplies[OV2680_NUM_SUPPLIES];
 
 	struct gpio_desc		*pwdn_gpio;
@@ -284,6 +299,11 @@ static int ov2680_stream_enable(struct ov2680_dev *sensor)
 {
 	int ret;
 
+	ret = cci_write(sensor->regmap, OV2680_REG_PLL_MULTIPLIER,
+			sensor->pll_mult, NULL);
+	if (ret < 0)
+		return ret;
+
 	ret = regmap_multi_reg_write(sensor->regmap,
 				     ov2680_mode_init_data.reg_data,
 				     ov2680_mode_init_data.reg_data_size);
@@ -699,7 +719,7 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor)
 	struct device *dev = sensor->dev;
 	struct gpio_desc *gpio;
 	unsigned int rate = 0;
-	int ret;
+	int i, ret;
 
 	/*
 	 * The pin we want is named XSHUTDN in the datasheet. Linux sensor
@@ -747,12 +767,19 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor)
 	}
 
 	sensor->xvclk_freq = rate ?: clk_get_rate(sensor->xvclk);
-	if (sensor->xvclk_freq != OV2680_XVCLK_VALUE) {
-		dev_err(dev, "wrong xvclk frequency %d HZ, expected: %d Hz\n",
-			sensor->xvclk_freq, OV2680_XVCLK_VALUE);
-		return -EINVAL;
+
+	for (i = 0; i < ARRAY_SIZE(ov2680_xvclk_freqs); i++) {
+		if (sensor->xvclk_freq == ov2680_xvclk_freqs[i])
+			break;
 	}
 
+	if (i == ARRAY_SIZE(ov2680_xvclk_freqs))
+		return dev_err_probe(dev, -EINVAL,
+				     "unsupported xvclk frequency %d Hz\n",
+				     sensor->xvclk_freq);
+
+	sensor->pll_mult = ov2680_pll_multipliers[i];
+
 	return 0;
 }
 
-- 
2.41.0

[PATCH v5 16/32] media: ov2680: Wait for endpoint fwnode before continuing with probe()

[Hans de Goede]

Defer probe() until the endpoint fwnode is available. This is necessary
on ACPI platforms where the bridge code creating the fwnodes may also e.g.
set the "clock-frequency" device property and add GPIO mappings.

Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v4:
- Only defer probe do not set v4l2_subdev.fwnode
---
 drivers/media/i2c/ov2680.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index a8c257f3bcd6..d4664581b49b 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -717,10 +717,22 @@ static int ov2680_check_id(struct ov2680_dev *sensor)
 static int ov2680_parse_dt(struct ov2680_dev *sensor)
 {
 	struct device *dev = sensor->dev;
+	struct fwnode_handle *ep_fwnode;
 	struct gpio_desc *gpio;
 	unsigned int rate = 0;
 	int i, ret;
 
+	/*
+	 * Sometimes the fwnode graph is initialized by the bridge driver.
+	 * Bridge drivers doing this may also add GPIO mappings, wait for this.
+	 */
+	ep_fwnode = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
+	if (!ep_fwnode)
+		return dev_err_probe(dev, -EPROBE_DEFER,
+				     "waiting for fwnode graph endpoint\n");
+
+	fwnode_handle_put(ep_fwnode);
+
 	/*
 	 * The pin we want is named XSHUTDN in the datasheet. Linux sensor
 	 * drivers have standardized on using "powerdown" as con-id name
@@ -801,7 +813,7 @@ static int ov2680_probe(struct i2c_client *client)
 
 	ret = ov2680_parse_dt(sensor);
 	if (ret < 0)
-		return -EINVAL;
+		return ret;
 
 	ret = ov2680_mode_init(sensor);
 	if (ret < 0)
-- 
2.41.0

[PATCH v5 17/32] media: ov2680: Add support for ACPI enumeration

[Hans de Goede]

Add an acpi_match_table now that all the other bits necessary for
ACPI support are in place.

The OVTI prefix used for the ACPI-HID is used for various OmniVision
sensors on many generations x86 tablets and laptops.

The OVTI2680 HID specifically is used on multiple models spanning at
least 4 different Intel CPU models (2 ISP2, 2 IPU3).

Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v5:
- Add some notes to the commit message about the OVTI2680 HID
---
 drivers/media/i2c/ov2680.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index d4664581b49b..0adfacc70735 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -932,11 +932,18 @@ static const struct of_device_id ov2680_dt_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, ov2680_dt_ids);
 
+static const struct acpi_device_id ov2680_acpi_ids[] = {
+	{ "OVTI2680" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(acpi, ov2680_acpi_ids);
+
 static struct i2c_driver ov2680_i2c_driver = {
 	.driver = {
 		.name  = "ov2680",
 		.pm = pm_sleep_ptr(&ov2680_pm_ops),
 		.of_match_table	= ov2680_dt_ids,
+		.acpi_match_table = ov2680_acpi_ids,
 	},
 	.probe		= ov2680_probe,
 	.remove		= ov2680_remove,
-- 
2.41.0

[PATCH v5 18/32] media: ov2680: Fix ov2680_enum_frame_interval()

[Hans de Goede]

Fix and simplify ov2680_enum_frame_interval(), the index is not
an index into ov2680_mode_data[], so using OV2680_MODE_MAX is wrong.

Instead it is an index indexing the different framerates for
the resolution specified in fie->width, fie->height.

Note validating fie->which is not necessary this is already done
by the v4l2-core.

Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v4:
- Verify that the passed in which, width and height are valid
---
 drivers/media/i2c/ov2680.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index 0adfacc70735..a83efd449993 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -527,21 +527,30 @@ static int ov2680_enum_frame_size(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static bool ov2680_valid_frame_size(struct v4l2_subdev_frame_interval_enum *fie)
+{
+	int i;
+
+	for (i = 0; i < OV2680_MODE_MAX; i++) {
+		if (fie->width == ov2680_mode_data[i].width &&
+		    fie->height == ov2680_mode_data[i].height)
+			return true;
+	}
+
+	return false;
+}
+
 static int ov2680_enum_frame_interval(struct v4l2_subdev *sd,
 			      struct v4l2_subdev_state *sd_state,
 			      struct v4l2_subdev_frame_interval_enum *fie)
 {
-	struct v4l2_fract tpf;
+	struct ov2680_dev *sensor = to_ov2680_dev(sd);
 
-	if (fie->index >= OV2680_MODE_MAX || fie->width > OV2680_WIDTH_MAX ||
-	    fie->height > OV2680_HEIGHT_MAX ||
-	    fie->which > V4L2_SUBDEV_FORMAT_ACTIVE)
+	/* Only 1 framerate */
+	if (fie->index || !ov2680_valid_frame_size(fie))
 		return -EINVAL;
 
-	tpf.denominator = OV2680_FRAME_RATE;
-	tpf.numerator = 1;
-
-	fie->interval = tpf;
+	fie->interval = sensor->frame_interval;
 
 	return 0;
 }
-- 
2.41.0

[PATCH v5 19/32] media: ov2680: Annotate the per mode register setting lists

[Hans de Goede]

Annotate the per mode register setting lists.

This is a preparation patch for moving to calculating the per mode
settings, allowing to set any mode through cropping.

The annotations make it easier to see which registers are mode
dependent and which are fixed.

Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/ov2680.c | 118 ++++++++++++++++++++++++++++++++-----
 1 file changed, 104 insertions(+), 14 deletions(-)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index a83efd449993..e7d2e555e1c6 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -140,27 +140,117 @@ static const int ov2680_hv_flip_bayer_order[] = {
 };
 
 static const struct reg_sequence ov2680_setting_30fps_QUXGA_800_600[] = {
-	{0x3086, 0x01}, {0x370a, 0x23}, {0x3808, 0x03}, {0x3809, 0x20},
-	{0x380a, 0x02}, {0x380b, 0x58}, {0x380c, 0x06}, {0x380d, 0xac},
-	{0x380e, 0x02}, {0x380f, 0x84}, {0x3811, 0x04}, {0x3813, 0x04},
-	{0x3814, 0x31}, {0x3815, 0x31}, {0x3820, 0xc0}, {0x4008, 0x00},
-	{0x4009, 0x03}, {0x4837, 0x1e}, {0x3501, 0x4e}, {0x3502, 0xe0},
+	/* Set PLL SP DIV to 1 for binning mode */
+	{0x3086, 0x01},
+
+	/* Sensor control register 0x0a to 0x23 for binning mode */
+	{0x370a, 0x23},
+
+	/* Set X and Y output size to 800x600 */
+	{0x3808, 0x03},
+	{0x3809, 0x20},
+	{0x380a, 0x02},
+	{0x380b, 0x58},
+
+	/* Set HTS + VTS to 1708x644 */
+	{0x380c, 0x06},
+	{0x380d, 0xac},
+	{0x380e, 0x02},
+	{0x380f, 0x84},
+
+	/* Set ISP WIN X and Y start to 4x4 */
+	{0x3811, 0x04},
+	{0x3813, 0x04},
+
+	/* Set X INC and Y INC for binning */
+	{0x3814, 0x31},
+	{0x3815, 0x31},
+
+	/* Initialize FORMAT1 to default/reset value (vflip disabled) */
+	{0x3820, 0xc0},
+
+	/* Set black level compensation range to 0 - 3 (default 0 - 11) */
+	{0x4008, 0x00},
+	{0x4009, 0x03},
+
+	/* Set MIPI pclk period to 0x1e (default/reset is 0x18) */
+	{0x4837, 0x1e},
+
+	/* Initialize exposure to 0x4ee (overridden by the ctrl, drop this */
+	{0x3501, 0x4e},
+	{0x3502, 0xe0},
+
+	/* R MANUAL set exposure and gain to manual (hw does not do auto) */
 	{0x3503, 0x03},
 };
 
 static const struct reg_sequence ov2680_setting_30fps_720P_1280_720[] = {
-	{0x3086, 0x00}, {0x3808, 0x05}, {0x3809, 0x00}, {0x380a, 0x02},
-	{0x380b, 0xd0}, {0x380c, 0x06}, {0x380d, 0xa8}, {0x380e, 0x05},
-	{0x380f, 0x0e}, {0x3811, 0x08}, {0x3813, 0x06}, {0x3814, 0x11},
-	{0x3815, 0x11}, {0x3820, 0xc0}, {0x4008, 0x00},
+	/* Set PLL SP DIV to 0 for not binning mode */
+	{0x3086, 0x00},
+
+	/* Set X and Y output size to 1280x720 */
+	{0x3808, 0x05},
+	{0x3809, 0x00},
+	{0x380a, 0x02},
+	{0x380b, 0xd0},
+
+	/* Set HTS + VTS to 1704x1294 */
+	{0x380c, 0x06},
+	{0x380d, 0xa8},
+	{0x380e, 0x05},
+	{0x380f, 0x0e},
+
+	/* Set ISP WIN X and Y start to 8x6 */
+	{0x3811, 0x08},
+	{0x3813, 0x06},
+
+	/* Set X INC and Y INC for non binning */
+	{0x3814, 0x11},
+	{0x3815, 0x11},
+
+	/* Initialize FORMAT1 to default/reset value (vflip disabled) */
+	{0x3820, 0xc0},
+
+	/* Set backlight compensation range start to 0 */
+	{0x4008, 0x00},
 };
 
 static const struct reg_sequence ov2680_setting_30fps_UXGA_1600_1200[] = {
-	{0x3086, 0x00}, {0x3501, 0x4e}, {0x3502, 0xe0}, {0x3808, 0x06},
-	{0x3809, 0x40}, {0x380a, 0x04}, {0x380b, 0xb0}, {0x380c, 0x06},
-	{0x380d, 0xa8}, {0x380e, 0x05}, {0x380f, 0x0e}, {0x3811, 0x00},
-	{0x3813, 0x00}, {0x3814, 0x11}, {0x3815, 0x11}, {0x3820, 0xc0},
-	{0x4008, 0x00}, {0x4837, 0x18}
+	/* Set PLL SP DIV to 0 for not binning mode */
+	{0x3086, 0x00},
+
+	/* Initialize exposure to 0x4ee (overridden by the ctrl, drop this */
+	{0x3501, 0x4e},
+	{0x3502, 0xe0},
+
+	/* Set X and Y output size to 1600x1200 */
+	{0x3808, 0x06},
+	{0x3809, 0x40},
+	{0x380a, 0x04},
+	{0x380b, 0xb0},
+
+	/* Set HTS + VTS to 1704x1294 */
+	{0x380c, 0x06},
+	{0x380d, 0xa8},
+	{0x380e, 0x05},
+	{0x380f, 0x0e},
+
+	/* Set ISP WIN X and Y start to 0x0 */
+	{0x3811, 0x00},
+	{0x3813, 0x00},
+
+	/* Set X INC and Y INC for non binning */
+	{0x3814, 0x11},
+	{0x3815, 0x11},
+
+	/* Initialize FORMAT1 to default/reset value (vflip disabled) */
+	{0x3820, 0xc0},
+
+	/* Set backlight compensation range start to 0 */
+	{0x4008, 0x00},
+
+	/* Set MIPI pclk period to default/reset value of 0x18 */
+	{0x4837, 0x18}
 };
 
 static const struct ov2680_mode_info ov2680_mode_init_data = {
-- 
2.41.0

[PATCH v5 20/32] media: ov2680: Add ov2680_mode struct

[Hans de Goede]

Add an ov2680_mode struct to group together mode related state.

For now this only containst the v4l2_mbus_framefmt and
the frame_interval.

This is a preparation patch for moving to calculating the per mode
settings, which will store more info in the new ov2680_mode struct.

Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/ov2680.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index e7d2e555e1c6..76f97d053e45 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -101,6 +101,11 @@ struct ov2680_ctrls {
 	struct v4l2_ctrl *test_pattern;
 };
 
+struct ov2680_mode {
+	struct v4l2_mbus_framefmt	fmt;
+	struct v4l2_fract		frame_interval;
+};
+
 struct ov2680_dev {
 	struct device			*dev;
 	struct regmap			*regmap;
@@ -118,8 +123,7 @@ struct ov2680_dev {
 	bool				is_streaming;
 
 	struct ov2680_ctrls		ctrls;
-	struct v4l2_mbus_framefmt	fmt;
-	struct v4l2_fract		frame_interval;
+	struct ov2680_mode		mode;
 
 	const struct ov2680_mode_info	*current_mode;
 };
@@ -338,7 +342,7 @@ static int ov2680_set_vflip(struct ov2680_dev *sensor, s32 val)
 	if (ret < 0)
 		return ret;
 
-	ov2680_set_bayer_order(sensor, &sensor->fmt);
+	ov2680_set_bayer_order(sensor, &sensor->mode.fmt);
 	return 0;
 }
 
@@ -354,7 +358,7 @@ static int ov2680_set_hflip(struct ov2680_dev *sensor, s32 val)
 	if (ret < 0)
 		return ret;
 
-	ov2680_set_bayer_order(sensor, &sensor->fmt);
+	ov2680_set_bayer_order(sensor, &sensor->mode.fmt);
 	return 0;
 }
 
@@ -467,7 +471,7 @@ static int ov2680_s_g_frame_interval(struct v4l2_subdev *sd,
 	struct ov2680_dev *sensor = to_ov2680_dev(sd);
 
 	mutex_lock(&sensor->lock);
-	fi->interval = sensor->frame_interval;
+	fi->interval = sensor->mode.frame_interval;
 	mutex_unlock(&sensor->lock);
 
 	return 0;
@@ -515,7 +519,7 @@ static int ov2680_enum_mbus_code(struct v4l2_subdev *sd,
 	if (code->pad != 0 || code->index != 0)
 		return -EINVAL;
 
-	code->code = sensor->fmt.code;
+	code->code = sensor->mode.fmt.code;
 
 	return 0;
 }
@@ -536,7 +540,7 @@ static int ov2680_get_fmt(struct v4l2_subdev *sd,
 		fmt = v4l2_subdev_get_try_format(&sensor->sd, sd_state,
 						 format->pad);
 	} else {
-		fmt = &sensor->fmt;
+		fmt = &sensor->mode.fmt;
 	}
 
 	format->format = *fmt;
@@ -582,7 +586,7 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
 	}
 
 	sensor->current_mode = mode;
-	sensor->fmt = format->format;
+	sensor->mode.fmt = format->format;
 
 unlock:
 	mutex_unlock(&sensor->lock);
@@ -640,7 +644,7 @@ static int ov2680_enum_frame_interval(struct v4l2_subdev *sd,
 	if (fie->index || !ov2680_valid_frame_size(fie))
 		return -EINVAL;
 
-	fie->interval = sensor->frame_interval;
+	fie->interval = sensor->mode.frame_interval;
 
 	return 0;
 }
@@ -653,7 +657,7 @@ static int ov2680_s_ctrl(struct v4l2_ctrl *ctrl)
 
 	/* Only apply changes to the controls if the device is powered up */
 	if (!pm_runtime_get_if_in_use(sensor->sd.dev)) {
-		ov2680_set_bayer_order(sensor, &sensor->fmt);
+		ov2680_set_bayer_order(sensor, &sensor->mode.fmt);
 		return 0;
 	}
 
@@ -711,11 +715,11 @@ static int ov2680_mode_init(struct ov2680_dev *sensor)
 	const struct ov2680_mode_info *init_mode;
 
 	/* set initial mode */
-	ov2680_fill_format(sensor, &sensor->fmt,
+	ov2680_fill_format(sensor, &sensor->mode.fmt,
 			   OV2680_DEFAULT_WIDTH, OV2680_DEFAULT_HEIGHT);
 
-	sensor->frame_interval.denominator = OV2680_FRAME_RATE;
-	sensor->frame_interval.numerator = 1;
+	sensor->mode.frame_interval.denominator = OV2680_FRAME_RATE;
+	sensor->mode.frame_interval.numerator = 1;
 
 	init_mode = &ov2680_mode_init_data;
 
-- 
2.41.0

[PATCH v5 21/32] media: ov2680: Make setting the mode algorithm based

[Hans de Goede]

Instead of using a long fixed register settings list for each resolution,
calculate the register settings based on the requested width + height.

This is based on atomisp-ov2680 commit 0611888592df ("media: atomisp:
ov2680: Make setting the modes algorithm based").

This will allow future enhancements like adding hblank and vblank controls
and adding selection support.

This also adds properly programming the ISP window and setting
the manual ISP window control bit in register 0x5708, this is
necessary for the hflip and vflip conrols to work properly.

Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v4:
- Make enum_frame_size() returned full-size + binned-/quarter-size,
  like the ov5693 code does
---
 drivers/media/i2c/ov2680.c | 334 +++++++++++++++++--------------------
 1 file changed, 157 insertions(+), 177 deletions(-)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index 76f97d053e45..012b95f90c1d 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -38,27 +38,48 @@
 #define OV2680_REG_EXPOSURE_PK			CCI_REG24(0x3500)
 #define OV2680_REG_R_MANUAL			CCI_REG8(0x3503)
 #define OV2680_REG_GAIN_PK			CCI_REG16(0x350a)
+
+#define OV2680_REG_SENSOR_CTRL_0A		CCI_REG8(0x370a)
+
+#define OV2680_REG_HORIZONTAL_START		CCI_REG16(0x3800)
+#define OV2680_REG_VERTICAL_START		CCI_REG16(0x3802)
+#define OV2680_REG_HORIZONTAL_END		CCI_REG16(0x3804)
+#define OV2680_REG_VERTICAL_END			CCI_REG16(0x3806)
+#define OV2680_REG_HORIZONTAL_OUTPUT_SIZE	CCI_REG16(0x3808)
+#define OV2680_REG_VERTICAL_OUTPUT_SIZE		CCI_REG16(0x380a)
 #define OV2680_REG_TIMING_HTS			CCI_REG16(0x380c)
 #define OV2680_REG_TIMING_VTS			CCI_REG16(0x380e)
+#define OV2680_REG_ISP_X_WIN			CCI_REG16(0x3810)
+#define OV2680_REG_ISP_Y_WIN			CCI_REG16(0x3812)
+#define OV2680_REG_X_INC			CCI_REG8(0x3814)
+#define OV2680_REG_Y_INC			CCI_REG8(0x3815)
 #define OV2680_REG_FORMAT1			CCI_REG8(0x3820)
 #define OV2680_REG_FORMAT2			CCI_REG8(0x3821)
 
 #define OV2680_REG_ISP_CTRL00			CCI_REG8(0x5080)
 
+#define OV2680_REG_X_WIN			CCI_REG16(0x5704)
+#define OV2680_REG_Y_WIN			CCI_REG16(0x5706)
+
 #define OV2680_FRAME_RATE			30
 
-#define OV2680_WIDTH_MAX			1600
-#define OV2680_HEIGHT_MAX			1200
+#define OV2680_NATIVE_WIDTH			1616
+#define OV2680_NATIVE_HEIGHT			1216
+#define OV2680_ACTIVE_WIDTH			1600
+#define OV2680_ACTIVE_HEIGHT			1200
+
+/* 66MHz pixel clock: 66MHz / 1704 * 1294 = 30fps */
+#define OV2680_PIXELS_PER_LINE			1704
+#define OV2680_LINES_PER_FRAME			1294
+
+/* If possible send 16 extra rows / lines to the ISP as padding */
+#define OV2680_END_MARGIN			16
 
 #define OV2680_DEFAULT_WIDTH			800
 #define OV2680_DEFAULT_HEIGHT			600
 
-enum ov2680_mode_id {
-	OV2680_MODE_QUXGA_800_600,
-	OV2680_MODE_720P_1280_720,
-	OV2680_MODE_UXGA_1600_1200,
-	OV2680_MODE_MAX,
-};
+/* For enum_frame_size() full-size + binned-/quarter-size */
+#define OV2680_FRAME_SIZES			2
 
 static const char * const ov2680_supply_name[] = {
 	"DOVDD",
@@ -83,15 +104,6 @@ static const u8 ov2680_pll_multipliers[] = {
 	[OV2680_24_MHZ] = 55,
 };
 
-struct ov2680_mode_info {
-	const char *name;
-	enum ov2680_mode_id id;
-	u32 width;
-	u32 height;
-	const struct reg_sequence *reg_data;
-	u32 reg_data_size;
-};
-
 struct ov2680_ctrls {
 	struct v4l2_ctrl_handler handler;
 	struct v4l2_ctrl *exposure;
@@ -104,6 +116,15 @@ struct ov2680_ctrls {
 struct ov2680_mode {
 	struct v4l2_mbus_framefmt	fmt;
 	struct v4l2_fract		frame_interval;
+	bool				binning;
+	u16				h_start;
+	u16				v_start;
+	u16				h_end;
+	u16				v_end;
+	u16				h_output_size;
+	u16				v_output_size;
+	u16				hts;
+	u16				vts;
 };
 
 struct ov2680_dev {
@@ -124,8 +145,6 @@ struct ov2680_dev {
 
 	struct ov2680_ctrls		ctrls;
 	struct ov2680_mode		mode;
-
-	const struct ov2680_mode_info	*current_mode;
 };
 
 static const char * const test_pattern_menu[] = {
@@ -143,136 +162,19 @@ static const int ov2680_hv_flip_bayer_order[] = {
 	MEDIA_BUS_FMT_SRGGB10_1X10,
 };
 
-static const struct reg_sequence ov2680_setting_30fps_QUXGA_800_600[] = {
-	/* Set PLL SP DIV to 1 for binning mode */
-	{0x3086, 0x01},
-
-	/* Sensor control register 0x0a to 0x23 for binning mode */
-	{0x370a, 0x23},
-
-	/* Set X and Y output size to 800x600 */
-	{0x3808, 0x03},
-	{0x3809, 0x20},
-	{0x380a, 0x02},
-	{0x380b, 0x58},
-
-	/* Set HTS + VTS to 1708x644 */
-	{0x380c, 0x06},
-	{0x380d, 0xac},
-	{0x380e, 0x02},
-	{0x380f, 0x84},
-
-	/* Set ISP WIN X and Y start to 4x4 */
-	{0x3811, 0x04},
-	{0x3813, 0x04},
-
-	/* Set X INC and Y INC for binning */
-	{0x3814, 0x31},
-	{0x3815, 0x31},
-
-	/* Initialize FORMAT1 to default/reset value (vflip disabled) */
-	{0x3820, 0xc0},
+static const struct reg_sequence ov2680_global_setting[] = {
+	/* R MANUAL set exposure and gain to manual (hw does not do auto) */
+	{0x3503, 0x03},
 
 	/* Set black level compensation range to 0 - 3 (default 0 - 11) */
 	{0x4008, 0x00},
 	{0x4009, 0x03},
 
-	/* Set MIPI pclk period to 0x1e (default/reset is 0x18) */
-	{0x4837, 0x1e},
-
-	/* Initialize exposure to 0x4ee (overridden by the ctrl, drop this */
-	{0x3501, 0x4e},
-	{0x3502, 0xe0},
-
-	/* R MANUAL set exposure and gain to manual (hw does not do auto) */
-	{0x3503, 0x03},
-};
-
-static const struct reg_sequence ov2680_setting_30fps_720P_1280_720[] = {
-	/* Set PLL SP DIV to 0 for not binning mode */
-	{0x3086, 0x00},
-
-	/* Set X and Y output size to 1280x720 */
-	{0x3808, 0x05},
-	{0x3809, 0x00},
-	{0x380a, 0x02},
-	{0x380b, 0xd0},
-
-	/* Set HTS + VTS to 1704x1294 */
-	{0x380c, 0x06},
-	{0x380d, 0xa8},
-	{0x380e, 0x05},
-	{0x380f, 0x0e},
-
-	/* Set ISP WIN X and Y start to 8x6 */
-	{0x3811, 0x08},
-	{0x3813, 0x06},
-
-	/* Set X INC and Y INC for non binning */
-	{0x3814, 0x11},
-	{0x3815, 0x11},
-
-	/* Initialize FORMAT1 to default/reset value (vflip disabled) */
-	{0x3820, 0xc0},
-
-	/* Set backlight compensation range start to 0 */
-	{0x4008, 0x00},
-};
-
-static const struct reg_sequence ov2680_setting_30fps_UXGA_1600_1200[] = {
-	/* Set PLL SP DIV to 0 for not binning mode */
-	{0x3086, 0x00},
-
-	/* Initialize exposure to 0x4ee (overridden by the ctrl, drop this */
-	{0x3501, 0x4e},
-	{0x3502, 0xe0},
-
-	/* Set X and Y output size to 1600x1200 */
-	{0x3808, 0x06},
-	{0x3809, 0x40},
-	{0x380a, 0x04},
-	{0x380b, 0xb0},
-
-	/* Set HTS + VTS to 1704x1294 */
-	{0x380c, 0x06},
-	{0x380d, 0xa8},
-	{0x380e, 0x05},
-	{0x380f, 0x0e},
-
-	/* Set ISP WIN X and Y start to 0x0 */
-	{0x3811, 0x00},
-	{0x3813, 0x00},
-
-	/* Set X INC and Y INC for non binning */
-	{0x3814, 0x11},
-	{0x3815, 0x11},
-
-	/* Initialize FORMAT1 to default/reset value (vflip disabled) */
-	{0x3820, 0xc0},
-
-	/* Set backlight compensation range start to 0 */
-	{0x4008, 0x00},
-
-	/* Set MIPI pclk period to default/reset value of 0x18 */
-	{0x4837, 0x18}
-};
-
-static const struct ov2680_mode_info ov2680_mode_init_data = {
-	"mode_quxga_800_600", OV2680_MODE_QUXGA_800_600, 800, 600,
-	ov2680_setting_30fps_QUXGA_800_600,
-	ARRAY_SIZE(ov2680_setting_30fps_QUXGA_800_600),
-};
-
-static const struct ov2680_mode_info ov2680_mode_data[OV2680_MODE_MAX] = {
-	{"mode_quxga_800_600", OV2680_MODE_QUXGA_800_600,
-	 800, 600, ov2680_setting_30fps_QUXGA_800_600,
-	 ARRAY_SIZE(ov2680_setting_30fps_QUXGA_800_600)},
-	{"mode_720p_1280_720", OV2680_MODE_720P_1280_720,
-	 1280, 720, ov2680_setting_30fps_720P_1280_720,
-	 ARRAY_SIZE(ov2680_setting_30fps_720P_1280_720)},
-	{"mode_uxga_1600_1200", OV2680_MODE_UXGA_1600_1200,
-	 1600, 1200, ov2680_setting_30fps_UXGA_1600_1200,
-	 ARRAY_SIZE(ov2680_setting_30fps_UXGA_1600_1200)},
+	/*
+	 * Window CONTROL 0x00 -> 0x01, enable manual window control,
+	 * this is necessary for full size flip and mirror support.
+	 */
+	{0x5708, 0x01},
 };
 
 static struct ov2680_dev *to_ov2680_dev(struct v4l2_subdev *sd)
@@ -330,6 +232,85 @@ static void ov2680_fill_format(struct ov2680_dev *sensor,
 	ov2680_set_bayer_order(sensor, fmt);
 }
 
+static void ov2680_calc_mode(struct ov2680_dev *sensor)
+{
+	int width = sensor->mode.fmt.width;
+	int height = sensor->mode.fmt.height;
+	int orig_width = width;
+	int orig_height = height;
+
+	if (width  <= (OV2680_NATIVE_WIDTH / 2) &&
+	    height <= (OV2680_NATIVE_HEIGHT / 2)) {
+		sensor->mode.binning = true;
+		width *= 2;
+		height *= 2;
+	} else {
+		sensor->mode.binning = false;
+	}
+
+	sensor->mode.h_start = ((OV2680_NATIVE_WIDTH - width) / 2) & ~1;
+	sensor->mode.v_start = ((OV2680_NATIVE_HEIGHT - height) / 2) & ~1;
+	sensor->mode.h_end =
+		min(sensor->mode.h_start + width + OV2680_END_MARGIN - 1,
+		    OV2680_NATIVE_WIDTH - 1);
+	sensor->mode.v_end =
+		min(sensor->mode.v_start + height + OV2680_END_MARGIN - 1,
+		    OV2680_NATIVE_HEIGHT - 1);
+	sensor->mode.h_output_size = orig_width;
+	sensor->mode.v_output_size = orig_height;
+	sensor->mode.hts = OV2680_PIXELS_PER_LINE;
+	sensor->mode.vts = OV2680_LINES_PER_FRAME;
+}
+
+static int ov2680_set_mode(struct ov2680_dev *sensor)
+{
+	u8 sensor_ctrl_0a, inc, fmt1, fmt2;
+	int ret = 0;
+
+	if (sensor->mode.binning) {
+		sensor_ctrl_0a = 0x23;
+		inc = 0x31;
+		fmt1 = 0xc2;
+		fmt2 = 0x01;
+	} else {
+		sensor_ctrl_0a = 0x21;
+		inc = 0x11;
+		fmt1 = 0xc0;
+		fmt2 = 0x00;
+	}
+
+	cci_write(sensor->regmap, OV2680_REG_SENSOR_CTRL_0A,
+		  sensor_ctrl_0a, &ret);
+	cci_write(sensor->regmap, OV2680_REG_HORIZONTAL_START,
+		  sensor->mode.h_start, &ret);
+	cci_write(sensor->regmap, OV2680_REG_VERTICAL_START,
+		  sensor->mode.v_start, &ret);
+	cci_write(sensor->regmap, OV2680_REG_HORIZONTAL_END,
+		  sensor->mode.h_end, &ret);
+	cci_write(sensor->regmap, OV2680_REG_VERTICAL_END,
+		  sensor->mode.v_end, &ret);
+	cci_write(sensor->regmap, OV2680_REG_HORIZONTAL_OUTPUT_SIZE,
+		  sensor->mode.h_output_size, &ret);
+	cci_write(sensor->regmap, OV2680_REG_VERTICAL_OUTPUT_SIZE,
+		  sensor->mode.v_output_size, &ret);
+	cci_write(sensor->regmap, OV2680_REG_TIMING_HTS,
+		  sensor->mode.hts, &ret);
+	cci_write(sensor->regmap, OV2680_REG_TIMING_VTS,
+		  sensor->mode.vts, &ret);
+	cci_write(sensor->regmap, OV2680_REG_ISP_X_WIN, 0, &ret);
+	cci_write(sensor->regmap, OV2680_REG_ISP_Y_WIN, 0, &ret);
+	cci_write(sensor->regmap, OV2680_REG_X_INC, inc, &ret);
+	cci_write(sensor->regmap, OV2680_REG_Y_INC, inc, &ret);
+	cci_write(sensor->regmap, OV2680_REG_X_WIN,
+		  sensor->mode.h_output_size, &ret);
+	cci_write(sensor->regmap, OV2680_REG_Y_WIN,
+		  sensor->mode.v_output_size, &ret);
+	cci_write(sensor->regmap, OV2680_REG_FORMAT1, fmt1, &ret);
+	cci_write(sensor->regmap, OV2680_REG_FORMAT2, fmt2, &ret);
+
+	return ret;
+}
+
 static int ov2680_set_vflip(struct ov2680_dev *sensor, s32 val)
 {
 	int ret;
@@ -399,14 +380,12 @@ static int ov2680_stream_enable(struct ov2680_dev *sensor)
 		return ret;
 
 	ret = regmap_multi_reg_write(sensor->regmap,
-				     ov2680_mode_init_data.reg_data,
-				     ov2680_mode_init_data.reg_data_size);
+				     ov2680_global_setting,
+				     ARRAY_SIZE(ov2680_global_setting));
 	if (ret < 0)
 		return ret;
 
-	ret = regmap_multi_reg_write(sensor->regmap,
-				     sensor->current_mode->reg_data,
-				     sensor->current_mode->reg_data_size);
+	ret = ov2680_set_mode(sensor);
 	if (ret < 0)
 		return ret;
 
@@ -556,21 +535,18 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
 {
 	struct ov2680_dev *sensor = to_ov2680_dev(sd);
 	struct v4l2_mbus_framefmt *try_fmt;
-	const struct ov2680_mode_info *mode;
+	unsigned int width, height;
 	int ret = 0;
 
 	if (format->pad != 0)
 		return -EINVAL;
 
-	mode = v4l2_find_nearest_size(ov2680_mode_data,
-				      ARRAY_SIZE(ov2680_mode_data),
-				      width, height,
-				      format->format.width,
-				      format->format.height);
-	if (!mode)
-		return -EINVAL;
+	width = min_t(unsigned int, ALIGN(format->format.width, 2),
+		      OV2680_NATIVE_WIDTH);
+	height = min_t(unsigned int, ALIGN(format->format.height, 2),
+		       OV2680_NATIVE_HEIGHT);
 
-	ov2680_fill_format(sensor, &format->format, mode->width, mode->height);
+	ov2680_fill_format(sensor, &format->format, width, height);
 
 	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
 		try_fmt = v4l2_subdev_get_try_format(sd, sd_state, 0);
@@ -585,8 +561,8 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
 		goto unlock;
 	}
 
-	sensor->current_mode = mode;
 	sensor->mode.fmt = format->format;
+	ov2680_calc_mode(sensor);
 
 unlock:
 	mutex_unlock(&sensor->lock);
@@ -608,26 +584,35 @@ static int ov2680_enum_frame_size(struct v4l2_subdev *sd,
 				  struct v4l2_subdev_state *sd_state,
 				  struct v4l2_subdev_frame_size_enum *fse)
 {
-	int index = fse->index;
-
-	if (index >= OV2680_MODE_MAX || index < 0)
+	if (fse->index >= OV2680_FRAME_SIZES)
 		return -EINVAL;
 
-	fse->min_width = ov2680_mode_data[index].width;
-	fse->min_height = ov2680_mode_data[index].height;
-	fse->max_width = ov2680_mode_data[index].width;
-	fse->max_height = ov2680_mode_data[index].height;
+	fse->min_width = OV2680_ACTIVE_WIDTH / (fse->index + 1);
+	fse->min_height = OV2680_ACTIVE_HEIGHT / (fse->index + 1);
+	fse->max_width = fse->min_width;
+	fse->max_height = fse->min_height;
 
 	return 0;
 }
 
-static bool ov2680_valid_frame_size(struct v4l2_subdev_frame_interval_enum *fie)
+static bool ov2680_valid_frame_size(struct v4l2_subdev *sd,
+				    struct v4l2_subdev_state *sd_state,
+				    struct v4l2_subdev_frame_interval_enum *fie)
 {
+	struct v4l2_subdev_frame_size_enum fse = {
+		.pad = fie->pad,
+		.which = fie->which,
+	};
 	int i;
 
-	for (i = 0; i < OV2680_MODE_MAX; i++) {
-		if (fie->width == ov2680_mode_data[i].width &&
-		    fie->height == ov2680_mode_data[i].height)
+	for (i = 0; i < OV2680_FRAME_SIZES; i++) {
+		fse.index = i;
+
+		if (ov2680_enum_frame_size(sd, sd_state, &fse))
+			return false;
+
+		if (fie->width == fse.min_width &&
+		    fie->height == fse.min_height)
 			return true;
 	}
 
@@ -641,7 +626,7 @@ static int ov2680_enum_frame_interval(struct v4l2_subdev *sd,
 	struct ov2680_dev *sensor = to_ov2680_dev(sd);
 
 	/* Only 1 framerate */
-	if (fie->index || !ov2680_valid_frame_size(fie))
+	if (fie->index || !ov2680_valid_frame_size(sd, sd_state, fie))
 		return -EINVAL;
 
 	fie->interval = sensor->mode.frame_interval;
@@ -712,19 +697,14 @@ static const struct v4l2_subdev_ops ov2680_subdev_ops = {
 
 static int ov2680_mode_init(struct ov2680_dev *sensor)
 {
-	const struct ov2680_mode_info *init_mode;
-
 	/* set initial mode */
 	ov2680_fill_format(sensor, &sensor->mode.fmt,
 			   OV2680_DEFAULT_WIDTH, OV2680_DEFAULT_HEIGHT);
+	ov2680_calc_mode(sensor);
 
 	sensor->mode.frame_interval.denominator = OV2680_FRAME_RATE;
 	sensor->mode.frame_interval.numerator = 1;
 
-	init_mode = &ov2680_mode_init_data;
-
-	sensor->current_mode = init_mode;
-
 	return 0;
 }
 
-- 
2.41.0

[PATCH v5 22/32] media: ov2680: Add an __ov2680_get_pad_format() helper function

[Hans de Goede]

Add an __ov2680_get_pad_format() helper function.

This is a preparation patch for adding selections support.

Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/ov2680.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index 012b95f90c1d..87c4c5ea47c9 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -220,6 +220,18 @@ static void ov2680_set_bayer_order(struct ov2680_dev *sensor,
 	fmt->code = ov2680_hv_flip_bayer_order[hv_flip];
 }
 
+static struct v4l2_mbus_framefmt *
+__ov2680_get_pad_format(struct ov2680_dev *sensor,
+			struct v4l2_subdev_state *state,
+			unsigned int pad,
+			enum v4l2_subdev_format_whence which)
+{
+	if (which == V4L2_SUBDEV_FORMAT_TRY)
+		return v4l2_subdev_get_try_format(&sensor->sd, state, pad);
+
+	return &sensor->mode.fmt;
+}
+
 static void ov2680_fill_format(struct ov2680_dev *sensor,
 			       struct v4l2_mbus_framefmt *fmt,
 			       unsigned int width, unsigned int height)
@@ -508,22 +520,16 @@ static int ov2680_get_fmt(struct v4l2_subdev *sd,
 			  struct v4l2_subdev_format *format)
 {
 	struct ov2680_dev *sensor = to_ov2680_dev(sd);
-	struct v4l2_mbus_framefmt *fmt = NULL;
+	struct v4l2_mbus_framefmt *fmt;
 
 	if (format->pad != 0)
 		return -EINVAL;
 
+	fmt = __ov2680_get_pad_format(sensor, sd_state, format->pad,
+				      format->which);
+
 	mutex_lock(&sensor->lock);
-
-	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
-		fmt = v4l2_subdev_get_try_format(&sensor->sd, sd_state,
-						 format->pad);
-	} else {
-		fmt = &sensor->mode.fmt;
-	}
-
 	format->format = *fmt;
-
 	mutex_unlock(&sensor->lock);
 
 	return 0;
-- 
2.41.0

[PATCH v5 23/32] media: ov2680: Implement selection support

[Hans de Goede]

Implement selection support. Modelled after ov5693 selection support,
but allow setting sizes smaller than crop-size through set_fmt() since
that was already allowed.

Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v4:
- Make enum_frame_size() take the crop target into account

Changes in v2:
- Use clamp_val() instead of clamp() / clamp_t()
---
 drivers/media/i2c/ov2680.c | 153 ++++++++++++++++++++++++++++++++++---
 1 file changed, 141 insertions(+), 12 deletions(-)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index 87c4c5ea47c9..ecd99e6669bd 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -65,8 +65,14 @@
 
 #define OV2680_NATIVE_WIDTH			1616
 #define OV2680_NATIVE_HEIGHT			1216
+#define OV2680_NATIVE_START_LEFT		0
+#define OV2680_NATIVE_START_TOP			0
 #define OV2680_ACTIVE_WIDTH			1600
 #define OV2680_ACTIVE_HEIGHT			1200
+#define OV2680_ACTIVE_START_LEFT		8
+#define OV2680_ACTIVE_START_TOP			8
+#define OV2680_MIN_CROP_WIDTH			2
+#define OV2680_MIN_CROP_HEIGHT			2
 
 /* 66MHz pixel clock: 66MHz / 1704 * 1294 = 30fps */
 #define OV2680_PIXELS_PER_LINE			1704
@@ -114,6 +120,7 @@ struct ov2680_ctrls {
 };
 
 struct ov2680_mode {
+	struct v4l2_rect		crop;
 	struct v4l2_mbus_framefmt	fmt;
 	struct v4l2_fract		frame_interval;
 	bool				binning;
@@ -147,6 +154,13 @@ struct ov2680_dev {
 	struct ov2680_mode		mode;
 };
 
+static const struct v4l2_rect ov2680_default_crop = {
+	.left = OV2680_ACTIVE_START_LEFT,
+	.top = OV2680_ACTIVE_START_TOP,
+	.width = OV2680_ACTIVE_WIDTH,
+	.height = OV2680_ACTIVE_HEIGHT,
+};
+
 static const char * const test_pattern_menu[] = {
 	"Disabled",
 	"Color Bars",
@@ -232,6 +246,18 @@ __ov2680_get_pad_format(struct ov2680_dev *sensor,
 	return &sensor->mode.fmt;
 }
 
+static struct v4l2_rect *
+__ov2680_get_pad_crop(struct ov2680_dev *sensor,
+		      struct v4l2_subdev_state *state,
+		      unsigned int pad,
+		      enum v4l2_subdev_format_whence which)
+{
+	if (which == V4L2_SUBDEV_FORMAT_TRY)
+		return v4l2_subdev_get_try_crop(&sensor->sd, state, pad);
+
+	return &sensor->mode.crop;
+}
+
 static void ov2680_fill_format(struct ov2680_dev *sensor,
 			       struct v4l2_mbus_framefmt *fmt,
 			       unsigned int width, unsigned int height)
@@ -251,8 +277,8 @@ static void ov2680_calc_mode(struct ov2680_dev *sensor)
 	int orig_width = width;
 	int orig_height = height;
 
-	if (width  <= (OV2680_NATIVE_WIDTH / 2) &&
-	    height <= (OV2680_NATIVE_HEIGHT / 2)) {
+	if (width  <= (sensor->mode.crop.width / 2) &&
+	    height <= (sensor->mode.crop.height / 2)) {
 		sensor->mode.binning = true;
 		width *= 2;
 		height *= 2;
@@ -260,8 +286,10 @@ static void ov2680_calc_mode(struct ov2680_dev *sensor)
 		sensor->mode.binning = false;
 	}
 
-	sensor->mode.h_start = ((OV2680_NATIVE_WIDTH - width) / 2) & ~1;
-	sensor->mode.v_start = ((OV2680_NATIVE_HEIGHT - height) / 2) & ~1;
+	sensor->mode.h_start = (sensor->mode.crop.left +
+				(sensor->mode.crop.width - width) / 2) & ~1;
+	sensor->mode.v_start = (sensor->mode.crop.top +
+				(sensor->mode.crop.height - height) / 2) & ~1;
 	sensor->mode.h_end =
 		min(sensor->mode.h_start + width + OV2680_END_MARGIN - 1,
 		    OV2680_NATIVE_WIDTH - 1);
@@ -541,16 +569,21 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
 {
 	struct ov2680_dev *sensor = to_ov2680_dev(sd);
 	struct v4l2_mbus_framefmt *try_fmt;
+	const struct v4l2_rect *crop;
 	unsigned int width, height;
 	int ret = 0;
 
 	if (format->pad != 0)
 		return -EINVAL;
 
-	width = min_t(unsigned int, ALIGN(format->format.width, 2),
-		      OV2680_NATIVE_WIDTH);
-	height = min_t(unsigned int, ALIGN(format->format.height, 2),
-		       OV2680_NATIVE_HEIGHT);
+	crop = __ov2680_get_pad_crop(sensor, sd_state, format->pad,
+				     format->which);
+
+	/* Limit set_fmt max size to crop width / height */
+	width = clamp_val(ALIGN(format->format.width, 2),
+			  OV2680_MIN_CROP_WIDTH, crop->width);
+	height = clamp_val(ALIGN(format->format.height, 2),
+			   OV2680_MIN_CROP_HEIGHT, crop->height);
 
 	ov2680_fill_format(sensor, &format->format, width, height);
 
@@ -576,11 +609,97 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
 	return ret;
 }
 
+static int ov2680_get_selection(struct v4l2_subdev *sd,
+				struct v4l2_subdev_state *state,
+				struct v4l2_subdev_selection *sel)
+{
+	struct ov2680_dev *sensor = to_ov2680_dev(sd);
+
+	switch (sel->target) {
+	case V4L2_SEL_TGT_CROP:
+		mutex_lock(&sensor->lock);
+		sel->r = *__ov2680_get_pad_crop(sensor, state, sel->pad,
+						sel->which);
+		mutex_unlock(&sensor->lock);
+		break;
+	case V4L2_SEL_TGT_NATIVE_SIZE:
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+		sel->r.top = 0;
+		sel->r.left = 0;
+		sel->r.width = OV2680_NATIVE_WIDTH;
+		sel->r.height = OV2680_NATIVE_HEIGHT;
+		break;
+	case V4L2_SEL_TGT_CROP_DEFAULT:
+		sel->r = ov2680_default_crop;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int ov2680_set_selection(struct v4l2_subdev *sd,
+				struct v4l2_subdev_state *state,
+				struct v4l2_subdev_selection *sel)
+{
+	struct ov2680_dev *sensor = to_ov2680_dev(sd);
+	struct v4l2_mbus_framefmt *format;
+	struct v4l2_rect *crop;
+	struct v4l2_rect rect;
+
+	if (sel->target != V4L2_SEL_TGT_CROP)
+		return -EINVAL;
+
+	/*
+	 * Clamp the boundaries of the crop rectangle to the size of the sensor
+	 * pixel array. Align to multiples of 2 to ensure Bayer pattern isn't
+	 * disrupted.
+	 */
+	rect.left = clamp_val(ALIGN(sel->r.left, 2),
+			      OV2680_NATIVE_START_LEFT, OV2680_NATIVE_WIDTH);
+	rect.top = clamp_val(ALIGN(sel->r.top, 2),
+			     OV2680_NATIVE_START_TOP, OV2680_NATIVE_HEIGHT);
+	rect.width = clamp_val(ALIGN(sel->r.width, 2),
+			       OV2680_MIN_CROP_WIDTH, OV2680_NATIVE_WIDTH);
+	rect.height = clamp_val(ALIGN(sel->r.height, 2),
+				OV2680_MIN_CROP_HEIGHT, OV2680_NATIVE_HEIGHT);
+
+	/* Make sure the crop rectangle isn't outside the bounds of the array */
+	rect.width = min_t(unsigned int, rect.width,
+			   OV2680_NATIVE_WIDTH - rect.left);
+	rect.height = min_t(unsigned int, rect.height,
+			    OV2680_NATIVE_HEIGHT - rect.top);
+
+	crop = __ov2680_get_pad_crop(sensor, state, sel->pad, sel->which);
+
+	mutex_lock(&sensor->lock);
+	if (rect.width != crop->width || rect.height != crop->height) {
+		/*
+		 * Reset the output image size if the crop rectangle size has
+		 * been modified.
+		 */
+		format = __ov2680_get_pad_format(sensor, state, sel->pad,
+						 sel->which);
+		format->width = rect.width;
+		format->height = rect.height;
+	}
+
+	*crop = rect;
+	mutex_unlock(&sensor->lock);
+
+	sel->r = rect;
+
+	return 0;
+}
+
 static int ov2680_init_cfg(struct v4l2_subdev *sd,
 			   struct v4l2_subdev_state *sd_state)
 {
 	struct ov2680_dev *sensor = to_ov2680_dev(sd);
 
+	sd_state->pads[0].try_crop = ov2680_default_crop;
+
 	ov2680_fill_format(sensor, &sd_state->pads[0].try_fmt,
 			   OV2680_DEFAULT_WIDTH, OV2680_DEFAULT_HEIGHT);
 	return 0;
@@ -590,11 +709,18 @@ static int ov2680_enum_frame_size(struct v4l2_subdev *sd,
 				  struct v4l2_subdev_state *sd_state,
 				  struct v4l2_subdev_frame_size_enum *fse)
 {
+	struct ov2680_dev *sensor = to_ov2680_dev(sd);
+	struct v4l2_rect *crop;
+
 	if (fse->index >= OV2680_FRAME_SIZES)
 		return -EINVAL;
 
-	fse->min_width = OV2680_ACTIVE_WIDTH / (fse->index + 1);
-	fse->min_height = OV2680_ACTIVE_HEIGHT / (fse->index + 1);
+	crop = __ov2680_get_pad_crop(sensor, sd_state, fse->pad, fse->which);
+	if (!crop)
+		return -EINVAL;
+
+	fse->min_width = crop->width / (fse->index + 1);
+	fse->min_height = crop->height / (fse->index + 1);
 	fse->max_width = fse->min_width;
 	fse->max_height = fse->min_height;
 
@@ -690,10 +816,12 @@ static const struct v4l2_subdev_video_ops ov2680_video_ops = {
 static const struct v4l2_subdev_pad_ops ov2680_pad_ops = {
 	.init_cfg		= ov2680_init_cfg,
 	.enum_mbus_code		= ov2680_enum_mbus_code,
-	.get_fmt		= ov2680_get_fmt,
-	.set_fmt		= ov2680_set_fmt,
 	.enum_frame_size	= ov2680_enum_frame_size,
 	.enum_frame_interval	= ov2680_enum_frame_interval,
+	.get_fmt		= ov2680_get_fmt,
+	.set_fmt		= ov2680_set_fmt,
+	.get_selection		= ov2680_get_selection,
+	.set_selection		= ov2680_set_selection,
 };
 
 static const struct v4l2_subdev_ops ov2680_subdev_ops = {
@@ -704,6 +832,7 @@ static const struct v4l2_subdev_ops ov2680_subdev_ops = {
 static int ov2680_mode_init(struct ov2680_dev *sensor)
 {
 	/* set initial mode */
+	sensor->mode.crop = ov2680_default_crop;
 	ov2680_fill_format(sensor, &sensor->mode.fmt,
 			   OV2680_DEFAULT_WIDTH, OV2680_DEFAULT_HEIGHT);
 	ov2680_calc_mode(sensor);
-- 
2.41.0

[PATCH v5 24/32] media: ov2680: Fix exposure and gain ctrls range and default value

[Hans de Goede]

The exposure control's max effective value is VTS - 8, set the control
range to match this. Thas means that if/when a future commit makes VTS
configurable as a control itself the exposure range needs to be
updated dynamically to match the VTS value.

The gain control goes from 0 - 1023, higher values wrap around to
the lowest gain setting.

The gain control, controls an analog gain so use V4L2_CID_ANALOGUE_GAIN
for it instead of V4L2_CID_GAIN.

Also stop setting 0 as default for both controls this leads to
a totally black picture which is no good. This is esp. important
for tests of the sensor driver without (userspace driven)
auto exposure / gain.

Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v4:
- Use V4L2_CID_ANALOGUE_GAIN
---
 drivers/media/i2c/ov2680.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index ecd99e6669bd..b251265594b0 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -81,6 +81,9 @@
 /* If possible send 16 extra rows / lines to the ISP as padding */
 #define OV2680_END_MARGIN			16
 
+/* Max exposure time is VTS - 8 */
+#define OV2680_INTEGRATION_TIME_MARGIN		8
+
 #define OV2680_DEFAULT_WIDTH			800
 #define OV2680_DEFAULT_HEIGHT			600
 
@@ -779,7 +782,7 @@ static int ov2680_s_ctrl(struct v4l2_ctrl *ctrl)
 	}
 
 	switch (ctrl->id) {
-	case V4L2_CID_GAIN:
+	case V4L2_CID_ANALOGUE_GAIN:
 		ret = ov2680_gain_set(sensor, ctrl->val);
 		break;
 	case V4L2_CID_EXPOSURE:
@@ -849,6 +852,7 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor)
 	const struct v4l2_ctrl_ops *ops = &ov2680_ctrl_ops;
 	struct ov2680_ctrls *ctrls = &sensor->ctrls;
 	struct v4l2_ctrl_handler *hdl = &ctrls->handler;
+	int exp_max = OV2680_LINES_PER_FRAME - OV2680_INTEGRATION_TIME_MARGIN;
 	int ret = 0;
 
 	v4l2_i2c_subdev_init(&sensor->sd, client, &ov2680_subdev_ops);
@@ -874,9 +878,10 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor)
 					0, 0, test_pattern_menu);
 
 	ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE,
-					    0, 32767, 1, 0);
+					    0, exp_max, 1, exp_max);
 
-	ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN, 0, 2047, 1, 0);
+	ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN,
+					0, 1023, 1, 250);
 
 	if (hdl->error) {
 		ret = hdl->error;
-- 
2.41.0

[PATCH v5 25/32] media: ov2680: Add a bunch of register tweaks

[Hans de Goede]

Usually when developing a sensor driver with help from the vendor
the vendor will provide a bunch of register tweaks for optimal
performance of the sensor.

The atomisp-ov2680.c driver was (presumably) developed by Intel with
help from OmniVision and indeed contains a bunch of register tweaks.

Add these register tweaks to the "main" ov2680.c driver.

Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/ov2680.c | 53 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index b251265594b0..e6c99c14da8f 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -180,18 +180,71 @@ static const int ov2680_hv_flip_bayer_order[] = {
 };
 
 static const struct reg_sequence ov2680_global_setting[] = {
+	/* MIPI PHY, 0x10 -> 0x1c enable bp_c_hs_en_lat and bp_d_hs_en_lat */
+	{0x3016, 0x1c},
+
 	/* R MANUAL set exposure and gain to manual (hw does not do auto) */
 	{0x3503, 0x03},
 
+	/* 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},
+
 	/* 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},
+
+	/* R ISP CTRL2 0x20 -> 0x30, set sof_sel bit */
+	{0x5002, 0x30},
+
 	/*
 	 * 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},
+
+	/* DPC GAIN LIST1 0x0f (15) -> 0x08 (8) */
+	{0x578e, 0x08},
+
+	/* DPC GAIN LIST2 0x3f (63) -> 0x0c (12) */
+	{0x578f, 0x0c},
+
+	/* DPC THRE RATIO 0x04 (4) -> 0x00 (0) */
+	{0x5792, 0x00},
 };
 
 static struct ov2680_dev *to_ov2680_dev(struct v4l2_subdev *sd)
-- 
2.41.0

[PATCH v5 26/32] media: ov2680: Drop unnecessary pad checks

[Hans de Goede]

Drop unnecessary pad checks in enum_mbus_code, get_fmt, set_fmt
this is already checked by check_pad() from
drivers/media/v4l2-core/v4l2-subdev.c.

Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/ov2680.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index e6c99c14da8f..c09a0e7f7787 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -591,7 +591,7 @@ static int ov2680_enum_mbus_code(struct v4l2_subdev *sd,
 {
 	struct ov2680_dev *sensor = to_ov2680_dev(sd);
 
-	if (code->pad != 0 || code->index != 0)
+	if (code->index != 0)
 		return -EINVAL;
 
 	code->code = sensor->mode.fmt.code;
@@ -606,9 +606,6 @@ static int ov2680_get_fmt(struct v4l2_subdev *sd,
 	struct ov2680_dev *sensor = to_ov2680_dev(sd);
 	struct v4l2_mbus_framefmt *fmt;
 
-	if (format->pad != 0)
-		return -EINVAL;
-
 	fmt = __ov2680_get_pad_format(sensor, sd_state, format->pad,
 				      format->which);
 
@@ -629,9 +626,6 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
 	unsigned int width, height;
 	int ret = 0;
 
-	if (format->pad != 0)
-		return -EINVAL;
-
 	crop = __ov2680_get_pad_crop(sensor, sd_state, format->pad,
 				     format->which);
 
-- 
2.41.0

[PATCH v5 27/32] media: ov2680: Read and log sensor revision during probe

[Hans de Goede]

Read and log sensor revision during probe.

Since this means that the driver will now already log a message on
successful probe drop the "ov2680 init correctly" log message.

Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/ov2680.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index c09a0e7f7787..1f59013e440c 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -33,6 +33,7 @@
 #define OV2680_REG_SOFT_RESET			CCI_REG8(0x0103)
 
 #define OV2680_REG_CHIP_ID			CCI_REG16(0x300a)
+#define OV2680_REG_SC_CMMN_SUB_ID		CCI_REG8(0x302a)
 #define OV2680_REG_PLL_MULTIPLIER		CCI_REG16(0x3081)
 
 #define OV2680_REG_EXPOSURE_PK			CCI_REG24(0x3500)
@@ -966,13 +967,14 @@ static int ov2680_get_regulators(struct ov2680_dev *sensor)
 
 static int ov2680_check_id(struct ov2680_dev *sensor)
 {
-	u64 chip_id;
-	int ret;
+	u64 chip_id, rev;
+	int ret = 0;
 
-	ret = cci_read(sensor->regmap, OV2680_REG_CHIP_ID, &chip_id, NULL);
+	cci_read(sensor->regmap, OV2680_REG_CHIP_ID, &chip_id, &ret);
+	cci_read(sensor->regmap, OV2680_REG_SC_CMMN_SUB_ID, &rev, &ret);
 	if (ret < 0) {
 		dev_err(sensor->dev, "failed to read chip id\n");
-		return -ENODEV;
+		return ret;
 	}
 
 	if (chip_id != OV2680_CHIP_ID) {
@@ -981,6 +983,9 @@ static int ov2680_check_id(struct ov2680_dev *sensor)
 		return -ENODEV;
 	}
 
+	dev_info(sensor->dev, "sensor_revision id = 0x%llx, rev= %lld\n",
+		 chip_id, rev & 0x0f);
+
 	return 0;
 }
 
@@ -1121,8 +1126,6 @@ static int ov2680_probe(struct i2c_client *client)
 	pm_runtime_use_autosuspend(&client->dev);
 	pm_runtime_put_autosuspend(&client->dev);
 
-	dev_info(dev, "ov2680 init correctly\n");
-
 	return 0;
 
 err_pm_runtime:
-- 
2.41.0

[PATCH v5 28/32] media: ov2680: Add link-freq and pixel-rate controls

[Hans de Goede]

Add read-only link-freq and pixel-rate controls. This is necessary for
the sensor to work with the ipu3-cio2 driver and for libcamera.

Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v5:
- Make pixel_rate u64 instead of s64 for do_div() to fix:
  drivers/media/i2c/ov2680.c:1092:2: warning: comparison of distinct pointer types ('typeof ((sensor->pixel_rate)) *' (aka 'long long *') and 'uint64_t *' (aka 'unsigned long long *')) [-Wcompare-distinct-pointer-types]

Changes in v4:
- Use do_div to calculate pixel_rate to avoid unresolved __divdi3 symbol
---
 drivers/media/i2c/ov2680.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index 1f59013e440c..83ec034b5307 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -75,6 +75,12 @@
 #define OV2680_MIN_CROP_WIDTH			2
 #define OV2680_MIN_CROP_HEIGHT			2
 
+/* Fixed pre-div of 1/2 */
+#define OV2680_PLL_PREDIV0			2
+
+/* Pre-div configurable through reg 0x3080, left at its default of 0x02 : 1/2 */
+#define OV2680_PLL_PREDIV			2
+
 /* 66MHz pixel clock: 66MHz / 1704 * 1294 = 30fps */
 #define OV2680_PIXELS_PER_LINE			1704
 #define OV2680_LINES_PER_FRAME			1294
@@ -121,6 +127,8 @@ struct ov2680_ctrls {
 	struct v4l2_ctrl *hflip;
 	struct v4l2_ctrl *vflip;
 	struct v4l2_ctrl *test_pattern;
+	struct v4l2_ctrl *link_freq;
+	struct v4l2_ctrl *pixel_rate;
 };
 
 struct ov2680_mode {
@@ -147,6 +155,8 @@ struct ov2680_dev {
 	struct clk			*xvclk;
 	u32				xvclk_freq;
 	u8				pll_mult;
+	s64				link_freq[1];
+	u64				pixel_rate;
 	struct regulator_bulk_data	supplies[OV2680_NUM_SUPPLIES];
 
 	struct gpio_desc		*pwdn_gpio;
@@ -931,6 +941,12 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor)
 	ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN,
 					0, 1023, 1, 250);
 
+	ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, NULL, V4L2_CID_LINK_FREQ,
+						  0, 0, sensor->link_freq);
+	ctrls->pixel_rate = v4l2_ctrl_new_std(hdl, NULL, V4L2_CID_PIXEL_RATE,
+					      0, sensor->pixel_rate,
+					      1, sensor->pixel_rate);
+
 	if (hdl->error) {
 		ret = hdl->error;
 		goto cleanup_entity;
@@ -938,6 +954,7 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor)
 
 	ctrls->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
 	ctrls->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
+	ctrls->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
 	sensor->sd.ctrl_handler = hdl;
 
@@ -1067,6 +1084,13 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor)
 
 	sensor->pll_mult = ov2680_pll_multipliers[i];
 
+	sensor->link_freq[0] = sensor->xvclk_freq / OV2680_PLL_PREDIV0 /
+			       OV2680_PLL_PREDIV * sensor->pll_mult;
+
+	/* CSI-2 is double data rate, bus-format is 10 bpp */
+	sensor->pixel_rate = sensor->link_freq[0] * 2;
+	do_div(sensor->pixel_rate, 10);
+
 	return 0;
 }
 
-- 
2.41.0

[PATCH v5 29/32] media: ov2680: Add bus-cfg / endpoint property verification

[Hans de Goede]

Verify that the number of CSI lanes and link-frequency specified
in the endpoint fwnode are correct.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v4:
- New patch in v4 of this patch-set
---
 drivers/media/i2c/ov2680.c | 60 +++++++++++++++++++++++++++++---------
 1 file changed, 47 insertions(+), 13 deletions(-)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index 83ec034b5307..72bab0ff8a36 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -25,6 +25,7 @@
 #include <media/v4l2-cci.h>
 #include <media/v4l2-common.h>
 #include <media/v4l2-ctrls.h>
+#include <media/v4l2-fwnode.h>
 #include <media/v4l2-subdev.h>
 
 #define OV2680_CHIP_ID				0x2680
@@ -1008,6 +1009,9 @@ static int ov2680_check_id(struct ov2680_dev *sensor)
 
 static int ov2680_parse_dt(struct ov2680_dev *sensor)
 {
+	struct v4l2_fwnode_endpoint bus_cfg = {
+		.bus_type = V4L2_MBUS_CSI2_DPHY,
+	};
 	struct device *dev = sensor->dev;
 	struct fwnode_handle *ep_fwnode;
 	struct gpio_desc *gpio;
@@ -1023,7 +1027,10 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor)
 		return dev_err_probe(dev, -EPROBE_DEFER,
 				     "waiting for fwnode graph endpoint\n");
 
+	ret = v4l2_fwnode_endpoint_alloc_parse(ep_fwnode, &bus_cfg);
 	fwnode_handle_put(ep_fwnode);
+	if (ret)
+		return ret;
 
 	/*
 	 * The pin we want is named XSHUTDN in the datasheet. Linux sensor
@@ -1038,15 +1045,16 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor)
 	ret = PTR_ERR_OR_ZERO(gpio);
 	if (ret < 0) {
 		dev_dbg(dev, "error while getting reset gpio: %d\n", ret);
-		return ret;
+		goto out_free_bus_cfg;
 	}
 
 	sensor->pwdn_gpio = gpio;
 
 	sensor->xvclk = devm_clk_get_optional(dev, "xvclk");
 	if (IS_ERR(sensor->xvclk)) {
-		dev_err(dev, "xvclk clock missing or invalid\n");
-		return PTR_ERR(sensor->xvclk);
+		ret = dev_err_probe(dev, PTR_ERR(sensor->xvclk),
+				    "xvclk clock missing or invalid\n");
+		goto out_free_bus_cfg;
 	}
 
 	/*
@@ -1060,14 +1068,17 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor)
 	 */
 	ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
 				       &rate);
-	if (ret && !sensor->xvclk)
-		return dev_err_probe(dev, ret, "invalid clock config\n");
+	if (ret && !sensor->xvclk) {
+		dev_err_probe(dev, ret, "invalid clock config\n");
+		goto out_free_bus_cfg;
+	}
 
 	if (!ret && sensor->xvclk) {
 		ret = clk_set_rate(sensor->xvclk, rate);
-		if (ret)
-			return dev_err_probe(dev, ret,
-					     "failed to set clock rate\n");
+		if (ret) {
+			dev_err_probe(dev, ret, "failed to set clock rate\n");
+			goto out_free_bus_cfg;
+		}
 	}
 
 	sensor->xvclk_freq = rate ?: clk_get_rate(sensor->xvclk);
@@ -1077,10 +1088,12 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor)
 			break;
 	}
 
-	if (i == ARRAY_SIZE(ov2680_xvclk_freqs))
-		return dev_err_probe(dev, -EINVAL,
-				     "unsupported xvclk frequency %d Hz\n",
-				     sensor->xvclk_freq);
+	if (i == ARRAY_SIZE(ov2680_xvclk_freqs)) {
+		ret = dev_err_probe(dev, -EINVAL,
+				    "unsupported xvclk frequency %d Hz\n",
+				    sensor->xvclk_freq);
+		goto out_free_bus_cfg;
+	}
 
 	sensor->pll_mult = ov2680_pll_multipliers[i];
 
@@ -1091,7 +1104,28 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor)
 	sensor->pixel_rate = sensor->link_freq[0] * 2;
 	do_div(sensor->pixel_rate, 10);
 
-	return 0;
+	/* Verify bus cfg */
+	if (bus_cfg.bus.mipi_csi2.num_data_lanes != 1) {
+		ret = dev_err_probe(dev, -EINVAL,
+				    "only a 1-lane CSI2 config is supported");
+		goto out_free_bus_cfg;
+	}
+
+	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
+		if (bus_cfg.link_frequencies[i] == sensor->link_freq[0])
+			break;
+
+	if (bus_cfg.nr_of_link_frequencies == 0 ||
+	    bus_cfg.nr_of_link_frequencies == i) {
+		ret = dev_err_probe(dev, -EINVAL,
+				    "supported link freq %lld not found\n",
+				    sensor->link_freq[0]);
+		goto out_free_bus_cfg;
+	}
+
+out_free_bus_cfg:
+	v4l2_fwnode_endpoint_free(&bus_cfg);
+	return ret;
 }
 
 static int ov2680_probe(struct i2c_client *client)
-- 
2.41.0

[PATCH v5 30/32] MAINTAINERS: Add Hans de Goede as OV2680 sensor driver maintainer

[Hans de Goede]

Add myself as OV2680 sensor driver maintainer.

Reviewed-by: Rui Miguel Silva <rmfrfs@gmail.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 0600c2f29080..9369a35c23a3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15608,6 +15608,7 @@ F:	drivers/media/i2c/ov13b10.c
 
 OMNIVISION OV2680 SENSOR DRIVER
 M:	Rui Miguel Silva <rmfrfs@gmail.com>
+M:	Hans de Goede <hansg@kernel.org>
 L:	linux-media@vger.kernel.org
 S:	Maintained
 T:	git git://linuxtv.org/media_tree.git
-- 
2.41.0

[PATCH v5 31/32] media: ipu-bridge: Add link-frequency to OV2680 ipu_supported_sensors[] entry

[Hans de Goede]

Now that the ov2680 driver verifies the bus-cfg from the endpoint
fwnode the link-frequency must be set for things to work.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/pci/intel/ipu-bridge.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index 940457940057..be9ab2ab6b8d 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -30,7 +30,7 @@ static const struct ipu_sensor_config ipu_supported_sensors[] = {
 	/* Omnivision OV7251 */
 	IPU_SENSOR_CONFIG("INT347E", 1, 319200000),
 	/* Omnivision OV2680 */
-	IPU_SENSOR_CONFIG("OVTI2680", 0),
+	IPU_SENSOR_CONFIG("OVTI2680", 1, 331200000),
 	/* Omnivision ov8856 */
 	IPU_SENSOR_CONFIG("OVTI8856", 3, 180000000, 360000000, 720000000),
 	/* Omnivision ov2740 */
-- 
2.41.0

[PATCH v5 32/32] media: atomisp: Drop atomisp-ov2680 sensor driver

[Hans de Goede]

After recent improvements to atomisp and the standard ov2680
sensor driver, the atomisp driver now works fine with
the standard ov2680 driver.

Drop the no longer necessary atomisp specific atomisp-ov2680
sensor driver.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/i2c/Kconfig     |  13 -
 drivers/staging/media/atomisp/i2c/Makefile    |   1 -
 .../media/atomisp/i2c/atomisp-ov2680.c        | 828 ------------------
 drivers/staging/media/atomisp/i2c/ov2680.h    | 173 ----
 4 files changed, 1015 deletions(-)
 delete mode 100644 drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
 delete mode 100644 drivers/staging/media/atomisp/i2c/ov2680.h

diff --git a/drivers/staging/media/atomisp/i2c/Kconfig b/drivers/staging/media/atomisp/i2c/Kconfig
index e34646d7dadc..2d4165cda2f1 100644
--- a/drivers/staging/media/atomisp/i2c/Kconfig
+++ b/drivers/staging/media/atomisp/i2c/Kconfig
@@ -57,19 +57,6 @@ config VIDEO_ATOMISP_GC0310
 	  This is a Video4Linux2 sensor-level driver for the Galaxycore
 	  GC0310 0.3MP sensor.
 
-config VIDEO_ATOMISP_OV2680
-	tristate "Omnivision OV2680 sensor support"
-	depends on ACPI
-	depends on I2C && VIDEO_DEV
-	select V4L2_CCI_I2C
-	help
-	  This is a Video4Linux2 sensor-level driver for the Omnivision
-	  OV2680 raw camera.
-
-	  ov2680 is a 2M raw sensor.
-
-	  It currently only works with the atomisp driver.
-
 config VIDEO_ATOMISP_OV5693
 	tristate "Omnivision ov5693 sensor support"
 	depends on ACPI
diff --git a/drivers/staging/media/atomisp/i2c/Makefile b/drivers/staging/media/atomisp/i2c/Makefile
index 8d022986e199..fc55af5f3422 100644
--- a/drivers/staging/media/atomisp/i2c/Makefile
+++ b/drivers/staging/media/atomisp/i2c/Makefile
@@ -7,7 +7,6 @@ obj-$(CONFIG_VIDEO_ATOMISP_OV5693)     += ov5693/
 obj-$(CONFIG_VIDEO_ATOMISP_MT9M114)    += atomisp-mt9m114.o
 obj-$(CONFIG_VIDEO_ATOMISP_GC2235)     += atomisp-gc2235.o
 obj-$(CONFIG_VIDEO_ATOMISP_OV2722)     += atomisp-ov2722.o
-obj-$(CONFIG_VIDEO_ATOMISP_OV2680)     += atomisp-ov2680.o
 obj-$(CONFIG_VIDEO_ATOMISP_GC0310)     += atomisp-gc0310.o
 
 obj-$(CONFIG_VIDEO_ATOMISP_MSRLIST_HELPER) += atomisp-libmsrlisthelper.o
diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
deleted file mode 100644
index f933a65ac8d4..000000000000
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
+++ /dev/null
@@ -1,828 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * Support for OmniVision OV2680 1080p HD camera sensor.
- *
- * Copyright (c) 2013 Intel Corporation. All Rights Reserved.
- * Copyright (c) 2023 Hans de Goede <hdegoede@redhat.com>
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License version
- * 2 as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- */
-
-#include <linux/acpi.h>
-#include <linux/device.h>
-#include <linux/gpio/consumer.h>
-#include <linux/gpio/machine.h>
-#include <linux/i2c.h>
-#include <linux/module.h>
-#include <linux/pm_runtime.h>
-#include <linux/regmap.h>
-#include <linux/types.h>
-
-#include <media/v4l2-device.h>
-
-#include "ov2680.h"
-
-#define OV2680_CHIP_ID				0x2680
-
-#define OV2680_REG_STREAM_CTRL			CCI_REG8(0x0100)
-#define OV2680_REG_SOFT_RESET			CCI_REG8(0x0103)
-
-#define OV2680_REG_CHIP_ID			CCI_REG16(0x300a)
-#define OV2680_REG_SC_CMMN_SUB_ID		CCI_REG8(0x302a)
-
-#define OV2680_REG_EXPOSURE_PK			CCI_REG24(0x3500)
-#define OV2680_REG_R_MANUAL			CCI_REG8(0x3503)
-#define OV2680_REG_GAIN_PK			CCI_REG16(0x350a)
-
-#define OV2680_REG_SENSOR_CTRL_0A		CCI_REG8(0x370a)
-
-#define OV2680_REG_HORIZONTAL_START		CCI_REG16(0x3800)
-#define OV2680_REG_VERTICAL_START		CCI_REG16(0x3802)
-#define OV2680_REG_HORIZONTAL_END		CCI_REG16(0x3804)
-#define OV2680_REG_VERTICAL_END			CCI_REG16(0x3806)
-#define OV2680_REG_HORIZONTAL_OUTPUT_SIZE	CCI_REG16(0x3808)
-#define OV2680_REG_VERTICAL_OUTPUT_SIZE		CCI_REG16(0x380a)
-#define OV2680_REG_TIMING_HTS			CCI_REG16(0x380c)
-#define OV2680_REG_TIMING_VTS			CCI_REG16(0x380e)
-#define OV2680_REG_ISP_X_WIN			CCI_REG16(0x3810)
-#define OV2680_REG_ISP_Y_WIN			CCI_REG16(0x3812)
-#define OV2680_REG_X_INC			CCI_REG8(0x3814)
-#define OV2680_REG_Y_INC			CCI_REG8(0x3815)
-#define OV2680_REG_FORMAT1			CCI_REG8(0x3820)
-#define OV2680_REG_FORMAT2			CCI_REG8(0x3821)
-
-#define OV2680_REG_ISP_CTRL00			CCI_REG8(0x5080)
-
-#define OV2680_REG_X_WIN			CCI_REG16(0x5704)
-#define OV2680_REG_Y_WIN			CCI_REG16(0x5706)
-
-#define OV2680_FRAME_RATE			30
-#define OV2680_INTEGRATION_TIME_MARGIN		8
-
-static const struct v4l2_rect ov2680_default_crop = {
-	.left = OV2680_ACTIVE_START_LEFT,
-	.top = OV2680_ACTIVE_START_TOP,
-	.width = OV2680_ACTIVE_WIDTH,
-	.height = OV2680_ACTIVE_HEIGHT,
-};
-
-static void ov2680_set_bayer_order(struct ov2680_dev *sensor, struct v4l2_mbus_framefmt *fmt)
-{
-	static const int ov2680_hv_flip_bayer_order[] = {
-		MEDIA_BUS_FMT_SBGGR10_1X10,
-		MEDIA_BUS_FMT_SGRBG10_1X10,
-		MEDIA_BUS_FMT_SGBRG10_1X10,
-		MEDIA_BUS_FMT_SRGGB10_1X10,
-	};
-	int hv_flip = 0;
-
-	if (sensor->ctrls.vflip->val)
-		hv_flip += 1;
-
-	if (sensor->ctrls.hflip->val)
-		hv_flip += 2;
-
-	fmt->code = ov2680_hv_flip_bayer_order[hv_flip];
-}
-
-static int ov2680_set_vflip(struct ov2680_dev *sensor, s32 val)
-{
-	int ret;
-
-	if (sensor->is_streaming)
-		return -EBUSY;
-
-	ret = cci_update_bits(sensor->regmap, OV2680_REG_FORMAT1, BIT(2),
-			      val ? BIT(2) : 0, NULL);
-	if (ret < 0)
-		return ret;
-
-	ov2680_set_bayer_order(sensor, &sensor->mode.fmt);
-	return 0;
-}
-
-static int ov2680_set_hflip(struct ov2680_dev *sensor, s32 val)
-{
-	int ret;
-
-	if (sensor->is_streaming)
-		return -EBUSY;
-
-	ret = cci_update_bits(sensor->regmap, OV2680_REG_FORMAT2, BIT(2),
-			      val ? BIT(2) : 0, NULL);
-	if (ret < 0)
-		return ret;
-
-	ov2680_set_bayer_order(sensor, &sensor->mode.fmt);
-	return 0;
-}
-
-static int ov2680_exposure_set(struct ov2680_dev *sensor, u32 exp)
-{
-	return cci_write(sensor->regmap, OV2680_REG_EXPOSURE_PK, exp << 4,
-			 NULL);
-}
-
-static int ov2680_gain_set(struct ov2680_dev *sensor, u32 gain)
-{
-	return cci_write(sensor->regmap, OV2680_REG_GAIN_PK, gain, NULL);
-}
-
-static int ov2680_test_pattern_set(struct ov2680_dev *sensor, int value)
-{
-	int ret = 0;
-
-	if (!value)
-		return cci_update_bits(sensor->regmap, OV2680_REG_ISP_CTRL00,
-				       BIT(7), 0, NULL);
-
-	cci_update_bits(sensor->regmap, OV2680_REG_ISP_CTRL00, 0x03, value - 1,
-			&ret);
-	cci_update_bits(sensor->regmap, OV2680_REG_ISP_CTRL00, BIT(7), BIT(7),
-			&ret);
-
-	return ret;
-}
-
-static int ov2680_s_ctrl(struct v4l2_ctrl *ctrl)
-{
-	struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
-	struct ov2680_dev *sensor = to_ov2680_sensor(sd);
-	int ret;
-
-	/* Only apply changes to the controls if the device is powered up */
-	if (!pm_runtime_get_if_in_use(sensor->sd.dev)) {
-		ov2680_set_bayer_order(sensor, &sensor->mode.fmt);
-		return 0;
-	}
-
-	switch (ctrl->id) {
-	case V4L2_CID_VFLIP:
-		ret = ov2680_set_vflip(sensor, ctrl->val);
-		break;
-	case V4L2_CID_HFLIP:
-		ret = ov2680_set_hflip(sensor, ctrl->val);
-		break;
-	case V4L2_CID_EXPOSURE:
-		ret = ov2680_exposure_set(sensor, ctrl->val);
-		break;
-	case V4L2_CID_GAIN:
-		ret = ov2680_gain_set(sensor, ctrl->val);
-		break;
-	case V4L2_CID_TEST_PATTERN:
-		ret = ov2680_test_pattern_set(sensor, ctrl->val);
-		break;
-	default:
-		ret = -EINVAL;
-	}
-
-	pm_runtime_put(sensor->sd.dev);
-	return ret;
-}
-
-static const struct v4l2_ctrl_ops ov2680_ctrl_ops = {
-	.s_ctrl = ov2680_s_ctrl,
-};
-
-static int ov2680_init_registers(struct v4l2_subdev *sd)
-{
-	struct ov2680_dev *sensor = to_ov2680_sensor(sd);
-	int ret;
-
-	ret = cci_write(sensor->regmap, OV2680_REG_SOFT_RESET, 0x01, NULL);
-	if (ret < 0)
-		return ret;
-
-	/* Wait for sensor reset */
-	usleep_range(1000, 2000);
-
-	return regmap_multi_reg_write(sensor->regmap, ov2680_global_setting,
-				      ARRAY_SIZE(ov2680_global_setting));
-}
-
-static struct v4l2_mbus_framefmt *
-__ov2680_get_pad_format(struct ov2680_dev *sensor, struct v4l2_subdev_state *state,
-			unsigned int pad, enum v4l2_subdev_format_whence which)
-{
-	if (which == V4L2_SUBDEV_FORMAT_TRY)
-		return v4l2_subdev_get_try_format(&sensor->sd, state, pad);
-
-	return &sensor->mode.fmt;
-}
-
-static struct v4l2_rect *
-__ov2680_get_pad_crop(struct ov2680_dev *sensor, struct v4l2_subdev_state *state,
-		      unsigned int pad, enum v4l2_subdev_format_whence which)
-{
-	if (which == V4L2_SUBDEV_FORMAT_TRY)
-		return v4l2_subdev_get_try_crop(&sensor->sd, state, pad);
-
-	return &sensor->mode.crop;
-}
-
-static void ov2680_fill_format(struct ov2680_dev *sensor,
-			       struct v4l2_mbus_framefmt *fmt,
-			       unsigned int width, unsigned int height)
-{
-	memset(fmt, 0, sizeof(*fmt));
-	fmt->width = width;
-	fmt->height = height;
-	fmt->field = V4L2_FIELD_NONE;
-	ov2680_set_bayer_order(sensor, fmt);
-}
-
-static void ov2680_calc_mode(struct ov2680_dev *sensor)
-{
-	int width = sensor->mode.fmt.width;
-	int height = sensor->mode.fmt.height;
-	int orig_width = width;
-	int orig_height = height;
-
-	if (width  <= (sensor->mode.crop.width / 2) &&
-	    height <= (sensor->mode.crop.height / 2)) {
-		sensor->mode.binning = true;
-		width *= 2;
-		height *= 2;
-	} else {
-		sensor->mode.binning = false;
-	}
-
-	sensor->mode.h_start =
-		(sensor->mode.crop.left + (sensor->mode.crop.width - width) / 2) & ~1;
-	sensor->mode.v_start =
-		(sensor->mode.crop.top + (sensor->mode.crop.height - height) / 2) & ~1;
-	sensor->mode.h_end = min(sensor->mode.h_start + width + OV2680_END_MARGIN - 1,
-				 OV2680_NATIVE_WIDTH - 1);
-	sensor->mode.v_end = min(sensor->mode.v_start + height + OV2680_END_MARGIN - 1,
-				 OV2680_NATIVE_HEIGHT - 1);
-	sensor->mode.h_output_size = orig_width;
-	sensor->mode.v_output_size = orig_height;
-	sensor->mode.hts = OV2680_PIXELS_PER_LINE;
-	sensor->mode.vts = OV2680_LINES_PER_FRAME;
-}
-
-static int ov2680_set_mode(struct ov2680_dev *sensor)
-{
-	u8 sensor_ctrl_0a, inc, fmt1, fmt2;
-	int ret = 0;
-
-	if (sensor->mode.binning) {
-		sensor_ctrl_0a = 0x23;
-		inc = 0x31;
-		fmt1 = 0xc2;
-		fmt2 = 0x01;
-	} else {
-		sensor_ctrl_0a = 0x21;
-		inc = 0x11;
-		fmt1 = 0xc0;
-		fmt2 = 0x00;
-	}
-
-	cci_write(sensor->regmap, OV2680_REG_SENSOR_CTRL_0A,
-		  sensor_ctrl_0a, &ret);
-	cci_write(sensor->regmap, OV2680_REG_HORIZONTAL_START,
-		  sensor->mode.h_start, &ret);
-	cci_write(sensor->regmap, OV2680_REG_VERTICAL_START,
-		  sensor->mode.v_start, &ret);
-	cci_write(sensor->regmap, OV2680_REG_HORIZONTAL_END,
-		  sensor->mode.h_end, &ret);
-	cci_write(sensor->regmap, OV2680_REG_VERTICAL_END,
-		  sensor->mode.v_end, &ret);
-	cci_write(sensor->regmap, OV2680_REG_HORIZONTAL_OUTPUT_SIZE,
-		  sensor->mode.h_output_size, &ret);
-	cci_write(sensor->regmap, OV2680_REG_VERTICAL_OUTPUT_SIZE,
-		  sensor->mode.v_output_size, &ret);
-	cci_write(sensor->regmap, OV2680_REG_TIMING_HTS,
-		  sensor->mode.hts, &ret);
-	cci_write(sensor->regmap, OV2680_REG_TIMING_VTS,
-		  sensor->mode.vts, &ret);
-	cci_write(sensor->regmap, OV2680_REG_ISP_X_WIN, 0, &ret);
-	cci_write(sensor->regmap, OV2680_REG_ISP_Y_WIN, 0, &ret);
-	cci_write(sensor->regmap, OV2680_REG_X_INC, inc, &ret);
-	cci_write(sensor->regmap, OV2680_REG_Y_INC, inc, &ret);
-	cci_write(sensor->regmap, OV2680_REG_X_WIN,
-		  sensor->mode.h_output_size, &ret);
-	cci_write(sensor->regmap, OV2680_REG_Y_WIN,
-		  sensor->mode.v_output_size, &ret);
-	cci_write(sensor->regmap, OV2680_REG_FORMAT1, fmt1, &ret);
-	cci_write(sensor->regmap, OV2680_REG_FORMAT2, fmt2, &ret);
-
-	return ret;
-}
-
-static int ov2680_set_fmt(struct v4l2_subdev *sd,
-			  struct v4l2_subdev_state *sd_state,
-			  struct v4l2_subdev_format *format)
-{
-	struct ov2680_dev *sensor = to_ov2680_sensor(sd);
-	struct v4l2_mbus_framefmt *fmt;
-	const struct v4l2_rect *crop;
-	unsigned int width, height;
-
-	crop = __ov2680_get_pad_crop(sensor, sd_state, format->pad, format->which);
-
-	/* Limit set_fmt max size to crop width / height */
-	width = clamp_t(unsigned int, ALIGN(format->format.width, 2),
-			OV2680_MIN_CROP_WIDTH, crop->width);
-	height = clamp_t(unsigned int, ALIGN(format->format.height, 2),
-			 OV2680_MIN_CROP_HEIGHT, crop->height);
-
-	fmt = __ov2680_get_pad_format(sensor, sd_state, format->pad, format->which);
-	ov2680_fill_format(sensor, fmt, width, height);
-
-	format->format = *fmt;
-
-	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
-		return 0;
-
-	mutex_lock(&sensor->lock);
-	ov2680_calc_mode(sensor);
-	mutex_unlock(&sensor->lock);
-	return 0;
-}
-
-static int ov2680_get_fmt(struct v4l2_subdev *sd,
-			  struct v4l2_subdev_state *sd_state,
-			  struct v4l2_subdev_format *format)
-{
-	struct ov2680_dev *sensor = to_ov2680_sensor(sd);
-	struct v4l2_mbus_framefmt *fmt;
-
-	fmt = __ov2680_get_pad_format(sensor, sd_state, format->pad, format->which);
-	format->format = *fmt;
-	return 0;
-}
-
-static int ov2680_get_selection(struct v4l2_subdev *sd,
-				struct v4l2_subdev_state *state,
-				struct v4l2_subdev_selection *sel)
-{
-	struct ov2680_dev *sensor = to_ov2680_sensor(sd);
-
-	switch (sel->target) {
-	case V4L2_SEL_TGT_CROP:
-		mutex_lock(&sensor->lock);
-		sel->r = *__ov2680_get_pad_crop(sensor, state, sel->pad, sel->which);
-		mutex_unlock(&sensor->lock);
-		break;
-	case V4L2_SEL_TGT_NATIVE_SIZE:
-	case V4L2_SEL_TGT_CROP_BOUNDS:
-		sel->r.top = 0;
-		sel->r.left = 0;
-		sel->r.width = OV2680_NATIVE_WIDTH;
-		sel->r.height = OV2680_NATIVE_HEIGHT;
-		break;
-	case V4L2_SEL_TGT_CROP_DEFAULT:
-		sel->r.top = OV2680_ACTIVE_START_TOP;
-		sel->r.left = OV2680_ACTIVE_START_LEFT;
-		sel->r.width = OV2680_ACTIVE_WIDTH;
-		sel->r.height = OV2680_ACTIVE_HEIGHT;
-		break;
-	default:
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
-static int ov2680_set_selection(struct v4l2_subdev *sd,
-				struct v4l2_subdev_state *state,
-				struct v4l2_subdev_selection *sel)
-{
-	struct ov2680_dev *sensor = to_ov2680_sensor(sd);
-	struct v4l2_mbus_framefmt *format;
-	struct v4l2_rect *__crop;
-	struct v4l2_rect rect;
-
-	if (sel->target != V4L2_SEL_TGT_CROP)
-		return -EINVAL;
-
-	/*
-	 * Clamp the boundaries of the crop rectangle to the size of the sensor
-	 * pixel array. Align to multiples of 2 to ensure Bayer pattern isn't
-	 * disrupted.
-	 */
-	rect.left = clamp(ALIGN(sel->r.left, 2), OV2680_NATIVE_START_LEFT,
-			  OV2680_NATIVE_WIDTH);
-	rect.top = clamp(ALIGN(sel->r.top, 2), OV2680_NATIVE_START_TOP,
-			 OV2680_NATIVE_HEIGHT);
-	rect.width = clamp_t(unsigned int, ALIGN(sel->r.width, 2),
-			     OV2680_MIN_CROP_WIDTH, OV2680_NATIVE_WIDTH);
-	rect.height = clamp_t(unsigned int, ALIGN(sel->r.height, 2),
-			      OV2680_MIN_CROP_HEIGHT, OV2680_NATIVE_HEIGHT);
-
-	/* Make sure the crop rectangle isn't outside the bounds of the array */
-	rect.width = min_t(unsigned int, rect.width,
-			   OV2680_NATIVE_WIDTH - rect.left);
-	rect.height = min_t(unsigned int, rect.height,
-			    OV2680_NATIVE_HEIGHT - rect.top);
-
-	__crop = __ov2680_get_pad_crop(sensor, state, sel->pad, sel->which);
-
-	if (rect.width != __crop->width || rect.height != __crop->height) {
-		/*
-		 * Reset the output image size if the crop rectangle size has
-		 * been modified.
-		 */
-		format = __ov2680_get_pad_format(sensor, state, sel->pad, sel->which);
-		format->width = rect.width;
-		format->height = rect.height;
-	}
-
-	*__crop = rect;
-	sel->r = rect;
-
-	return 0;
-}
-
-static int ov2680_init_cfg(struct v4l2_subdev *sd,
-			   struct v4l2_subdev_state *sd_state)
-{
-	struct v4l2_subdev_format fmt = {
-		.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY
-		: V4L2_SUBDEV_FORMAT_ACTIVE,
-		.format = {
-			.width = 800,
-			.height = 600,
-		},
-	};
-
-	sd_state->pads[0].try_crop = ov2680_default_crop;
-
-	return ov2680_set_fmt(sd, sd_state, &fmt);
-}
-
-static int ov2680_detect(struct ov2680_dev *sensor)
-{
-	u64 chip_id, rev;
-	int ret = 0;
-
-	cci_read(sensor->regmap, OV2680_REG_CHIP_ID, &chip_id, &ret);
-	cci_read(sensor->regmap, OV2680_REG_SC_CMMN_SUB_ID, &rev, &ret);
-	if (ret < 0) {
-		dev_err(sensor->dev, "failed to read chip id\n");
-		return -ENODEV;
-	}
-
-	if (chip_id != OV2680_CHIP_ID) {
-		dev_err(sensor->dev, "chip id: 0x%04llx does not match expected 0x%04x\n",
-			chip_id, OV2680_CHIP_ID);
-		return -ENODEV;
-	}
-
-	dev_info(sensor->dev, "sensor_revision id = 0x%llx, rev= %lld\n",
-		 chip_id, rev & 0x0f);
-
-	return 0;
-}
-
-static int ov2680_s_stream(struct v4l2_subdev *sd, int enable)
-{
-	struct ov2680_dev *sensor = to_ov2680_sensor(sd);
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
-	int ret = 0;
-
-	mutex_lock(&sensor->lock);
-
-	if (sensor->is_streaming == enable) {
-		dev_warn(&client->dev, "stream already %s\n", enable ? "started" : "stopped");
-		goto error_unlock;
-	}
-
-	if (enable) {
-		ret = pm_runtime_get_sync(sensor->sd.dev);
-		if (ret < 0)
-			goto error_power_down;
-
-		ret = ov2680_set_mode(sensor);
-		if (ret)
-			goto error_power_down;
-
-		/* Restore value of all ctrls */
-		ret = __v4l2_ctrl_handler_setup(&sensor->ctrls.handler);
-		if (ret)
-			goto error_power_down;
-
-		ret = cci_write(sensor->regmap, OV2680_REG_STREAM_CTRL, 1,
-				NULL);
-		if (ret)
-			goto error_power_down;
-	} else {
-		cci_write(sensor->regmap, OV2680_REG_STREAM_CTRL, 0, NULL);
-		pm_runtime_put(sensor->sd.dev);
-	}
-
-	sensor->is_streaming = enable;
-	v4l2_ctrl_activate(sensor->ctrls.vflip, !enable);
-	v4l2_ctrl_activate(sensor->ctrls.hflip, !enable);
-
-	mutex_unlock(&sensor->lock);
-	return 0;
-
-error_power_down:
-	pm_runtime_put(sensor->sd.dev);
-	sensor->is_streaming = false;
-error_unlock:
-	mutex_unlock(&sensor->lock);
-	return ret;
-}
-
-static int ov2680_s_config(struct v4l2_subdev *sd)
-{
-	struct ov2680_dev *sensor = to_ov2680_sensor(sd);
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
-	int ret;
-
-	ret = pm_runtime_get_sync(&client->dev);
-	if (ret < 0) {
-		dev_err(&client->dev, "ov2680 power-up err.\n");
-		goto fail_power_on;
-	}
-
-	/* config & detect sensor */
-	ret = ov2680_detect(sensor);
-	if (ret)
-		dev_err(&client->dev, "ov2680_detect err s_config.\n");
-
-fail_power_on:
-	pm_runtime_put(&client->dev);
-	return ret;
-}
-
-static int ov2680_g_frame_interval(struct v4l2_subdev *sd,
-				   struct v4l2_subdev_frame_interval *interval)
-{
-	interval->interval.numerator = 1;
-	interval->interval.denominator = OV2680_FRAME_RATE;
-	return 0;
-}
-
-static int ov2680_enum_mbus_code(struct v4l2_subdev *sd,
-				 struct v4l2_subdev_state *sd_state,
-				 struct v4l2_subdev_mbus_code_enum *code)
-{
-	/* We support only a single format */
-	if (code->index)
-		return -EINVAL;
-
-	code->code = MEDIA_BUS_FMT_SBGGR10_1X10;
-	return 0;
-}
-
-static int ov2680_enum_frame_size(struct v4l2_subdev *sd,
-				  struct v4l2_subdev_state *sd_state,
-				  struct v4l2_subdev_frame_size_enum *fse)
-{
-	static const struct v4l2_frmsize_discrete ov2680_frame_sizes[] = {
-		{ 1616, 1216 },
-		{ 1616, 1096 },
-		{ 1616,  916 },
-		{ 1456, 1096 },
-		{ 1296,  976 },
-		{ 1296,  736 },
-		{  784,  592 },
-		{  656,  496 },
-	};
-	int index = fse->index;
-
-	if (index >= ARRAY_SIZE(ov2680_frame_sizes))
-		return -EINVAL;
-
-	fse->min_width = ov2680_frame_sizes[index].width;
-	fse->min_height = ov2680_frame_sizes[index].height;
-	fse->max_width = ov2680_frame_sizes[index].width;
-	fse->max_height = ov2680_frame_sizes[index].height;
-
-	return 0;
-}
-
-static int ov2680_enum_frame_interval(struct v4l2_subdev *sd,
-				      struct v4l2_subdev_state *sd_state,
-				      struct v4l2_subdev_frame_interval_enum *fie)
-{
-	/* Only 1 framerate */
-	if (fie->index)
-		return -EINVAL;
-
-	fie->interval.numerator = 1;
-	fie->interval.denominator = OV2680_FRAME_RATE;
-	return 0;
-}
-
-static int ov2680_g_skip_frames(struct v4l2_subdev *sd, u32 *frames)
-{
-	*frames = OV2680_SKIP_FRAMES;
-	return 0;
-}
-
-static const struct v4l2_subdev_video_ops ov2680_video_ops = {
-	.s_stream = ov2680_s_stream,
-	.g_frame_interval = ov2680_g_frame_interval,
-};
-
-static const struct v4l2_subdev_sensor_ops ov2680_sensor_ops = {
-	.g_skip_frames	= ov2680_g_skip_frames,
-};
-
-static const struct v4l2_subdev_pad_ops ov2680_pad_ops = {
-	.init_cfg = ov2680_init_cfg,
-	.enum_mbus_code = ov2680_enum_mbus_code,
-	.enum_frame_size = ov2680_enum_frame_size,
-	.enum_frame_interval = ov2680_enum_frame_interval,
-	.get_fmt = ov2680_get_fmt,
-	.set_fmt = ov2680_set_fmt,
-	.get_selection = ov2680_get_selection,
-	.set_selection = ov2680_set_selection,
-};
-
-static const struct v4l2_subdev_ops ov2680_ops = {
-	.video = &ov2680_video_ops,
-	.pad = &ov2680_pad_ops,
-	.sensor = &ov2680_sensor_ops,
-};
-
-static int ov2680_init_controls(struct ov2680_dev *sensor)
-{
-	static const char * const test_pattern_menu[] = {
-		"Disabled",
-		"Color Bars",
-		"Random Data",
-		"Square",
-		"Black Image",
-	};
-	const struct v4l2_ctrl_ops *ops = &ov2680_ctrl_ops;
-	struct ov2680_ctrls *ctrls = &sensor->ctrls;
-	struct v4l2_ctrl_handler *hdl = &ctrls->handler;
-	int exp_max = OV2680_LINES_PER_FRAME - OV2680_INTEGRATION_TIME_MARGIN;
-
-	v4l2_ctrl_handler_init(hdl, 4);
-
-	hdl->lock = &sensor->lock;
-
-	ctrls->hflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HFLIP, 0, 1, 1, 0);
-	ctrls->vflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VFLIP, 0, 1, 1, 0);
-	ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE,
-					    0, exp_max, 1, exp_max);
-	ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN, 0, 1023, 1, 250);
-	ctrls->test_pattern =
-		v4l2_ctrl_new_std_menu_items(hdl,
-					     &ov2680_ctrl_ops, V4L2_CID_TEST_PATTERN,
-					     ARRAY_SIZE(test_pattern_menu) - 1,
-					     0, 0, test_pattern_menu);
-
-	ctrls->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
-	ctrls->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
-
-	if (hdl->error)
-		return hdl->error;
-
-	sensor->sd.ctrl_handler = hdl;
-	return 0;
-}
-
-static void ov2680_remove(struct i2c_client *client)
-{
-	struct v4l2_subdev *sd = i2c_get_clientdata(client);
-	struct ov2680_dev *sensor = to_ov2680_sensor(sd);
-
-	dev_dbg(&client->dev, "ov2680_remove...\n");
-
-	v4l2_async_unregister_subdev(&sensor->sd);
-	media_entity_cleanup(&sensor->sd.entity);
-	v4l2_ctrl_handler_free(&sensor->ctrls.handler);
-	mutex_destroy(&sensor->lock);
-	fwnode_handle_put(sensor->ep_fwnode);
-	pm_runtime_disable(&client->dev);
-}
-
-static int ov2680_probe(struct i2c_client *client)
-{
-	struct device *dev = &client->dev;
-	struct ov2680_dev *sensor;
-	int ret;
-
-	sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
-	if (!sensor)
-		return -ENOMEM;
-
-	sensor->regmap = devm_cci_regmap_init_i2c(client, 16);
-	if (IS_ERR(sensor->regmap))
-		return PTR_ERR(sensor->regmap);
-
-	mutex_init(&sensor->lock);
-
-	sensor->dev = &client->dev;
-	v4l2_i2c_subdev_init(&sensor->sd, client, &ov2680_ops);
-
-	/*
-	 * Sometimes the fwnode graph is initialized by the bridge driver.
-	 * Bridge drivers doing this may also add GPIO mappings, wait for this.
-	 */
-	sensor->ep_fwnode = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
-	if (!sensor->ep_fwnode)
-		return dev_err_probe(dev, -EPROBE_DEFER, "waiting for fwnode graph endpoint\n");
-
-	sensor->powerdown = devm_gpiod_get_optional(dev, "powerdown", GPIOD_OUT_HIGH);
-	if (IS_ERR(sensor->powerdown)) {
-		fwnode_handle_put(sensor->ep_fwnode);
-		return dev_err_probe(dev, PTR_ERR(sensor->powerdown), "getting powerdown GPIO\n");
-	}
-
-	pm_runtime_set_suspended(dev);
-	pm_runtime_enable(dev);
-	pm_runtime_set_autosuspend_delay(dev, 1000);
-	pm_runtime_use_autosuspend(dev);
-
-	ret = ov2680_s_config(&sensor->sd);
-	if (ret) {
-		ov2680_remove(client);
-		return ret;
-	}
-
-	sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
-	sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
-	sensor->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
-	sensor->sd.fwnode = sensor->ep_fwnode;
-
-	ret = ov2680_init_controls(sensor);
-	if (ret) {
-		ov2680_remove(client);
-		return ret;
-	}
-
-	ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad);
-	if (ret) {
-		ov2680_remove(client);
-		return ret;
-	}
-
-	sensor->mode.crop = ov2680_default_crop;
-	ov2680_fill_format(sensor, &sensor->mode.fmt, OV2680_NATIVE_WIDTH, OV2680_NATIVE_HEIGHT);
-	ov2680_calc_mode(sensor);
-
-	ret = v4l2_async_register_subdev_sensor(&sensor->sd);
-	if (ret) {
-		ov2680_remove(client);
-		return ret;
-	}
-
-	return 0;
-}
-
-static int ov2680_suspend(struct device *dev)
-{
-	struct v4l2_subdev *sd = dev_get_drvdata(dev);
-	struct ov2680_dev *sensor = to_ov2680_sensor(sd);
-
-	gpiod_set_value_cansleep(sensor->powerdown, 1);
-	return 0;
-}
-
-static int ov2680_resume(struct device *dev)
-{
-	struct v4l2_subdev *sd = dev_get_drvdata(dev);
-	struct ov2680_dev *sensor = to_ov2680_sensor(sd);
-
-	/* according to DS, at least 5ms is needed after DOVDD (enabled by ACPI) */
-	usleep_range(5000, 6000);
-
-	gpiod_set_value_cansleep(sensor->powerdown, 0);
-
-	/* according to DS, 20ms is needed between PWDN and i2c access */
-	msleep(20);
-
-	ov2680_init_registers(sd);
-	return 0;
-}
-
-static DEFINE_RUNTIME_DEV_PM_OPS(ov2680_pm_ops, ov2680_suspend, ov2680_resume, NULL);
-
-static const struct acpi_device_id ov2680_acpi_match[] = {
-	{"XXOV2680"},
-	{"OVTI2680"},
-	{},
-};
-MODULE_DEVICE_TABLE(acpi, ov2680_acpi_match);
-
-static struct i2c_driver ov2680_driver = {
-	.driver = {
-		.name = "ov2680",
-		.pm = pm_sleep_ptr(&ov2680_pm_ops),
-		.acpi_match_table = ov2680_acpi_match,
-	},
-	.probe = ov2680_probe,
-	.remove = ov2680_remove,
-};
-module_i2c_driver(ov2680_driver);
-
-MODULE_AUTHOR("Jacky Wang <Jacky_wang@ovt.com>");
-MODULE_DESCRIPTION("A low-level driver for OmniVision 2680 sensors");
-MODULE_LICENSE("GPL");
diff --git a/drivers/staging/media/atomisp/i2c/ov2680.h b/drivers/staging/media/atomisp/i2c/ov2680.h
deleted file mode 100644
index 7815522724f7..000000000000
--- a/drivers/staging/media/atomisp/i2c/ov2680.h
+++ /dev/null
@@ -1,173 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/*
- * Support for OmniVision OV2680 5M camera sensor.
- *
- * Copyright (c) 2013 Intel Corporation. All Rights Reserved.
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License version
- * 2 as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- *
- */
-
-#ifndef __OV2680_H__
-#define __OV2680_H__
-#include <linux/kernel.h>
-#include <linux/types.h>
-#include <linux/i2c.h>
-#include <linux/delay.h>
-#include <linux/videodev2.h>
-#include <linux/spinlock.h>
-#include <media/v4l2-cci.h>
-#include <media/v4l2-subdev.h>
-#include <media/v4l2-device.h>
-#include <media/v4l2-ctrls.h>
-#include <linux/v4l2-mediabus.h>
-#include <media/media-entity.h>
-
-#define OV2680_NATIVE_WIDTH			1616
-#define OV2680_NATIVE_HEIGHT			1216
-#define OV2680_NATIVE_START_LEFT		0
-#define OV2680_NATIVE_START_TOP			0
-#define OV2680_ACTIVE_WIDTH			1600
-#define OV2680_ACTIVE_HEIGHT			1200
-#define OV2680_ACTIVE_START_LEFT		8
-#define OV2680_ACTIVE_START_TOP			8
-#define OV2680_MIN_CROP_WIDTH			2
-#define OV2680_MIN_CROP_HEIGHT			2
-
-/* 1704 * 1294 * 30fps = 66MHz pixel clock */
-#define OV2680_PIXELS_PER_LINE			1704
-#define OV2680_LINES_PER_FRAME			1294
-
-#define OV2680_SKIP_FRAMES			3
-
-/* If possible send 16 extra rows / lines to the ISP as padding */
-#define OV2680_END_MARGIN			16
-
-/*
- * ov2680 device structure.
- */
-struct ov2680_dev {
-	struct v4l2_subdev sd;
-	struct media_pad pad;
-	/* Protect against concurrent changes to controls */
-	struct mutex lock;
-	struct device *dev;
-	struct regmap *regmap;
-	struct gpio_desc *powerdown;
-	struct fwnode_handle *ep_fwnode;
-	bool is_streaming;
-
-	struct ov2680_mode {
-		struct v4l2_rect crop;
-		struct v4l2_mbus_framefmt fmt;
-		bool binning;
-		u16 h_start;
-		u16 v_start;
-		u16 h_end;
-		u16 v_end;
-		u16 h_output_size;
-		u16 v_output_size;
-		u16 hts;
-		u16 vts;
-	} mode;
-
-	struct ov2680_ctrls {
-		struct v4l2_ctrl_handler handler;
-		struct v4l2_ctrl *hflip;
-		struct v4l2_ctrl *vflip;
-		struct v4l2_ctrl *exposure;
-		struct v4l2_ctrl *gain;
-		struct v4l2_ctrl *test_pattern;
-	} ctrls;
-};
-
-#define to_ov2680_sensor(x) container_of(x, struct ov2680_dev, sd)
-
-static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
-{
-	struct ov2680_dev *sensor =
-		container_of(ctrl->handler, struct ov2680_dev, ctrls.handler);
-
-	return &sensor->sd;
-}
-
-static const struct reg_sequence ov2680_global_setting[] = {
-	/* MIPI PHY, 0x10 -> 0x1c enable bp_c_hs_en_lat and bp_d_hs_en_lat */
-	{0x3016, 0x1c},
-
-	/* PLL MULT bits 0-7, datasheet default 0x37 for 24MHz extclk, use 0x45 for 19.2 Mhz extclk */
-	{0x3082, 0x45},
-
-	/* R MANUAL set exposure (0x01) and gain (0x02) to manual (hw does not do auto) */
-	{0x3503, 0x03},
-
-	/* 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},
-
-	/* 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},
-
-	/* R ISP CTRL2 0x20 -> 0x30, set sof_sel bit */
-	{0x5002, 0x30},
-
-	/*
-	 * 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},
-
-	/* DPC GAIN LIST1 0x0f (15) -> 0x08 (8) */
-	{0x578e, 0x08},
-
-	/* DPC GAIN LIST2 0x3f (63) -> 0x0c (12) */
-	{0x578f, 0x0c},
-
-	/* DPC THRE RATIO 0x04 (4) -> 0x00 (0) */
-	{0x5792, 0x00},
-};
-
-#endif
-- 
2.41.0

Re: [PATCH v5 02/32] media: ov2680: Fix ov2680_bayer_order()

[Sakari Ailus]

Hi Hans,

On Thu, Aug 03, 2023 at 11:33:17AM +0200, Hans de Goede wrote:
> The index into ov2680_hv_flip_bayer_order[] should be 0-3, but
> ov2680_bayer_order() was using 0 + BIT(2) + (BIT(2) << 1) as
> max index, while the intention was to use: 0 + 1 + 2 as max index.
> 
> Fix the index calculation in ov2680_bayer_order(), while at it
> also just use the ctrl values rather then reading them back using
> a slow i2c-read transaction.
> 
> This also allows making the function void, since there now are
> no more i2c-reads to error check.
> 
> Note the check for the ctrls being NULL is there to allow
> adding an ov2680_fill_format() helper later, which will call
> ov2680_set_bayer_order() during probe() before the ctrls are created.
> 
> Fixes: 3ee47cad3e69 ("media: ov2680: Add Omnivision OV2680 sensor driver")
> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

This doesn't seem to compile:

> /home/jenkins/linux/drivers/media/i2c/ov2680.c: In function                   
'ov2680_vflip_disable':                                                         
> /home/jenkins/linux/drivers/media/i2c/ov2680.c:351:9: error: implicit         
declaration of function 'ov2680_bayer_order'; did you mean                      
'ov2680_set_bayer_order'? [-Werror=implicit-function-declaration]               
>   351 |  return ov2680_bayer_order(sensor);                                   
>       |         ^~~~~~~~~~~~~~~~~~                                            
>       |         ov2680_set_bayer_order                                        
> cc1: some warnings being treated as errors                                    

I guess you missed converting some calls?

> ---
>  drivers/media/i2c/ov2680.c | 27 ++++++++++-----------------
>  1 file changed, 10 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
> index 3a737a1607a4..621144f16fdc 100644
> --- a/drivers/media/i2c/ov2680.c
> +++ b/drivers/media/i2c/ov2680.c
> @@ -315,26 +315,17 @@ static void ov2680_power_down(struct ov2680_dev *sensor)
>  	usleep_range(5000, 10000);
>  }
>  
> -static int ov2680_bayer_order(struct ov2680_dev *sensor)
> +static void ov2680_set_bayer_order(struct ov2680_dev *sensor)
>  {
> -	u32 format1;
> -	u32 format2;
> -	u32 hv_flip;
> -	int ret;
> +	int hv_flip = 0;
>  
> -	ret = ov2680_read_reg(sensor, OV2680_REG_FORMAT1, &format1);
> -	if (ret < 0)
> -		return ret;
> +	if (sensor->ctrls.vflip && sensor->ctrls.vflip->val)
> +		hv_flip += 1;
>  
> -	ret = ov2680_read_reg(sensor, OV2680_REG_FORMAT2, &format2);
> -	if (ret < 0)
> -		return ret;
> -
> -	hv_flip = (format2 & BIT(2)  << 1) | (format1 & BIT(2));
> +	if (sensor->ctrls.hflip && sensor->ctrls.hflip->val)
> +		hv_flip += 2;
>  
>  	sensor->fmt.code = ov2680_hv_flip_bayer_order[hv_flip];
> -
> -	return 0;
>  }
>  
>  static int ov2680_vflip_enable(struct ov2680_dev *sensor)
> @@ -345,7 +336,8 @@ static int ov2680_vflip_enable(struct ov2680_dev *sensor)
>  	if (ret < 0)
>  		return ret;
>  
> -	return ov2680_bayer_order(sensor);
> +	ov2680_set_bayer_order(sensor);
> +	return 0;
>  }
>  
>  static int ov2680_vflip_disable(struct ov2680_dev *sensor)
> @@ -378,7 +370,8 @@ static int ov2680_hflip_disable(struct ov2680_dev *sensor)
>  	if (ret < 0)
>  		return ret;
>  
> -	return ov2680_bayer_order(sensor);
> +	ov2680_set_bayer_order(sensor);
> +	return 0;
>  }
>  
>  static int ov2680_test_pattern_set(struct ov2680_dev *sensor, int value)

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v5 02/32] media: ov2680: Fix ov2680_bayer_order()

[Sakari Ailus]

On Thu, Aug 03, 2023 at 12:48:01PM +0000, Sakari Ailus wrote:
> Hi Hans,
> 
> On Thu, Aug 03, 2023 at 11:33:17AM +0200, Hans de Goede wrote:
> > The index into ov2680_hv_flip_bayer_order[] should be 0-3, but
> > ov2680_bayer_order() was using 0 + BIT(2) + (BIT(2) << 1) as
> > max index, while the intention was to use: 0 + 1 + 2 as max index.
> > 
> > Fix the index calculation in ov2680_bayer_order(), while at it
> > also just use the ctrl values rather then reading them back using
> > a slow i2c-read transaction.
> > 
> > This also allows making the function void, since there now are
> > no more i2c-reads to error check.
> > 
> > Note the check for the ctrls being NULL is there to allow
> > adding an ov2680_fill_format() helper later, which will call
> > ov2680_set_bayer_order() during probe() before the ctrls are created.
> > 
> > Fixes: 3ee47cad3e69 ("media: ov2680: Add Omnivision OV2680 sensor driver")
> > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> > Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> This doesn't seem to compile:
> 
> > /home/jenkins/linux/drivers/media/i2c/ov2680.c: In function                   
> 'ov2680_vflip_disable':                                                         
> > /home/jenkins/linux/drivers/media/i2c/ov2680.c:351:9: error: implicit         
> declaration of function 'ov2680_bayer_order'; did you mean                      
> 'ov2680_set_bayer_order'? [-Werror=implicit-function-declaration]               
> >   351 |  return ov2680_bayer_order(sensor);                                   
> >       |         ^~~~~~~~~~~~~~~~~~                                            
> >       |         ov2680_set_bayer_order                                        
> > cc1: some warnings being treated as errors                                    
> 
> I guess you missed converting some calls?

Actually the fix was trivial and the code is removed in the next patch
nonetheless. The change I made here was:

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index 621144f16fdc..60c4e575aa86 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -348,7 +348,8 @@ static int ov2680_vflip_disable(struct ov2680_dev *sensor)
 	if (ret < 0)
 		return ret;
 
-	return ov2680_bayer_order(sensor);
+	ov2680_set_bayer_order(sensor);
+	return 0;
 }
 
 static int ov2680_hflip_enable(struct ov2680_dev *sensor)
@@ -359,7 +360,8 @@ static int ov2680_hflip_enable(struct ov2680_dev *sensor)
 	if (ret < 0)
 		return ret;
 
-	return ov2680_bayer_order(sensor);
+	ov2680_set_bayer_order(sensor);
+	return 0;
 }
 
 static int ov2680_hflip_disable(struct ov2680_dev *sensor)

-- 
Sakari Ailus

Re: [PATCH v5 02/32] media: ov2680: Fix ov2680_bayer_order()

[Hans de Goede]

Hi,

On 8/3/23 14:52, Sakari Ailus wrote:
> On Thu, Aug 03, 2023 at 12:48:01PM +0000, Sakari Ailus wrote:
>> Hi Hans,
>>
>> On Thu, Aug 03, 2023 at 11:33:17AM +0200, Hans de Goede wrote:
>>> The index into ov2680_hv_flip_bayer_order[] should be 0-3, but
>>> ov2680_bayer_order() was using 0 + BIT(2) + (BIT(2) << 1) as
>>> max index, while the intention was to use: 0 + 1 + 2 as max index.
>>>
>>> Fix the index calculation in ov2680_bayer_order(), while at it
>>> also just use the ctrl values rather then reading them back using
>>> a slow i2c-read transaction.
>>>
>>> This also allows making the function void, since there now are
>>> no more i2c-reads to error check.
>>>
>>> Note the check for the ctrls being NULL is there to allow
>>> adding an ov2680_fill_format() helper later, which will call
>>> ov2680_set_bayer_order() during probe() before the ctrls are created.
>>>
>>> Fixes: 3ee47cad3e69 ("media: ov2680: Add Omnivision OV2680 sensor driver")
>>> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
>>> Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>
>> This doesn't seem to compile:
>>
>>> /home/jenkins/linux/drivers/media/i2c/ov2680.c: In function                   
>> 'ov2680_vflip_disable':                                                         
>>> /home/jenkins/linux/drivers/media/i2c/ov2680.c:351:9: error: implicit         
>> declaration of function 'ov2680_bayer_order'; did you mean                      
>> 'ov2680_set_bayer_order'? [-Werror=implicit-function-declaration]               
>>>   351 |  return ov2680_bayer_order(sensor);                                   
>>>       |         ^~~~~~~~~~~~~~~~~~                                            
>>>       |         ov2680_set_bayer_order                                        
>>> cc1: some warnings being treated as errors                                    
>>
>> I guess you missed converting some calls?
> 
> Actually the fix was trivial and the code is removed in the next patch
> nonetheless. The change I made here was:

Yes the below changes are correct and I'm pretty sure I had that in there
at one point in time (I did compile test every patch for v2 or v3 IIRC).

I guess this somehow got messed up during a rebase, thank you for
fixing this.

Regards,

Hans




> 
> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
> index 621144f16fdc..60c4e575aa86 100644
> --- a/drivers/media/i2c/ov2680.c
> +++ b/drivers/media/i2c/ov2680.c
> @@ -348,7 +348,8 @@ static int ov2680_vflip_disable(struct ov2680_dev *sensor)
>  	if (ret < 0)
>  		return ret;
>  
> -	return ov2680_bayer_order(sensor);
> +	ov2680_set_bayer_order(sensor);
> +	return 0;
>  }
>  
>  static int ov2680_hflip_enable(struct ov2680_dev *sensor)
> @@ -359,7 +360,8 @@ static int ov2680_hflip_enable(struct ov2680_dev *sensor)
>  	if (ret < 0)
>  		return ret;
>  
> -	return ov2680_bayer_order(sensor);
> +	ov2680_set_bayer_order(sensor);
> +	return 0;
>  }
>  
>  static int ov2680_hflip_disable(struct ov2680_dev *sensor)
> 

Re: [PATCH v5 17/32] media: ov2680: Add support for ACPI enumeration

[Andy Shevchenko]

On Thu, Aug 03, 2023 at 11:33:32AM +0200, Hans de Goede wrote:
> Add an acpi_match_table now that all the other bits necessary for
> ACPI support are in place.

> The OVTI prefix used for the ACPI-HID is used for various OmniVision
> sensors on many generations x86 tablets and laptops.

"OVTI is the official ACPI vendor ID for OmniVision Technologies, Inc."

> The OVTI2680 HID specifically is used on multiple models spanning at
> least 4 different Intel CPU models (2 ISP2, 2 IPU3).

With or without above (as it's still the official vendor ID :-)
Reviewed-by: Andy Shevchenko <andy@kernel.org>
from ACPI ID rules perspective.

But add that in case you need to send a new version.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 17/32] media: ov2680: Add support for ACPI enumeration

[Sakari Ailus]

Hi Andy,

On Thu, Aug 03, 2023 at 04:15:01PM +0300, Andy Shevchenko wrote:
> On Thu, Aug 03, 2023 at 11:33:32AM +0200, Hans de Goede wrote:
> > Add an acpi_match_table now that all the other bits necessary for
> > ACPI support are in place.
> 
> > The OVTI prefix used for the ACPI-HID is used for various OmniVision
> > sensors on many generations x86 tablets and laptops.
> 
> "OVTI is the official ACPI vendor ID for OmniVision Technologies, Inc."
> 
> > The OVTI2680 HID specifically is used on multiple models spanning at
> > least 4 different Intel CPU models (2 ISP2, 2 IPU3).
> 
> With or without above (as it's still the official vendor ID :-)
> Reviewed-by: Andy Shevchenko <andy@kernel.org>
> from ACPI ID rules perspective.
> 
> But add that in case you need to send a new version.

Thanks, Andy!

I'll take this version then, it's "just" a sensor driver patch.

-- 
Sakari Ailus

Re: [PATCH v5 17/32] media: ov2680: Add support for ACPI enumeration

[Hans de Goede]

Hi andy,

On 8/3/23 15:15, Andy Shevchenko wrote:
> On Thu, Aug 03, 2023 at 11:33:32AM +0200, Hans de Goede wrote:
>> Add an acpi_match_table now that all the other bits necessary for
>> ACPI support are in place.
> 
>> The OVTI prefix used for the ACPI-HID is used for various OmniVision
>> sensors on many generations x86 tablets and laptops.
> 
> "OVTI is the official ACPI vendor ID for OmniVision Technologies, Inc."

Is it though? Is there a list of official ACPI vendor IDs
published somewhere were we can check this ?

I think the Intel MIPI camera driver team may have come up with
the OVTI prefix independently ?

Just because it is used quite a lot does not mean that it is
official ...

Regards,

Hans

Re: [PATCH v5 17/32] media: ov2680: Add support for ACPI enumeration

[Sakari Ailus]

Hi Hans,

On Thu, Aug 03, 2023 at 04:57:24PM +0200, Hans de Goede wrote:
> Hi andy,
> 
> On 8/3/23 15:15, Andy Shevchenko wrote:
> > On Thu, Aug 03, 2023 at 11:33:32AM +0200, Hans de Goede wrote:
> >> Add an acpi_match_table now that all the other bits necessary for
> >> ACPI support are in place.
> > 
> >> The OVTI prefix used for the ACPI-HID is used for various OmniVision
> >> sensors on many generations x86 tablets and laptops.
> > 
> > "OVTI is the official ACPI vendor ID for OmniVision Technologies, Inc."
> 
> Is it though? Is there a list of official ACPI vendor IDs
> published somewhere were we can check this ?
> 
> I think the Intel MIPI camera driver team may have come up with
> the OVTI prefix independently ?
> 
> Just because it is used quite a lot does not mean that it is
> official ...

It is. Only ACPI and PNP vendor IDs allocated to vendors should be used
(and HIDs allocated by that vendor only).

<URL:https://uefi.org/acpi_id_list>
<URL:https://uefi.org/pnp_id_list>

-- 
Regards,

Sakari Ailus