[PATCH 00/10] Support OVTI7251 on Microsoft Surface line

[PATCH 00/10] 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. 

Thanks
Dan

Daniel Scally (10):
  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: Add ov7251_pll_configure()
  media: i2c: Add support for 19.2MHz clock 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/v4l/pixfmt-yuv-luma.rst             |  14 +-
 drivers/media/i2c/ov7251.c                    | 480 +++++++++++++-----
 drivers/media/pci/intel/ipu3/cio2-bridge.c    |   2 +
 drivers/media/pci/intel/ipu3/ipu3-cio2-main.c |   7 +-
 drivers/media/v4l2-core/v4l2-ioctl.c          |   1 +
 include/uapi/linux/videodev2.h                |   1 +
 6 files changed, 365 insertions(+), 140 deletions(-)

-- 
2.25.1

[PATCH 01/10] 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. Detail that
format.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
 .../userspace-api/media/v4l/pixfmt-yuv-luma.rst    | 14 +++++++++++++-
 drivers/media/v4l2-core/v4l2-ioctl.c               |  1 +
 include/uapi/linux/videodev2.h                     |  1 +
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/Documentation/userspace-api/media/v4l/pixfmt-yuv-luma.rst b/Documentation/userspace-api/media/v4l/pixfmt-yuv-luma.rst
index 8ebd58c3588f..5465ce3bb533 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 ip3y 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..b378c7e37eac 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 */
-- 
2.25.1

[PATCH 02/10] 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>
---
 drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
index 0e9b0503b62a..ea2f9f70a64e 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
@@ -65,7 +65,12 @@ 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 03/10] 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>
---
 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 04/10] 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. Finally, return
-EPROBE_DEFER if the endpoint is not available, as it could be built
by the ipu3-cio2 driver if that probes later.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
 drivers/media/i2c/ov7251.c | 84 ++++++++++++++++++++++++++++++--------
 1 file changed, 66 insertions(+), 18 deletions(-)

diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
index d6fe574cb9e0..5c5f7a15a640 100644
--- a/drivers/media/i2c/ov7251.c
+++ b/drivers/media/i2c/ov7251.c
@@ -1255,10 +1255,73 @@ 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;
+	bool freq_found;
+	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.bus_type != V4L2_MBUS_CSI2_DPHY) {
+		ret = -EINVAL;
+		dev_err(ov7251->dev, "invalid bus type (%u), must be (%u)\n",
+			bus_cfg.bus_type, V4L2_MBUS_CSI2_DPHY);
+		goto out_free_bus_cfg;
+	}
+
+	if (bus_cfg.bus.mipi_csi2.num_data_lanes != 1) {
+		dev_err(ov7251->dev, "only a 1-lane CSI2 config is supported");
+		ret = -EINVAL;
+		goto out_free_bus_cfg;
+	}
+
+	if (!bus_cfg.nr_of_link_frequencies) {
+		dev_err(ov7251->dev, "no link frequencies defined\n");
+		ret = -EINVAL;
+		goto out_free_bus_cfg;
+	}
+
+	freq_found = false;
+	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
+		if (freq_found)
+			break;
+
+		for (j = 0; j < ARRAY_SIZE(link_freq); j++)
+			if (bus_cfg.link_frequencies[i] == link_freq[j]) {
+				freq_found = true;
+				break;
+			}
+	}
+
+	if (i == bus_cfg.nr_of_link_frequencies) {
+		dev_err(ov7251->dev, "no supported link freq found\n");
+		ret = -EINVAL;
+		goto out_free_bus_cfg;
+	}
+
+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 +1333,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

[PATCH 05/10] 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 external clock
frequencies.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
 drivers/media/i2c/ov7251.c | 165 ++++++++++++++++++++++++++++++-------
 1 file changed, 135 insertions(+), 30 deletions(-)

diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
index 5c5f7a15a640..809450b5a99a 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,27 @@ struct ov7251_mode_info {
 	struct v4l2_fract timeperframe;
 };
 
+struct ov7251_pll1_config {
+	unsigned int pre_div;
+	unsigned int mult;
+	unsigned int div;
+	unsigned int pix_div;
+	unsigned int mipi_div;
+};
+
+struct ov7251_pll2_config {
+	unsigned int pre_div;
+	unsigned int mult;
+	unsigned int div;
+	unsigned int sys_div;
+	unsigned int adc_div;
+};
+
+struct ov7251_pll_configs {
+	const struct ov7251_pll1_config *pll1;
+	const struct ov7251_pll2_config *pll2;
+};
+
 struct ov7251 {
 	struct i2c_client *i2c_client;
 	struct device *dev;
@@ -71,6 +102,8 @@ struct ov7251 {
 	struct clk *xclk;
 	u32 xclk_freq;
 
+	const struct ov7251_pll_configs *pll_configs;
+
 	struct regulator *io_regulator;
 	struct regulator *core_regulator;
 	struct regulator *analog_regulator;
@@ -100,6 +133,36 @@ static inline struct ov7251 *to_ov7251(struct v4l2_subdev *sd)
 	return container_of(sd, struct ov7251, sd);
 }
 
+enum xclk_rate {
+	OV7251_24_MHZ,
+	OV7251_NUM_SUPPORTED_RATES
+};
+
+static const struct ov7251_pll1_config ov7251_pll1_config_24_mhz = {
+	.pre_div = 0x03,
+	.mult = 0x64,
+	.div = 0x01,
+	.pix_div = 0x0a,
+	.mipi_div = 0x05
+};
+
+static const struct ov7251_pll2_config ov7251_pll2_config_24_mhz = {
+	.pre_div = 0x04,
+	.mult = 0x28,
+	.div = 0x00,
+	.sys_div = 0x05,
+	.adc_div = 0x04
+};
+
+static const struct ov7251_pll_configs ov7251_pll_configs_24_mhz = {
+	.pll1 = &ov7251_pll1_config_24_mhz,
+	.pll2 = &ov7251_pll2_config_24_mhz
+};
+
+static const struct ov7251_pll_configs *ov7251_pll_configs[] = {
+	[OV7251_24_MHZ] = &ov7251_pll_configs_24_mhz
+};
+
 static const struct reg_value ov7251_global_init_setting[] = {
 	{ 0x0103, 0x01 },
 	{ 0x303b, 0x02 },
@@ -118,16 +181,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 },
@@ -256,16 +309,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 },
@@ -394,16 +437,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 },
@@ -519,6 +552,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[] = {
 	240000000,
 };
@@ -692,6 +729,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_configs *configs;
+	int ret;
+
+	configs = ov7251->pll_configs;
+
+	ret = ov7251_write_reg(ov7251, OV7251_PLL1_PRE_DIV_REG,
+			       configs->pll1->pre_div);
+	if (ret < 0)
+		return ret;
+
+	ret = ov7251_write_reg(ov7251, OV7251_PLL1_MULT_REG,
+			       configs->pll1->mult);
+	if (ret < 0)
+		return ret;
+	ret = ov7251_write_reg(ov7251, OV7251_PLL1_DIVIDER_REG,
+			       configs->pll1->div);
+	if (ret < 0)
+		return ret;
+
+	ret = ov7251_write_reg(ov7251, OV7251_PLL1_PIX_DIV_REG,
+			       configs->pll1->pix_div);
+	if (ret < 0)
+		return ret;
+
+	ret = ov7251_write_reg(ov7251, OV7251_PLL1_MIPI_DIV_REG,
+			       configs->pll1->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;
@@ -1143,6 +1237,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);
@@ -1325,6 +1424,7 @@ static int ov7251_probe(struct i2c_client *client)
 	struct ov7251 *ov7251;
 	u8 chip_id_high, chip_id_low, chip_rev;
 	int ret;
+	int i;
 
 	ov7251 = devm_kzalloc(dev, sizeof(struct ov7251), GFP_KERNEL);
 	if (!ov7251)
@@ -1363,6 +1463,11 @@ 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;
+
+	ov7251->pll_configs = ov7251_pll_configs[i];
 
 	ov7251->io_regulator = devm_regulator_get(dev, "vdddo");
 	if (IS_ERR(ov7251->io_regulator)) {
-- 
2.25.1

[PATCH 06/10] media: i2c: Add support for 19.2MHz clock 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. Add
the ability to support that rate to the driver by defining a new set
of PLL configs. Extend the clock handling in .probe() to check for
either supported frequency.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
 drivers/media/i2c/ov7251.c | 61 ++++++++++++++++++++++++++++----------
 1 file changed, 45 insertions(+), 16 deletions(-)

diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
index 809450b5a99a..92605ecdfaa4 100644
--- a/drivers/media/i2c/ov7251.c
+++ b/drivers/media/i2c/ov7251.c
@@ -134,10 +134,19 @@ static inline struct ov7251 *to_ov7251(struct v4l2_subdev *sd)
 }
 
 enum xclk_rate {
+	OV7251_19_2_MHZ,
 	OV7251_24_MHZ,
 	OV7251_NUM_SUPPORTED_RATES
 };
 
+static const struct ov7251_pll1_config ov7251_pll1_config_19_2_mhz = {
+	.pre_div = 0x03,
+	.mult = 0x4b,
+	.div = 0x01,
+	.pix_div = 0x0a,
+	.mipi_div = 0x05
+};
+
 static const struct ov7251_pll1_config ov7251_pll1_config_24_mhz = {
 	.pre_div = 0x03,
 	.mult = 0x64,
@@ -146,6 +155,14 @@ static const struct ov7251_pll1_config ov7251_pll1_config_24_mhz = {
 	.mipi_div = 0x05
 };
 
+static const struct ov7251_pll2_config ov7251_pll2_config_19_2_mhz = {
+	.pre_div = 0x04,
+	.mult = 0x32,
+	.div = 0x00,
+	.sys_div = 0x05,
+	.adc_div = 0x04
+};
+
 static const struct ov7251_pll2_config ov7251_pll2_config_24_mhz = {
 	.pre_div = 0x04,
 	.mult = 0x28,
@@ -154,12 +171,18 @@ static const struct ov7251_pll2_config ov7251_pll2_config_24_mhz = {
 	.adc_div = 0x04
 };
 
+static const struct ov7251_pll_configs ov7251_pll_configs_19_2_mhz = {
+	.pll1 = &ov7251_pll1_config_19_2_mhz,
+	.pll2 = &ov7251_pll2_config_19_2_mhz
+};
+
 static const struct ov7251_pll_configs ov7251_pll_configs_24_mhz = {
 	.pll1 = &ov7251_pll1_config_24_mhz,
 	.pll2 = &ov7251_pll2_config_24_mhz
 };
 
 static const struct ov7251_pll_configs *ov7251_pll_configs[] = {
+	[OV7251_19_2_MHZ] = &ov7251_pll_configs_19_2_mhz,
 	[OV7251_24_MHZ] = &ov7251_pll_configs_24_mhz
 };
 
@@ -553,6 +576,7 @@ 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,
 };
 
@@ -1423,6 +1447,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;
 	int ret;
 	int i;
 
@@ -1438,35 +1463,39 @@ static int ov7251_probe(struct i2c_client *client)
 		return ret;
 
 	/* get system clock (xclk) */
-	ov7251->xclk = devm_clk_get(dev, "xclk");
+	ov7251->xclk = devm_clk_get(dev, NULL);
 	if (IS_ERR(ov7251->xclk)) {
 		dev_err(dev, "could not get xclk");
 		return PTR_ERR(ov7251->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) {
+		ret = clk_set_rate(ov7251->xclk, rate);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "failed to set clock rate\n");
+	} else 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;
-	}
+	ov7251->xclk_freq = rate ? rate : clk_get_rate(ov7251->xclk);
 
-	ret = clk_set_rate(ov7251->xclk, ov7251->xclk_freq);
-	if (ret) {
-		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_configs = ov7251_pll_configs[i];
 
 	ov7251->io_regulator = devm_regulator_get(dev, "vdddo");
-- 
2.25.1

[PATCH 07/10] 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>
---
 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 92605ecdfaa4..f137a1e87537 100644
--- a/drivers/media/i2c/ov7251.c
+++ b/drivers/media/i2c/ov7251.c
@@ -1442,11 +1442,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;
 	int ret;
 	int i;
@@ -1578,34 +1610,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 08/10] 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>
---
 drivers/media/i2c/ov7251.c | 78 ++++++++++++++++++++++++++++++--------
 1 file changed, 63 insertions(+), 15 deletions(-)

diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
index f137a1e87537..d620ed6a4e42 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>
@@ -884,6 +885,24 @@ static void ov7251_set_power_off(struct ov7251 *ov7251)
 	ov7251_regulators_disable(ov7251);
 }
 
+static int __maybe_unused ov7251_sensor_suspend(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct ov7251 *ov7251 = to_ov7251(sd);
+
+	ov7251_set_power_off(ov7251);
+
+	return 0;
+}
+
+static int __maybe_unused ov7251_sensor_resume(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct ov7251 *ov7251 = to_ov7251(sd);
+
+	return ov7251_set_power_on(ov7251);
+}
+
 static int ov7251_s_power(struct v4l2_subdev *sd, int on)
 {
 	struct ov7251 *ov7251 = to_ov7251(sd);
@@ -985,7 +1004,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) {
@@ -1009,6 +1028,8 @@ static int ov7251_s_ctrl(struct v4l2_ctrl *ctrl)
 		break;
 	}
 
+	pm_runtime_put(ov7251->dev);
+
 	return ret;
 }
 
@@ -1261,10 +1282,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)
+			return ret;
+
 		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,
@@ -1273,23 +1299,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:
 	mutex_unlock(&ov7251->lock);
+	return ret;
 
+err_power_down:
+	pm_runtime_put_noidle(ov7251->dev);
+	mutex_unlock(&ov7251->lock);
 	return ret;
 }
 
@@ -1604,23 +1636,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);
+	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,
@@ -1628,7 +1661,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,
@@ -1636,10 +1669,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) {
@@ -1651,6 +1686,9 @@ 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);
 free_entity:
@@ -1672,9 +1710,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);
+	pm_runtime_set_suspended(ov7251->dev);
+
 	return 0;
 }
 
+static const struct dev_pm_ops ov7251_pm_ops = {
+	SET_RUNTIME_PM_OPS(ov7251_sensor_suspend, ov7251_sensor_resume, NULL)
+};
+
 static const struct of_device_id ov7251_of_match[] = {
 	{ .compatible = "ovti,ov7251" },
 	{ /* sentinel */ }
@@ -1692,6 +1739,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 09/10] 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>
---
 drivers/media/i2c/ov7251.c | 44 +-------------------------------------
 1 file changed, 1 insertion(+), 43 deletions(-)

diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
index d620ed6a4e42..5e7422ca4ab9 100644
--- a/drivers/media/i2c/ov7251.c
+++ b/drivers/media/i2c/ov7251.c
@@ -903,43 +903,6 @@ static int __maybe_unused ov7251_sensor_resume(struct device *dev)
 	return ov7251_set_power_on(ov7251);
 }
 
-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);
-		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);
-			goto exit;
-		}
-
-		ov7251->power_on = true;
-	} else {
-		ov7251_set_power_off(ov7251);
-		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;
@@ -1384,10 +1347,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,
@@ -1405,7 +1364,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,
 };
@@ -1690,7 +1648,7 @@ static int ov7251_probe(struct i2c_client *client)
 	pm_runtime_disable(ov7251->dev);
 	pm_runtime_put_noidle(ov7251->dev);
 power_down:
-	ov7251_s_power(&ov7251->sd, false);
+	ov7251_set_power_off(ov7251);
 free_entity:
 	media_entity_cleanup(&ov7251->sd.entity);
 free_ctrl:
-- 
2.25.1

[PATCH 10/10] media: ipu3-cio2: Add INT347E to cio2-bridge

[Daniel Scally]

ACPI _HID INT347E represents the ov7251 sensor, which can be supported
by the cio2-bridge. Add it to the list of supported devices so the
bridge builts the software nodes.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
 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..a27b4948b4f6 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, 240000000),
 	/* Omnivision OV2680 */
 	CIO2_SENSOR_CONFIG("OVTI2680", 0),
 };
-- 
2.25.1

Re: [PATCH 09/10] media: i2c: Remove .s_power() from 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.17-rc4 next-20220215]
[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/0day-ci/linux/commits/Daniel-Scally/Support-OVTI7251-on-Microsoft-Surface-line/20220216-071110
base:   git://linuxtv.org/media_tree.git master
config: riscv-randconfig-r003-20220216 (https://download.01.org/0day-ci/archive/20220216/202202161122.Qh05mcn0-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 0e628a783b935c70c80815db6c061ec84f884af5)
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
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv64-linux-gnu
        # https://github.com/0day-ci/linux/commit/5c7425707438fee74daeb7faf41774d88a04b561
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Daniel-Scally/Support-OVTI7251-on-Microsoft-Surface-line/20220216-071110
        git checkout 5c7425707438fee74daeb7faf41774d88a04b561
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv 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:190:31: warning: unused variable 'ov7251_global_init_setting' [-Wunused-const-variable]
   static const struct reg_value ov7251_global_init_setting[] = {
                                 ^
   1 warning generated.


vim +/ov7251_global_init_setting +190 drivers/media/i2c/ov7251.c

9276bc868a46fc Daniel Scally 2022-02-15  189  
d30bb512da3d8e Todor Tomov   2018-04-25 @190  static const struct reg_value ov7251_global_init_setting[] = {
d30bb512da3d8e Todor Tomov   2018-04-25  191  	{ 0x0103, 0x01 },
d30bb512da3d8e Todor Tomov   2018-04-25  192  	{ 0x303b, 0x02 },
d30bb512da3d8e Todor Tomov   2018-04-25  193  };
d30bb512da3d8e Todor Tomov   2018-04-25  194  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH 09/10] media: i2c: Remove .s_power() from ov7251

[kernel test robot]

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

Hi Daniel,

I love your patch! Perhaps something to improve:

[auto build test WARNING on media-tree/master]
[also build test WARNING on v5.17-rc4 next-20220215]
[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/0day-ci/linux/commits/Daniel-Scally/Support-OVTI7251-on-Microsoft-Surface-line/20220216-071110
base:   git://linuxtv.org/media_tree.git master
config: riscv-randconfig-r003-20220216 (https://download.01.org/0day-ci/archive/20220216/202202161122.Qh05mcn0-lkp(a)intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 0e628a783b935c70c80815db6c061ec84f884af5)
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
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv64-linux-gnu
        # https://github.com/0day-ci/linux/commit/5c7425707438fee74daeb7faf41774d88a04b561
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Daniel-Scally/Support-OVTI7251-on-Microsoft-Surface-line/20220216-071110
        git checkout 5c7425707438fee74daeb7faf41774d88a04b561
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv 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:190:31: warning: unused variable 'ov7251_global_init_setting' [-Wunused-const-variable]
   static const struct reg_value ov7251_global_init_setting[] = {
                                 ^
   1 warning generated.


vim +/ov7251_global_init_setting +190 drivers/media/i2c/ov7251.c

9276bc868a46fc Daniel Scally 2022-02-15  189  
d30bb512da3d8e Todor Tomov   2018-04-25 @190  static const struct reg_value ov7251_global_init_setting[] = {
d30bb512da3d8e Todor Tomov   2018-04-25  191  	{ 0x0103, 0x01 },
d30bb512da3d8e Todor Tomov   2018-04-25  192  	{ 0x303b, 0x02 },
d30bb512da3d8e Todor Tomov   2018-04-25  193  };
d30bb512da3d8e Todor Tomov   2018-04-25  194  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [PATCH 01/10] media: uapi: Add IPU3 packed Y10 format

[Nicolas Dufresne]

Le mardi 15 février 2022 à 23:07 +0000, Daniel Scally a écrit :
> 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. Detail that
> format.
> 
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
>  .../userspace-api/media/v4l/pixfmt-yuv-luma.rst    | 14 +++++++++++++-
>  drivers/media/v4l2-core/v4l2-ioctl.c               |  1 +
>  include/uapi/linux/videodev2.h                     |  1 +
>  3 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/pixfmt-yuv-luma.rst b/Documentation/userspace-api/media/v4l/pixfmt-yuv-luma.rst
> index 8ebd58c3588f..5465ce3bb533 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 ip3y format 25 pixels are packed into 32 bytes, which leaves

nit: ip3y-> IPU3_Y10, to be consistent with previous paragraph not using fourcc.

I don't have very strong preference, but this could have been sorted into vendor
formats, as its specific to a HW design, unlike MIPI which is a HW standard.

In any case, with the nit fixed, you can add my:

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

cheers,
Nicolas

> +    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..b378c7e37eac 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 */

Re: [PATCH 01/10] media: uapi: Add IPU3 packed Y10 format

[Daniel Scally]

Hi Nicolas

On 16/02/2022 13:28, Nicolas Dufresne wrote:
> Le mardi 15 février 2022 à 23:07 +0000, Daniel Scally a écrit :
>> 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. Detail that
>> format.
>>
>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>> ---
>>  .../userspace-api/media/v4l/pixfmt-yuv-luma.rst    | 14 +++++++++++++-
>>  drivers/media/v4l2-core/v4l2-ioctl.c               |  1 +
>>  include/uapi/linux/videodev2.h                     |  1 +
>>  3 files changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/userspace-api/media/v4l/pixfmt-yuv-luma.rst b/Documentation/userspace-api/media/v4l/pixfmt-yuv-luma.rst
>> index 8ebd58c3588f..5465ce3bb533 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 ip3y format 25 pixels are packed into 32 bytes, which leaves
> nit: ip3y-> IPU3_Y10, to be consistent with previous paragraph not using fourcc.


Ack

>
> I don't have very strong preference, but this could have been sorted into vendor
> formats, as its specific to a HW design, unlike MIPI which is a HW standard.


Yeah I vacillated between putting it in here and with the other IPU3
formats [1], but because those are in the "bayer formats" section it
sorta stuck out so I eventually settled on sticking it with
greyscale...but maybe there needs to be another section for "Hardware
Specific Formats" or something instead of either option.


[1]
https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/pixfmt-srggb10-ipu3.html

>
> In any case, with the nit fixed, you can add my:
>
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>


Thank you


Dan

>
> cheers,
> Nicolas
>
>> +    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..b378c7e37eac 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 */

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

[Sakari Ailus]

Hi Daniel,

Thanks for the set.

On Tue, Feb 15, 2022 at 11:07:31PM +0000, 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. Finally, return
> -EPROBE_DEFER if the endpoint is not available, as it could be built
> by the ipu3-cio2 driver if that probes later.
> 
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
>  drivers/media/i2c/ov7251.c | 84 ++++++++++++++++++++++++++++++--------
>  1 file changed, 66 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
> index d6fe574cb9e0..5c5f7a15a640 100644
> --- a/drivers/media/i2c/ov7251.c
> +++ b/drivers/media/i2c/ov7251.c
> @@ -1255,10 +1255,73 @@ 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;
> +	bool freq_found;
> +	int i, j;

unsigned int ?

> +	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.bus_type != V4L2_MBUS_CSI2_DPHY) {

You can drop this check as v4l2_fwnode_endpoint_alloc_parse() only parses
D-PHY bus type and returns error otherwise, as you've (correctly) specified
D-PHY in bus_cfg.

> +		ret = -EINVAL;
> +		dev_err(ov7251->dev, "invalid bus type (%u), must be (%u)\n",
> +			bus_cfg.bus_type, V4L2_MBUS_CSI2_DPHY);
> +		goto out_free_bus_cfg;
> +	}
> +
> +	if (bus_cfg.bus.mipi_csi2.num_data_lanes != 1) {

Is this a driver or hardware limitation?

If it's hardware, you could also ignore it --- there's nothing to
configure.

> +		dev_err(ov7251->dev, "only a 1-lane CSI2 config is supported");
> +		ret = -EINVAL;
> +		goto out_free_bus_cfg;
> +	}
> +
> +	if (!bus_cfg.nr_of_link_frequencies) {
> +		dev_err(ov7251->dev, "no link frequencies defined\n");
> +		ret = -EINVAL;
> +		goto out_free_bus_cfg;
> +	}
> +
> +	freq_found = false;

You could do this in initialisation.

> +	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
> +		if (freq_found)
> +			break;
> +
> +		for (j = 0; j < ARRAY_SIZE(link_freq); j++)
> +			if (bus_cfg.link_frequencies[i] == link_freq[j]) {
> +				freq_found = true;
> +				break;
> +			}
> +	}
> +
> +	if (i == bus_cfg.nr_of_link_frequencies) {
> +		dev_err(ov7251->dev, "no supported link freq found\n");
> +		ret = -EINVAL;
> +		goto out_free_bus_cfg;
> +	}
> +
> +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 +1333,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");

-- 
Kind regards,

Sakari Ailus

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

[Daniel Scally]

Hi Sakari - thanks for the comments

On 16/02/2022 15:26, Sakari Ailus wrote:
> Hi Daniel,
>
> Thanks for the set.
>
> On Tue, Feb 15, 2022 at 11:07:31PM +0000, 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. Finally, return
>> -EPROBE_DEFER if the endpoint is not available, as it could be built
>> by the ipu3-cio2 driver if that probes later.
>>
>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>> ---
>>  drivers/media/i2c/ov7251.c | 84 ++++++++++++++++++++++++++++++--------
>>  1 file changed, 66 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
>> index d6fe574cb9e0..5c5f7a15a640 100644
>> --- a/drivers/media/i2c/ov7251.c
>> +++ b/drivers/media/i2c/ov7251.c
>> @@ -1255,10 +1255,73 @@ 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;
>> +	bool freq_found;
>> +	int i, j;
> unsigned int ?


Ack

>
>> +	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.bus_type != V4L2_MBUS_CSI2_DPHY) {
> You can drop this check as v4l2_fwnode_endpoint_alloc_parse() only parses
> D-PHY bus type and returns error otherwise, as you've (correctly) specified
> D-PHY in bus_cfg.


Ah-ha, ok useful to know thanks.

>
>> +		ret = -EINVAL;
>> +		dev_err(ov7251->dev, "invalid bus type (%u), must be (%u)\n",
>> +			bus_cfg.bus_type, V4L2_MBUS_CSI2_DPHY);
>> +		goto out_free_bus_cfg;
>> +	}
>> +
>> +	if (bus_cfg.bus.mipi_csi2.num_data_lanes != 1) {
> Is this a driver or hardware limitation?
>
> If it's hardware, you could also ignore it --- there's nothing to
> configure.


Good point, it is a hardware limitation yes.

>
>> +		dev_err(ov7251->dev, "only a 1-lane CSI2 config is supported");
>> +		ret = -EINVAL;
>> +		goto out_free_bus_cfg;
>> +	}
>> +
>> +	if (!bus_cfg.nr_of_link_frequencies) {
>> +		dev_err(ov7251->dev, "no link frequencies defined\n");
>> +		ret = -EINVAL;
>> +		goto out_free_bus_cfg;
>> +	}
>> +
>> +	freq_found = false;
> You could do this in initialisation.
>
>> +	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {

Ack
>> +		if (freq_found)
>> +			break;
>> +
>> +		for (j = 0; j < ARRAY_SIZE(link_freq); j++)
>> +			if (bus_cfg.link_frequencies[i] == link_freq[j]) {
>> +				freq_found = true;
>> +				break;
>> +			}
>> +	}
>> +
>> +	if (i == bus_cfg.nr_of_link_frequencies) {
>> +		dev_err(ov7251->dev, "no supported link freq found\n");
>> +		ret = -EINVAL;
>> +		goto out_free_bus_cfg;
>> +	}
>> +
>> +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 +1333,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");

Re: [PATCH 09/10] media: i2c: Remove .s_power() from ov7251

[Sakari Ailus]

Hi Daniel,

On Tue, Feb 15, 2022 at 11:07:36PM +0000, Daniel Scally wrote:
> 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>
> ---
>  drivers/media/i2c/ov7251.c | 44 +-------------------------------------
>  1 file changed, 1 insertion(+), 43 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
> index d620ed6a4e42..5e7422ca4ab9 100644
> --- a/drivers/media/i2c/ov7251.c
> +++ b/drivers/media/i2c/ov7251.c
> @@ -903,43 +903,6 @@ static int __maybe_unused ov7251_sensor_resume(struct device *dev)
>  	return ov7251_set_power_on(ov7251);
>  }
>  
> -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);
> -		if (ret < 0)
> -			goto exit;
> -
> -		ret = ov7251_set_register_array(ov7251,
> -					ov7251_global_init_setting,
> -					ARRAY_SIZE(ov7251_global_init_setting));

