[PATCH v3 00/15] Support OVTI7251 on Microsoft Surface line

[PATCH v3 00/15] Support OVTI7251 on Microsoft Surface line

[Daniel Scally]

Hello all

This series extends the OV7251 driver so it's functional on the
Microsoft Surface line of laptops, where it's used as the IR
camera for face login. The camera sensor is connected to a CIO2
device which packs the 10-bit greyscale data into 25 pixels per 32
bytes similar to the IPU3 formats for Bayer data, so I also added
a new format to describe that and added it to the ipu3-cio2 driver's
list of supported formats. 

Series-level changes:

	- Added patches 12-15, which extend the driver's controls to fulfil libcamera
  requirements.

Thanks
Dan

Daniel Scally (15):
  media: uapi: Add IPU3 packed Y10 format
  media: ipu3-cio2: Add support for V4L2_PIX_FMT_IPU3_Y10
  media: i2c: Add acpi support to ov7251
  media: i2c: Provide ov7251_check_hwcfg()
  media: i2c: Remove per-mode frequencies from ov7251
  media: i2c: Add ov7251_pll_configure()
  media: i2c: Add support for new frequencies to ov7251
  media: i2c: Add ov7251_detect_chip()
  media: i2c: Add pm_runtime support to ov7251
  media: i2c: Remove .s_power() from ov7251
  media: ipu3-cio2: Add INT347E to cio2-bridge
  media: i2c: Extend .get_selection() for ov7251
  media: i2c: add ov7251_init_ctrls()
  media: i2c: Add hblank control to ov7251
  media: i2c: Add vblank control to ov7251 driver

 .../media/v4l/pixfmt-yuv-luma.rst             |  14 +-
 drivers/media/i2c/ov7251.c                    | 749 +++++++++++++-----
 drivers/media/pci/intel/ipu3/cio2-bridge.c    |   2 +
 drivers/media/pci/intel/ipu3/ipu3-cio2-main.c |   5 +
 drivers/media/v4l2-core/v4l2-ioctl.c          |   1 +
 include/uapi/linux/videodev2.h                |   3 +-
 6 files changed, 554 insertions(+), 220 deletions(-)

-- 
2.25.1

[PATCH v3 01/15] media: uapi: Add IPU3 packed Y10 format

[Daniel Scally]

Some platforms with an Intel IPU3 have an IR sensor producing 10 bit
greyscale format data that is transmitted over a CSI-2 bus to a CIO2
device - this packs the data into 32 bytes per 25 pixels. Add an entry
to the uAPI header defining that format.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3:

     - None

Changes in v2:

	- Switched away from using the fourcc in the explanatory note for
	pixfmt-yuv-luma.rst (Nicolas)

 .../userspace-api/media/v4l/pixfmt-yuv-luma.rst    | 14 +++++++++++++-
 drivers/media/v4l2-core/v4l2-ioctl.c               |  1 +
 include/uapi/linux/videodev2.h                     |  3 ++-
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/Documentation/userspace-api/media/v4l/pixfmt-yuv-luma.rst b/Documentation/userspace-api/media/v4l/pixfmt-yuv-luma.rst
index 8ebd58c3588f..6a387f9df3ba 100644
--- a/Documentation/userspace-api/media/v4l/pixfmt-yuv-luma.rst
+++ b/Documentation/userspace-api/media/v4l/pixfmt-yuv-luma.rst
@@ -48,6 +48,17 @@ are often referred to as greyscale formats.
       - ...
       - ...
 
+    * .. _V4L2-PIX-FMT-IPU3-Y10:
+
+      - ``V4L2_PIX_FMT_IPU3_Y10``
+      - 'ip3y'
+
+      - Y'\ :sub:`0`\ [7:0]
+      - Y'\ :sub:`1`\ [5:0] Y'\ :sub:`0`\ [9:8]
+      - Y'\ :sub:`2`\ [3:0] Y'\ :sub:`1`\ [9:6]
+      - Y'\ :sub:`3`\ [1:0] Y'\ :sub:`2`\ [9:4]
+      - Y'\ :sub:`3`\ [9:2]
+
     * .. _V4L2-PIX-FMT-Y10:
 
       - ``V4L2_PIX_FMT_Y10``
@@ -133,4 +144,5 @@ are often referred to as greyscale formats.
 
     For the Y16 and Y16_BE formats, the actual sampling precision may be lower
     than 16 bits. For example, 10 bits per pixel uses values in the range 0 to
-    1023.
+    1023. For the IPU3_Y10 format 25 pixels are packed into 32 bytes, which
+    leaves the 6 most significant bits of the last byte padded with 0.
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 642cb90f457c..89691bbb372d 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1265,6 +1265,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
 	case V4L2_PIX_FMT_Y16_BE:	descr = "16-bit Greyscale BE"; break;
 	case V4L2_PIX_FMT_Y10BPACK:	descr = "10-bit Greyscale (Packed)"; break;
 	case V4L2_PIX_FMT_Y10P:		descr = "10-bit Greyscale (MIPI Packed)"; break;
+	case V4L2_PIX_FMT_IPU3_Y10:	descr = "10-bit greyscale (IPU3 Packed)"; break;
 	case V4L2_PIX_FMT_Y8I:		descr = "Interleaved 8-bit Greyscale"; break;
 	case V4L2_PIX_FMT_Y12I:		descr = "Interleaved 12-bit Greyscale"; break;
 	case V4L2_PIX_FMT_Z16:		descr = "16-bit Depth"; break;
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index df8b9c486ba1..34329f4655e0 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -569,6 +569,7 @@ struct v4l2_pix_format {
 /* Grey bit-packed formats */
 #define V4L2_PIX_FMT_Y10BPACK    v4l2_fourcc('Y', '1', '0', 'B') /* 10  Greyscale bit-packed */
 #define V4L2_PIX_FMT_Y10P    v4l2_fourcc('Y', '1', '0', 'P') /* 10  Greyscale, MIPI RAW10 packed */
+#define V4L2_PIX_FMT_IPU3_Y10		v4l2_fourcc('i', 'p', '3', 'y') /* IPU3 packed 10-bit greyscale */
 
 /* Palette formats */
 #define V4L2_PIX_FMT_PAL8    v4l2_fourcc('P', 'A', 'L', '8') /*  8  8-bit palette */
@@ -745,7 +746,7 @@ struct v4l2_pix_format {
 #define V4L2_PIX_FMT_CNF4     v4l2_fourcc('C', 'N', 'F', '4') /* Intel 4-bit packed depth confidence information */
 #define V4L2_PIX_FMT_HI240    v4l2_fourcc('H', 'I', '2', '4') /* BTTV 8-bit dithered RGB */
 
-/* 10bit raw bayer packed, 32 bytes for every 25 pixels, last LSB 6 bits unused */
+/* 10bit raw packed, 32 bytes for every 25 pixels, last LSB 6 bits unused */
 #define V4L2_PIX_FMT_IPU3_SBGGR10	v4l2_fourcc('i', 'p', '3', 'b') /* IPU3 packed 10-bit BGGR bayer */
 #define V4L2_PIX_FMT_IPU3_SGBRG10	v4l2_fourcc('i', 'p', '3', 'g') /* IPU3 packed 10-bit GBRG bayer */
 #define V4L2_PIX_FMT_IPU3_SGRBG10	v4l2_fourcc('i', 'p', '3', 'G') /* IPU3 packed 10-bit GRBG bayer */
-- 
2.25.1

[PATCH v3 02/15] media: ipu3-cio2: Add support for V4L2_PIX_FMT_IPU3_Y10

[Daniel Scally]

We have platforms where a camera sensor transmits Y10 data to
the CIO2 device - add support for that (packed) format to the
ipu3-cio2 driver.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3:

	- None

Changes in v2:

	- None

 drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
index 0e9b0503b62a..93cc0577b6db 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
@@ -65,6 +65,11 @@ static const struct ipu3_cio2_fmt formats[] = {
 		.fourcc		= V4L2_PIX_FMT_IPU3_SRGGB10,
 		.mipicode	= 0x2b,
 		.bpp		= 10,
+	}, {
+		.mbus_code	= MEDIA_BUS_FMT_Y10_1X10,
+		.fourcc		= V4L2_PIX_FMT_IPU3_Y10,
+		.mipicode	= 0x2b,
+		.bpp		= 10,
 	},
 };
 
-- 
2.25.1

[PATCH v3 03/15] media: i2c: Add acpi support to ov7251

[Daniel Scally]

Add support for enumeration through ACPI to the ov7251 driver

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3:

	- None

Changes in v2:

	- None

 drivers/media/i2c/ov7251.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
index ebb299f207e5..d6fe574cb9e0 100644
--- a/drivers/media/i2c/ov7251.c
+++ b/drivers/media/i2c/ov7251.c
@@ -14,6 +14,7 @@
 #include <linux/i2c.h>
 #include <linux/init.h>
 #include <linux/module.h>
+#include <linux/mod_devicetable.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/types.h>
@@ -1490,9 +1491,16 @@ static const struct of_device_id ov7251_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, ov7251_of_match);
 
