[PATCH v2 00/12] Extensions to ov8865 driver

[PATCH v2 00/12] Extensions to ov8865 driver

[Daniel Scally]

Hello all

This series extends the ov8865 driver with:

* Support for binding to ACPI enumerated devices.
* Support for a 19.2MHz clock in addition to existing 24MHz clock support
* Another v4l2_subdev_pad_ops callback
* 4 more V4L2 controls
* makes the driver supported by the cio2-bridge

There's also a little bit of tidying that I did along the way.

The series is tested on an MS Surface Go 2.

Thanks
Dan

Series changes since V2:

	- Dropped a patch removing unused Macros from the driver.

Daniel Scally (12):
  media: i2c: Add ACPI support to ov8865
  media: i2c: Fix incorrect value in comment
  media: i2c: Defer probe if not endpoint found
  media: i2c: Support 19.2MHz input clock in ov8865
  media: i2c: Add .get_selection() support to ov8865
  media: i2c: Switch control to V4L2_CID_ANALOGUE_GAIN
  media: i2c: Add vblank control to ov8865
  media: i2c: Add hblank control to ov8865
  media: i2c: cap exposure at height + vblank in ov8865
  media: i2c: Add controls from fwnode to ov8865
  media: i2c: Switch exposure control unit to lines
  media: ipu3-cio2: Add INT347A to cio2-bridge

 drivers/media/i2c/ov8865.c                 | 334 +++++++++++++++++----
 drivers/media/pci/intel/ipu3/cio2-bridge.c |   2 +
 2 files changed, 282 insertions(+), 54 deletions(-)

-- 
2.25.1

[PATCH v2 01/12] media: i2c: Add ACPI support to ov8865

[Daniel Scally]

The ov8865 sensor is sometimes found on x86 platforms enumerated via ACPI.
Add an ACPI match table to the driver so that it's probed on those
platforms.

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

	- Fix the new include to be mod_devicetable.h, not acpi.h (Andy)

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

diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
index ce50f3ea87b8..cf6cc55e8506 100644
--- a/drivers/media/i2c/ov8865.c
+++ b/drivers/media/i2c/ov8865.c
@@ -9,6 +9,7 @@
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/i2c.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/of_graph.h>
 #include <linux/pm_runtime.h>
@@ -2946,6 +2947,12 @@ static const struct dev_pm_ops ov8865_pm_ops = {
 	SET_RUNTIME_PM_OPS(ov8865_suspend, ov8865_resume, NULL)
 };
 
+static const struct acpi_device_id ov8865_acpi_match[] = {
+	{"INT347A"},
+	{},
+};
+MODULE_DEVICE_TABLE(acpi, ov8865_acpi_match);
+
 static const struct of_device_id ov8865_of_match[] = {
 	{ .compatible = "ovti,ov8865" },
 	{ }
@@ -2956,6 +2963,7 @@ static struct i2c_driver ov8865_driver = {
 	.driver = {
 		.name = "ov8865",
 		.of_match_table = ov8865_of_match,
+		.acpi_match_table = ov8865_acpi_match,
 		.pm = &ov8865_pm_ops,
 	},
 	.probe_new = ov8865_probe,
-- 
2.25.1

Re: [PATCH v2 01/12] media: i2c: Add ACPI support to ov8865

[Andy Shevchenko]

On Tue, Aug 10, 2021 at 1:58 AM Daniel Scally <djrscally@gmail.com> wrote:
>
> The ov8865 sensor is sometimes found on x86 platforms enumerated via ACPI.
> Add an ACPI match table to the driver so that it's probed on those
> platforms.

...

> +static const struct acpi_device_id ov8865_acpi_match[] = {
> +       {"INT347A"},
> +       {},

No comma for terminator entry.

> +};

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 02/12] media: i2c: Fix incorrect value in comment

[Daniel Scally]

The PLL configuration defined here sets 72MHz (which is correct), not
80MHz. Correct the comment.

Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v2:

	- None

 drivers/media/i2c/ov8865.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
index cf6cc55e8506..8d973117f611 100644
--- a/drivers/media/i2c/ov8865.c
+++ b/drivers/media/i2c/ov8865.c
@@ -713,7 +713,7 @@ static const struct ov8865_pll2_config ov8865_pll2_config_native = {
 /*
  * EXTCLK = 24 MHz
  * DAC_CLK = 360 MHz
- * SCLK = 80 MHz
+ * SCLK = 72 MHz
  */
 
 static const struct ov8865_pll2_config ov8865_pll2_config_binning = {
-- 
2.25.1

[PATCH v2 03/12] media: i2c: Defer probe if not endpoint found

[Daniel Scally]

The ov8865 driver is one of those that can be connected to a CIO2
device by the cio2-bridge code. This means that the absence of an
endpoint for this device is not necessarily fatal, as one might be
built by the cio2-bridge when it probes. Return -EPROBE_DEFER if no
endpoint is found rather than a fatal error.

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

	- None

 drivers/media/i2c/ov8865.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
index 8d973117f611..fe700787bfb9 100644
--- a/drivers/media/i2c/ov8865.c
+++ b/drivers/media/i2c/ov8865.c
@@ -2796,10 +2796,8 @@ static int ov8865_probe(struct i2c_client *client)
 	/* Graph Endpoint */
 
 	handle = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
-	if (!handle) {
-		dev_err(dev, "unable to find endpoint node\n");
-		return -EINVAL;
-	}
+	if (!handle)
+		return -EPROBE_DEFER;
 
 	sensor->endpoint.bus_type = V4L2_MBUS_CSI2_DPHY;
 
-- 
2.25.1

[PATCH v2 04/12] media: i2c: Support 19.2MHz input clock in ov8865

[Daniel Scally]

The ov8865 driver as written expects a 24MHz input clock, but the sensor
is sometimes found on x86 platforms with a 19.2MHz input clock supplied.
Add a set of PLL configurations to the driver to support that rate too.
As ACPI doesn't auto-configure the clock rate, check for a clock-frequency
during probe and set that rate if one is found.

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

	- Added an enum defining the possible frequency rates to index the
	  array (Andy)

 drivers/media/i2c/ov8865.c | 164 +++++++++++++++++++++++++++----------
 1 file changed, 121 insertions(+), 43 deletions(-)

diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
index fe700787bfb9..1382b16d1a09 100644
--- a/drivers/media/i2c/ov8865.c
+++ b/drivers/media/i2c/ov8865.c
@@ -21,10 +21,6 @@
 #include <media/v4l2-image-sizes.h>
 #include <media/v4l2-mediabus.h>
 
-/* Clock rate */
-
-#define OV8865_EXTCLK_RATE			24000000
-
 /* Register definitions */
 
 /* System */
@@ -567,6 +563,19 @@ struct ov8865_sclk_config {
 	unsigned int sclk_div;
 };
 
+/* Clock rate */
+
+enum extclk_rate {
+	OV8865_19_2_MHZ,
+	OV8865_24_MHZ,
+	OV8865_NUM_SUPPORTED_RATES,
+};
+
+static const unsigned long supported_extclk_rates[] = {
+	[OV8865_19_2_MHZ] = 19200000,
+	[OV8865_24_MHZ] = 24000000,
+};
+
 /*
  * General formulas for (array-centered) mode calculation:
  * - photo_array_width = 3296
@@ -665,6 +674,9 @@ struct ov8865_sensor {
 	struct regulator *avdd;
 	struct regulator *dvdd;
 	struct regulator *dovdd;
+
+	unsigned long extclk_rate;
+	enum extclk_rate extclk_rate_idx;
 	struct clk *extclk;
 
 	struct v4l2_fwnode_endpoint endpoint;
@@ -680,49 +692,83 @@ struct ov8865_sensor {
 /* Static definitions */
 
 /*
- * EXTCLK = 24 MHz
  * PHY_SCLK = 720 MHz
  * MIPI_PCLK = 90 MHz
  */
-static const struct ov8865_pll1_config ov8865_pll1_config_native = {
-	.pll_pre_div_half	= 1,
-	.pll_pre_div		= 0,
-	.pll_mul		= 30,
-	.m_div			= 1,
-	.mipi_div		= 3,
-	.pclk_div		= 1,
-	.sys_pre_div		= 1,
-	.sys_div		= 2,
+
+static const struct ov8865_pll1_config ov8865_pll1_configs_native[] = {
+	{ /* 19.2 MHz input clock */
+		.pll_pre_div_half	= 1,
+		.pll_pre_div		= 2,
+		.pll_mul		= 75,
+		.m_div			= 1,
+		.mipi_div		= 3,
+		.pclk_div		= 1,
+		.sys_pre_div		= 1,
+		.sys_div		= 2,
+	},
+	{ /* 24MHz input clock */
+		.pll_pre_div_half	= 1,
+		.pll_pre_div		= 0,
+		.pll_mul		= 30,
+		.m_div			= 1,
+		.mipi_div		= 3,
+		.pclk_div		= 1,
+		.sys_pre_div		= 1,
+		.sys_div		= 2,
+	},
 };
 
 /*
- * EXTCLK = 24 MHz
  * DAC_CLK = 360 MHz
  * SCLK = 144 MHz
  */
 
-static const struct ov8865_pll2_config ov8865_pll2_config_native = {
-	.pll_pre_div_half	= 1,
-	.pll_pre_div		= 0,
-	.pll_mul		= 30,
-	.dac_div		= 2,
-	.sys_pre_div		= 5,
-	.sys_div		= 0,
+static const struct ov8865_pll2_config ov8865_pll2_configs_native[] = {
+	/* 19.2MHz input clock */
+	{
+		.pll_pre_div_half	= 1,
+		.pll_pre_div		= 5,
+		.pll_mul		= 75,
+		.dac_div		= 1,
+		.sys_pre_div		= 1,
+		.sys_div		= 3,
+	},
+	/* 24MHz input clock */
+	{
+		.pll_pre_div_half	= 1,
+		.pll_pre_div		= 0,
+		.pll_mul		= 30,
+		.dac_div		= 2,
+		.sys_pre_div		= 5,
+		.sys_div		= 0,
+	}
 };
 
 /*
- * EXTCLK = 24 MHz
  * DAC_CLK = 360 MHz
  * SCLK = 72 MHz
  */
 
-static const struct ov8865_pll2_config ov8865_pll2_config_binning = {
+static const struct ov8865_pll2_config ov8865_pll2_configs_binning[] = {
+	/* 19.2MHz input clock */
+	{
+	.pll_pre_div_half	= 1,
+	.pll_pre_div		= 2,
+	.pll_mul		= 75,
+	.dac_div		= 2,
+	.sys_pre_div		= 10,
+	.sys_div		= 0,
+	},
+	/* 24MHz input clock */
+	{
 	.pll_pre_div_half	= 1,
 	.pll_pre_div		= 0,
 	.pll_mul		= 30,
 	.dac_div		= 2,
 	.sys_pre_div		= 10,
 	.sys_div		= 0,
+	}
 };
 
 static const struct ov8865_sclk_config ov8865_sclk_config_native = {
@@ -934,8 +980,8 @@ static const struct ov8865_mode ov8865_modes[] = {
 		.frame_interval			= { 1, 30 },
 
 		/* PLL */
-		.pll1_config			= &ov8865_pll1_config_native,
-		.pll2_config			= &ov8865_pll2_config_native,
+		.pll1_config			= ov8865_pll1_configs_native,
+		.pll2_config			= ov8865_pll2_configs_native,
 		.sclk_config			= &ov8865_sclk_config_native,
 
 		/* Registers */
@@ -990,8 +1036,8 @@ static const struct ov8865_mode ov8865_modes[] = {
 		.frame_interval			= { 1, 30 },
 
 		/* PLL */
-		.pll1_config			= &ov8865_pll1_config_native,
-		.pll2_config			= &ov8865_pll2_config_native,
+		.pll1_config			= ov8865_pll1_configs_native,
+		.pll2_config			= ov8865_pll2_configs_native,
 		.sclk_config			= &ov8865_sclk_config_native,
 
 		/* Registers */
@@ -1050,8 +1096,8 @@ static const struct ov8865_mode ov8865_modes[] = {
 		.frame_interval			= { 1, 30 },
 
 		/* PLL */
-		.pll1_config			= &ov8865_pll1_config_native,
-		.pll2_config			= &ov8865_pll2_config_binning,
+		.pll1_config			= ov8865_pll1_configs_native,
+		.pll2_config			= ov8865_pll2_configs_binning,
 		.sclk_config			= &ov8865_sclk_config_native,
 
 		/* Registers */
@@ -1116,8 +1162,8 @@ static const struct ov8865_mode ov8865_modes[] = {
 		.frame_interval			= { 1, 90 },
 
 		/* PLL */
-		.pll1_config			= &ov8865_pll1_config_native,
-		.pll2_config			= &ov8865_pll2_config_binning,
+		.pll1_config			= ov8865_pll1_configs_native,
+		.pll2_config			= ov8865_pll2_configs_binning,
 		.sclk_config			= &ov8865_sclk_config_native,
 
 		/* Registers */
@@ -1513,12 +1559,11 @@ static int ov8865_isp_configure(struct ov8865_sensor *sensor)
 static unsigned long ov8865_mode_pll1_rate(struct ov8865_sensor *sensor,
 					   const struct ov8865_mode *mode)
 {
-	const struct ov8865_pll1_config *config = mode->pll1_config;
-	unsigned long extclk_rate;
+	const struct ov8865_pll1_config *config;
 	unsigned long pll1_rate;
 
-	extclk_rate = clk_get_rate(sensor->extclk);
-	pll1_rate = extclk_rate * config->pll_mul / config->pll_pre_div_half;
+	config = &mode->pll1_config[sensor->extclk_rate_idx];
+	pll1_rate = sensor->extclk_rate * config->pll_mul / config->pll_pre_div_half;
 
 	switch (config->pll_pre_div) {
 	case 0:
@@ -1552,10 +1597,12 @@ static int ov8865_mode_pll1_configure(struct ov8865_sensor *sensor,
 				      const struct ov8865_mode *mode,
 				      u32 mbus_code)
 {
-	const struct ov8865_pll1_config *config = mode->pll1_config;
+	const struct ov8865_pll1_config *config;
 	u8 value;
 	int ret;
 
+	config = &mode->pll1_config[sensor->extclk_rate_idx];
+
 	switch (mbus_code) {
 	case MEDIA_BUS_FMT_SBGGR10_1X10:
 		value = OV8865_MIPI_BIT_SEL(10);
@@ -1622,9 +1669,11 @@ static int ov8865_mode_pll1_configure(struct ov8865_sensor *sensor,
 static int ov8865_mode_pll2_configure(struct ov8865_sensor *sensor,
 				      const struct ov8865_mode *mode)
 {
-	const struct ov8865_pll2_config *config = mode->pll2_config;
+	const struct ov8865_pll2_config *config;
 	int ret;
 
+	config = &mode->pll2_config[sensor->extclk_rate_idx];
+
 	ret = ov8865_write(sensor, OV8865_PLL_CTRL12_REG,
 			   OV8865_PLL_CTRL12_PRE_DIV_HALF(config->pll_pre_div_half) |
 			   OV8865_PLL_CTRL12_DAC_DIV(config->dac_div));
@@ -2053,9 +2102,11 @@ static int ov8865_mode_configure(struct ov8865_sensor *sensor,
 static unsigned long ov8865_mode_mipi_clk_rate(struct ov8865_sensor *sensor,
 					       const struct ov8865_mode *mode)
 {
-	const struct ov8865_pll1_config *config = mode->pll1_config;
+	const struct ov8865_pll1_config *config;
 	unsigned long pll1_rate;
 
+	config = &mode->pll1_config[sensor->extclk_rate_idx];
+
 	pll1_rate = ov8865_mode_pll1_rate(sensor, mode);
 
 	return pll1_rate / config->m_div / 2;
@@ -2783,7 +2834,8 @@ static int ov8865_probe(struct i2c_client *client)
 	struct ov8865_sensor *sensor;
 	struct v4l2_subdev *subdev;
 	struct media_pad *pad;
-	unsigned long rate;
+	unsigned int rate;
+	unsigned int i;
 	int ret;
 
 	sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
@@ -2858,13 +2910,39 @@ static int ov8865_probe(struct i2c_client *client)
 		goto error_endpoint;
 	}
 
-	rate = clk_get_rate(sensor->extclk);
-	if (rate != OV8865_EXTCLK_RATE) {
-		dev_err(dev, "clock rate %lu Hz is unsupported\n", rate);
+	/*
+	 * We could have either a 24MHz or 19.2MHz clock rate. Check for a
+	 * clock-frequency property and if found, set that rate. This should
+	 * cover ACPI case. If the system uses devicetree then the configured
+	 * rate should already be set, so we'll have to check it.
+	 */
+
+	ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
+				       &rate);
+	if (!ret) {
+		ret = clk_set_rate(sensor->extclk, rate);
+		if (ret) {
+			dev_err(dev, "failed to set clock rate\n");
+			return ret;
+		}
+	}
+
+	sensor->extclk_rate = clk_get_rate(sensor->extclk);
+
+	for (i = 0; i < ARRAY_SIZE(supported_extclk_rates); i++) {
+		if (sensor->extclk_rate == supported_extclk_rates[i])
+			break;
+	}
+
+	if (i == ARRAY_SIZE(supported_extclk_rates)) {
+		dev_err(dev, "clock rate %lu Hz is unsupported\n",
+			sensor->extclk_rate);
 		ret = -EINVAL;
 		goto error_endpoint;
 	}
 
+	sensor->extclk_rate_idx = i;
+
 	/* Subdev, entity and pad */
 
 	subdev = &sensor->subdev;
-- 
2.25.1

Re: [PATCH v2 04/12] media: i2c: Support 19.2MHz input clock in ov8865

[Andy Shevchenko]

On Tue, Aug 10, 2021 at 1:59 AM Daniel Scally <djrscally@gmail.com> wrote:
>
> The ov8865 driver as written expects a 24MHz input clock, but the sensor
> is sometimes found on x86 platforms with a 19.2MHz input clock supplied.
> Add a set of PLL configurations to the driver to support that rate too.
> As ACPI doesn't auto-configure the clock rate, check for a clock-frequency
> during probe and set that rate if one is found.

...

> +enum extclk_rate {
> +       OV8865_19_2_MHZ,
> +       OV8865_24_MHZ,
> +       OV8865_NUM_SUPPORTED_RATES,

Same here, i.e. no comma,
If these are the only problems during review of v2, you can ignore
them, they are not critical at all.

> +};

...

> +static const struct ov8865_pll2_config ov8865_pll2_configs_native[] = {

> +       /* 24MHz input clock */
> +       {
> +               .pll_pre_div_half       = 1,
> +               .pll_pre_div            = 0,
> +               .pll_mul                = 30,
> +               .dac_div                = 2,
> +               .sys_pre_div            = 5,
> +               .sys_div                = 0,
> +       }

+ comma.

>  };

...

> +       /* 24MHz input clock */
> +       {
>         .pll_pre_div_half       = 1,
>         .pll_pre_div            = 0,
>         .pll_mul                = 30,
>         .dac_div                = 2,
>         .sys_pre_div            = 10,
>         .sys_div                = 0,
> +       }

Ditto.

...

> +       /*
> +        * We could have either a 24MHz or 19.2MHz clock rate. Check for a
> +        * clock-frequency property and if found, set that rate. This should
> +        * cover ACPI case. If the system uses devicetree then the configured

the ACPI case

> +        * rate should already be set, so we'll have to check it.
> +        */

> +

Redundant blank line.

> +       ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
> +                                      &rate);
> +       if (!ret) {

I'm wondering if something like

... rate = 0;

       fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency", &rate);
       if (rate > 0) {

can be used.

> +               ret = clk_set_rate(sensor->extclk, rate);
> +               if (ret) {
> +                       dev_err(dev, "failed to set clock rate\n");
> +                       return ret;
> +               }
> +       }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 04/12] media: i2c: Support 19.2MHz input clock in ov8865

[Daniel Scally]

Hi Andy

On 10/08/2021 14:04, Andy Shevchenko wrote:
>> +       ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
>> +                                      &rate);
>> +       if (!ret) {
> I'm wondering if something like
>
> ... rate = 0;
>
>        fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency", &rate);
>        if (rate > 0) {
>
> can be used.
Should be fine, and I agree that it makes the point of the check more
clear, thanks.

Re: [PATCH v2 04/12] media: i2c: Support 19.2MHz input clock in ov8865

[Sakari Ailus]

Hi Daniel,

Thanks for the set.

On Mon, Aug 09, 2021 at 11:58:37PM +0100, Daniel Scally wrote:
> The ov8865 driver as written expects a 24MHz input clock, but the sensor
> is sometimes found on x86 platforms with a 19.2MHz input clock supplied.
> Add a set of PLL configurations to the driver to support that rate too.
> As ACPI doesn't auto-configure the clock rate, check for a clock-frequency
> during probe and set that rate if one is found.
> 
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes in v2:
> 
> 	- Added an enum defining the possible frequency rates to index the
> 	  array (Andy)
> 
>  drivers/media/i2c/ov8865.c | 164 +++++++++++++++++++++++++++----------
>  1 file changed, 121 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
> index fe700787bfb9..1382b16d1a09 100644
> --- a/drivers/media/i2c/ov8865.c
> +++ b/drivers/media/i2c/ov8865.c
> @@ -21,10 +21,6 @@
>  #include <media/v4l2-image-sizes.h>
>  #include <media/v4l2-mediabus.h>
>  
> -/* Clock rate */
> -
> -#define OV8865_EXTCLK_RATE			24000000
> -
>  /* Register definitions */
>  
>  /* System */
> @@ -567,6 +563,19 @@ struct ov8865_sclk_config {
>  	unsigned int sclk_div;
>  };
>  
> +/* Clock rate */
> +
> +enum extclk_rate {
> +	OV8865_19_2_MHZ,
> +	OV8865_24_MHZ,
> +	OV8865_NUM_SUPPORTED_RATES,
> +};
> +
> +static const unsigned long supported_extclk_rates[] = {
> +	[OV8865_19_2_MHZ] = 19200000,
> +	[OV8865_24_MHZ] = 24000000,
> +};
> +
>  /*
>   * General formulas for (array-centered) mode calculation:
>   * - photo_array_width = 3296
> @@ -665,6 +674,9 @@ struct ov8865_sensor {
>  	struct regulator *avdd;
>  	struct regulator *dvdd;
>  	struct regulator *dovdd;
> +
> +	unsigned long extclk_rate;
> +	enum extclk_rate extclk_rate_idx;
>  	struct clk *extclk;
>  
>  	struct v4l2_fwnode_endpoint endpoint;
> @@ -680,49 +692,83 @@ struct ov8865_sensor {
>  /* Static definitions */
>  
>  /*
> - * EXTCLK = 24 MHz
>   * PHY_SCLK = 720 MHz
>   * MIPI_PCLK = 90 MHz
>   */
> -static const struct ov8865_pll1_config ov8865_pll1_config_native = {
> -	.pll_pre_div_half	= 1,
> -	.pll_pre_div		= 0,
> -	.pll_mul		= 30,
> -	.m_div			= 1,
> -	.mipi_div		= 3,
> -	.pclk_div		= 1,
> -	.sys_pre_div		= 1,
> -	.sys_div		= 2,
> +
> +static const struct ov8865_pll1_config ov8865_pll1_configs_native[] = {
> +	{ /* 19.2 MHz input clock */
> +		.pll_pre_div_half	= 1,
> +		.pll_pre_div		= 2,
> +		.pll_mul		= 75,
> +		.m_div			= 1,
> +		.mipi_div		= 3,
> +		.pclk_div		= 1,
> +		.sys_pre_div		= 1,
> +		.sys_div		= 2,
> +	},
> +	{ /* 24MHz input clock */
> +		.pll_pre_div_half	= 1,
> +		.pll_pre_div		= 0,
> +		.pll_mul		= 30,
> +		.m_div			= 1,
> +		.mipi_div		= 3,
> +		.pclk_div		= 1,
> +		.sys_pre_div		= 1,
> +		.sys_div		= 2,
> +	},

Could you instead add a struct specific to the clock frequency with
pointers to these? See e.g. the ov8856 driver how this could otherwise end
up...

>  };
>  
>  /*
> - * EXTCLK = 24 MHz
>   * DAC_CLK = 360 MHz
>   * SCLK = 144 MHz
>   */
>  
> -static const struct ov8865_pll2_config ov8865_pll2_config_native = {
> -	.pll_pre_div_half	= 1,
> -	.pll_pre_div		= 0,
> -	.pll_mul		= 30,
> -	.dac_div		= 2,
> -	.sys_pre_div		= 5,
> -	.sys_div		= 0,
> +static const struct ov8865_pll2_config ov8865_pll2_configs_native[] = {
> +	/* 19.2MHz input clock */
> +	{
> +		.pll_pre_div_half	= 1,
> +		.pll_pre_div		= 5,
> +		.pll_mul		= 75,
> +		.dac_div		= 1,
> +		.sys_pre_div		= 1,
> +		.sys_div		= 3,
> +	},
> +	/* 24MHz input clock */
> +	{
> +		.pll_pre_div_half	= 1,
> +		.pll_pre_div		= 0,
> +		.pll_mul		= 30,
> +		.dac_div		= 2,
> +		.sys_pre_div		= 5,
> +		.sys_div		= 0,
> +	}
>  };
>  
>  /*
> - * EXTCLK = 24 MHz
>   * DAC_CLK = 360 MHz
>   * SCLK = 72 MHz
>   */
>  
> -static const struct ov8865_pll2_config ov8865_pll2_config_binning = {
> +static const struct ov8865_pll2_config ov8865_pll2_configs_binning[] = {
> +	/* 19.2MHz input clock */
> +	{
> +	.pll_pre_div_half	= 1,
> +	.pll_pre_div		= 2,
> +	.pll_mul		= 75,
> +	.dac_div		= 2,
> +	.sys_pre_div		= 10,
> +	.sys_div		= 0,
> +	},
> +	/* 24MHz input clock */
> +	{
>  	.pll_pre_div_half	= 1,
>  	.pll_pre_div		= 0,
>  	.pll_mul		= 30,
>  	.dac_div		= 2,
>  	.sys_pre_div		= 10,
>  	.sys_div		= 0,
> +	}
>  };
>  
>  static const struct ov8865_sclk_config ov8865_sclk_config_native = {
> @@ -934,8 +980,8 @@ static const struct ov8865_mode ov8865_modes[] = {
>  		.frame_interval			= { 1, 30 },
>  
>  		/* PLL */
> -		.pll1_config			= &ov8865_pll1_config_native,
> -		.pll2_config			= &ov8865_pll2_config_native,
> +		.pll1_config			= ov8865_pll1_configs_native,
> +		.pll2_config			= ov8865_pll2_configs_native,
>  		.sclk_config			= &ov8865_sclk_config_native,
>  
>  		/* Registers */
> @@ -990,8 +1036,8 @@ static const struct ov8865_mode ov8865_modes[] = {
>  		.frame_interval			= { 1, 30 },
>  
>  		/* PLL */
> -		.pll1_config			= &ov8865_pll1_config_native,
> -		.pll2_config			= &ov8865_pll2_config_native,
> +		.pll1_config			= ov8865_pll1_configs_native,
> +		.pll2_config			= ov8865_pll2_configs_native,
>  		.sclk_config			= &ov8865_sclk_config_native,
>  
>  		/* Registers */
> @@ -1050,8 +1096,8 @@ static const struct ov8865_mode ov8865_modes[] = {
>  		.frame_interval			= { 1, 30 },
>  
>  		/* PLL */
> -		.pll1_config			= &ov8865_pll1_config_native,
> -		.pll2_config			= &ov8865_pll2_config_binning,
> +		.pll1_config			= ov8865_pll1_configs_native,
> +		.pll2_config			= ov8865_pll2_configs_binning,
>  		.sclk_config			= &ov8865_sclk_config_native,
>  
>  		/* Registers */
> @@ -1116,8 +1162,8 @@ static const struct ov8865_mode ov8865_modes[] = {
>  		.frame_interval			= { 1, 90 },
>  
>  		/* PLL */
> -		.pll1_config			= &ov8865_pll1_config_native,
> -		.pll2_config			= &ov8865_pll2_config_binning,
> +		.pll1_config			= ov8865_pll1_configs_native,
> +		.pll2_config			= ov8865_pll2_configs_binning,
>  		.sclk_config			= &ov8865_sclk_config_native,
>  
>  		/* Registers */
> @@ -1513,12 +1559,11 @@ static int ov8865_isp_configure(struct ov8865_sensor *sensor)
>  static unsigned long ov8865_mode_pll1_rate(struct ov8865_sensor *sensor,
>  					   const struct ov8865_mode *mode)
>  {
> -	const struct ov8865_pll1_config *config = mode->pll1_config;
> -	unsigned long extclk_rate;
> +	const struct ov8865_pll1_config *config;
>  	unsigned long pll1_rate;
>  
> -	extclk_rate = clk_get_rate(sensor->extclk);
> -	pll1_rate = extclk_rate * config->pll_mul / config->pll_pre_div_half;
> +	config = &mode->pll1_config[sensor->extclk_rate_idx];
> +	pll1_rate = sensor->extclk_rate * config->pll_mul / config->pll_pre_div_half;
>  
>  	switch (config->pll_pre_div) {
>  	case 0:
> @@ -1552,10 +1597,12 @@ static int ov8865_mode_pll1_configure(struct ov8865_sensor *sensor,
>  				      const struct ov8865_mode *mode,
>  				      u32 mbus_code)
>  {
> -	const struct ov8865_pll1_config *config = mode->pll1_config;
> +	const struct ov8865_pll1_config *config;
>  	u8 value;
>  	int ret;
>  
> +	config = &mode->pll1_config[sensor->extclk_rate_idx];
> +
>  	switch (mbus_code) {
>  	case MEDIA_BUS_FMT_SBGGR10_1X10:
>  		value = OV8865_MIPI_BIT_SEL(10);
> @@ -1622,9 +1669,11 @@ static int ov8865_mode_pll1_configure(struct ov8865_sensor *sensor,
>  static int ov8865_mode_pll2_configure(struct ov8865_sensor *sensor,
>  				      const struct ov8865_mode *mode)
>  {
> -	const struct ov8865_pll2_config *config = mode->pll2_config;
> +	const struct ov8865_pll2_config *config;
>  	int ret;
>  
> +	config = &mode->pll2_config[sensor->extclk_rate_idx];
> +
>  	ret = ov8865_write(sensor, OV8865_PLL_CTRL12_REG,
>  			   OV8865_PLL_CTRL12_PRE_DIV_HALF(config->pll_pre_div_half) |
>  			   OV8865_PLL_CTRL12_DAC_DIV(config->dac_div));
> @@ -2053,9 +2102,11 @@ static int ov8865_mode_configure(struct ov8865_sensor *sensor,
>  static unsigned long ov8865_mode_mipi_clk_rate(struct ov8865_sensor *sensor,
>  					       const struct ov8865_mode *mode)
>  {
> -	const struct ov8865_pll1_config *config = mode->pll1_config;
> +	const struct ov8865_pll1_config *config;
>  	unsigned long pll1_rate;
>  
> +	config = &mode->pll1_config[sensor->extclk_rate_idx];
> +
>  	pll1_rate = ov8865_mode_pll1_rate(sensor, mode);
>  
>  	return pll1_rate / config->m_div / 2;
> @@ -2783,7 +2834,8 @@ static int ov8865_probe(struct i2c_client *client)
>  	struct ov8865_sensor *sensor;
>  	struct v4l2_subdev *subdev;
>  	struct media_pad *pad;
> -	unsigned long rate;
> +	unsigned int rate;
> +	unsigned int i;
>  	int ret;
>  
>  	sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
> @@ -2858,13 +2910,39 @@ static int ov8865_probe(struct i2c_client *client)
>  		goto error_endpoint;
>  	}
>  
> -	rate = clk_get_rate(sensor->extclk);
> -	if (rate != OV8865_EXTCLK_RATE) {
> -		dev_err(dev, "clock rate %lu Hz is unsupported\n", rate);
> +	/*
> +	 * We could have either a 24MHz or 19.2MHz clock rate. Check for a
> +	 * clock-frequency property and if found, set that rate. This should
> +	 * cover ACPI case. If the system uses devicetree then the configured
> +	 * rate should already be set, so we'll have to check it.
> +	 */
> +
> +	ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
> +				       &rate);
> +	if (!ret) {
> +		ret = clk_set_rate(sensor->extclk, rate);
> +		if (ret) {
> +			dev_err(dev, "failed to set clock rate\n");
> +			return ret;
> +		}
> +	}
> +
> +	sensor->extclk_rate = clk_get_rate(sensor->extclk);
> +
> +	for (i = 0; i < ARRAY_SIZE(supported_extclk_rates); i++) {
> +		if (sensor->extclk_rate == supported_extclk_rates[i])
> +			break;
> +	}
> +
> +	if (i == ARRAY_SIZE(supported_extclk_rates)) {
> +		dev_err(dev, "clock rate %lu Hz is unsupported\n",
> +			sensor->extclk_rate);
>  		ret = -EINVAL;
>  		goto error_endpoint;
>  	}
>  
> +	sensor->extclk_rate_idx = i;
> +
>  	/* Subdev, entity and pad */
>  
>  	subdev = &sensor->subdev;

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 04/12] media: i2c: Support 19.2MHz input clock in ov8865

[Daniel Scally]

Hi Sakari - thanks for all the comments

On 10/08/2021 14:34, Sakari Ailus wrote:
> Hi Daniel,
>
> Thanks for the set.
>
> On Mon, Aug 09, 2021 at 11:58:37PM +0100, Daniel Scally wrote:
>> The ov8865 driver as written expects a 24MHz input clock, but the sensor
>> is sometimes found on x86 platforms with a 19.2MHz input clock supplied.
>> Add a set of PLL configurations to the driver to support that rate too.
>> As ACPI doesn't auto-configure the clock rate, check for a clock-frequency
>> during probe and set that rate if one is found.
>>
>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>> ---
>> Changes in v2:
>>
>> 	- Added an enum defining the possible frequency rates to index the
>> 	  array (Andy)
>>
>>  drivers/media/i2c/ov8865.c | 164 +++++++++++++++++++++++++++----------
>>  1 file changed, 121 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
>> index fe700787bfb9..1382b16d1a09 100644
>> --- a/drivers/media/i2c/ov8865.c
>> +++ b/drivers/media/i2c/ov8865.c
>> @@ -21,10 +21,6 @@
>>  #include <media/v4l2-image-sizes.h>
>>  #include <media/v4l2-mediabus.h>
>>  
>> -/* Clock rate */
>> -
>> -#define OV8865_EXTCLK_RATE			24000000
>> -
>>  /* Register definitions */
>>  
>>  /* System */
>> @@ -567,6 +563,19 @@ struct ov8865_sclk_config {
>>  	unsigned int sclk_div;
>>  };
>>  
>> +/* Clock rate */
>> +
>> +enum extclk_rate {
>> +	OV8865_19_2_MHZ,
>> +	OV8865_24_MHZ,
>> +	OV8865_NUM_SUPPORTED_RATES,
>> +};
>> +
>> +static const unsigned long supported_extclk_rates[] = {
>> +	[OV8865_19_2_MHZ] = 19200000,
>> +	[OV8865_24_MHZ] = 24000000,
>> +};
>> +
>>  /*
>>   * General formulas for (array-centered) mode calculation:
>>   * - photo_array_width = 3296
>> @@ -665,6 +674,9 @@ struct ov8865_sensor {
>>  	struct regulator *avdd;
>>  	struct regulator *dvdd;
>>  	struct regulator *dovdd;
>> +
>> +	unsigned long extclk_rate;
>> +	enum extclk_rate extclk_rate_idx;
>>  	struct clk *extclk;
>>  
>>  	struct v4l2_fwnode_endpoint endpoint;
>> @@ -680,49 +692,83 @@ struct ov8865_sensor {
>>  /* Static definitions */
>>  
>>  /*
>> - * EXTCLK = 24 MHz
>>   * PHY_SCLK = 720 MHz
>>   * MIPI_PCLK = 90 MHz
>>   */
>> -static const struct ov8865_pll1_config ov8865_pll1_config_native = {
>> -	.pll_pre_div_half	= 1,
>> -	.pll_pre_div		= 0,
>> -	.pll_mul		= 30,
>> -	.m_div			= 1,
>> -	.mipi_div		= 3,
>> -	.pclk_div		= 1,
>> -	.sys_pre_div		= 1,
>> -	.sys_div		= 2,
>> +
>> +static const struct ov8865_pll1_config ov8865_pll1_configs_native[] = {
>> +	{ /* 19.2 MHz input clock */
>> +		.pll_pre_div_half	= 1,
>> +		.pll_pre_div		= 2,
>> +		.pll_mul		= 75,
>> +		.m_div			= 1,
>> +		.mipi_div		= 3,
>> +		.pclk_div		= 1,
>> +		.sys_pre_div		= 1,
>> +		.sys_div		= 2,
>> +	},
>> +	{ /* 24MHz input clock */
>> +		.pll_pre_div_half	= 1,
>> +		.pll_pre_div		= 0,
>> +		.pll_mul		= 30,
>> +		.m_div			= 1,
>> +		.mipi_div		= 3,
>> +		.pclk_div		= 1,
>> +		.sys_pre_div		= 1,
>> +		.sys_div		= 2,
>> +	},
> Could you instead add a struct specific to the clock frequency with
> pointers to these? See e.g. the ov8856 driver how this could otherwise end
> up...


You mean something like


static struct ov8865_pll_configs_19_2_mhz {

    .pll1_config_native = &ov8865_pll1_config_native,

    ...

};



static struct ov8865_pll_configs_24_mhz {

    .pll1_config_native = &ov8865_pll1_config_native,

    ...

};


or am I misunderstanding?

>
>>  };
>>  
>>  /*
>> - * EXTCLK = 24 MHz
>>   * DAC_CLK = 360 MHz
>>   * SCLK = 144 MHz
>>   */
>>  
>> -static const struct ov8865_pll2_config ov8865_pll2_config_native = {
>> -	.pll_pre_div_half	= 1,
>> -	.pll_pre_div		= 0,
>> -	.pll_mul		= 30,
>> -	.dac_div		= 2,
>> -	.sys_pre_div		= 5,
>> -	.sys_div		= 0,
>> +static const struct ov8865_pll2_config ov8865_pll2_configs_native[] = {
>> +	/* 19.2MHz input clock */
>> +	{
>> +		.pll_pre_div_half	= 1,
>> +		.pll_pre_div		= 5,
>> +		.pll_mul		= 75,
>> +		.dac_div		= 1,
>> +		.sys_pre_div		= 1,
>> +		.sys_div		= 3,
>> +	},
>> +	/* 24MHz input clock */
>> +	{
>> +		.pll_pre_div_half	= 1,
>> +		.pll_pre_div		= 0,
>> +		.pll_mul		= 30,
>> +		.dac_div		= 2,
>> +		.sys_pre_div		= 5,
>> +		.sys_div		= 0,
>> +	}
>>  };
>>  
>>  /*
>> - * EXTCLK = 24 MHz
>>   * DAC_CLK = 360 MHz
>>   * SCLK = 72 MHz
>>   */
>>  
>> -static const struct ov8865_pll2_config ov8865_pll2_config_binning = {
>> +static const struct ov8865_pll2_config ov8865_pll2_configs_binning[] = {
>> +	/* 19.2MHz input clock */
>> +	{
>> +	.pll_pre_div_half	= 1,
>> +	.pll_pre_div		= 2,
>> +	.pll_mul		= 75,
>> +	.dac_div		= 2,
>> +	.sys_pre_div		= 10,
>> +	.sys_div		= 0,
>> +	},
>> +	/* 24MHz input clock */
>> +	{
>>  	.pll_pre_div_half	= 1,
>>  	.pll_pre_div		= 0,
>>  	.pll_mul		= 30,
>>  	.dac_div		= 2,
>>  	.sys_pre_div		= 10,
>>  	.sys_div		= 0,
>> +	}
>>  };
>>  
>>  static const struct ov8865_sclk_config ov8865_sclk_config_native = {
>> @@ -934,8 +980,8 @@ static const struct ov8865_mode ov8865_modes[] = {
>>  		.frame_interval			= { 1, 30 },
>>  
>>  		/* PLL */
>> -		.pll1_config			= &ov8865_pll1_config_native,
>> -		.pll2_config			= &ov8865_pll2_config_native,
>> +		.pll1_config			= ov8865_pll1_configs_native,
>> +		.pll2_config			= ov8865_pll2_configs_native,
>>  		.sclk_config			= &ov8865_sclk_config_native,
>>  
>>  		/* Registers */
>> @@ -990,8 +1036,8 @@ static const struct ov8865_mode ov8865_modes[] = {
>>  		.frame_interval			= { 1, 30 },
>>  
>>  		/* PLL */
>> -		.pll1_config			= &ov8865_pll1_config_native,
>> -		.pll2_config			= &ov8865_pll2_config_native,
>> +		.pll1_config			= ov8865_pll1_configs_native,
>> +		.pll2_config			= ov8865_pll2_configs_native,
>>  		.sclk_config			= &ov8865_sclk_config_native,
>>  
>>  		/* Registers */
>> @@ -1050,8 +1096,8 @@ static const struct ov8865_mode ov8865_modes[] = {
>>  		.frame_interval			= { 1, 30 },
>>  
>>  		/* PLL */
>> -		.pll1_config			= &ov8865_pll1_config_native,
>> -		.pll2_config			= &ov8865_pll2_config_binning,
>> +		.pll1_config			= ov8865_pll1_configs_native,
>> +		.pll2_config			= ov8865_pll2_configs_binning,
>>  		.sclk_config			= &ov8865_sclk_config_native,
>>  
>>  		/* Registers */
>> @@ -1116,8 +1162,8 @@ static const struct ov8865_mode ov8865_modes[] = {
>>  		.frame_interval			= { 1, 90 },
>>  
>>  		/* PLL */
>> -		.pll1_config			= &ov8865_pll1_config_native,
>> -		.pll2_config			= &ov8865_pll2_config_binning,
>> +		.pll1_config			= ov8865_pll1_configs_native,
>> +		.pll2_config			= ov8865_pll2_configs_binning,
>>  		.sclk_config			= &ov8865_sclk_config_native,
>>  
>>  		/* Registers */
>> @@ -1513,12 +1559,11 @@ static int ov8865_isp_configure(struct ov8865_sensor *sensor)
>>  static unsigned long ov8865_mode_pll1_rate(struct ov8865_sensor *sensor,
>>  					   const struct ov8865_mode *mode)
>>  {
>> -	const struct ov8865_pll1_config *config = mode->pll1_config;
>> -	unsigned long extclk_rate;
>> +	const struct ov8865_pll1_config *config;
>>  	unsigned long pll1_rate;
>>  
>> -	extclk_rate = clk_get_rate(sensor->extclk);
>> -	pll1_rate = extclk_rate * config->pll_mul / config->pll_pre_div_half;
>> +	config = &mode->pll1_config[sensor->extclk_rate_idx];
>> +	pll1_rate = sensor->extclk_rate * config->pll_mul / config->pll_pre_div_half;
>>  
>>  	switch (config->pll_pre_div) {
>>  	case 0:
>> @@ -1552,10 +1597,12 @@ static int ov8865_mode_pll1_configure(struct ov8865_sensor *sensor,
>>  				      const struct ov8865_mode *mode,
>>  				      u32 mbus_code)
>>  {
>> -	const struct ov8865_pll1_config *config = mode->pll1_config;
>> +	const struct ov8865_pll1_config *config;
>>  	u8 value;
>>  	int ret;
>>  
>> +	config = &mode->pll1_config[sensor->extclk_rate_idx];
>> +
>>  	switch (mbus_code) {
>>  	case MEDIA_BUS_FMT_SBGGR10_1X10:
>>  		value = OV8865_MIPI_BIT_SEL(10);
>> @@ -1622,9 +1669,11 @@ static int ov8865_mode_pll1_configure(struct ov8865_sensor *sensor,
>>  static int ov8865_mode_pll2_configure(struct ov8865_sensor *sensor,
>>  				      const struct ov8865_mode *mode)
>>  {
>> -	const struct ov8865_pll2_config *config = mode->pll2_config;
>> +	const struct ov8865_pll2_config *config;
>>  	int ret;
>>  
>> +	config = &mode->pll2_config[sensor->extclk_rate_idx];
>> +
>>  	ret = ov8865_write(sensor, OV8865_PLL_CTRL12_REG,
>>  			   OV8865_PLL_CTRL12_PRE_DIV_HALF(config->pll_pre_div_half) |
>>  			   OV8865_PLL_CTRL12_DAC_DIV(config->dac_div));
>> @@ -2053,9 +2102,11 @@ static int ov8865_mode_configure(struct ov8865_sensor *sensor,
>>  static unsigned long ov8865_mode_mipi_clk_rate(struct ov8865_sensor *sensor,
>>  					       const struct ov8865_mode *mode)
>>  {
>> -	const struct ov8865_pll1_config *config = mode->pll1_config;
>> +	const struct ov8865_pll1_config *config;
>>  	unsigned long pll1_rate;
>>  
>> +	config = &mode->pll1_config[sensor->extclk_rate_idx];
>> +
>>  	pll1_rate = ov8865_mode_pll1_rate(sensor, mode);
>>  
>>  	return pll1_rate / config->m_div / 2;
>> @@ -2783,7 +2834,8 @@ static int ov8865_probe(struct i2c_client *client)
>>  	struct ov8865_sensor *sensor;
>>  	struct v4l2_subdev *subdev;
>>  	struct media_pad *pad;
>> -	unsigned long rate;
>> +	unsigned int rate;
>> +	unsigned int i;
>>  	int ret;
>>  
>>  	sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
>> @@ -2858,13 +2910,39 @@ static int ov8865_probe(struct i2c_client *client)
>>  		goto error_endpoint;
>>  	}
>>  
>> -	rate = clk_get_rate(sensor->extclk);
>> -	if (rate != OV8865_EXTCLK_RATE) {
>> -		dev_err(dev, "clock rate %lu Hz is unsupported\n", rate);
>> +	/*
>> +	 * We could have either a 24MHz or 19.2MHz clock rate. Check for a
>> +	 * clock-frequency property and if found, set that rate. This should
>> +	 * cover ACPI case. If the system uses devicetree then the configured
>> +	 * rate should already be set, so we'll have to check it.
>> +	 */
>> +
>> +	ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
>> +				       &rate);
>> +	if (!ret) {
>> +		ret = clk_set_rate(sensor->extclk, rate);
>> +		if (ret) {
>> +			dev_err(dev, "failed to set clock rate\n");
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	sensor->extclk_rate = clk_get_rate(sensor->extclk);
>> +
>> +	for (i = 0; i < ARRAY_SIZE(supported_extclk_rates); i++) {
>> +		if (sensor->extclk_rate == supported_extclk_rates[i])
>> +			break;
>> +	}
>> +
>> +	if (i == ARRAY_SIZE(supported_extclk_rates)) {
>> +		dev_err(dev, "clock rate %lu Hz is unsupported\n",
>> +			sensor->extclk_rate);
>>  		ret = -EINVAL;
>>  		goto error_endpoint;
>>  	}
>>  
>> +	sensor->extclk_rate_idx = i;
>> +
>>  	/* Subdev, entity and pad */
>>  
>>  	subdev = &sensor->subdev;

Re: [PATCH v2 04/12] media: i2c: Support 19.2MHz input clock in ov8865

[Sakari Ailus]

On Tue, Aug 10, 2021 at 10:37:35PM +0100, Daniel Scally wrote:
> Hi Sakari - thanks for all the comments

You're welcome!

Nice patches btw.

> 
> On 10/08/2021 14:34, Sakari Ailus wrote:
> > Hi Daniel,
> >
> > Thanks for the set.
> >
> > On Mon, Aug 09, 2021 at 11:58:37PM +0100, Daniel Scally wrote:
> >> The ov8865 driver as written expects a 24MHz input clock, but the sensor
> >> is sometimes found on x86 platforms with a 19.2MHz input clock supplied.
> >> Add a set of PLL configurations to the driver to support that rate too.
> >> As ACPI doesn't auto-configure the clock rate, check for a clock-frequency
> >> during probe and set that rate if one is found.
> >>
> >> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> >> ---
> >> Changes in v2:
> >>
> >> 	- Added an enum defining the possible frequency rates to index the
> >> 	  array (Andy)
> >>
> >>  drivers/media/i2c/ov8865.c | 164 +++++++++++++++++++++++++++----------
> >>  1 file changed, 121 insertions(+), 43 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
> >> index fe700787bfb9..1382b16d1a09 100644
> >> --- a/drivers/media/i2c/ov8865.c
> >> +++ b/drivers/media/i2c/ov8865.c
> >> @@ -21,10 +21,6 @@
> >>  #include <media/v4l2-image-sizes.h>
> >>  #include <media/v4l2-mediabus.h>
> >>  
> >> -/* Clock rate */
> >> -
> >> -#define OV8865_EXTCLK_RATE			24000000
> >> -
> >>  /* Register definitions */
> >>  
> >>  /* System */
> >> @@ -567,6 +563,19 @@ struct ov8865_sclk_config {
> >>  	unsigned int sclk_div;
> >>  };
> >>  
> >> +/* Clock rate */
> >> +
> >> +enum extclk_rate {
> >> +	OV8865_19_2_MHZ,
> >> +	OV8865_24_MHZ,
> >> +	OV8865_NUM_SUPPORTED_RATES,
> >> +};
> >> +
> >> +static const unsigned long supported_extclk_rates[] = {
> >> +	[OV8865_19_2_MHZ] = 19200000,
> >> +	[OV8865_24_MHZ] = 24000000,
> >> +};
> >> +
> >>  /*
> >>   * General formulas for (array-centered) mode calculation:
> >>   * - photo_array_width = 3296
> >> @@ -665,6 +674,9 @@ struct ov8865_sensor {
> >>  	struct regulator *avdd;
> >>  	struct regulator *dvdd;
> >>  	struct regulator *dovdd;
> >> +
> >> +	unsigned long extclk_rate;
> >> +	enum extclk_rate extclk_rate_idx;
> >>  	struct clk *extclk;
> >>  
> >>  	struct v4l2_fwnode_endpoint endpoint;
> >> @@ -680,49 +692,83 @@ struct ov8865_sensor {
> >>  /* Static definitions */
> >>  
> >>  /*
> >> - * EXTCLK = 24 MHz
> >>   * PHY_SCLK = 720 MHz
> >>   * MIPI_PCLK = 90 MHz
> >>   */
> >> -static const struct ov8865_pll1_config ov8865_pll1_config_native = {
> >> -	.pll_pre_div_half	= 1,
> >> -	.pll_pre_div		= 0,
> >> -	.pll_mul		= 30,
> >> -	.m_div			= 1,
> >> -	.mipi_div		= 3,
> >> -	.pclk_div		= 1,
> >> -	.sys_pre_div		= 1,
> >> -	.sys_div		= 2,
> >> +
> >> +static const struct ov8865_pll1_config ov8865_pll1_configs_native[] = {
> >> +	{ /* 19.2 MHz input clock */
> >> +		.pll_pre_div_half	= 1,
> >> +		.pll_pre_div		= 2,
> >> +		.pll_mul		= 75,
> >> +		.m_div			= 1,
> >> +		.mipi_div		= 3,
> >> +		.pclk_div		= 1,
> >> +		.sys_pre_div		= 1,
> >> +		.sys_div		= 2,
> >> +	},
> >> +	{ /* 24MHz input clock */
> >> +		.pll_pre_div_half	= 1,
> >> +		.pll_pre_div		= 0,
> >> +		.pll_mul		= 30,
> >> +		.m_div			= 1,
> >> +		.mipi_div		= 3,
> >> +		.pclk_div		= 1,
> >> +		.sys_pre_div		= 1,
> >> +		.sys_div		= 2,
> >> +	},
> > Could you instead add a struct specific to the clock frequency with
> > pointers to these? See e.g. the ov8856 driver how this could otherwise end
> > up...
> 
> 
> You mean something like
> 
> 
> static struct ov8865_pll_configs_19_2_mhz {
> 
>     .pll1_config_native = &ov8865_pll1_config_native,
> 
>     ...
> 
> };
> 
> 
> 
> static struct ov8865_pll_configs_24_mhz {
> 
>     .pll1_config_native = &ov8865_pll1_config_native,
> 
>     ...
> 
> };
> 
> 
> or am I misunderstanding?

Yes, please --- ov8865_pll1_config_native above is thus the PLL
configuration for the 24 MHz clock.

-- 
Sakari Ailus

Re: [PATCH v2 04/12] media: i2c: Support 19.2MHz input clock in ov8865

[Daniel Scally]

Hi Sakari

On 10/08/2021 22:49, Sakari Ailus wrote:
> On Tue, Aug 10, 2021 at 10:37:35PM +0100, Daniel Scally wrote:
>> Hi Sakari - thanks for all the comments
> You're welcome!
>
> Nice patches btw.


Thanks!

>
>> On 10/08/2021 14:34, Sakari Ailus wrote:
>>> Hi Daniel,
>>>
>>> Thanks for the set.
>>>
>>> On Mon, Aug 09, 2021 at 11:58:37PM +0100, Daniel Scally wrote:
>>>> The ov8865 driver as written expects a 24MHz input clock, but the sensor
>>>> is sometimes found on x86 platforms with a 19.2MHz input clock supplied.
>>>> Add a set of PLL configurations to the driver to support that rate too.
>>>> As ACPI doesn't auto-configure the clock rate, check for a clock-frequency
>>>> during probe and set that rate if one is found.
>>>>
>>>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>>>> ---
>>>> Changes in v2:
>>>>
>>>> 	- Added an enum defining the possible frequency rates to index the
>>>> 	  array (Andy)
>>>>
>>>>  drivers/media/i2c/ov8865.c | 164 +++++++++++++++++++++++++++----------
>>>>  1 file changed, 121 insertions(+), 43 deletions(-)
>>>>
>>>> diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
>>>> index fe700787bfb9..1382b16d1a09 100644
>>>> --- a/drivers/media/i2c/ov8865.c
>>>> +++ b/drivers/media/i2c/ov8865.c
>>>> @@ -21,10 +21,6 @@
>>>>  #include <media/v4l2-image-sizes.h>
>>>>  #include <media/v4l2-mediabus.h>
>>>>  
>>>> -/* Clock rate */
>>>> -
>>>> -#define OV8865_EXTCLK_RATE			24000000
>>>> -
>>>>  /* Register definitions */
>>>>  
>>>>  /* System */
>>>> @@ -567,6 +563,19 @@ struct ov8865_sclk_config {
>>>>  	unsigned int sclk_div;
>>>>  };
>>>>  
>>>> +/* Clock rate */
>>>> +
>>>> +enum extclk_rate {
>>>> +	OV8865_19_2_MHZ,
>>>> +	OV8865_24_MHZ,
>>>> +	OV8865_NUM_SUPPORTED_RATES,
>>>> +};
>>>> +
>>>> +static const unsigned long supported_extclk_rates[] = {
>>>> +	[OV8865_19_2_MHZ] = 19200000,
>>>> +	[OV8865_24_MHZ] = 24000000,
>>>> +};
>>>> +
>>>>  /*
>>>>   * General formulas for (array-centered) mode calculation:
>>>>   * - photo_array_width = 3296
>>>> @@ -665,6 +674,9 @@ struct ov8865_sensor {
>>>>  	struct regulator *avdd;
>>>>  	struct regulator *dvdd;
>>>>  	struct regulator *dovdd;
>>>> +
>>>> +	unsigned long extclk_rate;
>>>> +	enum extclk_rate extclk_rate_idx;
>>>>  	struct clk *extclk;
>>>>  
>>>>  	struct v4l2_fwnode_endpoint endpoint;
>>>> @@ -680,49 +692,83 @@ struct ov8865_sensor {
>>>>  /* Static definitions */
>>>>  
>>>>  /*
>>>> - * EXTCLK = 24 MHz
>>>>   * PHY_SCLK = 720 MHz
>>>>   * MIPI_PCLK = 90 MHz
>>>>   */
>>>> -static const struct ov8865_pll1_config ov8865_pll1_config_native = {
>>>> -	.pll_pre_div_half	= 1,
>>>> -	.pll_pre_div		= 0,
>>>> -	.pll_mul		= 30,
>>>> -	.m_div			= 1,
>>>> -	.mipi_div		= 3,
>>>> -	.pclk_div		= 1,
>>>> -	.sys_pre_div		= 1,
>>>> -	.sys_div		= 2,
>>>> +
>>>> +static const struct ov8865_pll1_config ov8865_pll1_configs_native[] = {
>>>> +	{ /* 19.2 MHz input clock */
>>>> +		.pll_pre_div_half	= 1,
>>>> +		.pll_pre_div		= 2,
>>>> +		.pll_mul		= 75,
>>>> +		.m_div			= 1,
>>>> +		.mipi_div		= 3,
>>>> +		.pclk_div		= 1,
>>>> +		.sys_pre_div		= 1,
>>>> +		.sys_div		= 2,
>>>> +	},
>>>> +	{ /* 24MHz input clock */
>>>> +		.pll_pre_div_half	= 1,
>>>> +		.pll_pre_div		= 0,
>>>> +		.pll_mul		= 30,
>>>> +		.m_div			= 1,
>>>> +		.mipi_div		= 3,
>>>> +		.pclk_div		= 1,
>>>> +		.sys_pre_div		= 1,
>>>> +		.sys_div		= 2,
>>>> +	},
>>> Could you instead add a struct specific to the clock frequency with
>>> pointers to these? See e.g. the ov8856 driver how this could otherwise end
>>> up...I thin
>>
>> You mean something like
>>
>>
>> static struct ov8865_pll_configs_19_2_mhz {
>>
>>     .pll1_config_native = &ov8865_pll1_config_native,
>>
>>     ...
>>
>> };
>>
>>
>>
>> static struct ov8865_pll_configs_24_mhz {
>>
>>     .pll1_config_native = &ov8865_pll1_config_native,
>>
>>     ...
>>
>> };
>>
>>
>> or am I misunderstanding?
> Yes, please --- ov8865_pll1_config_native above is thus the PLL
> configuration for the 24 MHz clock.

I'm not sure about this actually. There's two versions of
ov8865_pll2_config, native and binning, so it becomes something like this:


struct ov8865_pll_configs {
    struct ov8865_pll1_config *pll1_config;
    struct ov8865_pll2_config *pll2_config_native;
    struct ov8865_pll2_config *pll2_config_binning;
};

static struct ov8865_pll_configs ov8865_pll_configs_19_2mhz = {
    .pll1_config = &ov8865_pll1_config_native_19_2mhz,
    .pll2_config_native = &ov8865_pll2_config_native_19_2mhz,
    .pll2_config_binning = &ov8865_pll2_config_binning_19_2mhz,
};

static struct ov8865_pll_configs ov8865_pll_configs_24mhz = {
    .pll1_config = &ov8865_pll1_config_native_24mhz,
    .pll2_config_native = &ov8865_pll2_config_native_24mhz,
    .pll2_config_binning = &ov8865_pll2_config_binning_24mhz,
};


Now because a mode might use either the native or binning version of the
pll2 configs, currently they're actually against the struct for a
particular mode like so:


struct ov8865_mode ov8865_modes[] = {

    {

        <snip>

        .pll1_config            = &ov8865_pll1_config_native,
        .pll2_config            = &ov8865_pll2_config_binning,
        .sclk_config            = &ov8865_sclk_config_native,

    }

};


The problem I'm having is that I can't really see a clean way to store
against the _mode_ whether it should access .pll2_config_native or
.pll2_config_binning, from the new struct ov8865_pll_configs. Do you
have any ideas about a way to do that?

Re: [PATCH v2 04/12] media: i2c: Support 19.2MHz input clock in ov8865

[Sakari Ailus]

Hi Daniel,

On Tue, Sep 07, 2021 at 11:44:12PM +0100, Daniel Scally wrote:
> Hi Sakari
> 
> On 10/08/2021 22:49, Sakari Ailus wrote:
> > On Tue, Aug 10, 2021 at 10:37:35PM +0100, Daniel Scally wrote:
> >> Hi Sakari - thanks for all the comments
> > You're welcome!
> >
> > Nice patches btw.
> 
> 
> Thanks!
> 
> >
> >> On 10/08/2021 14:34, Sakari Ailus wrote:
> >>> Hi Daniel,
> >>>
> >>> Thanks for the set.
> >>>
> >>> On Mon, Aug 09, 2021 at 11:58:37PM +0100, Daniel Scally wrote:
> >>>> The ov8865 driver as written expects a 24MHz input clock, but the sensor
> >>>> is sometimes found on x86 platforms with a 19.2MHz input clock supplied.
> >>>> Add a set of PLL configurations to the driver to support that rate too.
> >>>> As ACPI doesn't auto-configure the clock rate, check for a clock-frequency
> >>>> during probe and set that rate if one is found.
> >>>>
> >>>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> >>>> ---
> >>>> Changes in v2:
> >>>>
> >>>> 	- Added an enum defining the possible frequency rates to index the
> >>>> 	  array (Andy)
> >>>>
> >>>>  drivers/media/i2c/ov8865.c | 164 +++++++++++++++++++++++++++----------
> >>>>  1 file changed, 121 insertions(+), 43 deletions(-)
> >>>>
> >>>> diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
> >>>> index fe700787bfb9..1382b16d1a09 100644
> >>>> --- a/drivers/media/i2c/ov8865.c
> >>>> +++ b/drivers/media/i2c/ov8865.c
> >>>> @@ -21,10 +21,6 @@
> >>>>  #include <media/v4l2-image-sizes.h>
> >>>>  #include <media/v4l2-mediabus.h>
> >>>>  
> >>>> -/* Clock rate */
> >>>> -
> >>>> -#define OV8865_EXTCLK_RATE			24000000
> >>>> -
> >>>>  /* Register definitions */
> >>>>  
> >>>>  /* System */
> >>>> @@ -567,6 +563,19 @@ struct ov8865_sclk_config {
> >>>>  	unsigned int sclk_div;
> >>>>  };
> >>>>  
> >>>> +/* Clock rate */
> >>>> +
> >>>> +enum extclk_rate {
> >>>> +	OV8865_19_2_MHZ,
> >>>> +	OV8865_24_MHZ,
> >>>> +	OV8865_NUM_SUPPORTED_RATES,
> >>>> +};
> >>>> +
> >>>> +static const unsigned long supported_extclk_rates[] = {
> >>>> +	[OV8865_19_2_MHZ] = 19200000,
> >>>> +	[OV8865_24_MHZ] = 24000000,
> >>>> +};
> >>>> +
> >>>>  /*
> >>>>   * General formulas for (array-centered) mode calculation:
> >>>>   * - photo_array_width = 3296
> >>>> @@ -665,6 +674,9 @@ struct ov8865_sensor {
> >>>>  	struct regulator *avdd;
> >>>>  	struct regulator *dvdd;
> >>>>  	struct regulator *dovdd;
> >>>> +
> >>>> +	unsigned long extclk_rate;
> >>>> +	enum extclk_rate extclk_rate_idx;
> >>>>  	struct clk *extclk;
> >>>>  
> >>>>  	struct v4l2_fwnode_endpoint endpoint;
> >>>> @@ -680,49 +692,83 @@ struct ov8865_sensor {
> >>>>  /* Static definitions */
> >>>>  
> >>>>  /*
> >>>> - * EXTCLK = 24 MHz
> >>>>   * PHY_SCLK = 720 MHz
> >>>>   * MIPI_PCLK = 90 MHz
> >>>>   */
> >>>> -static const struct ov8865_pll1_config ov8865_pll1_config_native = {
> >>>> -	.pll_pre_div_half	= 1,
> >>>> -	.pll_pre_div		= 0,
> >>>> -	.pll_mul		= 30,
> >>>> -	.m_div			= 1,
> >>>> -	.mipi_div		= 3,
> >>>> -	.pclk_div		= 1,
> >>>> -	.sys_pre_div		= 1,
> >>>> -	.sys_div		= 2,
> >>>> +
> >>>> +static const struct ov8865_pll1_config ov8865_pll1_configs_native[] = {
> >>>> +	{ /* 19.2 MHz input clock */
> >>>> +		.pll_pre_div_half	= 1,
> >>>> +		.pll_pre_div		= 2,
> >>>> +		.pll_mul		= 75,
> >>>> +		.m_div			= 1,
> >>>> +		.mipi_div		= 3,
> >>>> +		.pclk_div		= 1,
> >>>> +		.sys_pre_div		= 1,
> >>>> +		.sys_div		= 2,
> >>>> +	},
> >>>> +	{ /* 24MHz input clock */
> >>>> +		.pll_pre_div_half	= 1,
> >>>> +		.pll_pre_div		= 0,
> >>>> +		.pll_mul		= 30,
> >>>> +		.m_div			= 1,
> >>>> +		.mipi_div		= 3,
> >>>> +		.pclk_div		= 1,
> >>>> +		.sys_pre_div		= 1,
> >>>> +		.sys_div		= 2,
> >>>> +	},
> >>> Could you instead add a struct specific to the clock frequency with
> >>> pointers to these? See e.g. the ov8856 driver how this could otherwise end
> >>> up...I thin
> >>
> >> You mean something like
> >>
> >>
> >> static struct ov8865_pll_configs_19_2_mhz {
> >>
> >>     .pll1_config_native = &ov8865_pll1_config_native,
> >>
> >>     ...
> >>
> >> };
> >>
> >>
> >>
> >> static struct ov8865_pll_configs_24_mhz {
> >>
> >>     .pll1_config_native = &ov8865_pll1_config_native,
> >>
> >>     ...
> >>
> >> };
> >>
> >>
> >> or am I misunderstanding?
> > Yes, please --- ov8865_pll1_config_native above is thus the PLL
> > configuration for the 24 MHz clock.
> 
> I'm not sure about this actually. There's two versions of
> ov8865_pll2_config, native and binning, so it becomes something like this:
> 
> 
> struct ov8865_pll_configs {
>     struct ov8865_pll1_config *pll1_config;
>     struct ov8865_pll2_config *pll2_config_native;
>     struct ov8865_pll2_config *pll2_config_binning;
> };
> 
> static struct ov8865_pll_configs ov8865_pll_configs_19_2mhz = {
>     .pll1_config = &ov8865_pll1_config_native_19_2mhz,
>     .pll2_config_native = &ov8865_pll2_config_native_19_2mhz,
>     .pll2_config_binning = &ov8865_pll2_config_binning_19_2mhz,
> };
> 
> static struct ov8865_pll_configs ov8865_pll_configs_24mhz = {
>     .pll1_config = &ov8865_pll1_config_native_24mhz,
>     .pll2_config_native = &ov8865_pll2_config_native_24mhz,
>     .pll2_config_binning = &ov8865_pll2_config_binning_24mhz,
> };
> 
> 
> Now because a mode might use either the native or binning version of the
> pll2 configs, currently they're actually against the struct for a
> particular mode like so:
> 
> 
> struct ov8865_mode ov8865_modes[] = {
> 
>     {
> 
>         <snip>
> 
>         .pll1_config            = &ov8865_pll1_config_native,
>         .pll2_config            = &ov8865_pll2_config_binning,
>         .sclk_config            = &ov8865_sclk_config_native,
> 
>     }
> 
> };
> 
> 
> The problem I'm having is that I can't really see a clean way to store
> against the _mode_ whether it should access .pll2_config_native or
> .pll2_config_binning, from the new struct ov8865_pll_configs. Do you
> have any ideas about a way to do that?

Ah, yes. I agree, that's where you'll need some code to pick the right one.
So you could use a function pointer in the struct and give it the necessary
arguments. I don't think it'd be overkill, things tend to develop over
time. See e.g. the ov8856 driver as a (warning) example.

-- 
Kind regards,

Sakari Ailus

[PATCH v2 05/12] media: i2c: Add .get_selection() support to ov8865

[Daniel Scally]

The ov8865 driver's v4l2_subdev_pad_ops currently does not include
.get_selection() - add support for that callback.

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

	- None

 drivers/media/i2c/ov8865.c | 61 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
index 1382b16d1a09..8c2b7d3cbc8c 100644
--- a/drivers/media/i2c/ov8865.c
+++ b/drivers/media/i2c/ov8865.c
@@ -450,6 +450,15 @@
 #define OV8865_PRE_CTRL0_PATTERN_COLOR_SQUARES	2
 #define OV8865_PRE_CTRL0_PATTERN_BLACK		3
 
+/* Pixel Array */
+
+#define OV8865_NATIVE_WIDTH			3296
+#define OV8865_NATIVE_HEIGHT			2528
+#define OV8865_ACTIVE_START_TOP			32
+#define OV8865_ACTIVE_START_LEFT		80
+#define OV8865_ACTIVE_WIDTH			3264
+#define OV8865_ACTIVE_HEIGHT			2448
+
 /* Macros */
 
 #define ov8865_subdev_sensor(s) \
@@ -2749,12 +2758,64 @@ static int ov8865_enum_frame_interval(struct v4l2_subdev *subdev,
 	return 0;
 }
 
+static void
+__ov8865_get_pad_crop(struct ov8865_sensor *sensor,
+		      struct v4l2_subdev_state *state, unsigned int pad,
+		      enum v4l2_subdev_format_whence which, struct v4l2_rect *r)
+{
+	switch (which) {
+	case V4L2_SUBDEV_FORMAT_TRY:
+		*r = *v4l2_subdev_get_try_crop(&sensor->subdev, state, pad);
+		break;
+	case V4L2_SUBDEV_FORMAT_ACTIVE:
+		r->height = sensor->state.mode->output_size_y;
+		r->width = sensor->state.mode->output_size_x;
+		r->top = (OV8865_NATIVE_HEIGHT - sensor->state.mode->output_size_y) / 2;
+		r->left = (OV8865_NATIVE_WIDTH - sensor->state.mode->output_size_x) / 2;
+		break;
+	}
+}
+
+static int ov8865_get_selection(struct v4l2_subdev *subdev,
+				struct v4l2_subdev_state *state,
+				struct v4l2_subdev_selection *sel)
+{
+	struct ov8865_sensor *sensor = ov8865_subdev_sensor(subdev);
+
+	switch (sel->target) {
+	case V4L2_SEL_TGT_CROP:
+		mutex_lock(&sensor->mutex);
+			__ov8865_get_pad_crop(sensor, state, sel->pad,
+					      sel->which, &sel->r);
+		mutex_unlock(&sensor->mutex);
+		break;
+	case V4L2_SEL_TGT_NATIVE_SIZE:
+		sel->r.top = 0;
+		sel->r.left = 0;
+		sel->r.width = OV8865_NATIVE_WIDTH;
+		sel->r.height = OV8865_NATIVE_HEIGHT;
+		break;
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+	case V4L2_SEL_TGT_CROP_DEFAULT:
+		sel->r.top = OV8865_ACTIVE_START_TOP;
+		sel->r.left = OV8865_ACTIVE_START_LEFT;
+		sel->r.width = OV8865_ACTIVE_WIDTH;
+		sel->r.height = OV8865_ACTIVE_HEIGHT;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static const struct v4l2_subdev_pad_ops ov8865_subdev_pad_ops = {
 	.enum_mbus_code		= ov8865_enum_mbus_code,
 	.get_fmt		= ov8865_get_fmt,
 	.set_fmt		= ov8865_set_fmt,
 	.enum_frame_size	= ov8865_enum_frame_size,
 	.enum_frame_interval	= ov8865_enum_frame_interval,
+	.get_selection		= ov8865_get_selection,
 };
 
 static const struct v4l2_subdev_ops ov8865_subdev_ops = {
-- 
2.25.1

Re: [PATCH v2 05/12] media: i2c: Add .get_selection() support to ov8865

[Sakari Ailus]

Hi Daniel,

On Mon, Aug 09, 2021 at 11:58:38PM +0100, Daniel Scally wrote:
> The ov8865 driver's v4l2_subdev_pad_ops currently does not include
> .get_selection() - add support for that callback.

Could you use the same for .set_selection()? Even if it doesn't change
anything.

-- 
Sakari Ailus

Re: [PATCH v2 05/12] media: i2c: Add .get_selection() support to ov8865

[Daniel Scally]

Hi Sakari - sorry delayed reply

On 10/08/2021 14:38, Sakari Ailus wrote:
> Hi Daniel,
>
> On Mon, Aug 09, 2021 at 11:58:38PM +0100, Daniel Scally wrote:
>> The ov8865 driver's v4l2_subdev_pad_ops currently does not include
>> .get_selection() - add support for that callback.
> Could you use the same for .set_selection()? Even if it doesn't change
> anything.


You mean do the same? Or use the same function?

Re: [PATCH v2 05/12] media: i2c: Add .get_selection() support to ov8865

[Sakari Ailus]

On Wed, Aug 25, 2021 at 12:17:15AM +0100, Daniel Scally wrote:
> Hi Sakari - sorry delayed reply
> 
> On 10/08/2021 14:38, Sakari Ailus wrote:
> > Hi Daniel,
> >
> > On Mon, Aug 09, 2021 at 11:58:38PM +0100, Daniel Scally wrote:
> >> The ov8865 driver's v4l2_subdev_pad_ops currently does not include
> >> .get_selection() - add support for that callback.
> > Could you use the same for .set_selection()? Even if it doesn't change
> > anything.
> 
> 
> You mean do the same? Or use the same function?

The same function. If the selection isn't changeable anyway, the
functionality is the same for both.

-- 
Sakari Ailus

Re: [PATCH v2 05/12] media: i2c: Add .get_selection() support to ov8865

[Laurent Pinchart]

On Wed, Aug 25, 2021 at 10:16:02AM +0300, Sakari Ailus wrote:
> On Wed, Aug 25, 2021 at 12:17:15AM +0100, Daniel Scally wrote:
> > Hi Sakari - sorry delayed reply
> > 
> > On 10/08/2021 14:38, Sakari Ailus wrote:
> > > Hi Daniel,
> > >
> > > On Mon, Aug 09, 2021 at 11:58:38PM +0100, Daniel Scally wrote:
> > >> The ov8865 driver's v4l2_subdev_pad_ops currently does not include
> > >> .get_selection() - add support for that callback.
> > > Could you use the same for .set_selection()? Even if it doesn't change
> > > anything.
> > 
> > You mean do the same? Or use the same function?
> 
> The same function. If the selection isn't changeable anyway, the
> functionality is the same for both.

Except that .s_selection() should return an error if you try to set the
bounds or default rectangles.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 05/12] media: i2c: Add .get_selection() support to ov8865

[Sakari Ailus]

On Wed, Aug 25, 2021 at 11:04:02AM +0300, Laurent Pinchart wrote:
> On Wed, Aug 25, 2021 at 10:16:02AM +0300, Sakari Ailus wrote:
> > On Wed, Aug 25, 2021 at 12:17:15AM +0100, Daniel Scally wrote:
> > > Hi Sakari - sorry delayed reply
> > > 
> > > On 10/08/2021 14:38, Sakari Ailus wrote:
> > > > Hi Daniel,
> > > >
> > > > On Mon, Aug 09, 2021 at 11:58:38PM +0100, Daniel Scally wrote:
> > > >> The ov8865 driver's v4l2_subdev_pad_ops currently does not include
> > > >> .get_selection() - add support for that callback.
> > > > Could you use the same for .set_selection()? Even if it doesn't change
> > > > anything.
> > > 
> > > You mean do the same? Or use the same function?
> > 
> > The same function. If the selection isn't changeable anyway, the
> > functionality is the same for both.
> 
> Except that .s_selection() should return an error if you try to set the
> bounds or default rectangles.

That would make sense. But it's not documented. And in any case it should
be implemented in the framework. :-)

-- 
Sakari Ailus

[PATCH v2 06/12] media: i2c: Switch control to V4L2_CID_ANALOGUE_GAIN

[Daniel Scally]

The V4L2_CID_GAIN control for this driver configures registers that
the datasheet specifies as analogue gain. Switch the control's ID
to V4L2_CID_ANALOGUE_GAIN.

Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v2:

	- None

 drivers/media/i2c/ov8865.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
index 8c2b7d3cbc8c..a97e355c1e07 100644
--- a/drivers/media/i2c/ov8865.c
+++ b/drivers/media/i2c/ov8865.c
@@ -2143,7 +2143,7 @@ static int ov8865_exposure_configure(struct ov8865_sensor *sensor, u32 exposure)
 
 /* Gain */
 
-static int ov8865_gain_configure(struct ov8865_sensor *sensor, u32 gain)
+static int ov8865_analog_gain_configure(struct ov8865_sensor *sensor, u32 gain)
 {
 	int ret;
 
@@ -2453,8 +2453,8 @@ static int ov8865_s_ctrl(struct v4l2_ctrl *ctrl)
 		if (ret)
 			return ret;
 		break;
-	case V4L2_CID_GAIN:
-		ret = ov8865_gain_configure(sensor, ctrl->val);
+	case V4L2_CID_ANALOGUE_GAIN:
+		ret = ov8865_analog_gain_configure(sensor, ctrl->val);
 		if (ret)
 			return ret;
 		break;
@@ -2499,7 +2499,7 @@ static int ov8865_ctrls_init(struct ov8865_sensor *sensor)
 
 	/* Gain */
 
-	v4l2_ctrl_new_std(handler, ops, V4L2_CID_GAIN, 128, 8191, 128, 128);
+	v4l2_ctrl_new_std(handler, ops, V4L2_CID_ANALOGUE_GAIN, 128, 8191, 128, 128);
 
 	/* White Balance */
 
-- 
2.25.1

Re: [PATCH v2 06/12] media: i2c: Switch control to V4L2_CID_ANALOGUE_GAIN

[Sakari Ailus]

On Mon, Aug 09, 2021 at 11:58:39PM +0100, Daniel Scally wrote:
> @@ -2143,7 +2143,7 @@ static int ov8865_exposure_configure(struct ov8865_sensor *sensor, u32 exposure)
>  
>  /* Gain */
>  
> -static int ov8865_gain_configure(struct ov8865_sensor *sensor, u32 gain)
> +static int ov8865_analog_gain_configure(struct ov8865_sensor *sensor, u32 gain)
>  {
>  	int ret;
>  
> @@ -2453,8 +2453,8 @@ static int ov8865_s_ctrl(struct v4l2_ctrl *ctrl)
>  		if (ret)
>  			return ret;
>  		break;
> -	case V4L2_CID_GAIN:
> -		ret = ov8865_gain_configure(sensor, ctrl->val);
> +	case V4L2_CID_ANALOGUE_GAIN:
> +		ret = ov8865_analog_gain_configure(sensor, ctrl->val);
>  		if (ret)
>  			return ret;
>  		break;
> @@ -2499,7 +2499,7 @@ static int ov8865_ctrls_init(struct ov8865_sensor *sensor)
>  
>  	/* Gain */
>  
> -	v4l2_ctrl_new_std(handler, ops, V4L2_CID_GAIN, 128, 8191, 128, 128);
> +	v4l2_ctrl_new_std(handler, ops, V4L2_CID_ANALOGUE_GAIN, 128, 8191, 128, 128);

Over 80, please wrap.

-- 
Sakari Ailus

[PATCH v2 07/12] media: i2c: Add vblank control to ov8865

[Daniel Scally]

Add a V4L2_CID_VBLANK control to the ov8865 driver.

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

	- None

 drivers/media/i2c/ov8865.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
index a97e355c1e07..810047c247b4 100644
--- a/drivers/media/i2c/ov8865.c
+++ b/drivers/media/i2c/ov8865.c
@@ -183,6 +183,8 @@
 #define OV8865_VTS_H(v)				(((v) & GENMASK(11, 8)) >> 8)
 #define OV8865_VTS_L_REG			0x380f
 #define OV8865_VTS_L(v)				((v) & GENMASK(7, 0))
+#define OV8865_TIMING_MAX_VTS			0xffff
+#define OV8865_TIMING_MIN_VTS			0x04
 #define OV8865_OFFSET_X_H_REG			0x3810
 #define OV8865_OFFSET_X_H(v)			(((v) & GENMASK(15, 8)) >> 8)
 #define OV8865_OFFSET_X_L_REG			0x3811
@@ -671,6 +673,7 @@ struct ov8865_state {
 struct ov8865_ctrls {
 	struct v4l2_ctrl *link_freq;
 	struct v4l2_ctrl *pixel_rate;
+	struct v4l2_ctrl *vblank;
 
 	struct v4l2_ctrl_handler handler;
 };
@@ -2218,6 +2221,20 @@ static int ov8865_test_pattern_configure(struct ov8865_sensor *sensor,
 			    ov8865_test_pattern_bits[index]);
 }
 
+/* Blanking */
+
+static int ov8865_vts_configure(struct ov8865_sensor *sensor, u32 vblank)
+{
+	u16 vts = sensor->state.mode->output_size_y + vblank;
+	int ret;
+
+	ret = ov8865_write(sensor, OV8865_VTS_H_REG, OV8865_VTS_H(vts));
+	if (ret)
+		return ret;
+
+	return ov8865_write(sensor, OV8865_VTS_L_REG, OV8865_VTS_L(vts));
+}
+
 /* State */
 
 static int ov8865_state_mipi_configure(struct ov8865_sensor *sensor,
@@ -2469,6 +2486,8 @@ static int ov8865_s_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_TEST_PATTERN:
 		index = (unsigned int)ctrl->val;
 		return ov8865_test_pattern_configure(sensor, index);
+	case V4L2_CID_VBLANK:
+		return ov8865_vts_configure(sensor, ctrl->val);
 	default:
 		return -EINVAL;
 	}
@@ -2485,6 +2504,8 @@ static int ov8865_ctrls_init(struct ov8865_sensor *sensor)
 	struct ov8865_ctrls *ctrls = &sensor->ctrls;
 	struct v4l2_ctrl_handler *handler = &ctrls->handler;
 	const struct v4l2_ctrl_ops *ops = &ov8865_ctrl_ops;
+	const struct ov8865_mode *mode = sensor->state.mode;
+	unsigned int vblank_max, vblank_def;
 	int ret;
 
 	v4l2_ctrl_handler_init(handler, 32);
@@ -2520,6 +2541,13 @@ static int ov8865_ctrls_init(struct ov8865_sensor *sensor)
 				     ARRAY_SIZE(ov8865_test_pattern_menu) - 1,
 				     0, 0, ov8865_test_pattern_menu);
 
+	/* Blanking */
+	vblank_max = OV8865_TIMING_MAX_VTS - mode->output_size_y;
+	vblank_def = mode->vts - mode->output_size_y;
+	ctrls->vblank = v4l2_ctrl_new_std(handler, ops, V4L2_CID_VBLANK,
+					  OV8865_TIMING_MIN_VTS, vblank_max, 1,
+					  vblank_def);
+
 	/* MIPI CSI-2 */
 
 	ctrls->link_freq =
@@ -2700,6 +2728,10 @@ static int ov8865_set_fmt(struct v4l2_subdev *subdev,
 		 sensor->state.mbus_code != mbus_code)
 		ret = ov8865_state_configure(sensor, mode, mbus_code);
 
+	__v4l2_ctrl_modify_range(sensor->ctrls.vblank, OV8865_TIMING_MIN_VTS,
+				 OV8865_TIMING_MAX_VTS - mode->output_size_y,
+				 1, mode->vts - mode->output_size_y);
+
 complete:
 	mutex_unlock(&sensor->mutex);
 
@@ -3025,6 +3057,8 @@ static int ov8865_probe(struct i2c_client *client)
 
 	/* Sensor */
 
+	sensor->state.mode =  &ov8865_modes[0];
+
 	ret = ov8865_ctrls_init(sensor);
 	if (ret)
 		goto error_mutex;
-- 
2.25.1

[PATCH v2 08/12] media: i2c: Add hblank control to ov8865

[Daniel Scally]

Add a V4L2_CID_HBLANK control to the ov8865 driver. This is read only
with timing control intended to be done via vblanking alone.

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

	- None

 drivers/media/i2c/ov8865.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
index 810047c247b4..db84294b7a03 100644
--- a/drivers/media/i2c/ov8865.c
+++ b/drivers/media/i2c/ov8865.c
@@ -673,6 +673,7 @@ struct ov8865_state {
 struct ov8865_ctrls {
 	struct v4l2_ctrl *link_freq;
 	struct v4l2_ctrl *pixel_rate;
+	struct v4l2_ctrl *hblank;
 	struct v4l2_ctrl *vblank;
 
 	struct v4l2_ctrl_handler handler;
@@ -2506,6 +2507,7 @@ static int ov8865_ctrls_init(struct ov8865_sensor *sensor)
 	const struct v4l2_ctrl_ops *ops = &ov8865_ctrl_ops;
 	const struct ov8865_mode *mode = sensor->state.mode;
 	unsigned int vblank_max, vblank_def;
+	unsigned int hblank;
 	int ret;
 
 	v4l2_ctrl_handler_init(handler, 32);
@@ -2542,6 +2544,13 @@ static int ov8865_ctrls_init(struct ov8865_sensor *sensor)
 				     0, 0, ov8865_test_pattern_menu);
 
 	/* Blanking */
+	hblank = mode->hts < mode->output_size_x ? 0 : mode->hts - mode->output_size_x;
+	ctrls->hblank = v4l2_ctrl_new_std(handler, ops, V4L2_CID_HBLANK, hblank,
+					  hblank, 1, hblank);
+
+	if (ctrls->hblank)
+		ctrls->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
 	vblank_max = OV8865_TIMING_MAX_VTS - mode->output_size_y;
 	vblank_def = mode->vts - mode->output_size_y;
 	ctrls->vblank = v4l2_ctrl_new_std(handler, ops, V4L2_CID_VBLANK,
@@ -2688,6 +2697,7 @@ static int ov8865_set_fmt(struct v4l2_subdev *subdev,
 	struct v4l2_mbus_framefmt *mbus_format = &format->format;
 	const struct ov8865_mode *mode;
 	u32 mbus_code = 0;
+	unsigned int hblank;
 	unsigned int index;
 	int ret = 0;
 
@@ -2732,6 +2742,10 @@ static int ov8865_set_fmt(struct v4l2_subdev *subdev,
 				 OV8865_TIMING_MAX_VTS - mode->output_size_y,
 				 1, mode->vts - mode->output_size_y);
 
+	hblank = mode->hts < mode->output_size_x ? 0 : mode->hts - mode->output_size_x;
+	__v4l2_ctrl_modify_range(sensor->ctrls.hblank, hblank, hblank, 1,
+				 hblank);
+
 complete:
 	mutex_unlock(&sensor->mutex);
 
-- 
2.25.1

Re: [PATCH v2 08/12] media: i2c: Add hblank control to ov8865

[Andy Shevchenko]

On Tue, Aug 10, 2021 at 1:59 AM Daniel Scally <djrscally@gmail.com> wrote:
>
> Add a V4L2_CID_HBLANK control to the ov8865 driver. This is read only
> with timing control intended to be done via vblanking alone.

...

> +       hblank = mode->hts < mode->output_size_x ? 0 : mode->hts - mode->output_size_x;

In both cases can be

       hblank = max(mode->output_size_x, mode->hts) - mode->output_size_x;

or max_t() in case types are different (but in this case the code
might be uglier).

> +       ctrls->hblank = v4l2_ctrl_new_std(handler, ops, V4L2_CID_HBLANK, hblank,
> +                                         hblank, 1, hblank);

> +

Redundant blank line (pun is not intended :).

> +       if (ctrls->hblank)
> +               ctrls->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;

...

> +       hblank = mode->hts < mode->output_size_x ? 0 : mode->hts - mode->output_size_x;

See above.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 08/12] media: i2c: Add hblank control to ov8865

[Sakari Ailus]

Hi Daniel,

On Mon, Aug 09, 2021 at 11:58:41PM +0100, Daniel Scally wrote:
> @@ -2542,6 +2544,13 @@ static int ov8865_ctrls_init(struct ov8865_sensor *sensor)
>  				     0, 0, ov8865_test_pattern_menu);
>  
>  	/* Blanking */
> +	hblank = mode->hts < mode->output_size_x ? 0 : mode->hts - mode->output_size_x;

Is the result in relation with the analogue crop size? Based on the above
it wouldn't seem like that.

> +	ctrls->hblank = v4l2_ctrl_new_std(handler, ops, V4L2_CID_HBLANK, hblank,
> +					  hblank, 1, hblank);
> +
> +	if (ctrls->hblank)
> +		ctrls->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
>  	vblank_max = OV8865_TIMING_MAX_VTS - mode->output_size_y;
>  	vblank_def = mode->vts - mode->output_size_y;
>  	ctrls->vblank = v4l2_ctrl_new_std(handler, ops, V4L2_CID_VBLANK,

-- 
Sakari Ailus

Re: [PATCH v2 08/12] media: i2c: Add hblank control to ov8865

[Daniel Scally]

Hi Sakari

On 10/08/2021 15:29, Sakari Ailus wrote:
> Hi Daniel,
>
> On Mon, Aug 09, 2021 at 11:58:41PM +0100, Daniel Scally wrote:
>> @@ -2542,6 +2544,13 @@ static int ov8865_ctrls_init(struct ov8865_sensor *sensor)
>>  				     0, 0, ov8865_test_pattern_menu);
>>  
>>  	/* Blanking */
>> +	hblank = mode->hts < mode->output_size_x ? 0 : mode->hts - mode->output_size_x;
> Is the result in relation with the analogue crop size? Based on the above
> it wouldn't seem like that.


This was a weird one actually. My understanding was that HTS should
always be >= the horizontal crop plus hblank...but that doesn't hold
true for some of this driver's modes and nor does it hold true when
running the camera in Windows (I checked the registers whilst running
it). So I went with setting hblank to 0 if the mode's HTS exceeded the
horizontal crop as the only way I could see to reconcile that.

>
>> +	ctrls->hblank = v4l2_ctrl_new_std(handler, ops, V4L2_CID_HBLANK, hblank,
>> +					  hblank, 1, hblank);
>> +
>> +	if (ctrls->hblank)
>> +		ctrls->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>> +
>>  	vblank_max = OV8865_TIMING_MAX_VTS - mode->output_size_y;
>>  	vblank_def = mode->vts - mode->output_size_y;
>>  	ctrls->vblank = v4l2_ctrl_new_std(handler, ops, V4L2_CID_VBLANK,

Re: [PATCH v2 08/12] media: i2c: Add hblank control to ov8865

[Laurent Pinchart]

Hi Daniel,

On Tue, Aug 10, 2021 at 11:07:22PM +0100, Daniel Scally wrote:
> On 10/08/2021 15:29, Sakari Ailus wrote:
> > On Mon, Aug 09, 2021 at 11:58:41PM +0100, Daniel Scally wrote:
> >> @@ -2542,6 +2544,13 @@ static int ov8865_ctrls_init(struct ov8865_sensor *sensor)
> >>  				     0, 0, ov8865_test_pattern_menu);
> >>  
> >>  	/* Blanking */
> >> +	hblank = mode->hts < mode->output_size_x ? 0 : mode->hts - mode->output_size_x;
> >
> > Is the result in relation with the analogue crop size? Based on the above
> > it wouldn't seem like that.
> 
> This was a weird one actually. My understanding was that HTS should
> always be >= the horizontal crop plus hblank...but that doesn't hold
> true for some of this driver's modes and nor does it hold true when
> running the camera in Windows (I checked the registers whilst running
> it). So I went with setting hblank to 0 if the mode's HTS exceeded the
> horizontal crop as the only way I could see to reconcile that.

There's something very fishy here, HTS is, by definition, equal to the
analog crop width plus the horizontal blanking. I suspect that the
values in ov8865_modes are wrong.

> >> +	ctrls->hblank = v4l2_ctrl_new_std(handler, ops, V4L2_CID_HBLANK, hblank,
> >> +					  hblank, 1, hblank);
> >> +
> >> +	if (ctrls->hblank)
> >> +		ctrls->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> >> +
> >>  	vblank_max = OV8865_TIMING_MAX_VTS - mode->output_size_y;
> >>  	vblank_def = mode->vts - mode->output_size_y;
> >>  	ctrls->vblank = v4l2_ctrl_new_std(handler, ops, V4L2_CID_VBLANK,

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 08/12] media: i2c: Add hblank control to ov8865

[Daniel Scally]

Hi Laurent

On 13/08/2021 04:05, Laurent Pinchart wrote:
> Hi Daniel,
>
> On Tue, Aug 10, 2021 at 11:07:22PM +0100, Daniel Scally wrote:
>> On 10/08/2021 15:29, Sakari Ailus wrote:
>>> On Mon, Aug 09, 2021 at 11:58:41PM +0100, Daniel Scally wrote:
>>>> @@ -2542,6 +2544,13 @@ static int ov8865_ctrls_init(struct ov8865_sensor *sensor)
>>>>  				     0, 0, ov8865_test_pattern_menu);
>>>>  
>>>>  	/* Blanking */
>>>> +	hblank = mode->hts < mode->output_size_x ? 0 : mode->hts - mode->output_size_x;
>>> Is the result in relation with the analogue crop size? Based on the above
>>> it wouldn't seem like that.
>> This was a weird one actually. My understanding was that HTS should
>> always be >= the horizontal crop plus hblank...but that doesn't hold
>> true for some of this driver's modes and nor does it hold true when
>> running the camera in Windows (I checked the registers whilst running
>> it). So I went with setting hblank to 0 if the mode's HTS exceeded the
>> horizontal crop as the only way I could see to reconcile that.
> There's something very fishy here, HTS is, by definition, equal to the
> analog crop width plus the horizontal blanking. I suspect that the
> values in ov8865_modes are wrong.


I thought that initially too but confirming that the same thing happened
running windows switched me into "you're probably wrong" mode. If we're
confident that the HTS is likely wrong though I can add an extra patch
to bring those into lining with that definition.

>
>>>> +	ctrls->hblank = v4l2_ctrl_new_std(handler, ops, V4L2_CID_HBLANK, hblank,
>>>> +					  hblank, 1, hblank);
>>>> +
>>>> +	if (ctrls->hblank)
>>>> +		ctrls->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>>>> +
>>>>  	vblank_max = OV8865_TIMING_MAX_VTS - mode->output_size_y;
>>>>  	vblank_def = mode->vts - mode->output_size_y;
>>>>  	ctrls->vblank = v4l2_ctrl_new_std(handler, ops, V4L2_CID_VBLANK,

Re: [PATCH v2 08/12] media: i2c: Add hblank control to ov8865

[Laurent Pinchart]

Hi Daniel,

On Fri, Aug 13, 2021 at 10:45:48AM +0100, Daniel Scally wrote:
> On 13/08/2021 04:05, Laurent Pinchart wrote:
> > On Tue, Aug 10, 2021 at 11:07:22PM +0100, Daniel Scally wrote:
> >> On 10/08/2021 15:29, Sakari Ailus wrote:
> >>> On Mon, Aug 09, 2021 at 11:58:41PM +0100, Daniel Scally wrote:
> >>>> @@ -2542,6 +2544,13 @@ static int ov8865_ctrls_init(struct ov8865_sensor *sensor)
> >>>>  				     0, 0, ov8865_test_pattern_menu);
> >>>>  
> >>>>  	/* Blanking */
> >>>> +	hblank = mode->hts < mode->output_size_x ? 0 : mode->hts - mode->output_size_x;
> >>>
> >>> Is the result in relation with the analogue crop size? Based on the above
> >>> it wouldn't seem like that.
> >>
> >> This was a weird one actually. My understanding was that HTS should
> >> always be >= the horizontal crop plus hblank...but that doesn't hold
> >> true for some of this driver's modes and nor does it hold true when
> >> running the camera in Windows (I checked the registers whilst running
> >> it). So I went with setting hblank to 0 if the mode's HTS exceeded the
> >> horizontal crop as the only way I could see to reconcile that.
> >
> > There's something very fishy here, HTS is, by definition, equal to the
> > analog crop width plus the horizontal blanking. I suspect that the
> > values in ov8865_modes are wrong.
> 
> I thought that initially too but confirming that the same thing happened
> running windows switched me into "you're probably wrong" mode. If we're
> confident that the HTS is likely wrong though I can add an extra patch
> to bring those into lining with that definition.

I think it's worth investigating this. The hblank computed here is
clearly incorrect, and would thus be useless for all practical purposes.
As usual with OmniVision, the datasheet is also quite useless.

Paul, do you have any information about this ?

> >>>> +	ctrls->hblank = v4l2_ctrl_new_std(handler, ops, V4L2_CID_HBLANK, hblank,
> >>>> +					  hblank, 1, hblank);
> >>>> +
> >>>> +	if (ctrls->hblank)
> >>>> +		ctrls->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> >>>> +
> >>>>  	vblank_max = OV8865_TIMING_MAX_VTS - mode->output_size_y;
> >>>>  	vblank_def = mode->vts - mode->output_size_y;
> >>>>  	ctrls->vblank = v4l2_ctrl_new_std(handler, ops, V4L2_CID_VBLANK,

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 08/12] media: i2c: Add hblank control to ov8865

[Daniel Scally]

Hi Paul

On 14/08/2021 21:56, Laurent Pinchart wrote:
> Hi Daniel,
>
> On Fri, Aug 13, 2021 at 10:45:48AM +0100, Daniel Scally wrote:
>> On 13/08/2021 04:05, Laurent Pinchart wrote:
>>> On Tue, Aug 10, 2021 at 11:07:22PM +0100, Daniel Scally wrote:
>>>> On 10/08/2021 15:29, Sakari Ailus wrote:
>>>>> On Mon, Aug 09, 2021 at 11:58:41PM +0100, Daniel Scally wrote:
>>>>>> @@ -2542,6 +2544,13 @@ static int ov8865_ctrls_init(struct ov8865_sensor *sensor)
>>>>>>  				     0, 0, ov8865_test_pattern_menu);
>>>>>>  
>>>>>>  	/* Blanking */
>>>>>> +	hblank = mode->hts < mode->output_size_x ? 0 : mode->hts - mode->output_size_x;
>>>>> Is the result in relation with the analogue crop size? Based on the above
>>>>> it wouldn't seem like that.
>>>> This was a weird one actually. My understanding was that HTS should
>>>> always be >= the horizontal crop plus hblank...but that doesn't hold
>>>> true for some of this driver's modes and nor does it hold true when
>>>> running the camera in Windows (I checked the registers whilst running
>>>> it). So I went with setting hblank to 0 if the mode's HTS exceeded the
>>>> horizontal crop as the only way I could see to reconcile that.
>>> There's something very fishy here, HTS is, by definition, equal to the
>>> analog crop width plus the horizontal blanking. I suspect that the
>>> values in ov8865_modes are wrong.
>> I thought that initially too but confirming that the same thing happened
>> running windows switched me into "you're probably wrong" mode. If we're
>> confident that the HTS is likely wrong though I can add an extra patch
>> to bring those into lining with that definition.
> I think it's worth investigating this. The hblank computed here is
> clearly incorrect, and would thus be useless for all practical purposes.
> As usual with OmniVision, the datasheet is also quite useless.
>
> Paul, do you have any information about this ?


A gentle ping on this...I played around setting HTS / VTS values whilst
the camera was running windows; and it behaves as you'd expect it to
(raising/lowering the frame rate), so as far as I can tell the sensor
itself isn't doing anything unusual...

>>>>>> +	ctrls->hblank = v4l2_ctrl_new_std(handler, ops, V4L2_CID_HBLANK, hblank,
>>>>>> +					  hblank, 1, hblank);
>>>>>> +
>>>>>> +	if (ctrls->hblank)
>>>>>> +		ctrls->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>>>>>> +
>>>>>>  	vblank_max = OV8865_TIMING_MAX_VTS - mode->output_size_y;
>>>>>>  	vblank_def = mode->vts - mode->output_size_y;
>>>>>>  	ctrls->vblank = v4l2_ctrl_new_std(handler, ops, V4L2_CID_VBLANK,

Re: [PATCH v2 08/12] media: i2c: Add hblank control to ov8865

[Daniel Scally]

Hello all

On 09/09/2021 23:36, Daniel Scally wrote:
> Hi Paul
>
> On 14/08/2021 21:56, Laurent Pinchart wrote:
>> Hi Daniel,
>>
>> On Fri, Aug 13, 2021 at 10:45:48AM +0100, Daniel Scally wrote:
>>> On 13/08/2021 04:05, Laurent Pinchart wrote:
>>>> On Tue, Aug 10, 2021 at 11:07:22PM +0100, Daniel Scally wrote:
>>>>> On 10/08/2021 15:29, Sakari Ailus wrote:
>>>>>> On Mon, Aug 09, 2021 at 11:58:41PM +0100, Daniel Scally wrote:
>>>>>>> @@ -2542,6 +2544,13 @@ static int ov8865_ctrls_init(struct ov8865_sensor *sensor)
>>>>>>>  				     0, 0, ov8865_test_pattern_menu);
>>>>>>>  
>>>>>>>  	/* Blanking */
>>>>>>> +	hblank = mode->hts < mode->output_size_x ? 0 : mode->hts - mode->output_size_x;
>>>>>> Is the result in relation with the analogue crop size? Based on the above
>>>>>> it wouldn't seem like that.
>>>>> This was a weird one actually. My understanding was that HTS should
>>>>> always be >= the horizontal crop plus hblank...but that doesn't hold
>>>>> true for some of this driver's modes and nor does it hold true when
>>>>> running the camera in Windows (I checked the registers whilst running
>>>>> it). So I went with setting hblank to 0 if the mode's HTS exceeded the
>>>>> horizontal crop as the only way I could see to reconcile that.
>>>> There's something very fishy here, HTS is, by definition, equal to the
>>>> analog crop width plus the horizontal blanking. I suspect that the
>>>> values in ov8865_modes are wrong.
>>> I thought that initially too but confirming that the same thing happened
>>> running windows switched me into "you're probably wrong" mode. If we're
>>> confident that the HTS is likely wrong though I can add an extra patch
>>> to bring those into lining with that definition.
>> I think it's worth investigating this. The hblank computed here is
>> clearly incorrect, and would thus be useless for all practical purposes.
>> As usual with OmniVision, the datasheet is also quite useless.
>>
>> Paul, do you have any information about this ?
>
> A gentle ping on this...I played around setting HTS / VTS values whilst
> the camera was running windows; and it behaves as you'd expect it to
> (raising/lowering the frame rate), so as far as I can tell the sensor
> itself isn't doing anything unusual...


So, looking at this again. The mode in question has:

.output_size_x      = 3264
.hts                         = 1944
.output_size_y      = 2448
.vts                         = 2470
.frame_interval    = { 1, 30 }
   
And the driver sets a link frequency of 360MHz. That makes the pixel
rate, depending on whether we're looking at 2 or 4 data lanes, either
144MHz or 288MHz. I think the HTS there is calculated so that the 2 lane
configuration can make 30 FPS. Perhaps it would be better to default in
the mode to the "ideal" 4-lane 30fps setting (by upping the .hts to
3888), but rather than hardcoding the frame interval there, calculate it
for .g_frame_interval() based on the number of data lanes found in the
bus (accepting that if we only have 2 it's going to be 15fps rather than
30, which doesn't seem unreasonable for that resolution)

[PATCH v2 09/12] media: i2c: cap exposure at height + vblank in ov8865

[Daniel Scally]

Exposure limits depend on the total height; when vblank is altered (and
thus the total height is altered), change the exposure limits to reflect
the new cap.

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

	- None

 drivers/media/i2c/ov8865.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
index db84294b7a03..70747552e32a 100644
--- a/drivers/media/i2c/ov8865.c
+++ b/drivers/media/i2c/ov8865.c
@@ -675,6 +675,7 @@ struct ov8865_ctrls {
 	struct v4l2_ctrl *pixel_rate;
 	struct v4l2_ctrl *hblank;
 	struct v4l2_ctrl *vblank;
+	struct v4l2_ctrl *exposure;
 
 	struct v4l2_ctrl_handler handler;
 };
@@ -2461,6 +2462,18 @@ static int ov8865_s_ctrl(struct v4l2_ctrl *ctrl)
 	unsigned int index;
 	int ret;
 
+	/* If VBLANK is altered we need to update exposure to compensate */
+	if (ctrl->id == V4L2_CID_VBLANK) {
+		int exposure_max;
+
+		exposure_max = sensor->state.mode->output_size_y + ctrl->val;
+		__v4l2_ctrl_modify_range(sensor->ctrls.exposure,
+					 sensor->ctrls.exposure->minimum,
+					 exposure_max,
+					 sensor->ctrls.exposure->step,
+					 min(sensor->ctrls.exposure->val, exposure_max));
+	}
+
 	/* Wait for the sensor to be on before setting controls. */
 	if (pm_runtime_suspended(sensor->dev))
 		return 0;
@@ -2517,8 +2530,8 @@ static int ov8865_ctrls_init(struct ov8865_sensor *sensor)
 
 	/* Exposure */
 
-	v4l2_ctrl_new_std(handler, ops, V4L2_CID_EXPOSURE, 16, 1048575, 16,
-			  512);
+	ctrls->exposure = v4l2_ctrl_new_std(handler, ops, V4L2_CID_EXPOSURE, 16,
+					    1048575, 16, 512);
 
 	/* Gain */
 
@@ -2699,6 +2712,7 @@ static int ov8865_set_fmt(struct v4l2_subdev *subdev,
 	u32 mbus_code = 0;
 	unsigned int hblank;
 	unsigned int index;
+	int exposure_max;
 	int ret = 0;
 
 	mutex_lock(&sensor->mutex);
@@ -2746,6 +2760,12 @@ static int ov8865_set_fmt(struct v4l2_subdev *subdev,
 	__v4l2_ctrl_modify_range(sensor->ctrls.hblank, hblank, hblank, 1,
 				 hblank);
 
+	exposure_max = mode->vts;
+	__v4l2_ctrl_modify_range(sensor->ctrls.exposure,
+				 sensor->ctrls.exposure->minimum, exposure_max,
+				 sensor->ctrls.exposure->step,
+				 min(sensor->ctrls.exposure->val, exposure_max));
+
 complete:
 	mutex_unlock(&sensor->mutex);
 
-- 
2.25.1

Re: [PATCH v2 09/12] media: i2c: cap exposure at height + vblank in ov8865

[Sakari Ailus]

Hi Daniel,

On Mon, Aug 09, 2021 at 11:58:42PM +0100, Daniel Scally wrote:
> Exposure limits depend on the total height; when vblank is altered (and
> thus the total height is altered), change the exposure limits to reflect
> the new cap.
> 
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes in v2:
> 
> 	- None
> 
>  drivers/media/i2c/ov8865.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
> index db84294b7a03..70747552e32a 100644
> --- a/drivers/media/i2c/ov8865.c
> +++ b/drivers/media/i2c/ov8865.c
> @@ -675,6 +675,7 @@ struct ov8865_ctrls {
>  	struct v4l2_ctrl *pixel_rate;
>  	struct v4l2_ctrl *hblank;
>  	struct v4l2_ctrl *vblank;
> +	struct v4l2_ctrl *exposure;
>  
>  	struct v4l2_ctrl_handler handler;
>  };
> @@ -2461,6 +2462,18 @@ static int ov8865_s_ctrl(struct v4l2_ctrl *ctrl)
>  	unsigned int index;
>  	int ret;
>  
> +	/* If VBLANK is altered we need to update exposure to compensate */
> +	if (ctrl->id == V4L2_CID_VBLANK) {
> +		int exposure_max;
> +
> +		exposure_max = sensor->state.mode->output_size_y + ctrl->val;
> +		__v4l2_ctrl_modify_range(sensor->ctrls.exposure,
> +					 sensor->ctrls.exposure->minimum,
> +					 exposure_max,
> +					 sensor->ctrls.exposure->step,
> +					 min(sensor->ctrls.exposure->val, exposure_max));
> +	}
> +
>  	/* Wait for the sensor to be on before setting controls. */
>  	if (pm_runtime_suspended(sensor->dev))
>  		return 0;
> @@ -2517,8 +2530,8 @@ static int ov8865_ctrls_init(struct ov8865_sensor *sensor)
>  
>  	/* Exposure */
>  
> -	v4l2_ctrl_new_std(handler, ops, V4L2_CID_EXPOSURE, 16, 1048575, 16,
> -			  512);
> +	ctrls->exposure = v4l2_ctrl_new_std(handler, ops, V4L2_CID_EXPOSURE, 16,
> +					    1048575, 16, 512);
>  
>  	/* Gain */
>  
> @@ -2699,6 +2712,7 @@ static int ov8865_set_fmt(struct v4l2_subdev *subdev,
>  	u32 mbus_code = 0;
>  	unsigned int hblank;
>  	unsigned int index;
> +	int exposure_max;
>  	int ret = 0;
>  
>  	mutex_lock(&sensor->mutex);
> @@ -2746,6 +2760,12 @@ static int ov8865_set_fmt(struct v4l2_subdev *subdev,
>  	__v4l2_ctrl_modify_range(sensor->ctrls.hblank, hblank, hblank, 1,
>  				 hblank);
>  
> +	exposure_max = mode->vts;
> +	__v4l2_ctrl_modify_range(sensor->ctrls.exposure,
> +				 sensor->ctrls.exposure->minimum, exposure_max,
> +				 sensor->ctrls.exposure->step,
> +				 min(sensor->ctrls.exposure->val, exposure_max));

Please wrap lines over 80 (unless there's a sound reason not to).

> +
>  complete:
>  	mutex_unlock(&sensor->mutex);
>  

-- 
Sakari Ailus

[PATCH v2 10/12] media: i2c: Add controls from fwnode to ov8865

[Daniel Scally]

Add V4L2_CID_CAMERA_ORIENTATION and V4L2_CID_CAMERA_SENSOR_ROTATION
controls to the ov8865 driver by attempting to parse them from firmware.

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

	- None

 drivers/media/i2c/ov8865.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
index 70747552e32a..d7926cadce70 100644
--- a/drivers/media/i2c/ov8865.c
+++ b/drivers/media/i2c/ov8865.c
@@ -2519,6 +2519,7 @@ static int ov8865_ctrls_init(struct ov8865_sensor *sensor)
 	struct v4l2_ctrl_handler *handler = &ctrls->handler;
 	const struct v4l2_ctrl_ops *ops = &ov8865_ctrl_ops;
 	const struct ov8865_mode *mode = sensor->state.mode;
+	struct v4l2_fwnode_device_properties props;
 	unsigned int vblank_max, vblank_def;
 	unsigned int hblank;
 	int ret;
@@ -2581,6 +2582,15 @@ static int ov8865_ctrls_init(struct ov8865_sensor *sensor)
 		v4l2_ctrl_new_std(handler, NULL, V4L2_CID_PIXEL_RATE, 1,
 				  INT_MAX, 1, 1);
 
+	/* set properties from fwnode (e.g. rotation, orientation) */
+	ret = v4l2_fwnode_device_parse(sensor->dev, &props);
+	if (ret)
+		goto error_ctrls;
+
+	ret = v4l2_ctrl_new_fwnode_properties(handler, ops, &props);
+	if (ret)
+		goto error_ctrls;
+
 	if (handler->error) {
 		ret = handler->error;
 		goto error_ctrls;
-- 
2.25.1

[PATCH v2 11/12] media: i2c: Switch exposure control unit to lines

[Daniel Scally]

The ov8865 driver currently has the unit of the V4L2_CID_EXPOSURE control
as 1/16th of a line. This is what the sensor expects, but isn't very
intuitive. Switch the control to be in units of a line and simply do the
16x multiplication before passing the value to the sensor.

The datasheet for this sensor gives minimum exposure as 2 lines, so take
the opportunity to correct the lower bounds of the control.

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

	- None

 drivers/media/i2c/ov8865.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
index d7926cadce70..ce4e0ae2c4d3 100644
--- a/drivers/media/i2c/ov8865.c
+++ b/drivers/media/i2c/ov8865.c
@@ -2132,6 +2132,9 @@ static int ov8865_exposure_configure(struct ov8865_sensor *sensor, u32 exposure)
 {
 	int ret;
 
+	/* The sensor stores exposure in units of 1/16th of a line */
+	exposure *= 16;
+
 	ret = ov8865_write(sensor, OV8865_EXPOSURE_CTRL_HH_REG,
 			   OV8865_EXPOSURE_CTRL_HH(exposure));
 	if (ret)
@@ -2531,8 +2534,8 @@ static int ov8865_ctrls_init(struct ov8865_sensor *sensor)
 
 	/* Exposure */
 
-	ctrls->exposure = v4l2_ctrl_new_std(handler, ops, V4L2_CID_EXPOSURE, 16,
-					    1048575, 16, 512);
+	ctrls->exposure = v4l2_ctrl_new_std(handler, ops, V4L2_CID_EXPOSURE, 2,
+					    65535, 1, 32);
 
 	/* Gain */
 
-- 
2.25.1

[PATCH v2 12/12] media: ipu3-cio2: Add INT347A to cio2-bridge

[Daniel Scally]

ACPI _HID INT347A represents the OV8865 sensor, the driver for which can
support the platforms that the cio2-bridge serves. Add it to the array
of supported sensors so the bridge will connect the sensor to the CIO2
device.

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

	- Ordered the list by ACPI _HID (Andy)

 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 4657e99df033..18a31196a4a3 100644
--- a/drivers/media/pci/intel/ipu3/cio2-bridge.c
+++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
@@ -22,6 +22,8 @@
 static const struct cio2_sensor_config cio2_supported_sensors[] = {
 	/* Omnivision OV5693 */
 	CIO2_SENSOR_CONFIG("INT33BE", 0),
+	/* Omnivision OV8865 */
+	CIO2_SENSOR_CONFIG("INT347A", 1, 360000000),
 	/* Omnivision OV2680 */
 	CIO2_SENSOR_CONFIG("OVTI2680", 0),
 };
-- 
2.25.1

Re: [PATCH v2 12/12] media: ipu3-cio2: Add INT347A to cio2-bridge

[Andy Shevchenko]

On Tue, Aug 10, 2021 at 1:59 AM Daniel Scally <djrscally@gmail.com> wrote:
>
> ACPI _HID INT347A represents the OV8865 sensor, the driver for which can
> support the platforms that the cio2-bridge serves. Add it to the array
> of supported sensors so the bridge will connect the sensor to the CIO2
> device.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes in V2:
>
>         - Ordered the list by ACPI _HID (Andy)
>
>  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 4657e99df033..18a31196a4a3 100644
> --- a/drivers/media/pci/intel/ipu3/cio2-bridge.c
> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
> @@ -22,6 +22,8 @@
>  static const struct cio2_sensor_config cio2_supported_sensors[] = {
>         /* Omnivision OV5693 */
>         CIO2_SENSOR_CONFIG("INT33BE", 0),
> +       /* Omnivision OV8865 */
> +       CIO2_SENSOR_CONFIG("INT347A", 1, 360000000),
>         /* Omnivision OV2680 */
>         CIO2_SENSOR_CONFIG("OVTI2680", 0),
>  };
> --
> 2.25.1
>


-- 
With Best Regards,
Andy Shevchenko