Could this be written as part of the power-on sequence after identifying
the sensor? Likewise in probe() if it's possible the device won't be
powered down before it gets used --- I guess nothing rules that out?

> -		if (ret < 0) {
> -			dev_err(ov7251->dev, "could not set init registers\n");
> -			ov7251_set_power_off(ov7251);
> -			goto exit;
> -		}
> -
> -		ov7251->power_on = true;
> -	} else {
> -		ov7251_set_power_off(ov7251);
> -		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;
> @@ -1384,10 +1347,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,
> @@ -1405,7 +1364,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,
>  };
> @@ -1690,7 +1648,7 @@ static int ov7251_probe(struct i2c_client *client)
>  	pm_runtime_disable(ov7251->dev);
>  	pm_runtime_put_noidle(ov7251->dev);
>  power_down:
> -	ov7251_s_power(&ov7251->sd, false);
> +	ov7251_set_power_off(ov7251);
>  free_entity:
>  	media_entity_cleanup(&ov7251->sd.entity);
>  free_ctrl:

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 08/10] media: i2c: Add pm_runtime support to ov7251

[Sakari Ailus]

Hi Daniel,

On Tue, Feb 15, 2022 at 11:07:35PM +0000, Daniel Scally wrote:
> Add pm_runtime support to the ov7251 driver.
> 
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
>  drivers/media/i2c/ov7251.c | 78 ++++++++++++++++++++++++++++++--------
>  1 file changed, 63 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
> index f137a1e87537..d620ed6a4e42 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>
> @@ -884,6 +885,24 @@ static void ov7251_set_power_off(struct ov7251 *ov7251)
>  	ov7251_regulators_disable(ov7251);
>  }
>  
> +static int __maybe_unused ov7251_sensor_suspend(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct ov7251 *ov7251 = to_ov7251(sd);
> +
> +	ov7251_set_power_off(ov7251);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused ov7251_sensor_resume(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct ov7251 *ov7251 = to_ov7251(sd);
> +
> +	return ov7251_set_power_on(ov7251);

Could you change the argument type of ov7251_set_power_o{n,ff}() to struct
device * so you'd have just one set of these functions?

> +}
> +
>  static int ov7251_s_power(struct v4l2_subdev *sd, int on)
>  {
>  	struct ov7251 *ov7251 = to_ov7251(sd);
> @@ -985,7 +1004,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) {
> @@ -1009,6 +1028,8 @@ static int ov7251_s_ctrl(struct v4l2_ctrl *ctrl)
>  		break;
>  	}
>  
> +	pm_runtime_put(ov7251->dev);
> +
>  	return ret;
>  }
>  
> @@ -1261,10 +1282,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)
> +			return ret;
> +
>  		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,
> @@ -1273,23 +1299,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:
>  	mutex_unlock(&ov7251->lock);
> +	return ret;
>  
> +err_power_down:
> +	pm_runtime_put_noidle(ov7251->dev);
> +	mutex_unlock(&ov7251->lock);
>  	return ret;
>  }
>  
> @@ -1604,23 +1636,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);
> +	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,
> @@ -1628,7 +1661,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,
> @@ -1636,10 +1669,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) {
> @@ -1651,6 +1686,9 @@ 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);
>  free_entity:
> @@ -1672,9 +1710,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);
> +	pm_runtime_set_suspended(ov7251->dev);
> +
>  	return 0;
>  }
>  
> +static const struct dev_pm_ops ov7251_pm_ops = {
> +	SET_RUNTIME_PM_OPS(ov7251_sensor_suspend, ov7251_sensor_resume, NULL)
> +};
> +
>  static const struct of_device_id ov7251_of_match[] = {
>  	{ .compatible = "ovti,ov7251" },
>  	{ /* sentinel */ }
> @@ -1692,6 +1739,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,

-- 
Kind regards,

Sakari Ailus

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

[Dave Stevenson]

Hi Daniel

On Tue, 15 Feb 2022 at 23:08, Daniel Scally <djrscally@gmail.com> 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. Finally, return
> -EPROBE_DEFER if the endpoint is not available, as it could be built
> by the ipu3-cio2 driver if that probes later.
>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
>  drivers/media/i2c/ov7251.c | 84 ++++++++++++++++++++++++++++++--------
>  1 file changed, 66 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
> index d6fe574cb9e0..5c5f7a15a640 100644
> --- a/drivers/media/i2c/ov7251.c
> +++ b/drivers/media/i2c/ov7251.c
> @@ -1255,10 +1255,73 @@ 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;
> +       bool freq_found;
> +       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.bus_type != V4L2_MBUS_CSI2_DPHY) {
> +               ret = -EINVAL;
> +               dev_err(ov7251->dev, "invalid bus type (%u), must be (%u)\n",
> +                       bus_cfg.bus_type, V4L2_MBUS_CSI2_DPHY);
> +               goto out_free_bus_cfg;
> +       }
> +
> +       if (bus_cfg.bus.mipi_csi2.num_data_lanes != 1) {
> +               dev_err(ov7251->dev, "only a 1-lane CSI2 config is supported");
> +               ret = -EINVAL;
> +               goto out_free_bus_cfg;
> +       }
> +
> +       if (!bus_cfg.nr_of_link_frequencies) {
> +               dev_err(ov7251->dev, "no link frequencies defined\n");
> +               ret = -EINVAL;
> +               goto out_free_bus_cfg;
> +       }
> +
> +       freq_found = false;
> +       for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
> +               if (freq_found)
> +                       break;
> +
> +               for (j = 0; j < ARRAY_SIZE(link_freq); j++)
> +                       if (bus_cfg.link_frequencies[i] == link_freq[j]) {
> +                               freq_found = true;
> +                               break;
> +                       }
> +       }
> +
> +       if (i == bus_cfg.nr_of_link_frequencies) {

This doesn't work if there is only one link frequency defined in the
config and it is valid.

Loop i=0, freq_found gets set to true.
Continue the outer loop.
i++ , so i=1.
i < bus_cfg.nr_of_link_frequencies is no longer true (both are 1), so
we quit the outer loop.
i == bus_cfg.nr_of_link_frequencies, so we now fail the function.

Doesn't this last check want to be if (!freq_found) ?

And/or amend the loop to move the "if (freq_found) break;" to the end,
so that we break out of the outer loop as soon as we have a frequency
found, and thereby avoid the i++.

It all feels a little odd as there is only one link frequency used by
all the modes, and we're not actually filtering the modes that can be
selected based on the configured link frequencies should there be
multiple frequencies in link_freq[].

  Dave

> +               dev_err(ov7251->dev, "no supported link freq found\n");
> +               ret = -EINVAL;
> +               goto out_free_bus_cfg;
> +       }
> +
> +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 +1333,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 04/10] media: i2c: Provide ov7251_check_hwcfg()