+static const struct acpi_device_id ov7251_acpi_match[] = {
+	{ "INT347E" },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, ov7251_acpi_match);
+
 static struct i2c_driver ov7251_i2c_driver = {
 	.driver = {
 		.of_match_table = ov7251_of_match,
+		.acpi_match_table = ov7251_acpi_match,
 		.name  = "ov7251",
 	},
 	.probe_new  = ov7251_probe,
-- 
2.25.1

[PATCH v3 04/15] media: i2c: Provide ov7251_check_hwcfg()

[Daniel Scally]

Move the endpoint checking from .probe() to a dedicated function,
and additionally check that the firmware provided link frequencies
are a match for those supported by the driver. Store the index to the
matching link frequency so it can be easily identified later.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3:

	- Replaced freq_found variable (Andy)

Changes in v2:

	- Switched to use unsigned int (Sakari)
	- Dropped the checks for bus_type and number of data lanes (Sakari)
	- Fixed the double-loop break (Dave)
	- Stored the index to the configured link frequency so it can be used
	later on.

 drivers/media/i2c/ov7251.c | 77 +++++++++++++++++++++++++++++---------
 1 file changed, 59 insertions(+), 18 deletions(-)

diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
index d6fe574cb9e0..f1965c8adbd7 100644
--- a/drivers/media/i2c/ov7251.c
+++ b/drivers/media/i2c/ov7251.c
@@ -60,6 +60,11 @@ struct ov7251_mode_info {
 	struct v4l2_fract timeperframe;
 };
 
+enum supported_link_freqs {
+	OV7251_LINK_FREQ_240_MHZ,
+	OV7251_NUM_SUPPORTED_LINK_FREQS
+};
+
 struct ov7251 {
 	struct i2c_client *i2c_client;
 	struct device *dev;
@@ -75,6 +80,7 @@ struct ov7251 {
 	struct regulator *core_regulator;
 	struct regulator *analog_regulator;
 
+	enum supported_link_freqs link_freq_idx;
 	const struct ov7251_mode_info *current_mode;
 
 	struct v4l2_ctrl_handler ctrls;
@@ -1255,10 +1261,60 @@ static const struct v4l2_subdev_ops ov7251_subdev_ops = {
 	.pad = &ov7251_subdev_pad_ops,
 };
 
+static int ov7251_check_hwcfg(struct ov7251 *ov7251)
+{
+	struct fwnode_handle *fwnode = dev_fwnode(ov7251->dev);
+	struct v4l2_fwnode_endpoint bus_cfg = {
+		.bus_type = V4L2_MBUS_CSI2_DPHY,
+	};
+	struct fwnode_handle *endpoint;
+	unsigned int i, j;
+	int ret;
+
+	endpoint = fwnode_graph_get_next_endpoint(fwnode, NULL);
+	if (!endpoint)
+		return -EPROBE_DEFER; /* could be provided by cio2-bridge */
+
+	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &bus_cfg);
+	fwnode_handle_put(endpoint);
+	if (ret)
+		return dev_err_probe(ov7251->dev, ret,
+				     "parsing endpoint node failed\n");
+
+	if (!bus_cfg.nr_of_link_frequencies) {
+		ret = -EINVAL;
+		dev_err_probe(ov7251->dev, ret,
+			      "no link frequencies defined\n");
+		goto out_free_bus_cfg;
+	}
+
+	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
+		for (j = 0; j < ARRAY_SIZE(link_freq); j++)
+			if (bus_cfg.link_frequencies[i] == link_freq[j])
+				break;
+
+		if (j < ARRAY_SIZE(link_freq))
+			break;
+	}
+
+	if (i == bus_cfg.nr_of_link_frequencies) {
+		ret = -EINVAL;
+		dev_err_probe(ov7251->dev, ret,
+			      "no supported link freq found\n");
+		goto out_free_bus_cfg;
+	}
+
+	ov7251->link_freq_idx = i;
+
+out_free_bus_cfg:
+	v4l2_fwnode_endpoint_free(&bus_cfg);
+
+	return ret;
+}
+
 static int ov7251_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
-	struct fwnode_handle *endpoint;
 	struct ov7251 *ov7251;
 	u8 chip_id_high, chip_id_low, chip_rev;
 	int ret;
@@ -1270,24 +1326,9 @@ static int ov7251_probe(struct i2c_client *client)
 	ov7251->i2c_client = client;
 	ov7251->dev = dev;
 
-	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
-	if (!endpoint) {
-		dev_err(dev, "endpoint node not found\n");
-		return -EINVAL;
-	}
-
-	ret = v4l2_fwnode_endpoint_parse(endpoint, &ov7251->ep);
-	fwnode_handle_put(endpoint);
-	if (ret < 0) {
-		dev_err(dev, "parsing endpoint node failed\n");
+	ret = ov7251_check_hwcfg(ov7251);
+	if (ret)
 		return ret;
-	}
-
-	if (ov7251->ep.bus_type != V4L2_MBUS_CSI2_DPHY) {
-		dev_err(dev, "invalid bus type (%u), must be CSI2 (%u)\n",
-			ov7251->ep.bus_type, V4L2_MBUS_CSI2_DPHY);
-		return -EINVAL;
-	}
 
 	/* get system clock (xclk) */
 	ov7251->xclk = devm_clk_get(dev, "xclk");
-- 
2.25.1

Re: [PATCH v3 04/15] media: i2c: Provide ov7251_check_hwcfg()

[Andy Shevchenko]

On Wed, May 04, 2022 at 11:30:16PM +0100, Daniel Scally wrote:
> Move the endpoint checking from .probe() to a dedicated function,
> and additionally check that the firmware provided link frequencies
> are a match for those supported by the driver. Store the index to the
> matching link frequency so it can be easily identified later.

...

> +	if (!bus_cfg.nr_of_link_frequencies) {
> +		ret = -EINVAL;
> +		dev_err_probe(ov7251->dev, ret,
> +			      "no link frequencies defined\n");

		ret = dev_err_probe(ov7251->dev, -EINVAL,
				    "no link frequencies defined\n");

?

> +		goto out_free_bus_cfg;
> +	}

...

> +	if (i == bus_cfg.nr_of_link_frequencies) {
> +		ret = -EINVAL;
> +		dev_err_probe(ov7251->dev, ret,
> +			      "no supported link freq found\n");

Ditto.

> +		goto out_free_bus_cfg;
> +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 04/15] media: i2c: Provide ov7251_check_hwcfg()

[Daniel Scally]

Hi Andy

On 05/05/2022 11:16, Andy Shevchenko wrote:
> On Wed, May 04, 2022 at 11:30:16PM +0100, Daniel Scally wrote:
>> Move the endpoint checking from .probe() to a dedicated function,
>> and additionally check that the firmware provided link frequencies
>> are a match for those supported by the driver. Store the index to the
>> matching link frequency so it can be easily identified later.
> ...
>
>> +	if (!bus_cfg.nr_of_link_frequencies) {
>> +		ret = -EINVAL;
>> +		dev_err_probe(ov7251->dev, ret,
>> +			      "no link frequencies defined\n");
> 		ret = dev_err_probe(ov7251->dev, -EINVAL,
> 				    "no link frequencies defined\n");
>
> ?


Derp - of course, thanks!

>> +		goto out_free_bus_cfg;
>> +	}
> ...
>
>> +	if (i == bus_cfg.nr_of_link_frequencies) {
>> +		ret = -EINVAL;
>> +		dev_err_probe(ov7251->dev, ret,
>> +			      "no supported link freq found\n");
> Ditto.
>
>> +		goto out_free_bus_cfg;
>> +	}

[PATCH v3 05/15] media: i2c: Remove per-mode frequencies from ov7251

[Daniel Scally]

Each of the defined modes in the ov7251 driver uses the same link
frequency and pixel rate; just drop those members of the modes and
set the controls to read only during initialisation. Add a new
table defining the supported pixel rates to substitue for the values
hardcoded in the modes.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3:

	- None

Changes in v2:

	- New patch

 drivers/media/i2c/ov7251.c | 43 +++++++++++++-------------------------
 1 file changed, 14 insertions(+), 29 deletions(-)

diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
index f1965c8adbd7..f21119064b2d 100644
--- a/drivers/media/i2c/ov7251.c
+++ b/drivers/media/i2c/ov7251.c
@@ -526,7 +526,11 @@ static const struct reg_value ov7251_setting_vga_90fps[] = {
 };
 
 static const s64 link_freq[] = {
-	240000000,
+	[OV7251_LINK_FREQ_240_MHZ] = 240000000,
+};
+
+static const s64 pixel_rates[] = {
+	[OV7251_LINK_FREQ_240_MHZ] = 48000000,
 };
 
 static const struct ov7251_mode_info ov7251_mode_info_data[] = {
@@ -535,8 +539,6 @@ static const struct ov7251_mode_info ov7251_mode_info_data[] = {
 		.height = 480,
 		.data = ov7251_setting_vga_30fps,
 		.data_size = ARRAY_SIZE(ov7251_setting_vga_30fps),
-		.pixel_clock = 48000000,
-		.link_freq = 0, /* an index in link_freq[] */
 		.exposure_max = 1704,
 		.exposure_def = 504,
 		.timeperframe = {
@@ -549,8 +551,6 @@ static const struct ov7251_mode_info ov7251_mode_info_data[] = {
 		.height = 480,
 		.data = ov7251_setting_vga_60fps,
 		.data_size = ARRAY_SIZE(ov7251_setting_vga_60fps),
-		.pixel_clock = 48000000,
-		.link_freq = 0, /* an index in link_freq[] */
 		.exposure_max = 840,
 		.exposure_def = 504,
 		.timeperframe = {
@@ -563,8 +563,6 @@ static const struct ov7251_mode_info ov7251_mode_info_data[] = {
 		.height = 480,
 		.data = ov7251_setting_vga_90fps,
 		.data_size = ARRAY_SIZE(ov7251_setting_vga_90fps),
-		.pixel_clock = 48000000,
-		.link_freq = 0, /* an index in link_freq[] */
 		.exposure_max = 552,
 		.exposure_def = 504,
 		.timeperframe = {
@@ -1059,16 +1057,6 @@ static int ov7251_set_format(struct v4l2_subdev *sd,
 	__crop->height = new_mode->height;
 
 	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
-		ret = __v4l2_ctrl_s_ctrl_int64(ov7251->pixel_clock,
-					       new_mode->pixel_clock);
-		if (ret < 0)
-			goto exit;
-
-		ret = __v4l2_ctrl_s_ctrl(ov7251->link_freq,
-					 new_mode->link_freq);
-		if (ret < 0)
-			goto exit;
-
 		ret = __v4l2_ctrl_modify_range(ov7251->exposure,
 					       1, new_mode->exposure_max,
 					       1, new_mode->exposure_def);
@@ -1199,16 +1187,6 @@ static int ov7251_set_frame_interval(struct v4l2_subdev *subdev,
 	new_mode = ov7251_find_mode_by_ival(ov7251, &fi->interval);
 
 	if (new_mode != ov7251->current_mode) {
-		ret = __v4l2_ctrl_s_ctrl_int64(ov7251->pixel_clock,
-					       new_mode->pixel_clock);
-		if (ret < 0)
-			goto exit;
-
-		ret = __v4l2_ctrl_s_ctrl(ov7251->link_freq,
-					 new_mode->link_freq);
-		if (ret < 0)
-			goto exit;
-
 		ret = __v4l2_ctrl_modify_range(ov7251->exposure,
 					       1, new_mode->exposure_max,
 					       1, new_mode->exposure_def);
@@ -1317,6 +1295,7 @@ static int ov7251_probe(struct i2c_client *client)
 	struct device *dev = &client->dev;
 	struct ov7251 *ov7251;
 	u8 chip_id_high, chip_id_low, chip_rev;
+	s64 pixel_rate;
 	int ret;
 
 	ov7251 = devm_kzalloc(dev, sizeof(struct ov7251), GFP_KERNEL);
@@ -1398,17 +1377,23 @@ static int ov7251_probe(struct i2c_client *client)
 				     V4L2_CID_TEST_PATTERN,
 				     ARRAY_SIZE(ov7251_test_pattern_menu) - 1,
 				     0, 0, ov7251_test_pattern_menu);
+
+	pixel_rate = pixel_rates[ov7251->link_freq_idx];
 	ov7251->pixel_clock = v4l2_ctrl_new_std(&ov7251->ctrls,
 						&ov7251_ctrl_ops,
 						V4L2_CID_PIXEL_RATE,
-						1, INT_MAX, 1, 1);
+						pixel_rate, INT_MAX,
+						pixel_rate, pixel_rate);
 	ov7251->link_freq = v4l2_ctrl_new_int_menu(&ov7251->ctrls,
 						   &ov7251_ctrl_ops,
 						   V4L2_CID_LINK_FREQ,
 						   ARRAY_SIZE(link_freq) - 1,
-						   0, link_freq);
+						   ov7251->link_freq_idx,
+						   link_freq);
 	if (ov7251->link_freq)
 		ov7251->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+	if (ov7251->pixel_clock)
+		ov7251->pixel_clock->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
 	ov7251->sd.ctrl_handler = &ov7251->ctrls;
 
-- 
2.25.1

[PATCH v3 06/15] media: i2c: Add ov7251_pll_configure()

[Daniel Scally]

Rather than having the pll settings hidden inside mode blobs, define
them in structs and use a dedicated function to set them. This makes
it simpler to extend the driver to support other frequencies for both
the external clock and desired link frequency.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3:

	- Added commas to last items in arrays (Andy)

Changes in v2:

	- Updated to support different link-frequencies in addition to different
	external clock frequencies.

 drivers/media/i2c/ov7251.c | 175 ++++++++++++++++++++++++++++++-------
 1 file changed, 145 insertions(+), 30 deletions(-)

diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
index f21119064b2d..3440077e8ba9 100644
--- a/drivers/media/i2c/ov7251.c
+++ b/drivers/media/i2c/ov7251.c
@@ -42,6 +42,16 @@
 #define OV7251_TIMING_FORMAT2_MIRROR	BIT(2)
 #define OV7251_PRE_ISP_00		0x5e00
 #define OV7251_PRE_ISP_00_TEST_PATTERN	BIT(7)
+#define OV7251_PLL1_PRE_DIV_REG		0x30b4
+#define OV7251_PLL1_MULT_REG		0x30b3
+#define OV7251_PLL1_DIVIDER_REG		0x30b1
+#define OV7251_PLL1_PIX_DIV_REG		0x30b0
+#define OV7251_PLL1_MIPI_DIV_REG	0x30b5
+#define OV7251_PLL2_PRE_DIV_REG		0x3098
+#define OV7251_PLL2_MULT_REG		0x3099
+#define OV7251_PLL2_DIVIDER_REG		0x309d
+#define OV7251_PLL2_SYS_DIV_REG		0x309a
+#define OV7251_PLL2_ADC_DIV_REG		0x309b
 
 struct reg_value {
 	u16 reg;
@@ -60,6 +70,36 @@ struct ov7251_mode_info {
 	struct v4l2_fract timeperframe;
 };
 
+struct ov7251_pll1_cfg {
+	unsigned int pre_div;
+	unsigned int mult;
+	unsigned int div;
+	unsigned int pix_div;
+	unsigned int mipi_div;
+};
+
+struct ov7251_pll2_cfg {
+	unsigned int pre_div;
+	unsigned int mult;
+	unsigned int div;
+	unsigned int sys_div;
+	unsigned int adc_div;
+};
+
+/*
+ * Rubbish ordering, but only PLL1 needs to have a separate configuration per
+ * link frequency and the array member needs to be last.
+ */
+struct ov7251_pll_cfgs {
+	const struct ov7251_pll2_cfg *pll2;
+	const struct ov7251_pll1_cfg *pll1[];
+};
+
+enum xclk_rate {
+	OV7251_24_MHZ,
+	OV7251_NUM_SUPPORTED_RATES
+};
+
 enum supported_link_freqs {
 	OV7251_LINK_FREQ_240_MHZ,
 	OV7251_NUM_SUPPORTED_LINK_FREQS
@@ -80,6 +120,7 @@ struct ov7251 {
 	struct regulator *core_regulator;
 	struct regulator *analog_regulator;
 
+	const struct ov7251_pll_cfgs *pll_cfgs;
 	enum supported_link_freqs link_freq_idx;
 	const struct ov7251_mode_info *current_mode;
 
@@ -106,6 +147,33 @@ static inline struct ov7251 *to_ov7251(struct v4l2_subdev *sd)
 	return container_of(sd, struct ov7251, sd);
 }
 
+static const struct ov7251_pll1_cfg ov7251_pll1_cfg_24_mhz_240_mhz = {
+	.pre_div = 0x03,
+	.mult = 0x64,
+	.div = 0x01,
+	.pix_div = 0x0a,
+	.mipi_div = 0x05,
+};
+
+static const struct ov7251_pll2_cfg ov7251_pll2_cfg_24_mhz = {
+	.pre_div = 0x04,
+	.mult = 0x28,
+	.div = 0x00,
+	.sys_div = 0x05,
+	.adc_div = 0x04,
+};
+
+static const struct ov7251_pll_cfgs ov7251_pll_cfgs_24_mhz = {
+	.pll2 = &ov7251_pll2_cfg_24_mhz,
+	.pll1 = {
+		[OV7251_LINK_FREQ_240_MHZ] = &ov7251_pll1_cfg_24_mhz_240_mhz,
+	},
+};
+
+static const struct ov7251_pll_cfgs *ov7251_pll_cfgs[] = {
+	[OV7251_24_MHZ] = &ov7251_pll_cfgs_24_mhz,
+};
+
 static const struct reg_value ov7251_global_init_setting[] = {
 	{ 0x0103, 0x01 },
 	{ 0x303b, 0x02 },
@@ -124,16 +192,6 @@ static const struct reg_value ov7251_setting_vga_30fps[] = {
 	{ 0x301c, 0xf0 },
 	{ 0x3023, 0x05 },
 	{ 0x3037, 0xf0 },
-	{ 0x3098, 0x04 }, /* pll2 pre divider */
-	{ 0x3099, 0x28 }, /* pll2 multiplier */
-	{ 0x309a, 0x05 }, /* pll2 sys divider */
-	{ 0x309b, 0x04 }, /* pll2 adc divider */
-	{ 0x309d, 0x00 }, /* pll2 divider */
-	{ 0x30b0, 0x0a }, /* pll1 pix divider */
-	{ 0x30b1, 0x01 }, /* pll1 divider */
-	{ 0x30b3, 0x64 }, /* pll1 multiplier */
-	{ 0x30b4, 0x03 }, /* pll1 pre divider */
-	{ 0x30b5, 0x05 }, /* pll1 mipi divider */
 	{ 0x3106, 0xda },
 	{ 0x3503, 0x07 },
 	{ 0x3509, 0x10 },
@@ -262,16 +320,6 @@ static const struct reg_value ov7251_setting_vga_60fps[] = {
 	{ 0x301c, 0x00 },
 	{ 0x3023, 0x05 },
 	{ 0x3037, 0xf0 },
-	{ 0x3098, 0x04 }, /* pll2 pre divider */
-	{ 0x3099, 0x28 }, /* pll2 multiplier */
-	{ 0x309a, 0x05 }, /* pll2 sys divider */
-	{ 0x309b, 0x04 }, /* pll2 adc divider */
-	{ 0x309d, 0x00 }, /* pll2 divider */
-	{ 0x30b0, 0x0a }, /* pll1 pix divider */
-	{ 0x30b1, 0x01 }, /* pll1 divider */
-	{ 0x30b3, 0x64 }, /* pll1 multiplier */
-	{ 0x30b4, 0x03 }, /* pll1 pre divider */
-	{ 0x30b5, 0x05 }, /* pll1 mipi divider */
 	{ 0x3106, 0xda },
 	{ 0x3503, 0x07 },
 	{ 0x3509, 0x10 },
@@ -400,16 +448,6 @@ static const struct reg_value ov7251_setting_vga_90fps[] = {
 	{ 0x301c, 0x00 },
 	{ 0x3023, 0x05 },
 	{ 0x3037, 0xf0 },
-	{ 0x3098, 0x04 }, /* pll2 pre divider */
-	{ 0x3099, 0x28 }, /* pll2 multiplier */
-	{ 0x309a, 0x05 }, /* pll2 sys divider */
-	{ 0x309b, 0x04 }, /* pll2 adc divider */
-	{ 0x309d, 0x00 }, /* pll2 divider */
-	{ 0x30b0, 0x0a }, /* pll1 pix divider */
-	{ 0x30b1, 0x01 }, /* pll1 divider */
-	{ 0x30b3, 0x64 }, /* pll1 multiplier */
-	{ 0x30b4, 0x03 }, /* pll1 pre divider */
-	{ 0x30b5, 0x05 }, /* pll1 mipi divider */
 	{ 0x3106, 0xda },
 	{ 0x3503, 0x07 },
 	{ 0x3509, 0x10 },
@@ -525,6 +563,10 @@ static const struct reg_value ov7251_setting_vga_90fps[] = {
 	{ 0x5001, 0x80 },
 };
 
+static const unsigned long supported_xclk_rates[] = {
+	[OV7251_24_MHZ] = 24000000,
+};
+
 static const s64 link_freq[] = {
 	[OV7251_LINK_FREQ_240_MHZ] = 240000000,
 };
@@ -696,6 +738,63 @@ static int ov7251_read_reg(struct ov7251 *ov7251, u16 reg, u8 *val)
 	return 0;
 }
 
+static int ov7251_pll_configure(struct ov7251 *ov7251)
+{
+	const struct ov7251_pll_cfgs *configs;
+	int ret;
+
+	configs = ov7251->pll_cfgs;
+
+	ret = ov7251_write_reg(ov7251, OV7251_PLL1_PRE_DIV_REG,
+			       configs->pll1[ov7251->link_freq_idx]->pre_div);
+	if (ret < 0)
+		return ret;
+
+	ret = ov7251_write_reg(ov7251, OV7251_PLL1_MULT_REG,
+			       configs->pll1[ov7251->link_freq_idx]->mult);
+	if (ret < 0)
+		return ret;
+	ret = ov7251_write_reg(ov7251, OV7251_PLL1_DIVIDER_REG,
+			       configs->pll1[ov7251->link_freq_idx]->div);
+	if (ret < 0)
+		return ret;
+
+	ret = ov7251_write_reg(ov7251, OV7251_PLL1_PIX_DIV_REG,
+			       configs->pll1[ov7251->link_freq_idx]->pix_div);
+	if (ret < 0)
+		return ret;
+
+	ret = ov7251_write_reg(ov7251, OV7251_PLL1_MIPI_DIV_REG,
+			       configs->pll1[ov7251->link_freq_idx]->mipi_div);
+	if (ret < 0)
+		return ret;
+
+	ret = ov7251_write_reg(ov7251, OV7251_PLL2_PRE_DIV_REG,
+			       configs->pll2->pre_div);
+	if (ret < 0)
+		return ret;
+
+	ret = ov7251_write_reg(ov7251, OV7251_PLL2_MULT_REG,
+			       configs->pll2->mult);
+	if (ret < 0)
+		return ret;
+
+	ret = ov7251_write_reg(ov7251, OV7251_PLL2_DIVIDER_REG,
+			       configs->pll2->div);
+	if (ret < 0)
+		return ret;
+
+	ret = ov7251_write_reg(ov7251, OV7251_PLL2_SYS_DIV_REG,
+			       configs->pll2->sys_div);
+	if (ret < 0)
+		return ret;
+
+	ret = ov7251_write_reg(ov7251, OV7251_PLL2_ADC_DIV_REG,
+			       configs->pll2->adc_div);
+
+	return ret;
+}
+
 static int ov7251_set_exposure(struct ov7251 *ov7251, s32 exposure)
 {
 	u16 reg;
@@ -1137,6 +1236,11 @@ static int ov7251_s_stream(struct v4l2_subdev *subdev, int enable)
 	mutex_lock(&ov7251->lock);
 
 	if (enable) {
+		ret = ov7251_pll_configure(ov7251);
+		if (ret)
+			return dev_err_probe(ov7251->dev, ret,
+					     "error configuring PLLs\n");
+
 		ret = ov7251_set_register_array(ov7251,
 					ov7251->current_mode->data,
 					ov7251->current_mode->data_size);
@@ -1297,6 +1401,7 @@ static int ov7251_probe(struct i2c_client *client)
 	u8 chip_id_high, chip_id_low, chip_rev;
 	s64 pixel_rate;
 	int ret;
+	int i;
 
 	ov7251 = devm_kzalloc(dev, sizeof(struct ov7251), GFP_KERNEL);
 	if (!ov7251)
@@ -1335,6 +1440,16 @@ static int ov7251_probe(struct i2c_client *client)
 		dev_err(dev, "could not set xclk frequency\n");
 		return ret;
 	}
+	for (i = 0; i < ARRAY_SIZE(supported_xclk_rates); i++)
+		if (ov7251->xclk_freq == supported_xclk_rates[i])
+			break;
+
+	if (i == ARRAY_SIZE(supported_xclk_rates))
+		return dev_err_probe(dev, -EINVAL,
+				     "clock rate %u Hz is unsupported\n",
+				     ov7251->xclk_freq);
+
+	ov7251->pll_cfgs = ov7251_pll_cfgs[i];
 
 	ov7251->io_regulator = devm_regulator_get(dev, "vdddo");
 	if (IS_ERR(ov7251->io_regulator)) {
-- 
2.25.1

[PATCH v3 07/15] media: i2c: Add support for new frequencies to ov7251

[Daniel Scally]

The OV7251 sensor is used as the IR camera sensor on the Microsoft
Surface line of tablets; this provides a 19.2MHz external clock, and
the Windows driver for this sensor configures a 319.2MHz link freq to
the CSI-2 receiver. Add the ability to support those rate to the
driver by defining a new set of PLL configs.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3:

	- Added commas to last items in arrays (Andy)

Changes in v2:

	- Added support for 319.2MHz link frequency

 drivers/media/i2c/ov7251.c | 93 +++++++++++++++++++++++++++++---------
 1 file changed, 72 insertions(+), 21 deletions(-)

diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
index 3440077e8ba9..9a026354a1bd 100644
--- a/drivers/media/i2c/ov7251.c
+++ b/drivers/media/i2c/ov7251.c
@@ -96,12 +96,14 @@ struct ov7251_pll_cfgs {
 };
 
 enum xclk_rate {
+	OV7251_19_2_MHZ,
 	OV7251_24_MHZ,
 	OV7251_NUM_SUPPORTED_RATES
 };
 
 enum supported_link_freqs {
 	OV7251_LINK_FREQ_240_MHZ,
+	OV7251_LINK_FREQ_319_2_MHZ,
 	OV7251_NUM_SUPPORTED_LINK_FREQS
 };
 
@@ -147,6 +149,22 @@ static inline struct ov7251 *to_ov7251(struct v4l2_subdev *sd)
 	return container_of(sd, struct ov7251, sd);
 }
 
+static const struct ov7251_pll1_cfg ov7251_pll1_cfg_19_2_mhz_240_mhz = {
+	.pre_div = 0x03,
+	.mult = 0x4b,
+	.div = 0x01,
+	.pix_div = 0x0a,
+	.mipi_div = 0x05,
+};
+
+static const struct ov7251_pll1_cfg ov7251_pll1_cfg_19_2_mhz_319_2_mhz = {
+	.pre_div = 0x01,
+	.mult = 0x85,
+	.div = 0x04,
+	.pix_div = 0x0a,
+	.mipi_div = 0x05,
+};
+
 static const struct ov7251_pll1_cfg ov7251_pll1_cfg_24_mhz_240_mhz = {
 	.pre_div = 0x03,
 	.mult = 0x64,
@@ -155,6 +173,22 @@ static const struct ov7251_pll1_cfg ov7251_pll1_cfg_24_mhz_240_mhz = {
 	.mipi_div = 0x05,
 };
 
+static const struct ov7251_pll1_cfg ov7251_pll1_cfg_24_mhz_319_2_mhz = {
+	.pre_div = 0x05,
+	.mult = 0x85,
+	.div = 0x02,
+	.pix_div = 0x0a,
+	.mipi_div = 0x05,
+};
+
+static const struct ov7251_pll2_cfg ov7251_pll2_cfg_19_2_mhz = {
+	.pre_div = 0x04,
+	.mult = 0x32,
+	.div = 0x00,
+	.sys_div = 0x05,
+	.adc_div = 0x04,
+};
+
 static const struct ov7251_pll2_cfg ov7251_pll2_cfg_24_mhz = {
 	.pre_div = 0x04,
 	.mult = 0x28,
@@ -163,14 +197,24 @@ static const struct ov7251_pll2_cfg ov7251_pll2_cfg_24_mhz = {
 	.adc_div = 0x04,
 };
 
+static const struct ov7251_pll_cfgs ov7251_pll_cfgs_19_2_mhz = {
+	.pll2 = &ov7251_pll2_cfg_19_2_mhz,
+	.pll1 = {
+		[OV7251_LINK_FREQ_240_MHZ] = &ov7251_pll1_cfg_19_2_mhz_240_mhz,
+		[OV7251_LINK_FREQ_319_2_MHZ] = &ov7251_pll1_cfg_19_2_mhz_319_2_mhz,
+	},
+};
+
 static const struct ov7251_pll_cfgs ov7251_pll_cfgs_24_mhz = {
 	.pll2 = &ov7251_pll2_cfg_24_mhz,
 	.pll1 = {
 		[OV7251_LINK_FREQ_240_MHZ] = &ov7251_pll1_cfg_24_mhz_240_mhz,
+		[OV7251_LINK_FREQ_319_2_MHZ] = &ov7251_pll1_cfg_19_2_mhz_319_2_mhz,
 	},
 };
 
 static const struct ov7251_pll_cfgs *ov7251_pll_cfgs[] = {
+	[OV7251_19_2_MHZ] = &ov7251_pll_cfgs_19_2_mhz,
 	[OV7251_24_MHZ] = &ov7251_pll_cfgs_24_mhz,
 };
 
@@ -564,15 +608,18 @@ static const struct reg_value ov7251_setting_vga_90fps[] = {
 };
 
 static const unsigned long supported_xclk_rates[] = {
+	[OV7251_19_2_MHZ] = 19200000,
 	[OV7251_24_MHZ] = 24000000,
 };
 
 static const s64 link_freq[] = {
 	[OV7251_LINK_FREQ_240_MHZ] = 240000000,
+	[OV7251_LINK_FREQ_319_2_MHZ] = 319200000,
 };
 
 static const s64 pixel_rates[] = {
 	[OV7251_LINK_FREQ_240_MHZ] = 48000000,
+	[OV7251_LINK_FREQ_319_2_MHZ] = 63840000,
 };
 
 static const struct ov7251_mode_info ov7251_mode_info_data[] = {
@@ -1399,6 +1446,7 @@ static int ov7251_probe(struct i2c_client *client)
 	struct device *dev = &client->dev;
 	struct ov7251 *ov7251;
 	u8 chip_id_high, chip_id_low, chip_rev;
+	unsigned int rate = 0, clk_rate = 0;
 	s64 pixel_rate;
 	int ret;
 	int i;
@@ -1415,31 +1463,34 @@ static int ov7251_probe(struct i2c_client *client)
 		return ret;
 
 	/* get system clock (xclk) */
-	ov7251->xclk = devm_clk_get(dev, "xclk");
-	if (IS_ERR(ov7251->xclk)) {
-		dev_err(dev, "could not get xclk");
-		return PTR_ERR(ov7251->xclk);
-	}
-
+	ov7251->xclk = devm_clk_get_optional(dev, NULL);
+	if (IS_ERR(ov7251->xclk))
+		return dev_err_probe(dev, PTR_ERR(ov7251->xclk),
+				     "could not get xclk");
+
+	/*
+	 * We could have either a 24MHz or 19.2MHz clock rate from either DT or
+	 * ACPI. We also need to support the IPU3 case which will have both an
+	 * external clock AND a clock-frequency property.
+	 */
 	ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
-				       &ov7251->xclk_freq);
-	if (ret) {
-		dev_err(dev, "could not get xclk frequency\n");
-		return ret;
-	}
+				       &rate);
+	if (ret && !ov7251->xclk)
+		return dev_err_probe(dev, ret, "invalid clock config\n");
 
-	/* external clock must be 24MHz, allow 1% tolerance */
-	if (ov7251->xclk_freq < 23760000 || ov7251->xclk_freq > 24240000) {
-		dev_err(dev, "external clock frequency %u is not supported\n",
-			ov7251->xclk_freq);
-		return -EINVAL;
-	}
+	clk_rate = clk_get_rate(ov7251->xclk);
+	ov7251->xclk_freq = clk_rate ? clk_rate : rate;
 
-	ret = clk_set_rate(ov7251->xclk, ov7251->xclk_freq);
-	if (ret) {
-		dev_err(dev, "could not set xclk frequency\n");
-		return ret;
+	if (ov7251->xclk_freq == 0)
+		return dev_err_probe(dev, -EINVAL, "invalid clock frequency\n");
+
+	if (!ret && ov7251->xclk) {
+		ret = clk_set_rate(ov7251->xclk, rate);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "failed to set clock rate\n");
 	}
+
 	for (i = 0; i < ARRAY_SIZE(supported_xclk_rates); i++)
 		if (ov7251->xclk_freq == supported_xclk_rates[i])
 			break;
-- 
2.25.1

Re: [PATCH v3 07/15] media: i2c: Add support for new frequencies to ov7251

[kernel test robot]

Hi Daniel,

I love your patch! Perhaps something to improve:

[auto build test WARNING on media-tree/master]
[also build test WARNING on v5.18-rc5]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Daniel-Scally/Support-OVTI7251-on-Microsoft-Surface-line/20220505-063608
base:   git://linuxtv.org/media_tree.git master
config: hexagon-buildonly-randconfig-r002-20220501 (https://download.01.org/0day-ci/archive/20220505/202205052045.9ovlO4pA-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 5e004fb787698440a387750db7f8028e7cb14cfc)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/4f69999ece8bcd0a4e1e616cac23441d2606348d
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Daniel-Scally/Support-OVTI7251-on-Microsoft-Surface-line/20220505-063608
        git checkout 4f69999ece8bcd0a4e1e616cac23441d2606348d
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/media/i2c/ov7251.c:176:37: warning: unused variable 'ov7251_pll1_cfg_24_mhz_319_2_mhz' [-Wunused-const-variable]
   static const struct ov7251_pll1_cfg ov7251_pll1_cfg_24_mhz_319_2_mhz = {
                                       ^
   1 warning generated.


vim +/ov7251_pll1_cfg_24_mhz_319_2_mhz +176 drivers/media/i2c/ov7251.c

   175	
 > 176	static const struct ov7251_pll1_cfg ov7251_pll1_cfg_24_mhz_319_2_mhz = {
   177		.pre_div = 0x05,
   178		.mult = 0x85,
   179		.div = 0x02,
   180		.pix_div = 0x0a,
   181		.mipi_div = 0x05,
   182	};
   183	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

[PATCH v3 08/15] media: i2c: Add ov7251_detect_chip()

[Daniel Scally]

.probe() is quite busy for this driver; make it cleaner by moving the
chip verification to a dedicated function.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3:

	- None

Changes in v2:

	- None

 drivers/media/i2c/ov7251.c | 62 +++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 27 deletions(-)

diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
index 9a026354a1bd..ff31629839f9 100644
--- a/drivers/media/i2c/ov7251.c
+++ b/drivers/media/i2c/ov7251.c
@@ -1441,11 +1441,43 @@ static int ov7251_check_hwcfg(struct ov7251 *ov7251)
 	return ret;
 }
 
+static int ov7251_detect_chip(struct ov7251 *ov7251)
+{
+	u8 chip_id_high, chip_id_low, chip_rev;
+	int ret;
+
+	ret = ov7251_read_reg(ov7251, OV7251_CHIP_ID_HIGH, &chip_id_high);
+	if (ret < 0 || chip_id_high != OV7251_CHIP_ID_HIGH_BYTE)
+		return dev_err_probe(ov7251->dev, -ENODEV,
+				     "could not read ID high\n");
+
+	ret = ov7251_read_reg(ov7251, OV7251_CHIP_ID_LOW, &chip_id_low);
+	if (ret < 0 || chip_id_low != OV7251_CHIP_ID_LOW_BYTE)
+		return dev_err_probe(ov7251->dev, -ENODEV,
+				     "could not read ID low\n");
+
+	ret = ov7251_read_reg(ov7251, OV7251_SC_GP_IO_IN1, &chip_rev);
+	if (ret < 0)
+		return dev_err_probe(ov7251->dev, -ENODEV,
+				     "could not read revision\n");
+	chip_rev >>= 4;
+
+	dev_info(ov7251->dev,
+		 "OV7251 revision %x (%s) detected at address 0x%02x\n",
+		 chip_rev,
+		 chip_rev == 0x4 ? "1A / 1B" :
+		 chip_rev == 0x5 ? "1C / 1D" :
+		 chip_rev == 0x6 ? "1E" :
+		 chip_rev == 0x7 ? "1F" : "unknown",
+		 ov7251->i2c_client->addr);
+
+	return 0;
+}
+
 static int ov7251_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
 	struct ov7251 *ov7251;
-	u8 chip_id_high, chip_id_low, chip_rev;
 	unsigned int rate = 0, clk_rate = 0;
 	s64 pixel_rate;
 	int ret;
@@ -1588,34 +1620,10 @@ static int ov7251_probe(struct i2c_client *client)
 		goto free_entity;
 	}
 
-	ret = ov7251_read_reg(ov7251, OV7251_CHIP_ID_HIGH, &chip_id_high);
-	if (ret < 0 || chip_id_high != OV7251_CHIP_ID_HIGH_BYTE) {
-		dev_err(dev, "could not read ID high\n");
-		ret = -ENODEV;
-		goto power_down;
-	}
-	ret = ov7251_read_reg(ov7251, OV7251_CHIP_ID_LOW, &chip_id_low);
-	if (ret < 0 || chip_id_low != OV7251_CHIP_ID_LOW_BYTE) {
-		dev_err(dev, "could not read ID low\n");
-		ret = -ENODEV;
-		goto power_down;
-	}
-
-	ret = ov7251_read_reg(ov7251, OV7251_SC_GP_IO_IN1, &chip_rev);
-	if (ret < 0) {
-		dev_err(dev, "could not read revision\n");
-		ret = -ENODEV;
+	ret = ov7251_detect_chip(ov7251);
+	if (ret)
 		goto power_down;
-	}
-	chip_rev >>= 4;
 
-	dev_info(dev, "OV7251 revision %x (%s) detected at address 0x%02x\n",
-		 chip_rev,
-		 chip_rev == 0x4 ? "1A / 1B" :
-		 chip_rev == 0x5 ? "1C / 1D" :
-		 chip_rev == 0x6 ? "1E" :
-		 chip_rev == 0x7 ? "1F" : "unknown",
-		 client->addr);
 
 	ret = ov7251_read_reg(ov7251, OV7251_PRE_ISP_00,
 			      &ov7251->pre_isp_00);
-- 
2.25.1

[PATCH v3 09/15] media: i2c: Add pm_runtime support to ov7251

[Daniel Scally]

Add pm_runtime support to the ov7251 driver.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3:

	- None

Changes in v2:

	- Switched ov7251_set_power_[on|off]() to take a struct device * as the
	input parameter and used those functions for pm_runtime instead of
	adding wrappers (Sakari)

 drivers/media/i2c/ov7251.c | 81 ++++++++++++++++++++++++++++----------
 1 file changed, 60 insertions(+), 21 deletions(-)

diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
index ff31629839f9..a9e890181200 100644
--- a/drivers/media/i2c/ov7251.c
+++ b/drivers/media/i2c/ov7251.c
@@ -15,6 +15,7 @@
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
+#include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/types.h>
@@ -883,8 +884,11 @@ static int ov7251_set_register_array(struct ov7251 *ov7251,
 	return 0;
 }
 
-static int ov7251_set_power_on(struct ov7251 *ov7251)
+static int ov7251_set_power_on(struct device *dev)
 {
+	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct ov7251 *ov7251 = to_ov7251(sd);
 	int ret;
 	u32 wait_us;
 
@@ -909,11 +913,17 @@ static int ov7251_set_power_on(struct ov7251 *ov7251)
 	return 0;
 }
 
-static void ov7251_set_power_off(struct ov7251 *ov7251)
+static int ov7251_set_power_off(struct device *dev)
 {
+	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct ov7251 *ov7251 = to_ov7251(sd);
+
 	clk_disable_unprepare(ov7251->xclk);
 	gpiod_set_value_cansleep(ov7251->enable_gpio, 0);
 	ov7251_regulators_disable(ov7251);
+
+	return 0;
 }
 
 static int ov7251_s_power(struct v4l2_subdev *sd, int on)
@@ -928,7 +938,7 @@ static int ov7251_s_power(struct v4l2_subdev *sd, int on)
 		goto exit;
 
 	if (on) {
-		ret = ov7251_set_power_on(ov7251);
+		ret = ov7251_set_power_on(ov7251->dev);
 		if (ret < 0)
 			goto exit;
 
@@ -937,13 +947,13 @@ static int ov7251_s_power(struct v4l2_subdev *sd, int on)
 					ARRAY_SIZE(ov7251_global_init_setting));
 		if (ret < 0) {
 			dev_err(ov7251->dev, "could not set init registers\n");
-			ov7251_set_power_off(ov7251);
+			ov7251_set_power_off(ov7251->dev);
 			goto exit;
 		}
 
 		ov7251->power_on = true;
 	} else {
-		ov7251_set_power_off(ov7251);
+		ov7251_set_power_off(ov7251->dev);
 		ov7251->power_on = false;
 	}
 
@@ -1017,7 +1027,7 @@ static int ov7251_s_ctrl(struct v4l2_ctrl *ctrl)
 
 	/* v4l2_ctrl_lock() locks our mutex */
 
-	if (!ov7251->power_on)
+	if (!pm_runtime_get_if_in_use(ov7251->dev))
 		return 0;
 
 	switch (ctrl->id) {
@@ -1041,6 +1051,8 @@ static int ov7251_s_ctrl(struct v4l2_ctrl *ctrl)
 		break;
 	}
 
+	pm_runtime_put(ov7251->dev);
+
 	return ret;
 }
 
@@ -1283,10 +1295,15 @@ static int ov7251_s_stream(struct v4l2_subdev *subdev, int enable)
 	mutex_lock(&ov7251->lock);
 
 	if (enable) {
+		ret = pm_runtime_get_sync(ov7251->dev);
+		if (ret < 0)
+			goto unlock_out;
+
 		ret = ov7251_pll_configure(ov7251);
-		if (ret)
-			return dev_err_probe(ov7251->dev, ret,
-					     "error configuring PLLs\n");
+		if (ret) {
+			dev_err(ov7251->dev, "error configuring PLLs\n");
+			goto err_power_down;
+		}
 
 		ret = ov7251_set_register_array(ov7251,
 					ov7251->current_mode->data,
@@ -1295,23 +1312,29 @@ static int ov7251_s_stream(struct v4l2_subdev *subdev, int enable)
 			dev_err(ov7251->dev, "could not set mode %dx%d\n",
 				ov7251->current_mode->width,
 				ov7251->current_mode->height);
-			goto exit;
+			goto err_power_down;
 		}
 		ret = __v4l2_ctrl_handler_setup(&ov7251->ctrls);
 		if (ret < 0) {
 			dev_err(ov7251->dev, "could not sync v4l2 controls\n");
-			goto exit;
+			goto err_power_down;
 		}
 		ret = ov7251_write_reg(ov7251, OV7251_SC_MODE_SELECT,
 				       OV7251_SC_MODE_SELECT_STREAMING);
+		if (ret)
+			goto err_power_down;
 	} else {
 		ret = ov7251_write_reg(ov7251, OV7251_SC_MODE_SELECT,
 				       OV7251_SC_MODE_SELECT_SW_STANDBY);
+		pm_runtime_put(ov7251->dev);
 	}
 
-exit:
+unlock_out:
 	mutex_unlock(&ov7251->lock);
+	return ret;
 
+err_power_down:
+	pm_runtime_put_noidle(ov7251->dev);
 	return ret;
 }
 
@@ -1614,23 +1637,24 @@ static int ov7251_probe(struct i2c_client *client)
 		goto free_ctrl;
 	}
 
-	ret = ov7251_s_power(&ov7251->sd, true);
-	if (ret < 0) {
-		dev_err(dev, "could not power up OV7251\n");
+	ret = ov7251_set_power_on(ov7251->dev);
+	if (ret)
 		goto free_entity;
-	}
 
 	ret = ov7251_detect_chip(ov7251);
 	if (ret)
 		goto power_down;
 
+	pm_runtime_set_active(&client->dev);
+	pm_runtime_get_noresume(&client->dev);
+	pm_runtime_enable(&client->dev);
 
 	ret = ov7251_read_reg(ov7251, OV7251_PRE_ISP_00,
 			      &ov7251->pre_isp_00);
 	if (ret < 0) {
 		dev_err(dev, "could not read test pattern value\n");
 		ret = -ENODEV;
-		goto power_down;
+		goto err_pm_runtime;
 	}
 
 	ret = ov7251_read_reg(ov7251, OV7251_TIMING_FORMAT1,
@@ -1638,7 +1662,7 @@ static int ov7251_probe(struct i2c_client *client)
 	if (ret < 0) {
 		dev_err(dev, "could not read vflip value\n");
 		ret = -ENODEV;
-		goto power_down;
+		goto err_pm_runtime;
 	}
 
 	ret = ov7251_read_reg(ov7251, OV7251_TIMING_FORMAT2,
@@ -1646,10 +1670,12 @@ static int ov7251_probe(struct i2c_client *client)
 	if (ret < 0) {
 		dev_err(dev, "could not read hflip value\n");
 		ret = -ENODEV;
-		goto power_down;
+		goto err_pm_runtime;
 	}
 
-	ov7251_s_power(&ov7251->sd, false);
+	pm_runtime_set_autosuspend_delay(&client->dev, 1000);
+	pm_runtime_use_autosuspend(&client->dev);
+	pm_runtime_put_autosuspend(&client->dev);
 
 	ret = v4l2_async_register_subdev(&ov7251->sd);
 	if (ret < 0) {
@@ -1661,8 +1687,11 @@ static int ov7251_probe(struct i2c_client *client)
 
 	return 0;
 
+err_pm_runtime:
+	pm_runtime_disable(ov7251->dev);
+	pm_runtime_put_noidle(ov7251->dev);
 power_down:
-	ov7251_s_power(&ov7251->sd, false);
+	ov7251_set_power_off(ov7251->dev);
 free_entity:
 	media_entity_cleanup(&ov7251->sd.entity);
 free_ctrl:
@@ -1682,9 +1711,18 @@ static int ov7251_remove(struct i2c_client *client)
 	v4l2_ctrl_handler_free(&ov7251->ctrls);
 	mutex_destroy(&ov7251->lock);
 
+	pm_runtime_disable(ov7251->dev);
+	if (!pm_runtime_status_suspended(ov7251->dev))
+		ov7251_set_power_off(ov7251->dev);
+	pm_runtime_set_suspended(ov7251->dev);
+
 	return 0;
 }
 
+static const struct dev_pm_ops ov7251_pm_ops = {
+	SET_RUNTIME_PM_OPS(ov7251_set_power_off, ov7251_set_power_on, NULL)
+};
+
 static const struct of_device_id ov7251_of_match[] = {
 	{ .compatible = "ovti,ov7251" },
 	{ /* sentinel */ }
@@ -1702,6 +1740,7 @@ static struct i2c_driver ov7251_i2c_driver = {
 		.of_match_table = ov7251_of_match,
 		.acpi_match_table = ov7251_acpi_match,
 		.name  = "ov7251",
+		.pm = &ov7251_pm_ops,
 	},
 	.probe_new  = ov7251_probe,
 	.remove = ov7251_remove,
-- 
2.25.1

[PATCH v3 10/15] media: i2c: Remove .s_power() from ov7251

[Daniel Scally]

The .s_power() callback is deprecated, and now that we have pm_runtime
functionality in the driver there's no further use for it. Delete the
function.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3:

	- None

Changes in v2:

	- Set the global init registers as part of ov7251_set_power_on() (Sakari)

 drivers/media/i2c/ov7251.c | 53 +++++++-------------------------------
 1 file changed, 10 insertions(+), 43 deletions(-)

diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
index a9e890181200..4f8c797839f6 100644
--- a/drivers/media/i2c/ov7251.c
+++ b/drivers/media/i2c/ov7251.c
@@ -910,7 +910,16 @@ static int ov7251_set_power_on(struct device *dev)
 			       DIV_ROUND_UP(ov7251->xclk_freq, 1000));
 	usleep_range(wait_us, wait_us + 1000);
 
-	return 0;
+	ret = ov7251_set_register_array(ov7251,
+					ov7251_global_init_setting,
+					ARRAY_SIZE(ov7251_global_init_setting));
+	if (ret < 0) {
+		dev_err(ov7251->dev, "error during global init\n");
+		ov7251_regulators_disable(ov7251);
+		return ret;
+	}
+
+	return ret;
 }
 
 static int ov7251_set_power_off(struct device *dev)
@@ -926,43 +935,6 @@ static int ov7251_set_power_off(struct device *dev)
 	return 0;
 }
 
-static int ov7251_s_power(struct v4l2_subdev *sd, int on)
-{
-	struct ov7251 *ov7251 = to_ov7251(sd);
-	int ret = 0;
-
-	mutex_lock(&ov7251->lock);
-
-	/* If the power state is not modified - no work to do. */
-	if (ov7251->power_on == !!on)
-		goto exit;
-
-	if (on) {
-		ret = ov7251_set_power_on(ov7251->dev);
-		if (ret < 0)
-			goto exit;
-
-		ret = ov7251_set_register_array(ov7251,
-					ov7251_global_init_setting,
-					ARRAY_SIZE(ov7251_global_init_setting));
-		if (ret < 0) {
-			dev_err(ov7251->dev, "could not set init registers\n");
-			ov7251_set_power_off(ov7251->dev);
-			goto exit;
-		}
-
-		ov7251->power_on = true;
-	} else {
-		ov7251_set_power_off(ov7251->dev);
-		ov7251->power_on = false;
-	}
-
-exit:
-	mutex_unlock(&ov7251->lock);
-
-	return ret;
-}
-
 static int ov7251_set_hflip(struct ov7251 *ov7251, s32 value)
 {
 	u8 val = ov7251->timing_format2;
@@ -1387,10 +1359,6 @@ static int ov7251_set_frame_interval(struct v4l2_subdev *subdev,
 	return ret;
 }
 
-static const struct v4l2_subdev_core_ops ov7251_core_ops = {
-	.s_power = ov7251_s_power,
-};
-
 static const struct v4l2_subdev_video_ops ov7251_video_ops = {
 	.s_stream = ov7251_s_stream,
 	.g_frame_interval = ov7251_get_frame_interval,
@@ -1408,7 +1376,6 @@ static const struct v4l2_subdev_pad_ops ov7251_subdev_pad_ops = {
 };
 
 static const struct v4l2_subdev_ops ov7251_subdev_ops = {
-	.core = &ov7251_core_ops,
 	.video = &ov7251_video_ops,
 	.pad = &ov7251_subdev_pad_ops,
 };
-- 
2.25.1

[PATCH v3 11/15] media: ipu3-cio2: Add INT347E to cio2-bridge

[Daniel Scally]

The OVTI7251 sensor can be found on x86 laptops with an IPU3, and so
needs to be supported by the cio2-bridge. Add it to the table of
supported sensors.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3:

	- None

Changes in v2:

	- Switched to 319.2MHz link frequency

 drivers/media/pci/intel/ipu3/cio2-bridge.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c
index 7ccb7b6eaa82..df6c94da2f6a 100644
--- a/drivers/media/pci/intel/ipu3/cio2-bridge.c
+++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
@@ -25,6 +25,8 @@ static const struct cio2_sensor_config cio2_supported_sensors[] = {
 	CIO2_SENSOR_CONFIG("INT33BE", 1, 419200000),
 	/* Omnivision OV8865 */
 	CIO2_SENSOR_CONFIG("INT347A", 1, 360000000),
+	/* Omnivision OV7251 */
+	CIO2_SENSOR_CONFIG("INT347E", 1, 319200000),
 	/* Omnivision OV2680 */
 	CIO2_SENSOR_CONFIG("OVTI2680", 0),
 };
-- 
2.25.1

[PATCH v3 12/15] media: i2c: Extend .get_selection() for ov7251

[Daniel Scally]

Extend the .get_selection() callback to support other values for
sel->target, primarily to satisfy libcamera's requirements.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3:

	- New patch

 drivers/media/i2c/ov7251.c | 35 +++++++++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
index 4f8c797839f6..40e42d19cddd 100644
--- a/drivers/media/i2c/ov7251.c
+++ b/drivers/media/i2c/ov7251.c
@@ -54,6 +54,13 @@
 #define OV7251_PLL2_SYS_DIV_REG		0x309a
 #define OV7251_PLL2_ADC_DIV_REG		0x309b
 
+#define OV7251_NATIVE_WIDTH		656
+#define OV7251_NATIVE_HEIGHT		496
+#define OV7251_ACTIVE_START_LEFT	4
+#define OV7251_ACTIVE_START_TOP		4
+#define OV7251_ACTIVE_WIDTH		648
+#define OV7251_ACTIVE_HEIGHT		488
+
 struct reg_value {
 	u16 reg;
 	u8 val;
@@ -1248,13 +1255,29 @@ static int ov7251_get_selection(struct v4l2_subdev *sd,
 {
 	struct ov7251 *ov7251 = to_ov7251(sd);
 
-	if (sel->target != V4L2_SEL_TGT_CROP)
-		return -EINVAL;
-
+	switch (sel->target) {
+	case V4L2_SEL_TGT_CROP_DEFAULT:
+	case V4L2_SEL_TGT_CROP:
 	mutex_lock(&ov7251->lock);
-	sel->r = *__ov7251_get_pad_crop(ov7251, sd_state, sel->pad,
-					sel->which);
-	mutex_unlock(&ov7251->lock);
+		sel->r = *__ov7251_get_pad_crop(ov7251, sd_state, sel->pad,
+						sel->which);
+		mutex_unlock(&ov7251->lock);
+		break;
+	case V4L2_SEL_TGT_NATIVE_SIZE:
+		sel->r.top = 0;
+		sel->r.left = 0;
+		sel->r.width = OV7251_NATIVE_WIDTH;
+		sel->r.height = OV7251_NATIVE_HEIGHT;
+		break;
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+		sel->r.top = OV7251_ACTIVE_START_TOP;
+		sel->r.left = OV7251_ACTIVE_START_LEFT;
+		sel->r.width = OV7251_ACTIVE_WIDTH;
+		sel->r.height = OV7251_ACTIVE_HEIGHT;
+		break;
+	default:
+		return -EINVAL;
+	}
 
 	return 0;
 }
-- 
2.25.1

[PATCH v3 13/15] media: i2c: add ov7251_init_ctrls()

[Daniel Scally]

V4L2 controls initialisation takes up a sizeable portion of the
driver's .probe() function. To keep things neat, move it to a
dedicated function.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3:

	- New patch

 drivers/media/i2c/ov7251.c | 93 +++++++++++++++++++++-----------------
 1 file changed, 52 insertions(+), 41 deletions(-)

diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
index 40e42d19cddd..b7d89ad49887 100644
--- a/drivers/media/i2c/ov7251.c
+++ b/drivers/media/i2c/ov7251.c
@@ -1487,12 +1487,58 @@ static int ov7251_detect_chip(struct ov7251 *ov7251)
 	return 0;
 }
 
+static int ov7251_init_ctrls(struct ov7251 *ov7251)
+{
+	s64 pixel_rate;
+
+	v4l2_ctrl_handler_init(&ov7251->ctrls, 7);
+	ov7251->ctrls.lock = &ov7251->lock;
+
+	v4l2_ctrl_new_std(&ov7251->ctrls, &ov7251_ctrl_ops,
+			  V4L2_CID_HFLIP, 0, 1, 1, 0);
+	v4l2_ctrl_new_std(&ov7251->ctrls, &ov7251_ctrl_ops,
+			  V4L2_CID_VFLIP, 0, 1, 1, 0);
+	ov7251->exposure = v4l2_ctrl_new_std(&ov7251->ctrls, &ov7251_ctrl_ops,
+					     V4L2_CID_EXPOSURE, 1, 32, 1, 32);
+	ov7251->gain = v4l2_ctrl_new_std(&ov7251->ctrls, &ov7251_ctrl_ops,
+					 V4L2_CID_GAIN, 16, 1023, 1, 16);
+	v4l2_ctrl_new_std_menu_items(&ov7251->ctrls, &ov7251_ctrl_ops,
+				     V4L2_CID_TEST_PATTERN,
+				     ARRAY_SIZE(ov7251_test_pattern_menu) - 1,
+				     0, 0, ov7251_test_pattern_menu);
+
+	pixel_rate = pixel_rates[ov7251->link_freq_idx];
+	ov7251->pixel_clock = v4l2_ctrl_new_std(&ov7251->ctrls,
+						&ov7251_ctrl_ops,
+						V4L2_CID_PIXEL_RATE,
+						pixel_rate, INT_MAX,
+						pixel_rate, pixel_rate);
+	ov7251->link_freq = v4l2_ctrl_new_int_menu(&ov7251->ctrls,
+						   &ov7251_ctrl_ops,
+						   V4L2_CID_LINK_FREQ,
+						   ARRAY_SIZE(link_freq) - 1,
+						   ov7251->link_freq_idx,
+						   link_freq);
+	if (ov7251->link_freq)
+		ov7251->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+	if (ov7251->pixel_clock)
+		ov7251->pixel_clock->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+	ov7251->sd.ctrl_handler = &ov7251->ctrls;
+
+	if (ov7251->ctrls.error) {
+		v4l2_ctrl_handler_free(&ov7251->ctrls);
+		return ov7251->ctrls.error;
+	}
+
+	return 0;
+}
+
 static int ov7251_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
 	struct ov7251 *ov7251;
 	unsigned int rate = 0, clk_rate = 0;
-	s64 pixel_rate;
 	int ret;
 	int i;
 
@@ -1573,46 +1619,10 @@ static int ov7251_probe(struct i2c_client *client)
 
 	mutex_init(&ov7251->lock);
 
-	v4l2_ctrl_handler_init(&ov7251->ctrls, 7);
-	ov7251->ctrls.lock = &ov7251->lock;
-
-	v4l2_ctrl_new_std(&ov7251->ctrls, &ov7251_ctrl_ops,
-			  V4L2_CID_HFLIP, 0, 1, 1, 0);
-	v4l2_ctrl_new_std(&ov7251->ctrls, &ov7251_ctrl_ops,
-			  V4L2_CID_VFLIP, 0, 1, 1, 0);
-	ov7251->exposure = v4l2_ctrl_new_std(&ov7251->ctrls, &ov7251_ctrl_ops,
-					     V4L2_CID_EXPOSURE, 1, 32, 1, 32);
-	ov7251->gain = v4l2_ctrl_new_std(&ov7251->ctrls, &ov7251_ctrl_ops,
-					 V4L2_CID_GAIN, 16, 1023, 1, 16);
-	v4l2_ctrl_new_std_menu_items(&ov7251->ctrls, &ov7251_ctrl_ops,
-				     V4L2_CID_TEST_PATTERN,
-				     ARRAY_SIZE(ov7251_test_pattern_menu) - 1,
-				     0, 0, ov7251_test_pattern_menu);
-
-	pixel_rate = pixel_rates[ov7251->link_freq_idx];
-	ov7251->pixel_clock = v4l2_ctrl_new_std(&ov7251->ctrls,
-						&ov7251_ctrl_ops,
-						V4L2_CID_PIXEL_RATE,
-						pixel_rate, INT_MAX,
-						pixel_rate, pixel_rate);
-	ov7251->link_freq = v4l2_ctrl_new_int_menu(&ov7251->ctrls,
-						   &ov7251_ctrl_ops,
-						   V4L2_CID_LINK_FREQ,
-						   ARRAY_SIZE(link_freq) - 1,
-						   ov7251->link_freq_idx,
-						   link_freq);
-	if (ov7251->link_freq)
-		ov7251->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
-	if (ov7251->pixel_clock)
-		ov7251->pixel_clock->flags |= V4L2_CTRL_FLAG_READ_ONLY;
-
-	ov7251->sd.ctrl_handler = &ov7251->ctrls;
-
-	if (ov7251->ctrls.error) {
-		dev_err(dev, "%s: control initialization error %d\n",
-			__func__, ov7251->ctrls.error);
-		ret = ov7251->ctrls.error;
-		goto free_ctrl;
+	ret = ov7251_init_ctrls(ov7251);
+	if (ret) {
+		dev_err_probe(dev, ret, "error during v4l2 ctrl init\n");
+		goto destroy_mutex;
 	}
 
 	v4l2_i2c_subdev_init(&ov7251->sd, client, &ov7251_subdev_ops);
@@ -1686,6 +1696,7 @@ static int ov7251_probe(struct i2c_client *client)
 	media_entity_cleanup(&ov7251->sd.entity);
 free_ctrl:
 	v4l2_ctrl_handler_free(&ov7251->ctrls);
+destroy_mutex:
 	mutex_destroy(&ov7251->lock);
 
 	return ret;
-- 
2.25.1

[PATCH v3 14/15] media: i2c: Add hblank control to ov7251

[Daniel Scally]

Add a hblank control to the ov7251 driver. This necessitates setting
a default mode, for which I am simply picking the first available.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3:

	- New patch

 drivers/media/i2c/ov7251.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
index b7d89ad49887..003a7a5ae038 100644
--- a/drivers/media/i2c/ov7251.c
+++ b/drivers/media/i2c/ov7251.c
@@ -61,6 +61,8 @@
 #define OV7251_ACTIVE_WIDTH		648
 #define OV7251_ACTIVE_HEIGHT		488
 
+#define OV7251_FIXED_PPL		928
+
 struct reg_value {
 	u16 reg;
 	u8 val;
@@ -139,6 +141,7 @@ struct ov7251 {
 	struct v4l2_ctrl *link_freq;
 	struct v4l2_ctrl *exposure;
 	struct v4l2_ctrl *gain;
+	struct v4l2_ctrl *hblank;
 
 	/* Cached register values */
 	u8 aec_pk_manual;
@@ -1490,6 +1493,7 @@ static int ov7251_detect_chip(struct ov7251 *ov7251)
 static int ov7251_init_ctrls(struct ov7251 *ov7251)
 {
 	s64 pixel_rate;
+	int hblank;
 
 	v4l2_ctrl_handler_init(&ov7251->ctrls, 7);
 	ov7251->ctrls.lock = &ov7251->lock;
@@ -1524,6 +1528,13 @@ static int ov7251_init_ctrls(struct ov7251 *ov7251)
 	if (ov7251->pixel_clock)
 		ov7251->pixel_clock->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
+	hblank = OV7251_FIXED_PPL - ov7251->current_mode->width;
+	ov7251->hblank = v4l2_ctrl_new_std(&ov7251->ctrls, &ov7251_ctrl_ops,
+					   V4L2_CID_HBLANK, hblank, hblank, 1,
+					   hblank);
+	if (ov7251->hblank)
+		ov7251->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
 	ov7251->sd.ctrl_handler = &ov7251->ctrls;
 
 	if (ov7251->ctrls.error) {
@@ -1619,6 +1630,7 @@ static int ov7251_probe(struct i2c_client *client)
 
 	mutex_init(&ov7251->lock);
 
+	ov7251->current_mode = &ov7251_mode_info_data[0];
 	ret = ov7251_init_ctrls(ov7251);
 	if (ret) {
 		dev_err_probe(dev, ret, "error during v4l2 ctrl init\n");
-- 
2.25.1

[PATCH v3 15/15] media: i2c: Add vblank control to ov7251 driver

[Daniel Scally]

Add a vblank control to the ov7251 driver.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3:

	- New patch

 drivers/media/i2c/ov7251.c | 53 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
index 003a7a5ae038..dc9d4e08efae 100644
--- a/drivers/media/i2c/ov7251.c
+++ b/drivers/media/i2c/ov7251.c
@@ -62,6 +62,10 @@
 #define OV7251_ACTIVE_HEIGHT		488
 
 #define OV7251_FIXED_PPL		928
+#define OV7251_TIMING_VTS_REG		0x380e
+#define OV7251_TIMING_MIN_VTS		1
+#define OV7251_TIMING_MAX_VTS		0xffff
+#define OV7251_INTEGRATION_MARGIN	20
 
 struct reg_value {
 	u16 reg;
@@ -71,6 +75,7 @@ struct reg_value {
 struct ov7251_mode_info {
 	u32 width;
 	u32 height;
+	u32 vts;
 	const struct reg_value *data;
 	u32 data_size;
 	u32 pixel_clock;
@@ -142,6 +147,7 @@ struct ov7251 {
 	struct v4l2_ctrl *exposure;
 	struct v4l2_ctrl *gain;
 	struct v4l2_ctrl *hblank;
+	struct v4l2_ctrl *vblank;
 
 	/* Cached register values */
 	u8 aec_pk_manual;
@@ -637,6 +643,7 @@ static const struct ov7251_mode_info ov7251_mode_info_data[] = {
 	{
 		.width = 640,
 		.height = 480,
+		.vts = 1724,
 		.data = ov7251_setting_vga_30fps,
 		.data_size = ARRAY_SIZE(ov7251_setting_vga_30fps),
 		.exposure_max = 1704,
@@ -649,6 +656,7 @@ static const struct ov7251_mode_info ov7251_mode_info_data[] = {
 	{
 		.width = 640,
 		.height = 480,
+		.vts = 860,
 		.data = ov7251_setting_vga_60fps,
 		.data_size = ARRAY_SIZE(ov7251_setting_vga_60fps),
 		.exposure_max = 840,
@@ -661,6 +669,7 @@ static const struct ov7251_mode_info ov7251_mode_info_data[] = {
 	{
 		.width = 640,
 		.height = 480,
+		.vts = 572,
 		.data = ov7251_setting_vga_90fps,
 		.data_size = ARRAY_SIZE(ov7251_setting_vga_90fps),
 		.exposure_max = 552,
@@ -1001,12 +1010,36 @@ static const char * const ov7251_test_pattern_menu[] = {
 	"Vertical Pattern Bars",
 };
 
+static int ov7251_vts_configure(struct ov7251 *ov7251, s32 vblank)
+{
+	u8 vts[2];
+
+	vts[0] = ((ov7251->current_mode->height + vblank) & 0xff00) >> 8;
+	vts[1] = ((ov7251->current_mode->height + vblank) & 0x00ff);
+
+	return ov7251_write_seq_regs(ov7251, OV7251_TIMING_VTS_REG, vts, 2);
+}
+
 static int ov7251_s_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct ov7251 *ov7251 = container_of(ctrl->handler,
 					     struct ov7251, ctrls);
 	int ret;
 
+	/* If VBLANK is altered we need to update exposure to compensate */
+	if (ctrl->id == V4L2_CID_VBLANK) {
+		int exposure_max;
+
+		exposure_max = ov7251->current_mode->height + ctrl->val -
+			       OV7251_INTEGRATION_MARGIN;
+		__v4l2_ctrl_modify_range(ov7251->exposure,
+					 ov7251->exposure->minimum,
+					 exposure_max,
+					 ov7251->exposure->step,
+					 min(ov7251->exposure->val,
+					     exposure_max));
+	}
+
 	/* v4l2_ctrl_lock() locks our mutex */
 
 	if (!pm_runtime_get_if_in_use(ov7251->dev))
@@ -1028,6 +1061,9 @@ static int ov7251_s_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_VFLIP:
 		ret = ov7251_set_vflip(ov7251, ctrl->val);
 		break;
+	case V4L2_CID_VBLANK:
+		ret = ov7251_vts_configure(ov7251, ctrl->val);
+		break;
 	default:
 		ret = -EINVAL;
 		break;
@@ -1179,6 +1215,7 @@ static int ov7251_set_format(struct v4l2_subdev *sd,
 {
 	struct ov7251 *ov7251 = to_ov7251(sd);
 	struct v4l2_mbus_framefmt *__format;
+	int vblank_max, vblank_def;
 	struct v4l2_rect *__crop;
 	const struct ov7251_mode_info *new_mode;
 	int ret = 0;
@@ -1212,6 +1249,14 @@ static int ov7251_set_format(struct v4l2_subdev *sd,
 		if (ret < 0)
 			goto exit;
 
+		vblank_max = OV7251_TIMING_MAX_VTS - new_mode->height;
+		vblank_def = new_mode->vts - new_mode->height;
+		ret = __v4l2_ctrl_modify_range(ov7251->vblank,
+					       OV7251_TIMING_MIN_VTS,
+					       vblank_max, 1, vblank_max);
+		if (ret < 0)
+			goto exit;
+
 		ov7251->current_mode = new_mode;
 	}
 
@@ -1492,6 +1537,7 @@ static int ov7251_detect_chip(struct ov7251 *ov7251)
 
 static int ov7251_init_ctrls(struct ov7251 *ov7251)
 {
+	int vblank_max, vblank_def;
 	s64 pixel_rate;
 	int hblank;
 
@@ -1535,6 +1581,13 @@ static int ov7251_init_ctrls(struct ov7251 *ov7251)
 	if (ov7251->hblank)
 		ov7251->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
+	vblank_max = OV7251_TIMING_MAX_VTS - ov7251->current_mode->height;
+	vblank_def = ov7251->current_mode->vts - ov7251->current_mode->height;
+	ov7251->vblank = v4l2_ctrl_new_std(&ov7251->ctrls, &ov7251_ctrl_ops,
+					   V4L2_CID_VBLANK,
+					   OV7251_TIMING_MIN_VTS, vblank_max, 1,
+					   vblank_def);
+
 	ov7251->sd.ctrl_handler = &ov7251->ctrls;
 
 	if (ov7251->ctrls.error) {
-- 
2.25.1

Re: [PATCH v3 15/15] media: i2c: Add vblank control to ov7251 driver

[kernel test robot]

Hi Daniel,

I love your patch! Perhaps something to improve:

[auto build test WARNING on media-tree/master]
[also build test WARNING on v5.18-rc5 next-20220504]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Daniel-Scally/Support-OVTI7251-on-Microsoft-Surface-line/20220505-063608
base:   git://linuxtv.org/media_tree.git master
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20220505/202205050844.k1CPWqtV-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/132a5a799bbe214b679bc8e242193c5c1ff1d967
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Daniel-Scally/Support-OVTI7251-on-Microsoft-Surface-line/20220505-063608
        git checkout 132a5a799bbe214b679bc8e242193c5c1ff1d967
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/media/i2c/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/media/i2c/ov7251.c: In function 'ov7251_set_format':
>> drivers/media/i2c/ov7251.c:1218:25: warning: variable 'vblank_def' set but not used [-Wunused-but-set-variable]
    1218 |         int vblank_max, vblank_def;
         |                         ^~~~~~~~~~
   At top level:
   drivers/media/i2c/ov7251.c:193:37: warning: 'ov7251_pll1_cfg_24_mhz_319_2_mhz' defined but not used [-Wunused-const-variable=]
     193 | static const struct ov7251_pll1_cfg ov7251_pll1_cfg_24_mhz_319_2_mhz = {
         |                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/vblank_def +1218 drivers/media/i2c/ov7251.c

  1211	
  1212	static int ov7251_set_format(struct v4l2_subdev *sd,
  1213				     struct v4l2_subdev_state *sd_state,
  1214				     struct v4l2_subdev_format *format)
  1215	{
  1216		struct ov7251 *ov7251 = to_ov7251(sd);
  1217		struct v4l2_mbus_framefmt *__format;
> 1218		int vblank_max, vblank_def;
  1219		struct v4l2_rect *__crop;
  1220		const struct ov7251_mode_info *new_mode;
  1221		int ret = 0;
  1222	
  1223		mutex_lock(&ov7251->lock);
  1224	
  1225		__crop = __ov7251_get_pad_crop(ov7251, sd_state, format->pad,
  1226					       format->which);
  1227	
  1228		new_mode = v4l2_find_nearest_size(ov7251_mode_info_data,
  1229					ARRAY_SIZE(ov7251_mode_info_data),
  1230					width, height,
  1231					format->format.width, format->format.height);
  1232	
  1233		__crop->width = new_mode->width;
  1234		__crop->height = new_mode->height;
  1235	
  1236		if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
  1237			ret = __v4l2_ctrl_modify_range(ov7251->exposure,
  1238						       1, new_mode->exposure_max,
  1239						       1, new_mode->exposure_def);
  1240			if (ret < 0)
  1241				goto exit;
  1242	
  1243			ret = __v4l2_ctrl_s_ctrl(ov7251->exposure,
  1244						 new_mode->exposure_def);
  1245			if (ret < 0)
  1246				goto exit;
  1247	
  1248			ret = __v4l2_ctrl_s_ctrl(ov7251->gain, 16);
  1249			if (ret < 0)
  1250				goto exit;
  1251	
  1252			vblank_max = OV7251_TIMING_MAX_VTS - new_mode->height;
  1253			vblank_def = new_mode->vts - new_mode->height;
  1254			ret = __v4l2_ctrl_modify_range(ov7251->vblank,
  1255						       OV7251_TIMING_MIN_VTS,
  1256						       vblank_max, 1, vblank_max);
  1257			if (ret < 0)
  1258				goto exit;
  1259	
  1260			ov7251->current_mode = new_mode;
  1261		}
  1262	
  1263		__format = __ov7251_get_pad_format(ov7251, sd_state, format->pad,
  1264						   format->which);
  1265		__format->width = __crop->width;
  1266		__format->height = __crop->height;
  1267		__format->code = MEDIA_BUS_FMT_Y10_1X10;
  1268		__format->field = V4L2_FIELD_NONE;
  1269		__format->colorspace = V4L2_COLORSPACE_SRGB;
  1270		__format->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(__format->colorspace);
  1271		__format->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
  1272					__format->colorspace, __format->ycbcr_enc);
  1273		__format->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(__format->colorspace);
  1274	
  1275		format->format = *__format;
  1276	
  1277	exit:
  1278		mutex_unlock(&ov7251->lock);
  1279	
  1280		return ret;
  1281	}
  1282	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v3 15/15] media: i2c: Add vblank control to ov7251 driver

[Daniel Scally]

Argh - I screwed up a rebase here, not sure how I missed that. Sorry
all, let me resend

On 05/05/2022 02:02, kernel test robot wrote:
> Hi Daniel,
>
> I love your patch! Perhaps something to improve:
>
> [auto build test WARNING on media-tree/master]
> [also build test WARNING on v5.18-rc5 next-20220504]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Daniel-Scally/Support-OVTI7251-on-Microsoft-Surface-line/20220505-063608
> base:   git://linuxtv.org/media_tree.git master
> config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20220505/202205050844.k1CPWqtV-lkp@intel.com/config)
> compiler: m68k-linux-gcc (GCC) 11.3.0
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # https://github.com/intel-lab-lkp/linux/commit/132a5a799bbe214b679bc8e242193c5c1ff1d967
>         git remote add linux-review https://github.com/intel-lab-lkp/linux
>         git fetch --no-tags linux-review Daniel-Scally/Support-OVTI7251-on-Microsoft-Surface-line/20220505-063608
>         git checkout 132a5a799bbe214b679bc8e242193c5c1ff1d967
>         # save the config file
>         mkdir build_dir && cp config build_dir/.config
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/media/i2c/
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
>
> All warnings (new ones prefixed by >>):
>
>    drivers/media/i2c/ov7251.c: In function 'ov7251_set_format':
>>> drivers/media/i2c/ov7251.c:1218:25: warning: variable 'vblank_def' set but not used [-Wunused-but-set-variable]
>     1218 |         int vblank_max, vblank_def;
>          |                         ^~~~~~~~~~
>    At top level:
>    drivers/media/i2c/ov7251.c:193:37: warning: 'ov7251_pll1_cfg_24_mhz_319_2_mhz' defined but not used [-Wunused-const-variable=]
>      193 | static const struct ov7251_pll1_cfg ov7251_pll1_cfg_24_mhz_319_2_mhz = {
>          |                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>
> vim +/vblank_def +1218 drivers/media/i2c/ov7251.c
>
>   1211	
>   1212	static int ov7251_set_format(struct v4l2_subdev *sd,
>   1213				     struct v4l2_subdev_state *sd_state,
>   1214				     struct v4l2_subdev_format *format)
>   1215	{
>   1216		struct ov7251 *ov7251 = to_ov7251(sd);
>   1217		struct v4l2_mbus_framefmt *__format;
>> 1218		int vblank_max, vblank_def;
>   1219		struct v4l2_rect *__crop;
>   1220		const struct ov7251_mode_info *new_mode;
>   1221		int ret = 0;
>   1222	
>   1223		mutex_lock(&ov7251->lock);
>   1224	
>   1225		__crop = __ov7251_get_pad_crop(ov7251, sd_state, format->pad,
>   1226					       format->which);
>   1227	
>   1228		new_mode = v4l2_find_nearest_size(ov7251_mode_info_data,
>   1229					ARRAY_SIZE(ov7251_mode_info_data),
>   1230					width, height,
>   1231					format->format.width, format->format.height);
>   1232	
>   1233		__crop->width = new_mode->width;
>   1234		__crop->height = new_mode->height;
>   1235	
>   1236		if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>   1237			ret = __v4l2_ctrl_modify_range(ov7251->exposure,
>   1238						       1, new_mode->exposure_max,
>   1239						       1, new_mode->exposure_def);
>   1240			if (ret < 0)
>   1241				goto exit;
>   1242	
>   1243			ret = __v4l2_ctrl_s_ctrl(ov7251->exposure,
>   1244						 new_mode->exposure_def);
>   1245			if (ret < 0)
>   1246				goto exit;
>   1247	
>   1248			ret = __v4l2_ctrl_s_ctrl(ov7251->gain, 16);
>   1249			if (ret < 0)
>   1250				goto exit;
>   1251	
>   1252			vblank_max = OV7251_TIMING_MAX_VTS - new_mode->height;
>   1253			vblank_def = new_mode->vts - new_mode->height;
>   1254			ret = __v4l2_ctrl_modify_range(ov7251->vblank,
>   1255						       OV7251_TIMING_MIN_VTS,
>   1256						       vblank_max, 1, vblank_max);
>   1257			if (ret < 0)
>   1258				goto exit;
>   1259	
>   1260			ov7251->current_mode = new_mode;
>   1261		}
>   1262	
>   1263		__format = __ov7251_get_pad_format(ov7251, sd_state, format->pad,
>   1264						   format->which);
>   1265		__format->width = __crop->width;
>   1266		__format->height = __crop->height;
>   1267		__format->code = MEDIA_BUS_FMT_Y10_1X10;
>   1268		__format->field = V4L2_FIELD_NONE;
>   1269		__format->colorspace = V4L2_COLORSPACE_SRGB;
>   1270		__format->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(__format->colorspace);
>   1271		__format->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
>   1272					__format->colorspace, __format->ycbcr_enc);
>   1273		__format->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(__format->colorspace);
>   1274	
>   1275		format->format = *__format;
>   1276	
>   1277	exit:
>   1278		mutex_unlock(&ov7251->lock);
>   1279	
>   1280		return ret;
>   1281	}
>   1282	
>

Re: [PATCH v3 15/15] media: i2c: Add vblank control to ov7251 driver

[Daniel Scally]

[-- Attachment #1: Type: text/plain, Size: 5197 bytes --]

Argh - I screwed up a rebase here, not sure how I missed that. Sorry
all, let me resend

On 05/05/2022 02:02, kernel test robot wrote:
> Hi Daniel,
>
> I love your patch! Perhaps something to improve:
>
> [auto build test WARNING on media-tree/master]
> [also build test WARNING on v5.18-rc5 next-20220504]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Daniel-Scally/Support-OVTI7251-on-Microsoft-Surface-line/20220505-063608
> base:   git://linuxtv.org/media_tree.git master
> config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20220505/202205050844.k1CPWqtV-lkp(a)intel.com/config)
> compiler: m68k-linux-gcc (GCC) 11.3.0
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # https://github.com/intel-lab-lkp/linux/commit/132a5a799bbe214b679bc8e242193c5c1ff1d967
>         git remote add linux-review https://github.com/intel-lab-lkp/linux
>         git fetch --no-tags linux-review Daniel-Scally/Support-OVTI7251-on-Microsoft-Surface-line/20220505-063608
>         git checkout 132a5a799bbe214b679bc8e242193c5c1ff1d967
>         # save the config file
>         mkdir build_dir && cp config build_dir/.config
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/media/i2c/
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
>
> All warnings (new ones prefixed by >>):
>
>    drivers/media/i2c/ov7251.c: In function 'ov7251_set_format':
>>> drivers/media/i2c/ov7251.c:1218:25: warning: variable 'vblank_def' set but not used [-Wunused-but-set-variable]
>     1218 |         int vblank_max, vblank_def;
>          |                         ^~~~~~~~~~
>    At top level:
>    drivers/media/i2c/ov7251.c:193:37: warning: 'ov7251_pll1_cfg_24_mhz_319_2_mhz' defined but not used [-Wunused-const-variable=]
>      193 | static const struct ov7251_pll1_cfg ov7251_pll1_cfg_24_mhz_319_2_mhz = {
>          |                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>
> vim +/vblank_def +1218 drivers/media/i2c/ov7251.c
>
>   1211	
>   1212	static int ov7251_set_format(struct v4l2_subdev *sd,
>   1213				     struct v4l2_subdev_state *sd_state,
>   1214				     struct v4l2_subdev_format *format)
>   1215	{
>   1216		struct ov7251 *ov7251 = to_ov7251(sd);
>   1217		struct v4l2_mbus_framefmt *__format;
>> 1218		int vblank_max, vblank_def;
>   1219		struct v4l2_rect *__crop;
>   1220		const struct ov7251_mode_info *new_mode;
>   1221		int ret = 0;
>   1222	
>   1223		mutex_lock(&ov7251->lock);
>   1224	
>   1225		__crop = __ov7251_get_pad_crop(ov7251, sd_state, format->pad,
>   1226					       format->which);
>   1227	
>   1228		new_mode = v4l2_find_nearest_size(ov7251_mode_info_data,
>   1229					ARRAY_SIZE(ov7251_mode_info_data),
>   1230					width, height,
>   1231					format->format.width, format->format.height);
>   1232	
>   1233		__crop->width = new_mode->width;
>   1234		__crop->height = new_mode->height;
>   1235	
>   1236		if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>   1237			ret = __v4l2_ctrl_modify_range(ov7251->exposure,
>   1238						       1, new_mode->exposure_max,
>   1239						       1, new_mode->exposure_def);
>   1240			if (ret < 0)
>   1241				goto exit;
>   1242	
>   1243			ret = __v4l2_ctrl_s_ctrl(ov7251->exposure,
>   1244						 new_mode->exposure_def);
>   1245			if (ret < 0)
>   1246				goto exit;
>   1247	
>   1248			ret = __v4l2_ctrl_s_ctrl(ov7251->gain, 16);
>   1249			if (ret < 0)
>   1250				goto exit;
>   1251	
>   1252			vblank_max = OV7251_TIMING_MAX_VTS - new_mode->height;
>   1253			vblank_def = new_mode->vts - new_mode->height;
>   1254			ret = __v4l2_ctrl_modify_range(ov7251->vblank,
>   1255						       OV7251_TIMING_MIN_VTS,
>   1256						       vblank_max, 1, vblank_max);
>   1257			if (ret < 0)
>   1258				goto exit;
>   1259	
>   1260			ov7251->current_mode = new_mode;
>   1261		}
>   1262	
>   1263		__format = __ov7251_get_pad_format(ov7251, sd_state, format->pad,
>   1264						   format->which);
>   1265		__format->width = __crop->width;
>   1266		__format->height = __crop->height;
>   1267		__format->code = MEDIA_BUS_FMT_Y10_1X10;
>   1268		__format->field = V4L2_FIELD_NONE;
>   1269		__format->colorspace = V4L2_COLORSPACE_SRGB;
>   1270		__format->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(__format->colorspace);
>   1271		__format->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
>   1272					__format->colorspace, __format->ycbcr_enc);
>   1273		__format->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(__format->colorspace);
>   1274	
>   1275		format->format = *__format;
>   1276	
>   1277	exit:
>   1278		mutex_unlock(&ov7251->lock);
>   1279	
>   1280		return ret;
>   1281	}
>   1282	
>

Re: [PATCH v3 15/15] media: i2c: Add vblank control to ov7251 driver

[Sakari Ailus]

On Thu, May 05, 2022 at 09:04:32AM +0100, Daniel Scally wrote:
> Argh - I screwed up a rebase here, not sure how I missed that. Sorry
> all, let me resend

If it's just that, I can fix it. No need to resend.

-- 
Sakari Ailus

Re: [PATCH v3 15/15] media: i2c: Add vblank control to ov7251 driver

[Sakari Ailus]

[-- Attachment #1: Type: text/plain, Size: 232 bytes --]

On Thu, May 05, 2022 at 09:04:32AM +0100, Daniel Scally wrote:
> Argh - I screwed up a rebase here, not sure how I missed that. Sorry
> all, let me resend

If it's just that, I can fix it. No need to resend.

-- 
Sakari Ailus

Re: [PATCH v3 15/15] media: i2c: Add vblank control to ov7251 driver

[Daniel Scally]

On 05/05/2022 09:32, Sakari Ailus wrote:
> On Thu, May 05, 2022 at 09:04:32AM +0100, Daniel Scally wrote:
>> Argh - I screwed up a rebase here, not sure how I missed that. Sorry
>> all, let me resend
> If it's just that, I can fix it. No need to resend.
>
Thanks...there's two problems shown though actually, I used 1 instead of
vblank_def in the __v4l2_ctrl_modify_range() call in
ov7251_set_format(), and the other problem
(ov7251_pll1_cfg_24_mhz_319_2_mhz defined but not used) was actually
introduced in patch #7 (media: i2c: Add support for new frequencies to
ov7251). This change:


|static const struct ov7251_pll_cfgs ov7251_pll_cfgs_24_mhz = { .pll2 =
&ov7251_pll2_cfg_24_mhz, .pll1 = { [OV7251_LINK_FREQ_240_MHZ] =
&ov7251_pll1_cfg_24_mhz_240_mhz, + [OV7251_LINK_FREQ_319_2_MHZ] =
&ov7251_pll1_cfg_19_2_mhz_319_2_mhz, }, }; Should have referred to |ov7251_pll1_cfg_24_mhz_319_2_mhz. I can fix them, but it wouldn't be until tonight (sorry, I expected to have this ready much earlier by the end of the weekend!)
||

Re: [PATCH v3 15/15] media: i2c: Add vblank control to ov7251 driver

[Daniel Scally]

[-- Attachment #1: Type: text/plain, Size: 1047 bytes --]


On 05/05/2022 09:32, Sakari Ailus wrote:
> On Thu, May 05, 2022 at 09:04:32AM +0100, Daniel Scally wrote:
>> Argh - I screwed up a rebase here, not sure how I missed that. Sorry
>> all, let me resend
> If it's just that, I can fix it. No need to resend.
>
Thanks...there's two problems shown though actually, I used 1 instead of
vblank_def in the __v4l2_ctrl_modify_range() call in
ov7251_set_format(), and the other problem
(ov7251_pll1_cfg_24_mhz_319_2_mhz defined but not used) was actually
introduced in patch #7 (media: i2c: Add support for new frequencies to
ov7251). This change:


|static const struct ov7251_pll_cfgs ov7251_pll_cfgs_24_mhz = { .pll2 =
&ov7251_pll2_cfg_24_mhz, .pll1 = { [OV7251_LINK_FREQ_240_MHZ] =
&ov7251_pll1_cfg_24_mhz_240_mhz, + [OV7251_LINK_FREQ_319_2_MHZ] =
&ov7251_pll1_cfg_19_2_mhz_319_2_mhz, }, }; Should have referred to |ov7251_pll1_cfg_24_mhz_319_2_mhz. I can fix them, but it wouldn't be until tonight (sorry, I expected to have this ready much earlier by the end of the weekend!)
||

Re: [PATCH v3 15/15] media: i2c: Add vblank control to ov7251 driver

[Andy Shevchenko]

On Wed, May 04, 2022 at 11:30:27PM +0100, Daniel Scally wrote:
> Add a vblank control to the ov7251 driver.

> +static int ov7251_vts_configure(struct ov7251 *ov7251, s32 vblank)
> +{
> +	u8 vts[2];
> +
> +	vts[0] = ((ov7251->current_mode->height + vblank) & 0xff00) >> 8;
> +	vts[1] = ((ov7251->current_mode->height + vblank) & 0x00ff);

__be16 vts;

cpu_to_be16();

> +	return ov7251_write_seq_regs(ov7251, OV7251_TIMING_VTS_REG, vts, 2);
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 15/15] media: i2c: Add vblank control to ov7251 driver

[Daniel Scally]

Hi Andy


On 05/05/2022 11:31, Andy Shevchenko wrote:
> On Wed, May 04, 2022 at 11:30:27PM +0100, Daniel Scally wrote:
>> Add a vblank control to the ov7251 driver.
>> +static int ov7251_vts_configure(struct ov7251 *ov7251, s32 vblank)
>> +{
>> +	u8 vts[2];
>> +
>> +	vts[0] = ((ov7251->current_mode->height + vblank) & 0xff00) >> 8;
>> +	vts[1] = ((ov7251->current_mode->height + vblank) & 0x00ff);
> __be16 vts;
>
> cpu_to_be16();
Most places that do this seem to do the conversion in the i2c read/write
functions, so in this case within ov7251_write_seq_regs(). Can I do it
there, as an extra patch? I actually have more changes to make on this
driver but they're not remotely read yet so there'll be another series
in the future
>> +	return ov7251_write_seq_regs(ov7251, OV7251_TIMING_VTS_REG, vts, 2);
>> +}

Re: [PATCH v3 15/15] media: i2c: Add vblank control to ov7251 driver

[Andy Shevchenko]

On Fri, May 6, 2022 at 1:46 PM Daniel Scally <djrscally@gmail.com> wrote:
> On 05/05/2022 11:31, Andy Shevchenko wrote:
> > On Wed, May 04, 2022 at 11:30:27PM +0100, Daniel Scally wrote:

...

> >> +    u8 vts[2];
> >> +
> >> +    vts[0] = ((ov7251->current_mode->height + vblank) & 0xff00) >> 8;
> >> +    vts[1] = ((ov7251->current_mode->height + vblank) & 0x00ff);
> > __be16 vts;
> >
> > cpu_to_be16();

> Most places that do this seem to do the conversion in the i2c read/write
> functions, so in this case within ov7251_write_seq_regs(). Can I do it
> there, as an extra patch? I actually have more changes to make on this
> driver but they're not remotely read yet so there'll be another series
> in the future

Ideally you should first convert them and then add your patch with
this change in mind.

> >> +    return ov7251_write_seq_regs(ov7251, OV7251_TIMING_VTS_REG, vts, 2);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 15/15] media: i2c: Add vblank control to ov7251 driver

[kernel test robot]

Hi Daniel,

I love your patch! Perhaps something to improve:

[auto build test WARNING on media-tree/master]
[also build test WARNING on v5.18-rc5 next-20220505]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Daniel-Scally/Support-OVTI7251-on-Microsoft-Surface-line/20220505-063608
base:   git://linuxtv.org/media_tree.git master
config: x86_64-randconfig-a014 (https://download.01.org/0day-ci/archive/20220506/202205060133.HrXVpuXG-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 5e004fb787698440a387750db7f8028e7cb14cfc)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/132a5a799bbe214b679bc8e242193c5c1ff1d967
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Daniel-Scally/Support-OVTI7251-on-Microsoft-Surface-line/20220505-063608
        git checkout 132a5a799bbe214b679bc8e242193c5c1ff1d967
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash arch/x86/crypto/ drivers/media/i2c/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/media/i2c/ov7251.c:1218:18: warning: variable 'vblank_def' set but not used [-Wunused-but-set-variable]
           int vblank_max, vblank_def;
                           ^
   drivers/media/i2c/ov7251.c:193:37: warning: unused variable 'ov7251_pll1_cfg_24_mhz_319_2_mhz' [-Wunused-const-variable]
   static const struct ov7251_pll1_cfg ov7251_pll1_cfg_24_mhz_319_2_mhz = {
                                       ^
   2 warnings generated.


vim +/vblank_def +1218 drivers/media/i2c/ov7251.c

  1211	
  1212	static int ov7251_set_format(struct v4l2_subdev *sd,
  1213				     struct v4l2_subdev_state *sd_state,
  1214				     struct v4l2_subdev_format *format)
  1215	{
  1216		struct ov7251 *ov7251 = to_ov7251(sd);
  1217		struct v4l2_mbus_framefmt *__format;
> 1218		int vblank_max, vblank_def;
  1219		struct v4l2_rect *__crop;
  1220		const struct ov7251_mode_info *new_mode;
  1221		int ret = 0;
  1222	
  1223		mutex_lock(&ov7251->lock);
  1224	
  1225		__crop = __ov7251_get_pad_crop(ov7251, sd_state, format->pad,
  1226					       format->which);
  1227	
  1228		new_mode = v4l2_find_nearest_size(ov7251_mode_info_data,
  1229					ARRAY_SIZE(ov7251_mode_info_data),
  1230					width, height,
  1231					format->format.width, format->format.height);
  1232	
  1233		__crop->width = new_mode->width;
  1234		__crop->height = new_mode->height;
  1235	
  1236		if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
  1237			ret = __v4l2_ctrl_modify_range(ov7251->exposure,
  1238						       1, new_mode->exposure_max,
  1239						       1, new_mode->exposure_def);
  1240			if (ret < 0)
  1241				goto exit;
  1242	
  1243			ret = __v4l2_ctrl_s_ctrl(ov7251->exposure,
  1244						 new_mode->exposure_def);
  1245			if (ret < 0)
  1246				goto exit;
  1247	
  1248			ret = __v4l2_ctrl_s_ctrl(ov7251->gain, 16);
  1249			if (ret < 0)
  1250				goto exit;
  1251	
  1252			vblank_max = OV7251_TIMING_MAX_VTS - new_mode->height;
  1253			vblank_def = new_mode->vts - new_mode->height;
  1254			ret = __v4l2_ctrl_modify_range(ov7251->vblank,
  1255						       OV7251_TIMING_MIN_VTS,
  1256						       vblank_max, 1, vblank_max);
  1257			if (ret < 0)
  1258				goto exit;
  1259	
  1260			ov7251->current_mode = new_mode;
  1261		}
  1262	
  1263		__format = __ov7251_get_pad_format(ov7251, sd_state, format->pad,
  1264						   format->which);
  1265		__format->width = __crop->width;
  1266		__format->height = __crop->height;
  1267		__format->code = MEDIA_BUS_FMT_Y10_1X10;
  1268		__format->field = V4L2_FIELD_NONE;
  1269		__format->colorspace = V4L2_COLORSPACE_SRGB;
  1270		__format->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(__format->colorspace);
  1271		__format->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
  1272					__format->colorspace, __format->ycbcr_enc);
  1273		__format->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(__format->colorspace);
  1274	
  1275		format->format = *__format;
  1276	
  1277	exit:
  1278		mutex_unlock(&ov7251->lock);
  1279	
  1280		return ret;
  1281	}
  1282	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp