[PATCH v4 00/16] Extensions to ov8865 driver

[PATCH v4 00/16] 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 V3:

	- Fixed a problem that prevented v3 applying properly due to the series
	being based on my WIP tree rather than a clean media tree. No code
	changes to the individual patches.

Daniel Scally (15):
  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: Update HTS values in 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: i2c: Re-order runtime pm initialisation
  media: i2c: Use dev_err_probe() in ov8865
  media: ipu3-cio2: Add INT347A to cio2-bridge

Hans de Goede (1):
  media: i2c: ov8865: Fix lockdep error

 drivers/media/i2c/ov8865.c                 | 459 +++++++++++++++------
 drivers/media/pci/intel/ipu3/cio2-bridge.c |   2 +
 2 files changed, 327 insertions(+), 134 deletions(-)

-- 
2.25.1

[PATCH v4 01/16] 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>
---
 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..7626c8608f8f 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 v4 01/16] media: i2c: Add ACPI support to ov8865

[Andy Shevchenko]

On Mon, Nov 1, 2021 at 2:12 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.
>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
>  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..7626c8608f8f 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>

I'm wondering if the code even uses of_*() APIs.
If nbo, maybe it's good to replace of_graph.h with property.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
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 01/16] media: i2c: Add ACPI support to ov8865

[Daniel Scally]

On 01/11/2021 10:01, Andy Shevchenko wrote:
> On Mon, Nov 1, 2021 at 2:12 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.
>>
>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>> ---
>>  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..7626c8608f8f 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>
> I'm wondering if the code even uses of_*() APIs.
> If nbo, maybe it's good to replace of_graph.h with property.h.


Yeah you're right; it doesn't use any of_*() calls. I'll add a patch

>>  #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
>>
>

[PATCH v4 02/16] 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>
---
 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 7626c8608f8f..8e3f8a554452 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 v4 03/16] 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>
---
 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 8e3f8a554452..9bc8d5d8199b 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 v4 04/16] 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>
---
 drivers/media/i2c/ov8865.c | 186 +++++++++++++++++++++++++++----------
 1 file changed, 135 insertions(+), 51 deletions(-)

diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
index 9bc8d5d8199b..4ddc1b277cc0 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,25 @@ struct ov8865_sclk_config {
 	unsigned int sclk_div;
 };
 
+struct ov8865_pll_configs {
+	const struct ov8865_pll1_config *pll1_config;
+	const struct ov8865_pll2_config *pll2_config_native;
+	const struct ov8865_pll2_config *pll2_config_binning;
+};
+
+/* 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
@@ -635,9 +650,7 @@ struct ov8865_mode {
 
 	struct v4l2_fract frame_interval;
 
-	const struct ov8865_pll1_config *pll1_config;
-	const struct ov8865_pll2_config *pll2_config;
-	const struct ov8865_sclk_config *sclk_config;
+	bool pll2_binning;
 
 	const struct ov8865_register_value *register_values;
 	unsigned int register_values_count;
@@ -665,6 +678,9 @@ struct ov8865_sensor {
 	struct regulator *avdd;
 	struct regulator *dvdd;
 	struct regulator *dovdd;
+
+	unsigned long extclk_rate;
+	const struct ov8865_pll_configs *pll_configs;
 	struct clk *extclk;
 
 	struct v4l2_fwnode_endpoint endpoint;
@@ -680,43 +696,70 @@ 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_config_native_19_2mhz = {
+		.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,
+};
+
+static const struct ov8865_pll1_config ov8865_pll1_config_native_24mhz = {
+		.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_config_native_19_2mhz = {
+		.pll_pre_div_half	= 1,
+		.pll_pre_div		= 5,
+		.pll_mul		= 75,
+		.dac_div		= 1,
+		.sys_pre_div		= 1,
+		.sys_div		= 3,
+};
+
+static const struct ov8865_pll2_config ov8865_pll2_config_native_24mhz = {
+		.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_config_binning_19_2mhz = {
+	.pll_pre_div_half	= 1,
+	.pll_pre_div		= 2,
+	.pll_mul		= 75,
+	.dac_div		= 2,
+	.sys_pre_div		= 10,
+	.sys_div		= 0,
+};
+
+static const struct ov8865_pll2_config ov8865_pll2_config_binning_24mhz = {
 	.pll_pre_div_half	= 1,
 	.pll_pre_div		= 0,
 	.pll_mul		= 30,
@@ -725,6 +768,23 @@ static const struct ov8865_pll2_config ov8865_pll2_config_binning = {
 	.sys_div		= 0,
 };
 
+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,
+};
+
+static const struct ov8865_pll_configs *ov8865_pll_configs[] = {
+	&ov8865_pll_configs_19_2mhz,
+	&ov8865_pll_configs_24mhz,
+};
+
 static const struct ov8865_sclk_config ov8865_sclk_config_native = {
 	.sys_sel		= 1,
 	.sclk_sel		= 0,
@@ -934,9 +994,7 @@ static const struct ov8865_mode ov8865_modes[] = {
 		.frame_interval			= { 1, 30 },
 
 		/* PLL */
-		.pll1_config			= &ov8865_pll1_config_native,
-		.pll2_config			= &ov8865_pll2_config_native,
-		.sclk_config			= &ov8865_sclk_config_native,
+		.pll2_binning			= false,
 
 		/* Registers */
 		.register_values	= ov8865_register_values_native,
@@ -990,9 +1048,7 @@ static const struct ov8865_mode ov8865_modes[] = {
 		.frame_interval			= { 1, 30 },
 
 		/* PLL */
-		.pll1_config			= &ov8865_pll1_config_native,
-		.pll2_config			= &ov8865_pll2_config_native,
-		.sclk_config			= &ov8865_sclk_config_native,
+		.pll2_binning			= false,
 
 		/* Registers */
 		.register_values	= ov8865_register_values_native,
@@ -1050,9 +1106,7 @@ static const struct ov8865_mode ov8865_modes[] = {
 		.frame_interval			= { 1, 30 },
 
 		/* PLL */
-		.pll1_config			= &ov8865_pll1_config_native,
-		.pll2_config			= &ov8865_pll2_config_binning,
-		.sclk_config			= &ov8865_sclk_config_native,
+		.pll2_binning			= true,
 
 		/* Registers */
 		.register_values	= ov8865_register_values_binning,
@@ -1116,9 +1170,7 @@ static const struct ov8865_mode ov8865_modes[] = {
 		.frame_interval			= { 1, 90 },
 
 		/* PLL */
-		.pll1_config			= &ov8865_pll1_config_native,
-		.pll2_config			= &ov8865_pll2_config_binning,
-		.sclk_config			= &ov8865_sclk_config_native,
+		.pll2_binning			= true,
 
 		/* Registers */
 		.register_values	= ov8865_register_values_binning,
@@ -1513,12 +1565,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 = sensor->pll_configs->pll1_config;
+	pll1_rate = sensor->extclk_rate * config->pll_mul / config->pll_pre_div_half;
 
 	switch (config->pll_pre_div) {
 	case 0:
@@ -1552,10 +1603,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 = sensor->pll_configs->pll1_config;
+
 	switch (mbus_code) {
 	case MEDIA_BUS_FMT_SBGGR10_1X10:
 		value = OV8865_MIPI_BIT_SEL(10);
@@ -1622,9 +1675,12 @@ 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_binning ? sensor->pll_configs->pll2_config_binning :
+				      sensor->pll_configs->pll2_config_native;
+
 	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));
@@ -1658,7 +1714,7 @@ static int ov8865_mode_pll2_configure(struct ov8865_sensor *sensor,
 static int ov8865_mode_sclk_configure(struct ov8865_sensor *sensor,
 				      const struct ov8865_mode *mode)
 {
-	const struct ov8865_sclk_config *config = mode->sclk_config;
+	const struct ov8865_sclk_config *config = &ov8865_sclk_config_native;
 	int ret;
 
 	ret = ov8865_write(sensor, OV8865_CLK_SEL0_REG,
@@ -2053,9 +2109,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 = sensor->pll_configs->pll1_config;
+
 	pll1_rate = ov8865_mode_pll1_rate(sensor, mode);
 
 	return pll1_rate / config->m_div / 2;
@@ -2783,7 +2841,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 +2917,38 @@ 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 the 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->pll_configs = ov8865_pll_configs[i];
+
 	/* Subdev, entity and pad */
 
 	subdev = &sensor->subdev;
-- 
2.25.1

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

[Andy Shevchenko]

On Mon, Nov 1, 2021 at 2:12 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.

...

> +       /*
> +        * 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 the 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;
> +               }

dev_err_probe()
7065f92255bb ("driver core: Clarify that dev_err_probe() is OK even
w/out -EPROBE_DEFER")

> +       }

...

> +       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;
>         }

find_closest() ?

-- 
With Best Regards,
Andy Shevchenko

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

[Daniel Scally]

Hi Andy

On 01/11/2021 10:29, Andy Shevchenko wrote:
> On Mon, Nov 1, 2021 at 2:12 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.
> ...
>
>> +       /*
>> +        * 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 the 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;
>> +               }
> dev_err_probe()
> 7065f92255bb ("driver core: Clarify that dev_err_probe() is OK even
> w/out -EPROBE_DEFER")


That cuts down on boilerplate usefully, thank you!

>
>> +       }
> ...
>
>> +       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;
>>         }
> find_closest() ?
>
Not sure about this one though; the values might not match exactly, but
if someone makes a device with this sensor supplying a totally different
clock than 19.2MHz or 24MHz, it ought to fail here rather than attempt
to continue I think

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

[Sakari Ailus]

Hi Daniel,

Thanks for the set.

On Mon, Nov 01, 2021 at 12:11:07AM +0000, 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>
> ---
>  drivers/media/i2c/ov8865.c | 186 +++++++++++++++++++++++++++----------
>  1 file changed, 135 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
> index 9bc8d5d8199b..4ddc1b277cc0 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,25 @@ struct ov8865_sclk_config {
>  	unsigned int sclk_div;
>  };
>  
> +struct ov8865_pll_configs {
> +	const struct ov8865_pll1_config *pll1_config;
> +	const struct ov8865_pll2_config *pll2_config_native;
> +	const struct ov8865_pll2_config *pll2_config_binning;
> +};
> +
> +/* 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
> @@ -635,9 +650,7 @@ struct ov8865_mode {
>  
>  	struct v4l2_fract frame_interval;
>  
> -	const struct ov8865_pll1_config *pll1_config;
> -	const struct ov8865_pll2_config *pll2_config;
> -	const struct ov8865_sclk_config *sclk_config;
> +	bool pll2_binning;
>  
>  	const struct ov8865_register_value *register_values;
>  	unsigned int register_values_count;
> @@ -665,6 +678,9 @@ struct ov8865_sensor {
>  	struct regulator *avdd;
>  	struct regulator *dvdd;
>  	struct regulator *dovdd;
> +
> +	unsigned long extclk_rate;
> +	const struct ov8865_pll_configs *pll_configs;
>  	struct clk *extclk;
>  
>  	struct v4l2_fwnode_endpoint endpoint;
> @@ -680,43 +696,70 @@ 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_config_native_19_2mhz = {
> +		.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,
> +};
> +
> +static const struct ov8865_pll1_config ov8865_pll1_config_native_24mhz = {
> +		.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_config_native_19_2mhz = {
> +		.pll_pre_div_half	= 1,
> +		.pll_pre_div		= 5,
> +		.pll_mul		= 75,
> +		.dac_div		= 1,
> +		.sys_pre_div		= 1,
> +		.sys_div		= 3,
> +};
> +
> +static const struct ov8865_pll2_config ov8865_pll2_config_native_24mhz = {
> +		.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_config_binning_19_2mhz = {
> +	.pll_pre_div_half	= 1,
> +	.pll_pre_div		= 2,
> +	.pll_mul		= 75,
> +	.dac_div		= 2,
> +	.sys_pre_div		= 10,
> +	.sys_div		= 0,
> +};
> +
> +static const struct ov8865_pll2_config ov8865_pll2_config_binning_24mhz = {
>  	.pll_pre_div_half	= 1,
>  	.pll_pre_div		= 0,
>  	.pll_mul		= 30,
> @@ -725,6 +768,23 @@ static const struct ov8865_pll2_config ov8865_pll2_config_binning = {
>  	.sys_div		= 0,
>  };
>  
> +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,
> +};

These should be const.

> +
> +static const struct ov8865_pll_configs *ov8865_pll_configs[] = {
> +	&ov8865_pll_configs_19_2mhz,
> +	&ov8865_pll_configs_24mhz,
> +};
> +
>  static const struct ov8865_sclk_config ov8865_sclk_config_native = {
>  	.sys_sel		= 1,
>  	.sclk_sel		= 0,
> @@ -934,9 +994,7 @@ static const struct ov8865_mode ov8865_modes[] = {
>  		.frame_interval			= { 1, 30 },
>  
>  		/* PLL */
> -		.pll1_config			= &ov8865_pll1_config_native,
> -		.pll2_config			= &ov8865_pll2_config_native,
> -		.sclk_config			= &ov8865_sclk_config_native,
> +		.pll2_binning			= false,
>  
>  		/* Registers */
>  		.register_values	= ov8865_register_values_native,
> @@ -990,9 +1048,7 @@ static const struct ov8865_mode ov8865_modes[] = {
>  		.frame_interval			= { 1, 30 },
>  
>  		/* PLL */
> -		.pll1_config			= &ov8865_pll1_config_native,
> -		.pll2_config			= &ov8865_pll2_config_native,
> -		.sclk_config			= &ov8865_sclk_config_native,
> +		.pll2_binning			= false,
>  
>  		/* Registers */
>  		.register_values	= ov8865_register_values_native,
> @@ -1050,9 +1106,7 @@ static const struct ov8865_mode ov8865_modes[] = {
>  		.frame_interval			= { 1, 30 },
>  
>  		/* PLL */
> -		.pll1_config			= &ov8865_pll1_config_native,
> -		.pll2_config			= &ov8865_pll2_config_binning,
> -		.sclk_config			= &ov8865_sclk_config_native,
> +		.pll2_binning			= true,
>  
>  		/* Registers */
>  		.register_values	= ov8865_register_values_binning,
> @@ -1116,9 +1170,7 @@ static const struct ov8865_mode ov8865_modes[] = {
>  		.frame_interval			= { 1, 90 },
>  
>  		/* PLL */
> -		.pll1_config			= &ov8865_pll1_config_native,
> -		.pll2_config			= &ov8865_pll2_config_binning,
> -		.sclk_config			= &ov8865_sclk_config_native,
> +		.pll2_binning			= true,
>  
>  		/* Registers */
>  		.register_values	= ov8865_register_values_binning,
> @@ -1513,12 +1565,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 = sensor->pll_configs->pll1_config;
> +	pll1_rate = sensor->extclk_rate * config->pll_mul / config->pll_pre_div_half;
>  
>  	switch (config->pll_pre_div) {
>  	case 0:
> @@ -1552,10 +1603,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 = sensor->pll_configs->pll1_config;
> +
>  	switch (mbus_code) {
>  	case MEDIA_BUS_FMT_SBGGR10_1X10:
>  		value = OV8865_MIPI_BIT_SEL(10);
> @@ -1622,9 +1675,12 @@ 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_binning ? sensor->pll_configs->pll2_config_binning :
> +				      sensor->pll_configs->pll2_config_native;
> +
>  	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));
> @@ -1658,7 +1714,7 @@ static int ov8865_mode_pll2_configure(struct ov8865_sensor *sensor,
>  static int ov8865_mode_sclk_configure(struct ov8865_sensor *sensor,
>  				      const struct ov8865_mode *mode)
>  {
> -	const struct ov8865_sclk_config *config = mode->sclk_config;
> +	const struct ov8865_sclk_config *config = &ov8865_sclk_config_native;
>  	int ret;
>  
>  	ret = ov8865_write(sensor, OV8865_CLK_SEL0_REG,
> @@ -2053,9 +2109,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 = sensor->pll_configs->pll1_config;
> +
>  	pll1_rate = ov8865_mode_pll1_rate(sensor, mode);
>  
>  	return pll1_rate / config->m_div / 2;
> @@ -2783,7 +2841,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 +2917,38 @@ 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 the 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);

clk_get_rate() returns 0 if you don't have a clock. But you can still have
clock-frequency property that tells the frequency. This is generally the
case on ACPI based systems apart from some exceptions (which I understand
you're well aware of).

See e.g. drivers/media/i2c/ccs/ccs-core.c .

> +
> +	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->pll_configs = ov8865_pll_configs[i];
> +
>  	/* Subdev, entity and pad */
>  
>  	subdev = &sensor->subdev;

-- 
Kind regards,

Sakari Ailus

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

[Daniel Scally]

Hi Sakari - sorry, only just coming back to this series.

On 01/11/2021 14:52, Sakari Ailus wrote:
>> +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,
>> +};
> 
> These should be const.

Done, thank you.


>> @@ -2858,13 +2917,38 @@ 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 the 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);
> 
> clk_get_rate() returns 0 if you don't have a clock. But you can still have
> clock-frequency property that tells the frequency. This is generally the
> case on ACPI based systems apart from some exceptions (which I understand
> you're well aware of).
> 
> See e.g. drivers/media/i2c/ccs/ccs-core.c .

I'm checking the clock-frequency property above...that should be
sufficient here I think right?

>> +
>> +	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->pll_configs = ov8865_pll_configs[i];
>> +
>>  	/* Subdev, entity and pad */
>>  
>>  	subdev = &sensor->subdev;
> 

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

[Daniel Scally]

Hi Sakari

On 01/11/2021 14:52, Sakari Ailus wrote:
> Hi Daniel,
>
> Thanks for the set.
>
> On Mon, Nov 01, 2021 at 12:11:07AM +0000, 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>
>> ---
>>  drivers/media/i2c/ov8865.c | 186 +++++++++++++++++++++++++++----------
>>  1 file changed, 135 insertions(+), 51 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
>> index 9bc8d5d8199b..4ddc1b277cc0 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,25 @@ struct ov8865_sclk_config {
>>  	unsigned int sclk_div;
>>  };
>>  
>> +struct ov8865_pll_configs {
>> +	const struct ov8865_pll1_config *pll1_config;
>> +	const struct ov8865_pll2_config *pll2_config_native;
>> +	const struct ov8865_pll2_config *pll2_config_binning;
>> +};
>> +
>> +/* 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
>> @@ -635,9 +650,7 @@ struct ov8865_mode {
>>  
>>  	struct v4l2_fract frame_interval;
>>  
>> -	const struct ov8865_pll1_config *pll1_config;
>> -	const struct ov8865_pll2_config *pll2_config;
>> -	const struct ov8865_sclk_config *sclk_config;
>> +	bool pll2_binning;
>>  
>>  	const struct ov8865_register_value *register_values;
>>  	unsigned int register_values_count;
>> @@ -665,6 +678,9 @@ struct ov8865_sensor {
>>  	struct regulator *avdd;
>>  	struct regulator *dvdd;
>>  	struct regulator *dovdd;
>> +
>> +	unsigned long extclk_rate;
>> +	const struct ov8865_pll_configs *pll_configs;
>>  	struct clk *extclk;
>>  
>>  	struct v4l2_fwnode_endpoint endpoint;
>> @@ -680,43 +696,70 @@ 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_config_native_19_2mhz = {
>> +		.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,
>> +};
>> +
>> +static const struct ov8865_pll1_config ov8865_pll1_config_native_24mhz = {
>> +		.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_config_native_19_2mhz = {
>> +		.pll_pre_div_half	= 1,
>> +		.pll_pre_div		= 5,
>> +		.pll_mul		= 75,
>> +		.dac_div		= 1,
>> +		.sys_pre_div		= 1,
>> +		.sys_div		= 3,
>> +};
>> +
>> +static const struct ov8865_pll2_config ov8865_pll2_config_native_24mhz = {
>> +		.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_config_binning_19_2mhz = {
>> +	.pll_pre_div_half	= 1,
>> +	.pll_pre_div		= 2,
>> +	.pll_mul		= 75,
>> +	.dac_div		= 2,
>> +	.sys_pre_div		= 10,
>> +	.sys_div		= 0,
>> +};
>> +
>> +static const struct ov8865_pll2_config ov8865_pll2_config_binning_24mhz = {
>>  	.pll_pre_div_half	= 1,
>>  	.pll_pre_div		= 0,
>>  	.pll_mul		= 30,
>> @@ -725,6 +768,23 @@ static const struct ov8865_pll2_config ov8865_pll2_config_binning = {
>>  	.sys_div		= 0,
>>  };
>>  
>> +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,
>> +};
> These should be const.
>
>> +
>> +static const struct ov8865_pll_configs *ov8865_pll_configs[] = {
>> +	&ov8865_pll_configs_19_2mhz,
>> +	&ov8865_pll_configs_24mhz,
>> +};
>> +
>>  static const struct ov8865_sclk_config ov8865_sclk_config_native = {
>>  	.sys_sel		= 1,
>>  	.sclk_sel		= 0,
>> @@ -934,9 +994,7 @@ static const struct ov8865_mode ov8865_modes[] = {
>>  		.frame_interval			= { 1, 30 },
>>  
>>  		/* PLL */
>> -		.pll1_config			= &ov8865_pll1_config_native,
>> -		.pll2_config			= &ov8865_pll2_config_native,
>> -		.sclk_config			= &ov8865_sclk_config_native,
>> +		.pll2_binning			= false,
>>  
>>  		/* Registers */
>>  		.register_values	= ov8865_register_values_native,
>> @@ -990,9 +1048,7 @@ static const struct ov8865_mode ov8865_modes[] = {
>>  		.frame_interval			= { 1, 30 },
>>  
>>  		/* PLL */
>> -		.pll1_config			= &ov8865_pll1_config_native,
>> -		.pll2_config			= &ov8865_pll2_config_native,
>> -		.sclk_config			= &ov8865_sclk_config_native,
>> +		.pll2_binning			= false,
>>  
>>  		/* Registers */
>>  		.register_values	= ov8865_register_values_native,
>> @@ -1050,9 +1106,7 @@ static const struct ov8865_mode ov8865_modes[] = {
>>  		.frame_interval			= { 1, 30 },
>>  
>>  		/* PLL */
>> -		.pll1_config			= &ov8865_pll1_config_native,
>> -		.pll2_config			= &ov8865_pll2_config_binning,
>> -		.sclk_config			= &ov8865_sclk_config_native,
>> +		.pll2_binning			= true,
>>  
>>  		/* Registers */
>>  		.register_values	= ov8865_register_values_binning,
>> @@ -1116,9 +1170,7 @@ static const struct ov8865_mode ov8865_modes[] = {
>>  		.frame_interval			= { 1, 90 },
>>  
>>  		/* PLL */
>> -		.pll1_config			= &ov8865_pll1_config_native,
>> -		.pll2_config			= &ov8865_pll2_config_binning,
>> -		.sclk_config			= &ov8865_sclk_config_native,
>> +		.pll2_binning			= true,
>>  
>>  		/* Registers */
>>  		.register_values	= ov8865_register_values_binning,
>> @@ -1513,12 +1565,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 = sensor->pll_configs->pll1_config;
>> +	pll1_rate = sensor->extclk_rate * config->pll_mul / config->pll_pre_div_half;
>>  
>>  	switch (config->pll_pre_div) {
>>  	case 0:
>> @@ -1552,10 +1603,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 = sensor->pll_configs->pll1_config;
>> +
>>  	switch (mbus_code) {
>>  	case MEDIA_BUS_FMT_SBGGR10_1X10:
>>  		value = OV8865_MIPI_BIT_SEL(10);
>> @@ -1622,9 +1675,12 @@ 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_binning ? sensor->pll_configs->pll2_config_binning :
>> +				      sensor->pll_configs->pll2_config_native;
>> +
>>  	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));
>> @@ -1658,7 +1714,7 @@ static int ov8865_mode_pll2_configure(struct ov8865_sensor *sensor,
>>  static int ov8865_mode_sclk_configure(struct ov8865_sensor *sensor,
>>  				      const struct ov8865_mode *mode)
>>  {
>> -	const struct ov8865_sclk_config *config = mode->sclk_config;
>> +	const struct ov8865_sclk_config *config = &ov8865_sclk_config_native;
>>  	int ret;
>>  
>>  	ret = ov8865_write(sensor, OV8865_CLK_SEL0_REG,
>> @@ -2053,9 +2109,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 = sensor->pll_configs->pll1_config;
>> +
>>  	pll1_rate = ov8865_mode_pll1_rate(sensor, mode);
>>  
>>  	return pll1_rate / config->m_div / 2;
>> @@ -2783,7 +2841,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 +2917,38 @@ 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 the 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);
> clk_get_rate() returns 0 if you don't have a clock. But you can still have
> clock-frequency property that tells the frequency. This is generally the
> case on ACPI based systems apart from some exceptions (which I understand
> you're well aware of).
>
> See e.g. drivers/media/i2c/ccs/ccs-core.c .


Sorry; misunderstood this in my previous reply, but having reviewed the
code you told me to review I understand what you're saying now - I'll
re-work it to follow the ccs-core methodology


>> +
>> +	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->pll_configs = ov8865_pll_configs[i];
>> +
>>  	/* Subdev, entity and pad */
>>  
>>  	subdev = &sensor->subdev;

[PATCH v4 05/16] 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>
---
 drivers/media/i2c/ov8865.c | 64 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
index 4ddc1b277cc0..0f2776390a8e 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) \
@@ -2756,12 +2765,67 @@ 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)
+{
+	const struct ov8865_mode *mode = sensor->state.mode;
+
+	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 = mode->output_size_y;
+		r->width = mode->output_size_x;
+		r->top = (OV8865_NATIVE_HEIGHT - mode->output_size_y) / 2;
+		r->left = (OV8865_NATIVE_WIDTH - 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,
+	.set_selection		= ov8865_get_selection,
 };
 
 static const struct v4l2_subdev_ops ov8865_subdev_ops = {
-- 
2.25.1

[PATCH v4 06/16] 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>
---
 drivers/media/i2c/ov8865.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
index 0f2776390a8e..a832938c33b6 100644
--- a/drivers/media/i2c/ov8865.c
+++ b/drivers/media/i2c/ov8865.c
@@ -2150,7 +2150,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;
 
@@ -2460,8 +2460,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;
@@ -2506,7 +2506,8 @@ 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

[PATCH v4 07/16] 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>
---
 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 a832938c33b6..f741c0713ca4 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
@@ -675,6 +677,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;
 };
@@ -2225,6 +2228,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,
@@ -2476,6 +2493,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;
 	}
@@ -2492,6 +2511,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);
@@ -2528,6 +2549,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 =
@@ -2708,6 +2736,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);
 
@@ -3035,6 +3067,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

Re: [PATCH v4 07/16] media: i2c: Add vblank control to ov8865

[Sakari Ailus]

Hi Daniel,

On Mon, Nov 01, 2021 at 12:11:10AM +0000, Daniel Scally wrote:
> Add a V4L2_CID_VBLANK control to the ov8865 driver.
> 
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
>  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 a832938c33b6..f741c0713ca4 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
> @@ -675,6 +677,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;
>  };
> @@ -2225,6 +2228,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,
> @@ -2476,6 +2493,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;
>  	}
> @@ -2492,6 +2511,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);
> @@ -2528,6 +2549,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 =
> @@ -2708,6 +2736,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);
>  
> @@ -3035,6 +3067,8 @@ static int ov8865_probe(struct i2c_client *client)
>  
>  	/* Sensor */
>  
> +	sensor->state.mode =  &ov8865_modes[0];

This seems to be an unrelated change. Does it belong here?

> +
>  	ret = ov8865_ctrls_init(sensor);
>  	if (ret)
>  		goto error_mutex;

-- 
Regards,

Sakari Ailus

Re: [PATCH v4 07/16] media: i2c: Add vblank control to ov8865

[Daniel Scally]

Hi Sakari

On 01/11/2021 14:54, Sakari Ailus wrote:
> Hi Daniel,
>
> On Mon, Nov 01, 2021 at 12:11:10AM +0000, Daniel Scally wrote:
>> Add a V4L2_CID_VBLANK control to the ov8865 driver.
>>
>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>> ---
>>  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 a832938c33b6..f741c0713ca4 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
>> @@ -675,6 +677,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;
>>  };
>> @@ -2225,6 +2228,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,
>> @@ -2476,6 +2493,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;
>>  	}
>> @@ -2492,6 +2511,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);
>> @@ -2528,6 +2549,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 =
>> @@ -2708,6 +2736,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);
>>  
>> @@ -3035,6 +3067,8 @@ static int ov8865_probe(struct i2c_client *client)
>>  
>>  	/* Sensor */
>>  
>> +	sensor->state.mode =  &ov8865_modes[0];
> This seems to be an unrelated change. Does it belong here?


In ov8865_ctrls_init() I'm using sensor->state.mode to initialise the
new vblank control. I guess instead of


+	const struct ov8865_mode *mode = sensor->state.mode;

I could have

+	const struct ov8865_mode *mode = &ov8865_modes[0];


in that function though

>
>> +
>>  	ret = ov8865_ctrls_init(sensor);
>>  	if (ret)
>>  		goto error_mutex;

Re: [PATCH v4 07/16] media: i2c: Add vblank control to ov8865

[Sakari Ailus]

Hi Daniel,

On Mon, Nov 01, 2021 at 11:45:45PM +0000, Daniel Scally wrote:
> Hi Sakari
> 
> On 01/11/2021 14:54, Sakari Ailus wrote:
> > Hi Daniel,
> >
> > On Mon, Nov 01, 2021 at 12:11:10AM +0000, Daniel Scally wrote:
> >> Add a V4L2_CID_VBLANK control to the ov8865 driver.
> >>
> >> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> >> ---
> >>  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 a832938c33b6..f741c0713ca4 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
> >> @@ -675,6 +677,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;
> >>  };
> >> @@ -2225,6 +2228,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,
> >> @@ -2476,6 +2493,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;
> >>  	}
> >> @@ -2492,6 +2511,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);
> >> @@ -2528,6 +2549,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 =
> >> @@ -2708,6 +2736,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);
> >>  
> >> @@ -3035,6 +3067,8 @@ static int ov8865_probe(struct i2c_client *client)
> >>  
> >>  	/* Sensor */
> >>  
> >> +	sensor->state.mode =  &ov8865_modes[0];
> > This seems to be an unrelated change. Does it belong here?
> 
> 
> In ov8865_ctrls_init() I'm using sensor->state.mode to initialise the
> new vblank control. I guess instead of
> 
> 
> +	const struct ov8865_mode *mode = sensor->state.mode;
> 
> I could have
> 
> +	const struct ov8865_mode *mode = &ov8865_modes[0];
> 
> 
> in that function though

You're right indeed.

ov8865_state_init() refers to the same mode. I think it'd be cleaner if
you'd e.g. call ov8865_state_configure() with sensor->state.mode instead,
so that you don't initialise the same variable in two places.

-- 
Kind regards,

Sakari Ailus

[PATCH v4 08/16] 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>
---
 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 f741c0713ca4..4b18cc80f985 100644
--- a/drivers/media/i2c/ov8865.c
+++ b/drivers/media/i2c/ov8865.c
@@ -677,6 +677,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;
@@ -2513,6 +2514,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);
@@ -2550,6 +2552,13 @@ static int ov8865_ctrls_init(struct ov8865_sensor *sensor)
 				     0, 0, ov8865_test_pattern_menu);
 
 	/* Blanking */