[Daniel Scally]

Hi Dave

On 17/02/2022 15:54, Dave Stevenson wrote:
> Hi Daniel
>
> On Tue, 15 Feb 2022 at 23:08, Daniel Scally <djrscally@gmail.com> 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. Finally, return
>> -EPROBE_DEFER if the endpoint is not available, as it could be built
>> by the ipu3-cio2 driver if that probes later.
>>
>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>> ---
>>  drivers/media/i2c/ov7251.c | 84 ++++++++++++++++++++++++++++++--------
>>  1 file changed, 66 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
>> index d6fe574cb9e0..5c5f7a15a640 100644
>> --- a/drivers/media/i2c/ov7251.c
>> +++ b/drivers/media/i2c/ov7251.c
>> @@ -1255,10 +1255,73 @@ 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;
>> +       bool freq_found;
>> +       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.bus_type != V4L2_MBUS_CSI2_DPHY) {
>> +               ret = -EINVAL;
>> +               dev_err(ov7251->dev, "invalid bus type (%u), must be (%u)\n",
>> +                       bus_cfg.bus_type, V4L2_MBUS_CSI2_DPHY);
>> +               goto out_free_bus_cfg;
>> +       }
>> +
>> +       if (bus_cfg.bus.mipi_csi2.num_data_lanes != 1) {
>> +               dev_err(ov7251->dev, "only a 1-lane CSI2 config is supported");
>> +               ret = -EINVAL;
>> +               goto out_free_bus_cfg;
>> +       }
>> +
>> +       if (!bus_cfg.nr_of_link_frequencies) {
>> +               dev_err(ov7251->dev, "no link frequencies defined\n");
>> +               ret = -EINVAL;
>> +               goto out_free_bus_cfg;
>> +       }
>> +
>> +       freq_found = false;
>> +       for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
>> +               if (freq_found)
>> +                       break;
>> +
>> +               for (j = 0; j < ARRAY_SIZE(link_freq); j++)
>> +                       if (bus_cfg.link_frequencies[i] == link_freq[j]) {
>> +                               freq_found = true;
>> +                               break;
>> +                       }
>> +       }
>> +
>> +       if (i == bus_cfg.nr_of_link_frequencies) {
> This doesn't work if there is only one link frequency defined in the
> config and it is valid.
>
> Loop i=0, freq_found gets set to true.
> Continue the outer loop.
> i++ , so i=1.
> i < bus_cfg.nr_of_link_frequencies is no longer true (both are 1), so
> we quit the outer loop.
> i == bus_cfg.nr_of_link_frequencies, so we now fail the function.
>
> Doesn't this last check want to be if (!freq_found) ?
>
> And/or amend the loop to move the "if (freq_found) break;" to the end,
> so that we break out of the outer loop as soon as we have a frequency
> found, and thereby avoid the i++.


Ah, damn you're right. Sorry - I originally broke the double loop with a
goto and then decided at the last minute that it was too ugly so I
changed it. Thought I was careful enough there to not need to test it -
lesson learned.

> It all feels a little odd as there is only one link frequency used by
> all the modes, and we're not actually filtering the modes that can be
> selected based on the configured link frequencies should there be
> multiple frequencies in link_freq[].


At present no, but I was thinking about adding one (the windows driver
for my platform uses a different link freq, and I'd like to support it)
- it'll just mean a different set of ov7251_pll_configs.

>
>   Dave
>
>> +               dev_err(ov7251->dev, "no supported link freq found\n");
>> +               ret = -EINVAL;
>> +               goto out_free_bus_cfg;
>> +       }
>> +
>> +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 +1333,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 04/10] media: i2c: Provide ov7251_check_hwcfg()

[Dave Stevenson]

Hi Daniel

On Fri, 18 Feb 2022 at 08:37, Daniel Scally <djrscally@gmail.com> wrote:
>
> Hi Dave
>
> On 17/02/2022 15:54, Dave Stevenson wrote:
> > Hi Daniel
> >
> > On Tue, 15 Feb 2022 at 23:08, Daniel Scally <djrscally@gmail.com> 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. Finally, return
> >> -EPROBE_DEFER if the endpoint is not available, as it could be built
> >> by the ipu3-cio2 driver if that probes later.
> >>
> >> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> >> ---
> >>  drivers/media/i2c/ov7251.c | 84 ++++++++++++++++++++++++++++++--------
> >>  1 file changed, 66 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
> >> index d6fe574cb9e0..5c5f7a15a640 100644
> >> --- a/drivers/media/i2c/ov7251.c
> >> +++ b/drivers/media/i2c/ov7251.c
> >> @@ -1255,10 +1255,73 @@ 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;
> >> +       bool freq_found;
> >> +       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.bus_type != V4L2_MBUS_CSI2_DPHY) {
> >> +               ret = -EINVAL;
> >> +               dev_err(ov7251->dev, "invalid bus type (%u), must be (%u)\n",
> >> +                       bus_cfg.bus_type, V4L2_MBUS_CSI2_DPHY);
> >> +               goto out_free_bus_cfg;
> >> +       }
> >> +
> >> +       if (bus_cfg.bus.mipi_csi2.num_data_lanes != 1) {
> >> +               dev_err(ov7251->dev, "only a 1-lane CSI2 config is supported");
> >> +               ret = -EINVAL;
> >> +               goto out_free_bus_cfg;
> >> +       }
> >> +
> >> +       if (!bus_cfg.nr_of_link_frequencies) {
> >> +               dev_err(ov7251->dev, "no link frequencies defined\n");
> >> +               ret = -EINVAL;
> >> +               goto out_free_bus_cfg;
> >> +       }
> >> +
> >> +       freq_found = false;
> >> +       for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
> >> +               if (freq_found)
> >> +                       break;
> >> +
> >> +               for (j = 0; j < ARRAY_SIZE(link_freq); j++)
> >> +                       if (bus_cfg.link_frequencies[i] == link_freq[j]) {
> >> +                               freq_found = true;
> >> +                               break;
> >> +                       }
> >> +       }
> >> +
> >> +       if (i == bus_cfg.nr_of_link_frequencies) {
> > This doesn't work if there is only one link frequency defined in the
> > config and it is valid.
> >
> > Loop i=0, freq_found gets set to true.
> > Continue the outer loop.
> > i++ , so i=1.
> > i < bus_cfg.nr_of_link_frequencies is no longer true (both are 1), so
> > we quit the outer loop.
> > i == bus_cfg.nr_of_link_frequencies, so we now fail the function.
> >
> > Doesn't this last check want to be if (!freq_found) ?
> >
> > And/or amend the loop to move the "if (freq_found) break;" to the end,
> > so that we break out of the outer loop as soon as we have a frequency
> > found, and thereby avoid the i++.
>
>
> Ah, damn you're right. Sorry - I originally broke the double loop with a
> goto and then decided at the last minute that it was too ugly so I
> changed it. Thought I was careful enough there to not need to test it -
> lesson learned.

Been there, done that :-)