+	hblank = 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,
@@ -2696,6 +2705,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;
 
@@ -2740,6 +2750,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;
+	__v4l2_ctrl_modify_range(sensor->ctrls.hblank, hblank, hblank, 1,
+				 hblank);
+
 complete:
 	mutex_unlock(&sensor->mutex);
 
-- 
2.25.1

[PATCH v4 09/16] media: i2c: Update HTS values in ov8865

[Daniel Scally]

The HTS values for some of the modes in the ov8865 driver are a bit
unusual, coming in lower than the output_size_x is set to. It seems
like they might be calculated to fit the desired framerate into a
configuration with just two data lanes. To bring this more in line
with expected behaviour, raise the HTS values above the output_size_x.

The corollary of that change is that the hardcoded frame intervals
against the modes no longer make sense, so remove those entirely.
Update the .g/s_frame_interval() callbacks to calculate the frame
interval based on the current mode and the vblank and hblank settings.

The implementation of the .enum_frame_interval() callback is no longer
suitable since the possible frame rate is now a continuous range depending
on the vblank control setting, so remove that callback entirely.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
 drivers/media/i2c/ov8865.c | 65 +++++++-------------------------------
 1 file changed, 11 insertions(+), 54 deletions(-)

diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
index 4b18cc80f985..1b8674152750 100644
--- a/drivers/media/i2c/ov8865.c
+++ b/drivers/media/i2c/ov8865.c
@@ -659,8 +659,6 @@ struct ov8865_mode {
 	unsigned int blc_anchor_right_start;
 	unsigned int blc_anchor_right_end;
 
-	struct v4l2_fract frame_interval;
-
 	bool pll2_binning;
 
 	const struct ov8865_register_value *register_values;
@@ -964,7 +962,7 @@ static const struct ov8865_mode ov8865_modes[] = {
 	{
 		/* Horizontal */
 		.output_size_x			= 3264,
-		.hts				= 1944,
+		.hts				= 3888,
 
 		/* Vertical */
 		.output_size_y			= 2448,
@@ -1003,9 +1001,6 @@ static const struct ov8865_mode ov8865_modes[] = {
 		.blc_anchor_right_start		= 1984,
 		.blc_anchor_right_end		= 2239,
 
-		/* Frame Interval */
-		.frame_interval			= { 1, 30 },
-
 		/* PLL */
 		.pll2_binning			= false,
 
@@ -1018,11 +1013,11 @@ static const struct ov8865_mode ov8865_modes[] = {
 	{
 		/* Horizontal */
 		.output_size_x			= 3264,
-		.hts				= 2582,
+		.hts				= 3888,
 
 		/* Vertical */
 		.output_size_y			= 1836,
-		.vts				= 2002,
+		.vts				= 2470,
 
 		.size_auto			= true,
 		.size_auto_boundary_x		= 8,
@@ -1057,9 +1052,6 @@ static const struct ov8865_mode ov8865_modes[] = {
 		.blc_anchor_right_start		= 1984,
 		.blc_anchor_right_end		= 2239,
 
-		/* Frame Interval */
-		.frame_interval			= { 1, 30 },
-
 		/* PLL */
 		.pll2_binning			= false,
 
@@ -1115,9 +1107,6 @@ static const struct ov8865_mode ov8865_modes[] = {
 		.blc_anchor_right_start		= 992,
 		.blc_anchor_right_end		= 1119,
 
-		/* Frame Interval */
-		.frame_interval			= { 1, 30 },
-
 		/* PLL */
 		.pll2_binning			= true,
 
@@ -1179,9 +1168,6 @@ static const struct ov8865_mode ov8865_modes[] = {
 		.blc_anchor_right_start		= 992,
 		.blc_anchor_right_end		= 1119,
 
-		/* Frame Interval */
-		.frame_interval			= { 1, 90 },
-
 		/* PLL */
 		.pll2_binning			= true,
 
@@ -2628,11 +2614,18 @@ static int ov8865_g_frame_interval(struct v4l2_subdev *subdev,
 {
 	struct ov8865_sensor *sensor = ov8865_subdev_sensor(subdev);
 	const struct ov8865_mode *mode;
+	unsigned int framesize;
+	unsigned int fps;
 
 	mutex_lock(&sensor->mutex);
 
 	mode = sensor->state.mode;
-	interval->interval = mode->frame_interval;
+	framesize = mode->hts * (mode->output_size_y +
+				 sensor->ctrls.vblank->val);
+	fps = DIV_ROUND_CLOSEST(sensor->ctrls.pixel_rate->val, framesize);
+
+	interval->interval.numerator = 1;
+	interval->interval.denominator = fps;
 
 	mutex_unlock(&sensor->mutex);
 
@@ -2777,41 +2770,6 @@ static int ov8865_enum_frame_size(struct v4l2_subdev *subdev,
 	return 0;
 }
 
-static int ov8865_enum_frame_interval(struct v4l2_subdev *subdev,
-				      struct v4l2_subdev_state *sd_state,
-				      struct v4l2_subdev_frame_interval_enum *interval_enum)
-{
-	const struct ov8865_mode *mode = NULL;
-	unsigned int mode_index;
-	unsigned int interval_index;
-
-	if (interval_enum->index > 0)
-		return -EINVAL;
-	/*
-	 * Multiple modes with the same dimensions may have different frame
-	 * intervals, so look up each relevant mode.
-	 */
-	for (mode_index = 0, interval_index = 0;
-	     mode_index < ARRAY_SIZE(ov8865_modes); mode_index++) {
-		mode = &ov8865_modes[mode_index];
-
-		if (mode->output_size_x == interval_enum->width &&
-		    mode->output_size_y == interval_enum->height) {
-			if (interval_index == interval_enum->index)
-				break;
-
-			interval_index++;
-		}
-	}
-
-	if (mode_index == ARRAY_SIZE(ov8865_modes))
-		return -EINVAL;
-
-	interval_enum->interval = mode->frame_interval;
-
-	return 0;
-}
-
 static void
 __ov8865_get_pad_crop(struct ov8865_sensor *sensor,
 		      struct v4l2_subdev_state *state, unsigned int pad,
@@ -2870,7 +2828,6 @@ static const struct v4l2_subdev_pad_ops ov8865_subdev_pad_ops = {
 	.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,
 	.set_selection		= ov8865_get_selection,
 };
-- 
2.25.1

Re: [PATCH v4 09/16] media: i2c: Update HTS values in ov8865

[Sakari Ailus]

Hi Daniel,

On Mon, Nov 01, 2021 at 12:11:12AM +0000, Daniel Scally wrote:
> The HTS values for some of the modes in the ov8865 driver are a bit
> unusual, coming in lower than the output_size_x is set to. It seems
> like they might be calculated to fit the desired framerate into a
> configuration with just two data lanes. To bring this more in line
> with expected behaviour, raise the HTS values above the output_size_x.

This isn't necessarily a problem as binning may be done in analogue domain.
I don't know about this sensor though.

Is the frame interval still as expected in binned modes after this patch?

> 
> The corollary of that change is that the hardcoded frame intervals
> against the modes no longer make sense, so remove those entirely.
> Update the .g/s_frame_interval() callbacks to calculate the frame
> interval based on the current mode and the vblank and hblank settings.
> 
> The implementation of the .enum_frame_interval() callback is no longer
> suitable since the possible frame rate is now a continuous range depending
> on the vblank control setting, so remove that callback entirely.
> 
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
>  drivers/media/i2c/ov8865.c | 65 +++++++-------------------------------
>  1 file changed, 11 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
> index 4b18cc80f985..1b8674152750 100644
> --- a/drivers/media/i2c/ov8865.c
> +++ b/drivers/media/i2c/ov8865.c
> @@ -659,8 +659,6 @@ struct ov8865_mode {
>  	unsigned int blc_anchor_right_start;
>  	unsigned int blc_anchor_right_end;
>  
> -	struct v4l2_fract frame_interval;
> -
>  	bool pll2_binning;
>  
>  	const struct ov8865_register_value *register_values;
> @@ -964,7 +962,7 @@ static const struct ov8865_mode ov8865_modes[] = {
>  	{
>  		/* Horizontal */
>  		.output_size_x			= 3264,
> -		.hts				= 1944,
> +		.hts				= 3888,
>  
>  		/* Vertical */
>  		.output_size_y			= 2448,
> @@ -1003,9 +1001,6 @@ static const struct ov8865_mode ov8865_modes[] = {
>  		.blc_anchor_right_start		= 1984,
>  		.blc_anchor_right_end		= 2239,
>  
> -		/* Frame Interval */
> -		.frame_interval			= { 1, 30 },
> -
>  		/* PLL */
>  		.pll2_binning			= false,
>  
> @@ -1018,11 +1013,11 @@ static const struct ov8865_mode ov8865_modes[] = {
>  	{
>  		/* Horizontal */
>  		.output_size_x			= 3264,
> -		.hts				= 2582,
> +		.hts				= 3888,
>  
>  		/* Vertical */
>  		.output_size_y			= 1836,
> -		.vts				= 2002,
> +		.vts				= 2470,
>  
>  		.size_auto			= true,
>  		.size_auto_boundary_x		= 8,
> @@ -1057,9 +1052,6 @@ static const struct ov8865_mode ov8865_modes[] = {
>  		.blc_anchor_right_start		= 1984,
>  		.blc_anchor_right_end		= 2239,
>  
> -		/* Frame Interval */
> -		.frame_interval			= { 1, 30 },
> -
>  		/* PLL */
>  		.pll2_binning			= false,
>  
> @@ -1115,9 +1107,6 @@ static const struct ov8865_mode ov8865_modes[] = {
>  		.blc_anchor_right_start		= 992,
>  		.blc_anchor_right_end		= 1119,
>  
> -		/* Frame Interval */
> -		.frame_interval			= { 1, 30 },
> -
>  		/* PLL */
>  		.pll2_binning			= true,
>  
> @@ -1179,9 +1168,6 @@ static const struct ov8865_mode ov8865_modes[] = {
>  		.blc_anchor_right_start		= 992,
>  		.blc_anchor_right_end		= 1119,
>  
> -		/* Frame Interval */
> -		.frame_interval			= { 1, 90 },
> -
>  		/* PLL */
>  		.pll2_binning			= true,
>  
> @@ -2628,11 +2614,18 @@ static int ov8865_g_frame_interval(struct v4l2_subdev *subdev,
>  {
>  	struct ov8865_sensor *sensor = ov8865_subdev_sensor(subdev);
>  	const struct ov8865_mode *mode;
> +	unsigned int framesize;
> +	unsigned int fps;
>  
>  	mutex_lock(&sensor->mutex);
>  
>  	mode = sensor->state.mode;
> -	interval->interval = mode->frame_interval;
> +	framesize = mode->hts * (mode->output_size_y +
> +				 sensor->ctrls.vblank->val);
> +	fps = DIV_ROUND_CLOSEST(sensor->ctrls.pixel_rate->val, framesize);
> +
> +	interval->interval.numerator = 1;
> +	interval->interval.denominator = fps;
>  
>  	mutex_unlock(&sensor->mutex);
>  
> @@ -2777,41 +2770,6 @@ static int ov8865_enum_frame_size(struct v4l2_subdev *subdev,
>  	return 0;
>  }
>  
> -static int ov8865_enum_frame_interval(struct v4l2_subdev *subdev,
> -				      struct v4l2_subdev_state *sd_state,
> -				      struct v4l2_subdev_frame_interval_enum *interval_enum)
> -{
> -	const struct ov8865_mode *mode = NULL;
> -	unsigned int mode_index;
> -	unsigned int interval_index;
> -
> -	if (interval_enum->index > 0)
> -		return -EINVAL;
> -	/*
> -	 * Multiple modes with the same dimensions may have different frame
> -	 * intervals, so look up each relevant mode.
> -	 */
> -	for (mode_index = 0, interval_index = 0;
> -	     mode_index < ARRAY_SIZE(ov8865_modes); mode_index++) {
> -		mode = &ov8865_modes[mode_index];
> -
> -		if (mode->output_size_x == interval_enum->width &&
> -		    mode->output_size_y == interval_enum->height) {
> -			if (interval_index == interval_enum->index)
> -				break;
> -
> -			interval_index++;
> -		}
> -	}
> -
> -	if (mode_index == ARRAY_SIZE(ov8865_modes))
> -		return -EINVAL;
> -
> -	interval_enum->interval = mode->frame_interval;
> -
> -	return 0;
> -}
> -
>  static void
>  __ov8865_get_pad_crop(struct ov8865_sensor *sensor,
>  		      struct v4l2_subdev_state *state, unsigned int pad,
> @@ -2870,7 +2828,6 @@ static const struct v4l2_subdev_pad_ops ov8865_subdev_pad_ops = {
>  	.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,
>  	.set_selection		= ov8865_get_selection,
>  };

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v4 09/16] media: i2c: Update HTS values in ov8865

[Daniel Scally]

Hi Sakari

On 01/11/2021 15:04, Sakari Ailus wrote:
> Hi Daniel,
>
> On Mon, Nov 01, 2021 at 12:11:12AM +0000, Daniel Scally wrote:
>> The HTS values for some of the modes in the ov8865 driver are a bit
>> unusual, coming in lower than the output_size_x is set to. It seems
>> like they might be calculated to fit the desired framerate into a
>> configuration with just two data lanes. To bring this more in line
>> with expected behaviour, raise the HTS values above the output_size_x.
> This isn't necessarily a problem as binning may be done in analogue domain.
> I don't know about this sensor though.
>
> Is the frame interval still as expected in binned modes after this patch?


Actually none of the modes have binning in the X dimension set on. The
ones with Y binning still have the framerates I expect, and the only
modes who's HTS values I'm changing here don't use binning at all

>
>> The corollary of that change is that the hardcoded frame intervals
>> against the modes no longer make sense, so remove those entirely.
>> Update the .g/s_frame_interval() callbacks to calculate the frame
>> interval based on the current mode and the vblank and hblank settings.
>>
>> The implementation of the .enum_frame_interval() callback is no longer
>> suitable since the possible frame rate is now a continuous range depending
>> on the vblank control setting, so remove that callback entirely.
>>
>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>> ---
>>  drivers/media/i2c/ov8865.c | 65 +++++++-------------------------------
>>  1 file changed, 11 insertions(+), 54 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
>> index 4b18cc80f985..1b8674152750 100644
>> --- a/drivers/media/i2c/ov8865.c
>> +++ b/drivers/media/i2c/ov8865.c
>> @@ -659,8 +659,6 @@ struct ov8865_mode {
>>  	unsigned int blc_anchor_right_start;
>>  	unsigned int blc_anchor_right_end;
>>  
>> -	struct v4l2_fract frame_interval;
>> -
>>  	bool pll2_binning;
>>  
>>  	const struct ov8865_register_value *register_values;
>> @@ -964,7 +962,7 @@ static const struct ov8865_mode ov8865_modes[] = {
>>  	{
>>  		/* Horizontal */
>>  		.output_size_x			= 3264,
>> -		.hts				= 1944,
>> +		.hts				= 3888,
>>  
>>  		/* Vertical */
>>  		.output_size_y			= 2448,
>> @@ -1003,9 +1001,6 @@ static const struct ov8865_mode ov8865_modes[] = {
>>  		.blc_anchor_right_start		= 1984,
>>  		.blc_anchor_right_end		= 2239,
>>  
>> -		/* Frame Interval */
>> -		.frame_interval			= { 1, 30 },
>> -
>>  		/* PLL */
>>  		.pll2_binning			= false,
>>  
>> @@ -1018,11 +1013,11 @@ static const struct ov8865_mode ov8865_modes[] = {
>>  	{
>>  		/* Horizontal */
>>  		.output_size_x			= 3264,
>> -		.hts				= 2582,
>> +		.hts				= 3888,
>>  
>>  		/* Vertical */
>>  		.output_size_y			= 1836,
>> -		.vts				= 2002,
>> +		.vts				= 2470,
>>  
>>  		.size_auto			= true,
>>  		.size_auto_boundary_x		= 8,
>> @@ -1057,9 +1052,6 @@ static const struct ov8865_mode ov8865_modes[] = {
>>  		.blc_anchor_right_start		= 1984,
>>  		.blc_anchor_right_end		= 2239,
>>  
>> -		/* Frame Interval */
>> -		.frame_interval			= { 1, 30 },
>> -
>>  		/* PLL */
>>  		.pll2_binning			= false,
>>  
>> @@ -1115,9 +1107,6 @@ static const struct ov8865_mode ov8865_modes[] = {
>>  		.blc_anchor_right_start		= 992,
>>  		.blc_anchor_right_end		= 1119,
>>  
>> -		/* Frame Interval */
>> -		.frame_interval			= { 1, 30 },
>> -
>>  		/* PLL */
>>  		.pll2_binning			= true,
>>  
>> @@ -1179,9 +1168,6 @@ static const struct ov8865_mode ov8865_modes[] = {
>>  		.blc_anchor_right_start		= 992,
>>  		.blc_anchor_right_end		= 1119,
>>  
>> -		/* Frame Interval */
>> -		.frame_interval			= { 1, 90 },
>> -
>>  		/* PLL */
>>  		.pll2_binning			= true,
>>  
>> @@ -2628,11 +2614,18 @@ static int ov8865_g_frame_interval(struct v4l2_subdev *subdev,
>>  {
>>  	struct ov8865_sensor *sensor = ov8865_subdev_sensor(subdev);
>>  	const struct ov8865_mode *mode;
>> +	unsigned int framesize;
>> +	unsigned int fps;
>>  
>>  	mutex_lock(&sensor->mutex);
>>  
>>  	mode = sensor->state.mode;
>> -	interval->interval = mode->frame_interval;
>> +	framesize = mode->hts * (mode->output_size_y +
>> +				 sensor->ctrls.vblank->val);
>> +	fps = DIV_ROUND_CLOSEST(sensor->ctrls.pixel_rate->val, framesize);
>> +
>> +	interval->interval.numerator = 1;
>> +	interval->interval.denominator = fps;
>>  
>>  	mutex_unlock(&sensor->mutex);
>>  
>> @@ -2777,41 +2770,6 @@ static int ov8865_enum_frame_size(struct v4l2_subdev *subdev,
>>  	return 0;
>>  }
>>  
>> -static int ov8865_enum_frame_interval(struct v4l2_subdev *subdev,
>> -				      struct v4l2_subdev_state *sd_state,
>> -				      struct v4l2_subdev_frame_interval_enum *interval_enum)
>> -{
>> -	const struct ov8865_mode *mode = NULL;
>> -	unsigned int mode_index;
>> -	unsigned int interval_index;
>> -
>> -	if (interval_enum->index > 0)
>> -		return -EINVAL;
>> -	/*
>> -	 * Multiple modes with the same dimensions may have different frame
>> -	 * intervals, so look up each relevant mode.
>> -	 */
>> -	for (mode_index = 0, interval_index = 0;
>> -	     mode_index < ARRAY_SIZE(ov8865_modes); mode_index++) {
>> -		mode = &ov8865_modes[mode_index];
>> -
>> -		if (mode->output_size_x == interval_enum->width &&
>> -		    mode->output_size_y == interval_enum->height) {
>> -			if (interval_index == interval_enum->index)
>> -				break;
>> -
>> -			interval_index++;
>> -		}
>> -	}
>> -
>> -	if (mode_index == ARRAY_SIZE(ov8865_modes))
>> -		return -EINVAL;
>> -
>> -	interval_enum->interval = mode->frame_interval;
>> -
>> -	return 0;
>> -}
>> -
>>  static void
>>  __ov8865_get_pad_crop(struct ov8865_sensor *sensor,
>>  		      struct v4l2_subdev_state *state, unsigned int pad,
>> @@ -2870,7 +2828,6 @@ static const struct v4l2_subdev_pad_ops ov8865_subdev_pad_ops = {
>>  	.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,
>>  	.set_selection		= ov8865_get_selection,
>>  };

[PATCH v4 10/16] 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>
---
 drivers/media/i2c/ov8865.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
index 1b8674152750..99548ad15dcd 100644
--- a/drivers/media/i2c/ov8865.c
+++ b/drivers/media/i2c/ov8865.c
@@ -677,6 +677,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;
 };
@@ -2454,6 +2455,19 @@ 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;
@@ -2510,8 +2524,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 */
 
@@ -2700,6 +2714,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);
@@ -2747,6 +2762,13 @@ 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

[PATCH v4 11/16] 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>
---
 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 99548ad15dcd..dfb5095ef16b 100644
--- a/drivers/media/i2c/ov8865.c
+++ b/drivers/media/i2c/ov8865.c
@@ -2513,6 +2513,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;
@@ -2576,6 +2577,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 v4 12/16] 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>
---
 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 dfb5095ef16b..5f19d82554df 100644
--- a/drivers/media/i2c/ov8865.c
+++ b/drivers/media/i2c/ov8865.c
@@ -2125,6 +2125,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)
@@ -2525,8 +2528,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 v4 13/16] media: i2c: Re-order runtime pm initialisation

[Daniel Scally]

The kerneldoc for pm_runtime_set_suspended() says:

"It is not valid to call this function for devices with runtime PM
enabled"

To satisfy that requirement, re-order the calls so that
pm_runtime_enable() is the last one.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
 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 5f19d82554df..18b5f1e8e9a7 100644
--- a/drivers/media/i2c/ov8865.c
+++ b/drivers/media/i2c/ov8865.c
@@ -3085,8 +3085,8 @@ static int ov8865_probe(struct i2c_client *client)
 
 	/* Runtime PM */
 
-	pm_runtime_enable(sensor->dev);
 	pm_runtime_set_suspended(sensor->dev);
+	pm_runtime_enable(sensor->dev);
 
 	/* V4L2 subdev register */
 
-- 
2.25.1

Re: [PATCH v4 13/16] media: i2c: Re-order runtime pm initialisation

[Andy Shevchenko]

On Mon, Nov 01, 2021 at 12:11:16AM +0000, Daniel Scally wrote:
> The kerneldoc for pm_runtime_set_suspended() says:
> 
> "It is not valid to call this function for devices with runtime PM
> enabled"
> 
> To satisfy that requirement, re-order the calls so that
> pm_runtime_enable() is the last one.

Shouldn't this be a fix with Fixes tag applied?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 13/16] media: i2c: Re-order runtime pm initialisation

[Daniel Scally]

Hi Andy

On 01/11/2021 11:30, Andy Shevchenko wrote:
> On Mon, Nov 01, 2021 at 12:11:16AM +0000, Daniel Scally wrote:
>> The kerneldoc for pm_runtime_set_suspended() says:
>>
>> "It is not valid to call this function for devices with runtime PM
>> enabled"
>>
>> To satisfy that requirement, re-order the calls so that
>> pm_runtime_enable() is the last one.
> Shouldn't this be a fix with Fixes tag applied?
>
Yes I guess so - thanks, I'll add it

[PATCH v4 14/16] media: i2c: Use dev_err_probe() in ov8865

[Daniel Scally]

There is a chance that regulator_get() returns -EPROBE_DEFER, in which
case printing an error message is undesirable. To avoid spurious messages
in dmesg in the event that -EPROBE_DEFER is returned, use dev_err_probe()
on error paths for regulator_get().

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
 drivers/media/i2c/ov8865.c | 46 +++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 26 deletions(-)

diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
index 18b5f1e8e9a7..ab225fb616b9 100644
--- a/drivers/media/i2c/ov8865.c
+++ b/drivers/media/i2c/ov8865.c
@@ -2955,6 +2955,26 @@ static int ov8865_probe(struct i2c_client *client)
 	sensor->dev = dev;
 	sensor->i2c_client = client;
 
+	/* Regulators */
+
+	/* DVDD: digital core */
+	sensor->dvdd = devm_regulator_get(dev, "dvdd");
+	if (IS_ERR(sensor->dvdd))
+		return dev_err_probe(dev, PTR_ERR(sensor->dvdd),
+				     "cannot get DVDD regulator\n");
+
+	/* DOVDD: digital I/O */
+	sensor->dovdd = devm_regulator_get(dev, "dovdd");
+	if (IS_ERR(sensor->dovdd))
+		return dev_err_probe(dev, PTR_ERR(sensor->dovdd),
+				     "cannot get DOVDD regulator\n");
+
+	/* AVDD: analog */
+	sensor->avdd = devm_regulator_get(dev, "avdd");
+	if (IS_ERR(sensor->avdd))
+		return dev_err_probe(dev, PTR_ERR(sensor->avdd),
+				     "cannot get AVDD (analog) regulator\n");
+
 	/* Graph Endpoint */
 
 	handle = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
@@ -2985,32 +3005,6 @@ static int ov8865_probe(struct i2c_client *client)
 		goto error_endpoint;
 	}
 
-	/* Regulators */
-
-	/* DVDD: digital core */
-	sensor->dvdd = devm_regulator_get(dev, "dvdd");
-	if (IS_ERR(sensor->dvdd)) {
-		dev_err(dev, "cannot get DVDD (digital core) regulator\n");
-		ret = PTR_ERR(sensor->dvdd);
-		goto error_endpoint;
-	}
-
-	/* DOVDD: digital I/O */
-	sensor->dovdd = devm_regulator_get(dev, "dovdd");
-	if (IS_ERR(sensor->dovdd)) {
-		dev_err(dev, "cannot get DOVDD (digital I/O) regulator\n");
-		ret = PTR_ERR(sensor->dovdd);
-		goto error_endpoint;
-	}
-
-	/* AVDD: analog */
-	sensor->avdd = devm_regulator_get(dev, "avdd");
-	if (IS_ERR(sensor->avdd)) {
-		dev_err(dev, "cannot get AVDD (analog) regulator\n");
-		ret = PTR_ERR(sensor->avdd);
-		goto error_endpoint;
-	}
-
 	/* External Clock */
 
 	sensor->extclk = devm_clk_get(dev, NULL);
-- 
2.25.1

[PATCH v4 15/16] 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.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
 drivers/media/pci/intel/ipu3/cio2-bridge.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c
index 59a36f922675..fb7d708b16e2 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

[PATCH v4 16/16] media: i2c: ov8865: Fix lockdep error

[Daniel Scally]

From: Hans de Goede <hdegoede@redhat.com>

ov8865_state_init() calls ov8865_state_mipi_configure() which uses
__v4l2_ctrl_s_ctrl[_int64](). This means that sensor->mutex (which
is also sensor->ctrls.handler.lock) must be locked before calling
ov8865_state_init().

Note ov8865_state_mipi_configure() is also used in other places where
the lock is already held so it cannot be changed itself.

This fixes the following lockdep kernel WARN:

[   13.233413] ------------[ cut here ]------------
[   13.233421] WARNING: CPU: 0 PID: 8 at drivers/media/v4l2-core/v4l2-ctrls-api.c:833 __v4l2_ctrl_s_ctrl+0x4d/0x60 [videodev]
...
[   13.234063] Call Trace:
[   13.234074]  ov8865_state_configure+0x98b/0xc00 [ov8865]
[   13.234095]  ov8865_probe+0x4b1/0x54c [ov8865]
[   13.234117]  i2c_device_probe+0x13c/0x2d0

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

diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
index ab225fb616b9..c5fe290317e8 100644
--- a/drivers/media/i2c/ov8865.c
+++ b/drivers/media/i2c/ov8865.c
@@ -3073,7 +3073,9 @@ static int ov8865_probe(struct i2c_client *client)
 	if (ret)
 		goto error_mutex;
 
+	mutex_lock(&sensor->mutex);
 	ret = ov8865_state_init(sensor);
+	mutex_unlock(&sensor->mutex);
 	if (ret)
 		goto error_ctrls;
 
-- 
2.25.1

Re: [PATCH v4 16/16] media: i2c: ov8865: Fix lockdep error

[Andy Shevchenko]

On Mon, Nov 01, 2021 at 12:11:19AM +0000, Daniel Scally wrote:
> From: Hans de Goede <hdegoede@redhat.com>
> 
> ov8865_state_init() calls ov8865_state_mipi_configure() which uses
> __v4l2_ctrl_s_ctrl[_int64](). This means that sensor->mutex (which
> is also sensor->ctrls.handler.lock) must be locked before calling
> ov8865_state_init().
> 
> Note ov8865_state_mipi_configure() is also used in other places where
> the lock is already held so it cannot be changed itself.
> 
> This fixes the following lockdep kernel WARN:

> [   13.233413] ------------[ cut here ]------------

You may drop this noisy line.

> [   13.233421] WARNING: CPU: 0 PID: 8 at drivers/media/v4l2-core/v4l2-ctrls-api.c:833 __v4l2_ctrl_s_ctrl+0x4d/0x60 [videodev]
> ...
> [   13.234063] Call Trace:
> [   13.234074]  ov8865_state_configure+0x98b/0xc00 [ov8865]
> [   13.234095]  ov8865_probe+0x4b1/0x54c [ov8865]
> [   13.234117]  i2c_device_probe+0x13c/0x2d0

Seems to me that this should be moved to the head of the series with Fixes tag
added.

-- 
With Best Regards,
Andy Shevchenko