> > It all feels a little odd as there is only one link frequency used by
> > all the modes, and we're not actually filtering the modes that can be
> > selected based on the configured link frequencies should there be
> > multiple frequencies in link_freq[].
>
>
> At present no, but I was thinking about adding one (the windows driver
> for my platform uses a different link freq, and I'd like to support it)
> - it'll just mean a different set of ov7251_pll_configs.

OK, that seems reasonable.

Someone recently asked about running ov7251 with libcamera on a Pi
which was why I was testing your patches. I've a branch[1] that takes
your patches and then adds the relevant controls to make it work -
I'll send them once your current changes are merged to avoid
conflicts.
I've removed the link freq per mode as generally all modes run at the
same link freq, and there's really only one mode anyway. Is that valid
for your alternate link freq? Does it alter the pixel rate as well, or
just the CSI config? There's no point in removing options if you'll
want them back again.

Cheers
  Dave

[1] https://github.com/6by9/linux/tree/rpi-5.15.y-cam


> >
> >   Dave
> >
> >> +               dev_err(ov7251->dev, "no supported link freq found\n");
> >> +               ret = -EINVAL;
> >> +               goto out_free_bus_cfg;
> >> +       }
> >> +
> >> +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 +1333,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 04/10] media: i2c: Provide ov7251_check_hwcfg()

[Daniel Scally]

On 18/02/2022 11:58, Dave Stevenson wrote:
> Hi Daniel
>
> On Fri, 18 Feb 2022 at 08:37, Daniel Scally <djrscally@gmail.com> wrote:
>> Hi Dave
>>
>> On 17/02/2022 15:54, Dave Stevenson wrote:
>>> Hi Daniel
>>>
>>> On Tue, 15 Feb 2022 at 23:08, Daniel Scally <djrscally@gmail.com> 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. Finally, return
>>>> -EPROBE_DEFER if the endpoint is not available, as it could be built
>>>> by the ipu3-cio2 driver if that probes later.
>>>>
>>>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>>>> ---
>>>>  drivers/media/i2c/ov7251.c | 84 ++++++++++++++++++++++++++++++--------
>>>>  1 file changed, 66 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
>>>> index d6fe574cb9e0..5c5f7a15a640 100644
>>>> --- a/drivers/media/i2c/ov7251.c
>>>> +++ b/drivers/media/i2c/ov7251.c
>>>> @@ -1255,10 +1255,73 @@ 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;
>>>> +       bool freq_found;
>>>> +       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.bus_type != V4L2_MBUS_CSI2_DPHY) {
>>>> +               ret = -EINVAL;
>>>> +               dev_err(ov7251->dev, "invalid bus type (%u), must be (%u)\n",
>>>> +                       bus_cfg.bus_type, V4L2_MBUS_CSI2_DPHY);
>>>> +               goto out_free_bus_cfg;
>>>> +       }
>>>> +
>>>> +       if (bus_cfg.bus.mipi_csi2.num_data_lanes != 1) {
>>>> +               dev_err(ov7251->dev, "only a 1-lane CSI2 config is supported");
>>>> +               ret = -EINVAL;
>>>> +               goto out_free_bus_cfg;
>>>> +       }
>>>> +
>>>> +       if (!bus_cfg.nr_of_link_frequencies) {
>>>> +               dev_err(ov7251->dev, "no link frequencies defined\n");
>>>> +               ret = -EINVAL;
>>>> +               goto out_free_bus_cfg;
>>>> +       }
>>>> +
>>>> +       freq_found = false;
>>>> +       for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
>>>> +               if (freq_found)
>>>> +                       break;
>>>> +
>>>> +               for (j = 0; j < ARRAY_SIZE(link_freq); j++)
>>>> +                       if (bus_cfg.link_frequencies[i] == link_freq[j]) {
>>>> +                               freq_found = true;
>>>> +                               break;
>>>> +                       }
>>>> +       }
>>>> +
>>>> +       if (i == bus_cfg.nr_of_link_frequencies) {
>>> This doesn't work if there is only one link frequency defined in the
>>> config and it is valid.
>>>
>>> Loop i=0, freq_found gets set to true.
>>> Continue the outer loop.
>>> i++ , so i=1.
>>> i < bus_cfg.nr_of_link_frequencies is no longer true (both are 1), so
>>> we quit the outer loop.
>>> i == bus_cfg.nr_of_link_frequencies, so we now fail the function.
>>>
>>> Doesn't this last check want to be if (!freq_found) ?
>>>
>>> And/or amend the loop to move the "if (freq_found) break;" to the end,
>>> so that we break out of the outer loop as soon as we have a frequency
>>> found, and thereby avoid the i++.
>>
>> Ah, damn you're right. Sorry - I originally broke the double loop with a
>> goto and then decided at the last minute that it was too ugly so I
>> changed it. Thought I was careful enough there to not need to test it -
>> lesson learned.
> Been there, done that :-)


Hah hoping that this will sink in strongly for at least a little while!

>
>>> It all feels a little odd as there is only one link frequency used by
>>> all the modes, and we're not actually filtering the modes that can be
>>> selected based on the configured link frequencies should there be
>>> multiple frequencies in link_freq[].
>>
>> At present no, but I was thinking about adding one (the windows driver
>> for my platform uses a different link freq, and I'd like to support it)
>> - it'll just mean a different set of ov7251_pll_configs.
> OK, that seems reasonable.
>
> Someone recently asked about running ov7251 with libcamera on a Pi
> which was why I was testing your patches. I've a branch[1] that takes
> your patches and then adds the relevant controls to make it work -
> I'll send them once your current changes are merged to avoid
> conflicts.


Ah excellent, hadn't gotten as far as trying to run this through
libcamera yet, it's connected to an IPU3 for me so there's a bit more
work to do there first.

> I've removed the link freq per mode as generally all modes run at the
> same link freq, and there's really only one mode anyway. Is that valid
> for your alternate link freq? Does it alter the pixel rate as well, or
> just the CSI config? There's no point in removing options if you'll
> want them back again.


Can't tell what Windows does as I have no way of forcing it to change
the mode (in fact I can only make it turn on by starting Windows Hello
set up and then waving my head around like a lunatic so it doesn't
complete / fail out before I get the chance to read the registers) but
I'd expect to implement it as a static (I.E. not changing per mode)
pixel rate that depends on the selected link freq. I think it's fine to
drop both options from the mode struct


Cheers

Dan

>
> Cheers
>   Dave
>
> [1] https://github.com/6by9/linux/tree/rpi-5.15.y-cam
>
>
>>>   Dave
>>>
>>>> +               dev_err(ov7251->dev, "no supported link freq found\n");
>>>> +               ret = -EINVAL;
>>>> +               goto out_free_bus_cfg;
>>>> +       }
>>>> +
>>>> +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 +1333,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
>>>>