[PATCH v2 0/7] [PATCH v2 0/7] Add support of OV9655 camera

[PATCH v2 0/7] [PATCH v2 0/7] Add support of OV9655 camera

[Hugues Fruchet]

This patchset enables OV9655 camera support.

OV9655 support has been tested using STM32F4DIS-CAM extension board
plugged on connector P1 of STM32F746G-DISCO board.
Due to lack of OV9650/52 hardware support, the modified related code
could not have been checked for non-regression.

First patches upgrade current support of OV9650/52 to prepare then
introduction of OV9655 variant patch.
Because of OV9655 register set slightly different from OV9650/9652,
not all of the driver features are supported (controls). Supported
resolutions are limited to VGA, QVGA, QQVGA.
Supported format is limited to RGB565.
Controls are limited to color bar test pattern for test purpose.

OV9655 initial support is based on a driver written by H. Nikolaus Schaller [1].
OV9655 registers sequences come from STM32CubeF7 embedded software [2].

[1] http://git.goldelico.com/?p=gta04-kernel.git;a=shortlog;h=refs/heads/work/hns/video/ov9655
[2] https://developer.mbed.org/teams/ST/code/BSP_DISCO_F746NG/file/e1d9da7fe856/Drivers/BSP/Components/ov9655/ov9655.c

===========
= history =
===========
version 2:
  - Remove some unneeded semicolons (kbuild test robot):
      http://www.mail-archive.com/linux-media at vger.kernel.org/msg114616.html
  - Remove patch [media] ov9650: select the nearest higher resolution:
    it is up to the application to find the best matching resolution
    using ENUM_FRAMESIZES/S_FMT/S_SELECTION (S_CROP), see
      http://www.mail-archive.com/linux-media at vger.kernel.org/msg114667.html
  - dt-bindings: Fix remarks from Rob Herring about polarity:
      http://www.mail-archive.com/linux-media at vger.kernel.org/msg114705.html
  - dt-bindings: Add optional regulators avdd, dvdd, dovdd:
      http://www.mail-archive.com/linux-media at vger.kernel.org/msg114785.html
  - fix missing semicolons in if condition:
      http://www.mail-archive.com/linux-media at vger.kernel.org/msg114611.html
  - move ov965x_pixfmt relocation in right patch:
      http://www.mail-archive.com/linux-media at vger.kernel.org/msg114849.html
  - revisit OV965x renaming to ov965x for device id names and DT compatible strings,
    drop of_device_id .data device identification
      http://www.mail-archive.com/linux-media at vger.kernel.org/msg114635.html
      http://www.mail-archive.com/linux-media at vger.kernel.org/msg114738.html
  - Add analog power supply and clock gating, needed for GTA04 platform:
      http://www.mail-archive.com/linux-media at vger.kernel.org/msg114519.html

version 1:
  - Initial submission.

H. Nikolaus Schaller (1):
  DT bindings: add bindings for ov965x camera module

Hugues Fruchet (6):
  [media] ov9650: switch i2c device id to lower case
  [media] ov9650: add device tree support
  [media] ov9650: use write_array() for resolution sequences
  [media] ov9650: add multiple variant support
  [media] ov9650: add support of OV9655 variant
  [media] ov9650: add analog power supply and clock gating

 .../devicetree/bindings/media/i2c/ov965x.txt       |  45 ++
 drivers/media/i2c/Kconfig                          |   6 +-
 drivers/media/i2c/ov9650.c                         | 816 +++++++++++++++++----
 3 files changed, 736 insertions(+), 131 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov965x.txt

-- 
1.9.1

[PATCH v2 1/7] DT bindings: add bindings for ov965x camera module

[Hugues Fruchet]

From: "H. Nikolaus Schaller" <hns@goldelico.com>

This adds documentation of device tree bindings
for the OV965X family camera sensor module.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
---
 .../devicetree/bindings/media/i2c/ov965x.txt       | 45 ++++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov965x.txt

diff --git a/Documentation/devicetree/bindings/media/i2c/ov965x.txt b/Documentation/devicetree/bindings/media/i2c/ov965x.txt
new file mode 100644
index 0000000..4ceb727
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ov965x.txt
@@ -0,0 +1,45 @@
+* Omnivision OV9650/9652/9655 CMOS sensor
+
+The Omnivision OV965x sensor support multiple resolutions output, such as
+CIF, SVGA, UXGA. It also can support YUV422/420, RGB565/555 or raw RGB
+output format.
+
+Required Properties:
+- compatible: should be one of
+	"ovti,ov9650"
+	"ovti,ov9652"
+	"ovti,ov9655"
+- clocks: reference to the mclk input clock.
+
+Optional Properties:
+- resetb-gpios: reference to the GPIO connected to the RESETB pin, if any,
+		polarity is active low.
+- pwdn-gpios: reference to the GPIO connected to the PWDN pin, if any,
+		polarity is active high.
+- avdd-supply: reference to the analog power supply regulator connected
+		to the AVDD pin, if any.
+- dvdd-supply: reference to the digital power supply regulator connected
+		to the DVDD pin, if any.
+- dovdd-supply: reference to the digital I/O power supply regulator
+		connected to the DOVDD pin, if any.
+
+The device node must contain one 'port' child node for its digital output
+video port, in accordance with the video interface bindings defined in
+Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+Example:
+
+&i2c2 {
+	ov9655: camera at 30 {
+		compatible = "ovti,ov9655";
+		reg = <0x30>;
+		pwdn-gpios = <&gpioh 13 GPIO_ACTIVE_HIGH>;
+		clocks = <&clk_ext_camera>;
+
+		port {
+			ov9655: endpoint {
+				remote-endpoint = <&dcmi_0>;
+			};
+		};
+	};
+};
-- 
1.9.1

[PATCH v2 1/7] DT bindings: add bindings for ov965x camera module

[Rob Herring]

On Mon, Jul 03, 2017 at 11:16:02AM +0200, Hugues Fruchet wrote:
> From: "H. Nikolaus Schaller" <hns@goldelico.com>
> 
> This adds documentation of device tree bindings
> for the OV965X family camera sensor module.
> 
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> ---
>  .../devicetree/bindings/media/i2c/ov965x.txt       | 45 ++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov965x.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov965x.txt b/Documentation/devicetree/bindings/media/i2c/ov965x.txt
> new file mode 100644
> index 0000000..4ceb727
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov965x.txt
> @@ -0,0 +1,45 @@
> +* Omnivision OV9650/9652/9655 CMOS sensor
> +
> +The Omnivision OV965x sensor support multiple resolutions output, such as
> +CIF, SVGA, UXGA. It also can support YUV422/420, RGB565/555 or raw RGB
> +output format.
> +
> +Required Properties:
> +- compatible: should be one of
> +	"ovti,ov9650"
> +	"ovti,ov9652"
> +	"ovti,ov9655"
> +- clocks: reference to the mclk input clock.
> +
> +Optional Properties:
> +- resetb-gpios: reference to the GPIO connected to the RESETB pin, if any,
> +		polarity is active low.

reset-gpios

> +- pwdn-gpios: reference to the GPIO connected to the PWDN pin, if any,
> +		polarity is active high.

powerdown-gpios

Both are standardish names for such signals.

Rob

[PATCH v2 1/7] DT bindings: add bindings for ov965x camera module

[Hugues FRUCHET]

On 07/05/2017 04:03 PM, Rob Herring wrote:
> On Mon, Jul 03, 2017 at 11:16:02AM +0200, Hugues Fruchet wrote:
>> From: "H. Nikolaus Schaller" <hns@goldelico.com>
>>
>> This adds documentation of device tree bindings
>> for the OV965X family camera sensor module.
>>
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
>> ---
>>   .../devicetree/bindings/media/i2c/ov965x.txt       | 45 ++++++++++++++++++++++
>>   1 file changed, 45 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/media/i2c/ov965x.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov965x.txt b/Documentation/devicetree/bindings/media/i2c/ov965x.txt
>> new file mode 100644
>> index 0000000..4ceb727
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/ov965x.txt
>> @@ -0,0 +1,45 @@
>> +* Omnivision OV9650/9652/9655 CMOS sensor
>> +
>> +The Omnivision OV965x sensor support multiple resolutions output, such as
>> +CIF, SVGA, UXGA. It also can support YUV422/420, RGB565/555 or raw RGB
>> +output format.
>> +
>> +Required Properties:
>> +- compatible: should be one of
>> +	"ovti,ov9650"
>> +	"ovti,ov9652"
>> +	"ovti,ov9655"
>> +- clocks: reference to the mclk input clock.
>> +
>> +Optional Properties:
>> +- resetb-gpios: reference to the GPIO connected to the RESETB pin, if any,
>> +		polarity is active low.
> 
> reset-gpios
> 
>> +- pwdn-gpios: reference to the GPIO connected to the PWDN pin, if any,
>> +		polarity is active high.
> 
> powerdown-gpios
> 
> Both are standardish names for such signals.
> 
> Rob

Thanks Rob, I will fix,
Hugues.

> 

[PATCH v2 2/7] [media] ov9650: switch i2c device id to lower case

[Hugues Fruchet]

Switch i2c device id to lower case as it is
done for other omnivision cameras.

Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
---
 drivers/media/i2c/ov9650.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c
index 2de2fbb..1e4e99e 100644
--- a/drivers/media/i2c/ov9650.c
+++ b/drivers/media/i2c/ov9650.c
@@ -1545,8 +1545,8 @@ static int ov965x_remove(struct i2c_client *client)
 }
 
 static const struct i2c_device_id ov965x_id[] = {
-	{ "OV9650", 0 },
-	{ "OV9652", 0 },
+	{ "ov9650", 0 },
+	{ "ov9652", 0 },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(i2c, ov965x_id);
-- 
1.9.1

[PATCH v2 2/7] [media] ov9650: switch i2c device id to lower case

[Sylwester Nawrocki]

On 07/03/2017 11:16 AM, Hugues Fruchet wrote:
> Switch i2c device id to lower case as it is

s/i2c/I2C ?

> done for other omnivision cameras.

s/omnivision/Omnivision

This is required for properly matching driver with device on DT platforms,
right? It might be worth to mention that so it is clear why we break any
non-dt platform that could be already using this driver. There seem to be 
none in the mainline kernel tree though.

> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>

Reviewed-by: Sylwester Nawrocki <snawrocki@kernel.org>

> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> ---
>   drivers/media/i2c/ov9650.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c
> index 2de2fbb..1e4e99e 100644
> --- a/drivers/media/i2c/ov9650.c
> +++ b/drivers/media/i2c/ov9650.c
> @@ -1545,8 +1545,8 @@ static int ov965x_remove(struct i2c_client *client)
>   }
>   
>   static const struct i2c_device_id ov965x_id[] = {
> -	{ "OV9650", 0 },
> -	{ "OV9652", 0 },
> +	{ "ov9650", 0 },
> +	{ "ov9652", 0 },
>   	{ /* sentinel */ }
>   };
>   MODULE_DEVICE_TABLE(i2c, ov965x_id);
 

[PATCH v2 3/7] [media] ov9650: add device tree support

[Hugues Fruchet]

Allows use of device tree configuration data.
If no device tree data is there, configuration is taken from platform data.
In order to keep GPIOs configuration compatible between both way of doing,
GPIOs are switched to descriptor-based interface.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
---
 drivers/media/i2c/Kconfig  |  2 +-
 drivers/media/i2c/ov9650.c | 77 ++++++++++++++++++++++++++++++++++------------
 2 files changed, 59 insertions(+), 20 deletions(-)

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 121b3b5..168115c 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -615,7 +615,7 @@ config VIDEO_OV7670
 
 config VIDEO_OV9650
 	tristate "OmniVision OV9650/OV9652 sensor support"
-	depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+	depends on GPIOLIB && I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
 	---help---
 	  This is a V4L2 sensor-level driver for the Omnivision
 	  OV9650 and OV9652 camera sensors.
diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c
index 1e4e99e..7e9a902 100644
--- a/drivers/media/i2c/ov9650.c
+++ b/drivers/media/i2c/ov9650.c
@@ -11,12 +11,14 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
+#include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/gpio.h>
 #include <linux/i2c.h>
 #include <linux/kernel.h>
 #include <linux/media.h>
 #include <linux/module.h>
+#include <linux/of_gpio.h>
 #include <linux/ratelimit.h>
 #include <linux/slab.h>
 #include <linux/string.h>
@@ -249,9 +251,10 @@ struct ov965x {
 	struct v4l2_subdev sd;
 	struct media_pad pad;
 	enum v4l2_mbus_type bus_type;
-	int gpios[NUM_GPIOS];
+	struct gpio_desc *gpios[NUM_GPIOS];
 	/* External master clock frequency */
 	unsigned long mclk_frequency;
+	struct clk *clk;
 
 	/* Protects the struct fields below */
 	struct mutex lock;
@@ -511,10 +514,10 @@ static int ov965x_set_color_matrix(struct ov965x *ov965x)
 	return 0;
 }
 
-static void ov965x_gpio_set(int gpio, int val)
+static void ov965x_gpio_set(struct gpio_desc *gpio, int val)
 {
-	if (gpio_is_valid(gpio))
-		gpio_set_value(gpio, val);
+	if (gpio)
+		gpiod_set_value_cansleep(gpio, val);
 }
 
 static void __ov965x_set_power(struct ov965x *ov965x, int on)
@@ -1406,24 +1409,28 @@ static int ov965x_configure_gpios(struct ov965x *ov965x,
 				  const struct ov9650_platform_data *pdata)
 {
 	int ret, i;
+	int gpios[NUM_GPIOS];
 
-	ov965x->gpios[GPIO_PWDN] = pdata->gpio_pwdn;
-	ov965x->gpios[GPIO_RST]  = pdata->gpio_reset;
+	gpios[GPIO_PWDN] = pdata->gpio_pwdn;
+	gpios[GPIO_RST]  = pdata->gpio_reset;
 
-	for (i = 0; i < ARRAY_SIZE(ov965x->gpios); i++) {
-		int gpio = ov965x->gpios[i];
+	for (i = 0; i < ARRAY_SIZE(gpios); i++) {
+		int gpio = gpios[i];
 
 		if (!gpio_is_valid(gpio))
 			continue;
 		ret = devm_gpio_request_one(&ov965x->client->dev, gpio,
-					    GPIOF_OUT_INIT_HIGH, "OV965X");
-		if (ret < 0)
+					    GPIOF_OUT_INIT_HIGH, DRIVER_NAME);
+		if (ret < 0) {
+			dev_err(&ov965x->client->dev,
+				"Failed to request gpio%d (%d)\n", gpio, ret);
 			return ret;
+		}
 		v4l2_dbg(1, debug, &ov965x->sd, "set gpio %d to 1\n", gpio);
 
 		gpio_set_value(gpio, 1);
 		gpio_export(gpio, 0);
-		ov965x->gpios[i] = gpio;
+		ov965x->gpios[i] = gpio_to_desc(gpio);
 	}
 
 	return 0;
@@ -1469,14 +1476,10 @@ static int ov965x_probe(struct i2c_client *client,
 	struct v4l2_subdev *sd;
 	struct ov965x *ov965x;
 	int ret;
+	struct device_node *np = client->dev.of_node;
 
-	if (pdata == NULL) {
-		dev_err(&client->dev, "platform data not specified\n");
-		return -EINVAL;
-	}
-
-	if (pdata->mclk_frequency == 0) {
-		dev_err(&client->dev, "MCLK frequency not specified\n");
+	if (!pdata && !np) {
+		dev_err(&client->dev, "Platform data or device tree data must be provided\n");
 		return -EINVAL;
 	}
 
@@ -1486,7 +1489,35 @@ static int ov965x_probe(struct i2c_client *client,
 
 	mutex_init(&ov965x->lock);
 	ov965x->client = client;
-	ov965x->mclk_frequency = pdata->mclk_frequency;
+	mutex_init(&ov965x->lock);
+
+	if (np) {
+		/* Device tree */
+		ov965x->gpios[GPIO_RST] =
+			devm_gpiod_get_optional(&client->dev, "resetb",
+						GPIOD_OUT_LOW);
+		ov965x->gpios[GPIO_PWDN] =
+			devm_gpiod_get_optional(&client->dev, "pwdn",
+						GPIOD_OUT_HIGH);
+
+		ov965x->clk = devm_clk_get(&client->dev, NULL);
+		if (IS_ERR(ov965x->clk)) {
+			dev_err(&client->dev, "Could not get clock\n");
+			return PTR_ERR(ov965x->clk);
+		}
+		ov965x->mclk_frequency = clk_get_rate(ov965x->clk);
+	} else {
+		/* Platform data */
+		ret = ov965x_configure_gpios(ov965x, pdata);
+		if (ret < 0)
+			return ret;
+
+		if (pdata->mclk_frequency == 0) {
+			dev_err(&client->dev, "MCLK frequency is mandatory\n");
+			return -EINVAL;
+		}
+		ov965x->mclk_frequency = pdata->mclk_frequency;
+	}
 
 	sd = &ov965x->sd;
 	v4l2_i2c_subdev_init(sd, client, &ov965x_subdev_ops);
@@ -1551,9 +1582,17 @@ static int ov965x_remove(struct i2c_client *client)
 };
 MODULE_DEVICE_TABLE(i2c, ov965x_id);
 
+static const struct of_device_id ov965x_of_match[] = {
+	{ .compatible = "ovti,ov9650", },
+	{ .compatible = "ovti,ov9652", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ov965x_of_match);
+
 static struct i2c_driver ov965x_i2c_driver = {
 	.driver = {
 		.name	= DRIVER_NAME,
+		.of_match_table = of_match_ptr(ov965x_of_match),
 	},
 	.probe		= ov965x_probe,
 	.remove		= ov965x_remove,
-- 
1.9.1

[PATCH v2 3/7] [media] ov9650: add device tree support

[Sakari Ailus]

Hi Hugues,

On Mon, Jul 03, 2017 at 11:16:04AM +0200, Hugues Fruchet wrote:
> Allows use of device tree configuration data.
> If no device tree data is there, configuration is taken from platform data.
> In order to keep GPIOs configuration compatible between both way of doing,
> GPIOs are switched to descriptor-based interface.
> 
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> ---
>  drivers/media/i2c/Kconfig  |  2 +-
>  drivers/media/i2c/ov9650.c | 77 ++++++++++++++++++++++++++++++++++------------
>  2 files changed, 59 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 121b3b5..168115c 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -615,7 +615,7 @@ config VIDEO_OV7670
>  
>  config VIDEO_OV9650
>  	tristate "OmniVision OV9650/OV9652 sensor support"
> -	depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> +	depends on GPIOLIB && I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
>  	---help---
>  	  This is a V4L2 sensor-level driver for the Omnivision
>  	  OV9650 and OV9652 camera sensors.
> diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c
> index 1e4e99e..7e9a902 100644
> --- a/drivers/media/i2c/ov9650.c
> +++ b/drivers/media/i2c/ov9650.c
> @@ -11,12 +11,14 @@
>   * it under the terms of the GNU General Public License version 2 as
>   * published by the Free Software Foundation.
>   */
> +#include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/gpio.h>
>  #include <linux/i2c.h>
>  #include <linux/kernel.h>
>  #include <linux/media.h>
>  #include <linux/module.h>
> +#include <linux/of_gpio.h>
>  #include <linux/ratelimit.h>
>  #include <linux/slab.h>
>  #include <linux/string.h>
> @@ -249,9 +251,10 @@ struct ov965x {
>  	struct v4l2_subdev sd;
>  	struct media_pad pad;
>  	enum v4l2_mbus_type bus_type;
> -	int gpios[NUM_GPIOS];
> +	struct gpio_desc *gpios[NUM_GPIOS];
>  	/* External master clock frequency */
>  	unsigned long mclk_frequency;
> +	struct clk *clk;
>  
>  	/* Protects the struct fields below */
>  	struct mutex lock;
> @@ -511,10 +514,10 @@ static int ov965x_set_color_matrix(struct ov965x *ov965x)
>  	return 0;
>  }
>  
> -static void ov965x_gpio_set(int gpio, int val)
> +static void ov965x_gpio_set(struct gpio_desc *gpio, int val)
>  {
> -	if (gpio_is_valid(gpio))
> -		gpio_set_value(gpio, val);
> +	if (gpio)
> +		gpiod_set_value_cansleep(gpio, val);

gpiod_set_value_cansleep() can manage with NULL gpio parameter, no need to
check it.

>  }
>  
>  static void __ov965x_set_power(struct ov965x *ov965x, int on)
> @@ -1406,24 +1409,28 @@ static int ov965x_configure_gpios(struct ov965x *ov965x,
>  				  const struct ov9650_platform_data *pdata)
>  {
>  	int ret, i;
> +	int gpios[NUM_GPIOS];
>  
> -	ov965x->gpios[GPIO_PWDN] = pdata->gpio_pwdn;
> -	ov965x->gpios[GPIO_RST]  = pdata->gpio_reset;
> +	gpios[GPIO_PWDN] = pdata->gpio_pwdn;
> +	gpios[GPIO_RST]  = pdata->gpio_reset;
>  
> -	for (i = 0; i < ARRAY_SIZE(ov965x->gpios); i++) {
> -		int gpio = ov965x->gpios[i];
> +	for (i = 0; i < ARRAY_SIZE(gpios); i++) {
> +		int gpio = gpios[i];
>  
>  		if (!gpio_is_valid(gpio))
>  			continue;
>  		ret = devm_gpio_request_one(&ov965x->client->dev, gpio,
> -					    GPIOF_OUT_INIT_HIGH, "OV965X");
> -		if (ret < 0)
> +					    GPIOF_OUT_INIT_HIGH, DRIVER_NAME);

DRIVER_NAME is different from "OV965X". Is this an intended change?

> +		if (ret < 0) {
> +			dev_err(&ov965x->client->dev,
> +				"Failed to request gpio%d (%d)\n", gpio, ret);
>  			return ret;
> +		}
>  		v4l2_dbg(1, debug, &ov965x->sd, "set gpio %d to 1\n", gpio);
>  
>  		gpio_set_value(gpio, 1);
>  		gpio_export(gpio, 0);
> -		ov965x->gpios[i] = gpio;
> +		ov965x->gpios[i] = gpio_to_desc(gpio);
>  	}
>  
>  	return 0;
> @@ -1469,14 +1476,10 @@ static int ov965x_probe(struct i2c_client *client,
>  	struct v4l2_subdev *sd;
>  	struct ov965x *ov965x;
>  	int ret;
> +	struct device_node *np = client->dev.of_node;

It'd be nice to declare this next to pdata, rather than after ret and other
short declarations.

>  
> -	if (pdata == NULL) {
> -		dev_err(&client->dev, "platform data not specified\n");
> -		return -EINVAL;
> -	}
> -
> -	if (pdata->mclk_frequency == 0) {
> -		dev_err(&client->dev, "MCLK frequency not specified\n");
> +	if (!pdata && !np) {
> +		dev_err(&client->dev, "Platform data or device tree data must be provided\n");
>  		return -EINVAL;
>  	}
>  
> @@ -1486,7 +1489,35 @@ static int ov965x_probe(struct i2c_client *client,
>  
>  	mutex_init(&ov965x->lock);
>  	ov965x->client = client;
> -	ov965x->mclk_frequency = pdata->mclk_frequency;
> +	mutex_init(&ov965x->lock);
> +
> +	if (np) {
> +		/* Device tree */
> +		ov965x->gpios[GPIO_RST] =
> +			devm_gpiod_get_optional(&client->dev, "resetb",
> +						GPIOD_OUT_LOW);
> +		ov965x->gpios[GPIO_PWDN] =
> +			devm_gpiod_get_optional(&client->dev, "pwdn",
> +						GPIOD_OUT_HIGH);
> +
> +		ov965x->clk = devm_clk_get(&client->dev, NULL);
> +		if (IS_ERR(ov965x->clk)) {
> +			dev_err(&client->dev, "Could not get clock\n");

mutex_destroy() should called on an initialised mutex if probe is going to
fail. It's certainly not a problem introduced by this patch, but it'd be
nice to fix that (in a separate patch) now that it's found. The same goes
for remove below.

> +			return PTR_ERR(ov965x->clk);
> +		}
> +		ov965x->mclk_frequency = clk_get_rate(ov965x->clk);
> +	} else {
> +		/* Platform data */
> +		ret = ov965x_configure_gpios(ov965x, pdata);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (pdata->mclk_frequency == 0) {
> +			dev_err(&client->dev, "MCLK frequency is mandatory\n");
> +			return -EINVAL;
> +		}
> +		ov965x->mclk_frequency = pdata->mclk_frequency;
> +	}
>  
>  	sd = &ov965x->sd;
>  	v4l2_i2c_subdev_init(sd, client, &ov965x_subdev_ops);
> @@ -1551,9 +1582,17 @@ static int ov965x_remove(struct i2c_client *client)
>  };
>  MODULE_DEVICE_TABLE(i2c, ov965x_id);
>  
> +static const struct of_device_id ov965x_of_match[] = {
> +	{ .compatible = "ovti,ov9650", },
> +	{ .compatible = "ovti,ov9652", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ov965x_of_match);
> +
>  static struct i2c_driver ov965x_i2c_driver = {
>  	.driver = {
>  		.name	= DRIVER_NAME,
> +		.of_match_table = of_match_ptr(ov965x_of_match),
>  	},
>  	.probe		= ov965x_probe,
>  	.remove		= ov965x_remove,

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus at iki.fi	XMPP: sailus at retiisi.org.uk

[PATCH v2 3/7] [media] ov9650: add device tree support

[Hugues FRUCHET]

Hi Sakari, thks for review.

On 07/09/2017 01:06 AM, Sakari Ailus wrote:
> Hi Hugues,
> 
> On Mon, Jul 03, 2017 at 11:16:04AM +0200, Hugues Fruchet wrote:
>> Allows use of device tree configuration data.
>> If no device tree data is there, configuration is taken from platform data.
>> In order to keep GPIOs configuration compatible between both way of doing,
>> GPIOs are switched to descriptor-based interface.
>>
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
>> ---
>>   drivers/media/i2c/Kconfig  |  2 +-
>>   drivers/media/i2c/ov9650.c | 77 ++++++++++++++++++++++++++++++++++------------
>>   2 files changed, 59 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>> index 121b3b5..168115c 100644
>> --- a/drivers/media/i2c/Kconfig
>> +++ b/drivers/media/i2c/Kconfig
>> @@ -615,7 +615,7 @@ config VIDEO_OV7670
>>   
>>   config VIDEO_OV9650
>>   	tristate "OmniVision OV9650/OV9652 sensor support"
>> -	depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
>> +	depends on GPIOLIB && I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
>>   	---help---
>>   	  This is a V4L2 sensor-level driver for the Omnivision
>>   	  OV9650 and OV9652 camera sensors.
>> diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c
>> index 1e4e99e..7e9a902 100644
>> --- a/drivers/media/i2c/ov9650.c
>> +++ b/drivers/media/i2c/ov9650.c
>> @@ -11,12 +11,14 @@
>>    * it under the terms of the GNU General Public License version 2 as
>>    * published by the Free Software Foundation.
>>    */
>> +#include <linux/clk.h>
>>   #include <linux/delay.h>
>>   #include <linux/gpio.h>
>>   #include <linux/i2c.h>
>>   #include <linux/kernel.h>
>>   #include <linux/media.h>
>>   #include <linux/module.h>
>> +#include <linux/of_gpio.h>
>>   #include <linux/ratelimit.h>
>>   #include <linux/slab.h>
>>   #include <linux/string.h>
>> @@ -249,9 +251,10 @@ struct ov965x {
>>   	struct v4l2_subdev sd;
>>   	struct media_pad pad;
>>   	enum v4l2_mbus_type bus_type;
>> -	int gpios[NUM_GPIOS];
>> +	struct gpio_desc *gpios[NUM_GPIOS];
>>   	/* External master clock frequency */
>>   	unsigned long mclk_frequency;
>> +	struct clk *clk;
>>   
>>   	/* Protects the struct fields below */
>>   	struct mutex lock;
>> @@ -511,10 +514,10 @@ static int ov965x_set_color_matrix(struct ov965x *ov965x)
>>   	return 0;
>>   }
>>   
>> -static void ov965x_gpio_set(int gpio, int val)
>> +static void ov965x_gpio_set(struct gpio_desc *gpio, int val)
>>   {
>> -	if (gpio_is_valid(gpio))
>> -		gpio_set_value(gpio, val);
>> +	if (gpio)
>> +		gpiod_set_value_cansleep(gpio, val);
> 
> gpiod_set_value_cansleep() can manage with NULL gpio parameter, no need to
> check it.

done

> 
>>   }
>>   
>>   static void __ov965x_set_power(struct ov965x *ov965x, int on)
>> @@ -1406,24 +1409,28 @@ static int ov965x_configure_gpios(struct ov965x *ov965x,
>>   				  const struct ov9650_platform_data *pdata)
>>   {
>>   	int ret, i;
>> +	int gpios[NUM_GPIOS];
>>   
>> -	ov965x->gpios[GPIO_PWDN] = pdata->gpio_pwdn;
>> -	ov965x->gpios[GPIO_RST]  = pdata->gpio_reset;
>> +	gpios[GPIO_PWDN] = pdata->gpio_pwdn;
>> +	gpios[GPIO_RST]  = pdata->gpio_reset;
>>   
>> -	for (i = 0; i < ARRAY_SIZE(ov965x->gpios); i++) {
>> -		int gpio = ov965x->gpios[i];
>> +	for (i = 0; i < ARRAY_SIZE(gpios); i++) {
>> +		int gpio = gpios[i];
>>   
>>   		if (!gpio_is_valid(gpio))
>>   			continue;
>>   		ret = devm_gpio_request_one(&ov965x->client->dev, gpio,
>> -					    GPIOF_OUT_INIT_HIGH, "OV965X");
>> -		if (ret < 0)
>> +					    GPIOF_OUT_INIT_HIGH, DRIVER_NAME);
> 
> DRIVER_NAME is different from "OV965X". Is this an intended change?

Yes it was to unify namings around a single DRIVER_NAME definition.

> 
>> +		if (ret < 0) {
>> +			dev_err(&ov965x->client->dev,
>> +				"Failed to request gpio%d (%d)\n", gpio, ret);
>>   			return ret;
>> +		}
>>   		v4l2_dbg(1, debug, &ov965x->sd, "set gpio %d to 1\n", gpio);
>>   
>>   		gpio_set_value(gpio, 1);
>>   		gpio_export(gpio, 0);
>> -		ov965x->gpios[i] = gpio;
>> +		ov965x->gpios[i] = gpio_to_desc(gpio);
>>   	}
>>   
>>   	return 0;
>> @@ -1469,14 +1476,10 @@ static int ov965x_probe(struct i2c_client *client,
>>   	struct v4l2_subdev *sd;
>>   	struct ov965x *ov965x;
>>   	int ret;
>> +	struct device_node *np = client->dev.of_node;
> 
> It'd be nice to declare this next to pdata, rather than after ret and other
> short declarations.

done

> 
>>   
>> -	if (pdata == NULL) {
>> -		dev_err(&client->dev, "platform data not specified\n");
>> -		return -EINVAL;
>> -	}
>> -
>> -	if (pdata->mclk_frequency == 0) {
>> -		dev_err(&client->dev, "MCLK frequency not specified\n");
>> +	if (!pdata && !np) {
>> +		dev_err(&client->dev, "Platform data or device tree data must be provided\n");
>>   		return -EINVAL;
>>   	}
>>   
>> @@ -1486,7 +1489,35 @@ static int ov965x_probe(struct i2c_client *client,
>>   
>>   	mutex_init(&ov965x->lock);
>>   	ov965x->client = client;
>> -	ov965x->mclk_frequency = pdata->mclk_frequency;
>> +	mutex_init(&ov965x->lock);
>> +
>> +	if (np) {
>> +		/* Device tree */
>> +		ov965x->gpios[GPIO_RST] =
>> +			devm_gpiod_get_optional(&client->dev, "resetb",
>> +						GPIOD_OUT_LOW);
>> +		ov965x->gpios[GPIO_PWDN] =
>> +			devm_gpiod_get_optional(&client->dev, "pwdn",
>> +						GPIOD_OUT_HIGH);
>> +
>> +		ov965x->clk = devm_clk_get(&client->dev, NULL);
>> +		if (IS_ERR(ov965x->clk)) {
>> +			dev_err(&client->dev, "Could not get clock\n");
> 
> mutex_destroy() should called on an initialised mutex if probe is going to
> fail. It's certainly not a problem introduced by this patch, but it'd be
> nice to fix that (in a separate patch) now that it's found. The same goes
> for remove below.

Will do.

> 
>> +			return PTR_ERR(ov965x->clk);
>> +		}
>> +		ov965x->mclk_frequency = clk_get_rate(ov965x->clk);
>> +	} else {
>> +		/* Platform data */
>> +		ret = ov965x_configure_gpios(ov965x, pdata);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		if (pdata->mclk_frequency == 0) {
>> +			dev_err(&client->dev, "MCLK frequency is mandatory\n");
>> +			return -EINVAL;
>> +		}
>> +		ov965x->mclk_frequency = pdata->mclk_frequency;
>> +	}
>>   
>>   	sd = &ov965x->sd;
>>   	v4l2_i2c_subdev_init(sd, client, &ov965x_subdev_ops);
>> @@ -1551,9 +1582,17 @@ static int ov965x_remove(struct i2c_client *client)
>>   };
>>   MODULE_DEVICE_TABLE(i2c, ov965x_id);
>>   
>> +static const struct of_device_id ov965x_of_match[] = {
>> +	{ .compatible = "ovti,ov9650", },
>> +	{ .compatible = "ovti,ov9652", },
>> +	{ /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, ov965x_of_match);
>> +
>>   static struct i2c_driver ov965x_i2c_driver = {
>>   	.driver = {
>>   		.name	= DRIVER_NAME,
>> +		.of_match_table = of_match_ptr(ov965x_of_match),
>>   	},
>>   	.probe		= ov965x_probe,
>>   	.remove		= ov965x_remove,
> 

[PATCH v2 3/7] [media] ov9650: add device tree support

[Sylwester Nawrocki]

On 07/03/2017 11:16 AM, Hugues Fruchet wrote:
> Allows use of device tree configuration data.
> If no device tree data is there, configuration is taken from platform data.
> In order to keep GPIOs configuration compatible between both way of doing,
> GPIOs are switched to descriptor-based interface.
> 
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> ---
>   drivers/media/i2c/Kconfig  |  2 +-
>   drivers/media/i2c/ov9650.c | 77 ++++++++++++++++++++++++++++++++++------------
>   2 files changed, 59 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 121b3b5..168115c 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -615,7 +615,7 @@ config VIDEO_OV7670
>   
>   config VIDEO_OV9650
>   	tristate "OmniVision OV9650/OV9652 sensor support"
> -	depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> +	depends on GPIOLIB && I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
>   	---help---
>   	  This is a V4L2 sensor-level driver for the Omnivision
>   	  OV9650 and OV9652 camera sensors.
> diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c
> index 1e4e99e..7e9a902 100644
> --- a/drivers/media/i2c/ov9650.c
> +++ b/drivers/media/i2c/ov9650.c
> @@ -11,12 +11,14 @@
>    * it under the terms of the GNU General Public License version 2 as
>    * published by the Free Software Foundation.
>    */
> +#include <linux/clk.h>
>   #include <linux/delay.h>
>   #include <linux/gpio.h>
>   #include <linux/i2c.h>
>   #include <linux/kernel.h>
>   #include <linux/media.h>
>   #include <linux/module.h>
> +#include <linux/of_gpio.h>
>   #include <linux/ratelimit.h>
>   #include <linux/slab.h>
>   #include <linux/string.h>
> @@ -249,9 +251,10 @@ struct ov965x {
>   	struct v4l2_subdev sd;
>   	struct media_pad pad;
>   	enum v4l2_mbus_type bus_type;
> -	int gpios[NUM_GPIOS];
> +	struct gpio_desc *gpios[NUM_GPIOS];
>   	/* External master clock frequency */
>   	unsigned long mclk_frequency;
> +	struct clk *clk;
>   
>   	/* Protects the struct fields below */
>   	struct mutex lock;
> @@ -511,10 +514,10 @@ static int ov965x_set_color_matrix(struct ov965x *ov965x)
>   	return 0;
>   }
>   
> -static void ov965x_gpio_set(int gpio, int val)
> +static void ov965x_gpio_set(struct gpio_desc *gpio, int val)
>   {
> -	if (gpio_is_valid(gpio))
> -		gpio_set_value(gpio, val);
> +	if (gpio)
> +		gpiod_set_value_cansleep(gpio, val);
>   }
>   
>   static void __ov965x_set_power(struct ov965x *ov965x, int on)
> @@ -1406,24 +1409,28 @@ static int ov965x_configure_gpios(struct ov965x *ov965x,
>   				  const struct ov9650_platform_data *pdata)
>   {
>   	int ret, i;
> +	int gpios[NUM_GPIOS];
>   
> -	ov965x->gpios[GPIO_PWDN] = pdata->gpio_pwdn;
> -	ov965x->gpios[GPIO_RST]  = pdata->gpio_reset;
> +	gpios[GPIO_PWDN] = pdata->gpio_pwdn;
> +	gpios[GPIO_RST]  = pdata->gpio_reset;
>   
> -	for (i = 0; i < ARRAY_SIZE(ov965x->gpios); i++) {
> -		int gpio = ov965x->gpios[i];
> +	for (i = 0; i < ARRAY_SIZE(gpios); i++) {
> +		int gpio = gpios[i];
>   
>   		if (!gpio_is_valid(gpio))
>   			continue;
>   		ret = devm_gpio_request_one(&ov965x->client->dev, gpio,
> -					    GPIOF_OUT_INIT_HIGH, "OV965X");
> -		if (ret < 0)
> +					    GPIOF_OUT_INIT_HIGH, DRIVER_NAME);
> +		if (ret < 0) {
> +			dev_err(&ov965x->client->dev,
> +				"Failed to request gpio%d (%d)\n", gpio, ret);
>   			return ret;
> +		}
>   		v4l2_dbg(1, debug, &ov965x->sd, "set gpio %d to 1\n", gpio);
>   
>   		gpio_set_value(gpio, 1);
>   		gpio_export(gpio, 0);
> -		ov965x->gpios[i] = gpio;
> +		ov965x->gpios[i] = gpio_to_desc(gpio);
>   	}
>   
>   	return 0;
> @@ -1469,14 +1476,10 @@ static int ov965x_probe(struct i2c_client *client,
>   	struct v4l2_subdev *sd;
>   	struct ov965x *ov965x;
>   	int ret;
> +	struct device_node *np = client->dev.of_node;
>   
> -	if (pdata == NULL) {
> -		dev_err(&client->dev, "platform data not specified\n");
> -		return -EINVAL;
> -	}
> -
> -	if (pdata->mclk_frequency == 0) {
> -		dev_err(&client->dev, "MCLK frequency not specified\n");
> +	if (!pdata && !np) {
> +		dev_err(&client->dev, "Platform data or device tree data must be provided\n");
>   		return -EINVAL;
>   	}
>   
> @@ -1486,7 +1489,35 @@ static int ov965x_probe(struct i2c_client *client,
>   
>   	mutex_init(&ov965x->lock);
>   	ov965x->client = client;
> -	ov965x->mclk_frequency = pdata->mclk_frequency;
> +	mutex_init(&ov965x->lock);

Are you initializing the mutex twice?

> +	if (np) {
> +		/* Device tree */
> +		ov965x->gpios[GPIO_RST] =
> +			devm_gpiod_get_optional(&client->dev, "resetb",
> +						GPIOD_OUT_LOW);
> +		ov965x->gpios[GPIO_PWDN] =
> +			devm_gpiod_get_optional(&client->dev, "pwdn",
> +						GPIOD_OUT_HIGH);
> +
> +		ov965x->clk = devm_clk_get(&client->dev, NULL);
> +		if (IS_ERR(ov965x->clk)) {
> +			dev_err(&client->dev, "Could not get clock\n");
> +			return PTR_ERR(ov965x->clk);
> +		}
> +		ov965x->mclk_frequency = clk_get_rate(ov965x->clk);
> +	} else {
> +		/* Platform data */
> +		ret = ov965x_configure_gpios(ov965x, pdata);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (pdata->mclk_frequency == 0) {
> +			dev_err(&client->dev, "MCLK frequency is mandatory\n");

I think the original message ("MCLK frequency not specified\n") was more
helpful.

Why you don't need this check in DT case? The clock is defined as mandatory 
in the DT binding.

> +			return -EINVAL;
> +		}
> +		ov965x->mclk_frequency = pdata->mclk_frequency;
> +	}
>   
>   	sd = &ov965x->sd;
>   	v4l2_i2c_subdev_init(sd, client, &ov965x_subdev_ops);
> @@ -1551,9 +1582,17 @@ static int ov965x_remove(struct i2c_client *client)
>   };
>   MODULE_DEVICE_TABLE(i2c, ov965x_id);
>   
> +static const struct of_device_id ov965x_of_match[] = {
> +	{ .compatible = "ovti,ov9650", },
> +	{ .compatible = "ovti,ov9652", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ov965x_of_match);
> +
>   static struct i2c_driver ov965x_i2c_driver = {
>   	.driver = {
>   		.name	= DRIVER_NAME,
> +		.of_match_table = of_match_ptr(ov965x_of_match),

You don't need of_match_ptr() as ov965x_of_match table is always built in.

>   	},

Otherwise looks good.

[PATCH v2 4/7] [media] ov9650: use write_array() for resolution sequences

[Hugues Fruchet]

Align resolution sequences on initialization sequence using
i2c_rv structure NULL terminated .This add flexibility
on resolution sequence size.
Document resolution related registers by using corresponding
define instead of hexa address/value.

Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
---
 drivers/media/i2c/ov9650.c | 88 +++++++++++++++++++++++++++++++---------------
 1 file changed, 59 insertions(+), 29 deletions(-)

diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c
index 7e9a902..db96698 100644
--- a/drivers/media/i2c/ov9650.c
+++ b/drivers/media/i2c/ov9650.c
@@ -227,11 +227,16 @@ struct ov965x_ctrls {
 	u8 update;
 };
 
+struct i2c_rv {
+	u8 addr;
+	u8 value;
+};
+
 struct ov965x_framesize {
 	u16 width;
 	u16 height;
 	u16 max_exp_lines;
-	const u8 *regs;
+	const struct i2c_rv *regs;
 };
 
 struct ov965x_interval {
@@ -280,11 +285,6 @@ struct ov965x {
 	u8 apply_frame_fmt;
 };
 
-struct i2c_rv {
-	u8 addr;
-	u8 value;
-};
-
 static const struct i2c_rv ov965x_init_regs[] = {
 	{ REG_COM2, 0x10 },	/* Set soft sleep mode */
 	{ REG_COM5, 0x00 },	/* System clock options */
@@ -342,30 +342,59 @@ struct i2c_rv {
 	{ REG_NULL, 0 }
 };
 
-#define NUM_FMT_REGS 14
-/*
- * COM7,  COM3,  COM4, HSTART, HSTOP, HREF, VSTART, VSTOP, VREF,
- * EXHCH, EXHCL, ADC,  OCOM,   OFON
- */
-static const u8 frame_size_reg_addr[NUM_FMT_REGS] = {
-	0x12, 0x0c, 0x0d, 0x17, 0x18, 0x32, 0x19, 0x1a, 0x03,
-	0x2a, 0x2b, 0x37, 0x38, 0x39,
-};
-
-static const u8 ov965x_sxga_regs[NUM_FMT_REGS] = {
-	0x00, 0x00, 0x00, 0x1e, 0xbe, 0xbf, 0x01, 0x81, 0x12,
-	0x10, 0x34, 0x81, 0x93, 0x51,
+static const struct i2c_rv ov965x_sxga_regs[] = {
+	{ REG_COM7, 0x00 },
+	{ REG_COM3, 0x00 },
+	{ REG_COM4, 0x00 },
+	{ REG_HSTART, 0x1e },
+	{ REG_HSTOP, 0xbe },
+	{ 0x32, 0xbf },
+	{ REG_VSTART, 0x01 },
+	{ REG_VSTOP, 0x81 },
+	{ REG_VREF, 0x12 },
+	{ REG_EXHCH, 0x10 },
+	{ REG_EXHCL, 0x34 },
+	{ REG_ADC, 0x81 },
+	{ REG_ACOM, 0x93 },
+	{ REG_OFON, 0x51 },
+	{ REG_NULL, 0 },
 };
 
-static const u8 ov965x_vga_regs[NUM_FMT_REGS] = {
-	0x40, 0x04, 0x80, 0x26, 0xc6, 0xed, 0x01, 0x3d, 0x00,
-	0x10, 0x40, 0x91, 0x12, 0x43,
+static const struct i2c_rv ov965x_vga_regs[] = {
+	{ REG_COM7, 0x40 },
+	{ REG_COM3, 0x04 },
+	{ REG_COM4, 0x80 },
+	{ REG_HSTART, 0x26 },
+	{ REG_HSTOP, 0xc6 },
+	{ 0x32, 0xed },
+	{ REG_VSTART, 0x01 },
+	{ REG_VSTOP, 0x3d },
+	{ REG_VREF, 0x00 },
+	{ REG_EXHCH, 0x10 },
+	{ REG_EXHCL, 0x40 },
+	{ REG_ADC, 0x91 },
+	{ REG_ACOM, 0x12 },
+	{ REG_OFON, 0x43 },
+	{ REG_NULL, 0 },
 };
 
 /* Determined empirically. */
-static const u8 ov965x_qvga_regs[NUM_FMT_REGS] = {
-	0x10, 0x04, 0x80, 0x25, 0xc5, 0xbf, 0x00, 0x80, 0x12,
-	0x10, 0x40, 0x91, 0x12, 0x43,
+static const struct i2c_rv ov965x_qvga_regs[] = {
+	{ REG_COM7, 0x10 },
+	{ REG_COM3, 0x04 },
+	{ REG_COM4, 0x80 },
+	{ REG_HSTART, 0x25 },
+	{ REG_HSTOP, 0xc5 },
+	{ 0x32, 0xbf },
+	{ REG_VSTART, 0x00 },
+	{ REG_VSTOP, 0x80 },
+	{ REG_VREF, 0x12 },
+	{ REG_EXHCH, 0x10 },
+	{ REG_EXHCL, 0x40 },
+	{ REG_ADC, 0x91 },
+	{ REG_ACOM, 0x12 },
+	{ REG_OFON, 0x43 },
+	{ REG_NULL, 0 },
 };
 
 static const struct ov965x_framesize ov965x_framesizes[] = {
@@ -1266,11 +1295,12 @@ static int ov965x_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_pad_config
 
 static int ov965x_set_frame_size(struct ov965x *ov965x)
 {
-	int i, ret = 0;
+	int ret = 0;
+
+	v4l2_dbg(1, debug, ov965x->client, "%s\n", __func__);
 
-	for (i = 0; ret == 0 && i < NUM_FMT_REGS; i++)
-		ret = ov965x_write(ov965x->client, frame_size_reg_addr[i],
-				   ov965x->frame_size->regs[i]);
+	ret = ov965x_write_array(ov965x->client,
+				 ov965x->frame_size->regs);
 	return ret;
 }
 
-- 
1.9.1

[PATCH v2 4/7] [media] ov9650: use write_array() for resolution sequences

[Sakari Ailus]

On Mon, Jul 03, 2017 at 11:16:05AM +0200, Hugues Fruchet wrote:
> Align resolution sequences on initialization sequence using
> i2c_rv structure NULL terminated .This add flexibility
> on resolution sequence size.
> Document resolution related registers by using corresponding
> define instead of hexa address/value.
> 
> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> ---
>  drivers/media/i2c/ov9650.c | 88 +++++++++++++++++++++++++++++++---------------
>  1 file changed, 59 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c
> index 7e9a902..db96698 100644
> --- a/drivers/media/i2c/ov9650.c
> +++ b/drivers/media/i2c/ov9650.c
> @@ -227,11 +227,16 @@ struct ov965x_ctrls {
>  	u8 update;
>  };
>  
> +struct i2c_rv {
> +	u8 addr;
> +	u8 value;
> +};
> +
>  struct ov965x_framesize {
>  	u16 width;
>  	u16 height;
>  	u16 max_exp_lines;
> -	const u8 *regs;
> +	const struct i2c_rv *regs;
>  };
>  
>  struct ov965x_interval {
> @@ -280,11 +285,6 @@ struct ov965x {
>  	u8 apply_frame_fmt;
>  };
>  
> -struct i2c_rv {
> -	u8 addr;
> -	u8 value;
> -};
> -
>  static const struct i2c_rv ov965x_init_regs[] = {
>  	{ REG_COM2, 0x10 },	/* Set soft sleep mode */
>  	{ REG_COM5, 0x00 },	/* System clock options */
> @@ -342,30 +342,59 @@ struct i2c_rv {
>  	{ REG_NULL, 0 }
>  };
>  
> -#define NUM_FMT_REGS 14
> -/*
> - * COM7,  COM3,  COM4, HSTART, HSTOP, HREF, VSTART, VSTOP, VREF,
> - * EXHCH, EXHCL, ADC,  OCOM,   OFON
> - */
> -static const u8 frame_size_reg_addr[NUM_FMT_REGS] = {
> -	0x12, 0x0c, 0x0d, 0x17, 0x18, 0x32, 0x19, 0x1a, 0x03,
> -	0x2a, 0x2b, 0x37, 0x38, 0x39,
> -};
> -
> -static const u8 ov965x_sxga_regs[NUM_FMT_REGS] = {
> -	0x00, 0x00, 0x00, 0x1e, 0xbe, 0xbf, 0x01, 0x81, 0x12,
> -	0x10, 0x34, 0x81, 0x93, 0x51,
> +static const struct i2c_rv ov965x_sxga_regs[] = {
> +	{ REG_COM7, 0x00 },
> +	{ REG_COM3, 0x00 },
> +	{ REG_COM4, 0x00 },
> +	{ REG_HSTART, 0x1e },
> +	{ REG_HSTOP, 0xbe },
> +	{ 0x32, 0xbf },
> +	{ REG_VSTART, 0x01 },
> +	{ REG_VSTOP, 0x81 },
> +	{ REG_VREF, 0x12 },
> +	{ REG_EXHCH, 0x10 },
> +	{ REG_EXHCL, 0x34 },
> +	{ REG_ADC, 0x81 },
> +	{ REG_ACOM, 0x93 },
> +	{ REG_OFON, 0x51 },
> +	{ REG_NULL, 0 },
>  };
>  
> -static const u8 ov965x_vga_regs[NUM_FMT_REGS] = {
> -	0x40, 0x04, 0x80, 0x26, 0xc6, 0xed, 0x01, 0x3d, 0x00,
> -	0x10, 0x40, 0x91, 0x12, 0x43,
> +static const struct i2c_rv ov965x_vga_regs[] = {
> +	{ REG_COM7, 0x40 },
> +	{ REG_COM3, 0x04 },
> +	{ REG_COM4, 0x80 },
> +	{ REG_HSTART, 0x26 },
> +	{ REG_HSTOP, 0xc6 },
> +	{ 0x32, 0xed },
> +	{ REG_VSTART, 0x01 },
> +	{ REG_VSTOP, 0x3d },
> +	{ REG_VREF, 0x00 },
> +	{ REG_EXHCH, 0x10 },
> +	{ REG_EXHCL, 0x40 },
> +	{ REG_ADC, 0x91 },
> +	{ REG_ACOM, 0x12 },
> +	{ REG_OFON, 0x43 },
> +	{ REG_NULL, 0 },
>  };
>  
>  /* Determined empirically. */
> -static const u8 ov965x_qvga_regs[NUM_FMT_REGS] = {
> -	0x10, 0x04, 0x80, 0x25, 0xc5, 0xbf, 0x00, 0x80, 0x12,
> -	0x10, 0x40, 0x91, 0x12, 0x43,
> +static const struct i2c_rv ov965x_qvga_regs[] = {
> +	{ REG_COM7, 0x10 },
> +	{ REG_COM3, 0x04 },
> +	{ REG_COM4, 0x80 },
> +	{ REG_HSTART, 0x25 },
> +	{ REG_HSTOP, 0xc5 },
> +	{ 0x32, 0xbf },
> +	{ REG_VSTART, 0x00 },
> +	{ REG_VSTOP, 0x80 },
> +	{ REG_VREF, 0x12 },
> +	{ REG_EXHCH, 0x10 },
> +	{ REG_EXHCL, 0x40 },
> +	{ REG_ADC, 0x91 },
> +	{ REG_ACOM, 0x12 },
> +	{ REG_OFON, 0x43 },
> +	{ REG_NULL, 0 },
>  };
>  
>  static const struct ov965x_framesize ov965x_framesizes[] = {
> @@ -1266,11 +1295,12 @@ static int ov965x_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_pad_config
>  
>  static int ov965x_set_frame_size(struct ov965x *ov965x)
>  {
> -	int i, ret = 0;
> +	int ret = 0;
> +
> +	v4l2_dbg(1, debug, ov965x->client, "%s\n", __func__);
>  
> -	for (i = 0; ret == 0 && i < NUM_FMT_REGS; i++)
> -		ret = ov965x_write(ov965x->client, frame_size_reg_addr[i],
> -				   ov965x->frame_size->regs[i]);
> +	ret = ov965x_write_array(ov965x->client,
> +				 ov965x->frame_size->regs);
>  	return ret;

return ov96...();

And you can remove ret, too.

>  }
>  

-- 
Sakari Ailus
e-mail: sakari.ailus at iki.fi	XMPP: sailus at retiisi.org.uk

[PATCH v2 5/7] [media] ov9650: add multiple variant support

[Hugues Fruchet]

Ops support and registers set can now be different
from a variant to another.

Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
---
 drivers/media/i2c/ov9650.c | 156 ++++++++++++++++++++++++++++-----------------
 1 file changed, 99 insertions(+), 57 deletions(-)

diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c
index db96698..50397e6 100644
--- a/drivers/media/i2c/ov9650.c
+++ b/drivers/media/i2c/ov9650.c
@@ -38,7 +38,7 @@
 module_param(debug, int, 0644);
 MODULE_PARM_DESC(debug, "Debug level (0-2)");
 
-#define DRIVER_NAME "OV9650"
+#define DRIVER_NAME "ov965x"
 
 /*
  * OV9650/OV9652 register definitions
@@ -239,6 +239,13 @@ struct ov965x_framesize {
 	const struct i2c_rv *regs;
 };
 
+struct ov965x_pixfmt {
+	u32 code;
+	u32 colorspace;
+	/* REG_TSLB value, only bits [3:2] may be set. */
+	u8 tslb_reg;
+};
+
 struct ov965x_interval {
 	struct v4l2_fract interval;
 	/* Maximum resolution for this interval */
@@ -257,6 +264,21 @@ struct ov965x {
 	struct media_pad pad;
 	enum v4l2_mbus_type bus_type;
 	struct gpio_desc *gpios[NUM_GPIOS];
+
+	/* Variant specific regs and ops */
+	const struct i2c_rv *init_regs;
+	const struct ov965x_framesize *framesizes;
+	unsigned int nb_of_framesizes;
+	const struct ov965x_pixfmt *formats;
+	unsigned int nb_of_formats;
+	const struct ov965x_interval *intervals;
+	unsigned int nb_of_intervals;
+	int (*initialize_controls)(struct ov965x *ov965x);
+	int (*set_frame_interval)(struct ov965x *ov965x,
+				  struct v4l2_subdev_frame_interval *fi);
+	void (*update_exposure_ctrl)(struct ov965x *ov965x);
+	int (*set_params)(struct ov965x *ov965x);
+
 	/* External master clock frequency */
 	unsigned long mclk_frequency;
 	struct clk *clk;
@@ -416,13 +438,6 @@ struct ov965x {
 	},
 };
 
-struct ov965x_pixfmt {
-	u32 code;
-	u32 colorspace;
-	/* REG_TSLB value, only bits [3:2] may be set. */
-	u8 tslb_reg;
-};
-
 static const struct ov965x_pixfmt ov965x_formats[] = {
 	{ MEDIA_BUS_FMT_YUYV8_2X8, V4L2_COLORSPACE_JPEG, 0x00},
 	{ MEDIA_BUS_FMT_YVYU8_2X8, V4L2_COLORSPACE_JPEG, 0x04},
@@ -576,7 +591,7 @@ static int ov965x_s_power(struct v4l2_subdev *sd, int on)
 		__ov965x_set_power(ov965x, on);
 		if (on) {
 			ret = ov965x_write_array(client,
-						 ov965x_init_regs);
+						 ov965x->init_regs);
 			ov965x->apply_frame_fmt = 1;
 			ov965x->ctrls.update = 1;
 		}
@@ -1090,12 +1105,13 @@ static int ov965x_initialize_controls(struct ov965x *ov965x)
 /*
  * V4L2 subdev video and pad level operations
  */
-static void ov965x_get_default_format(struct v4l2_mbus_framefmt *mf)
+static void ov965x_get_default_format(struct ov965x *ov965x,
+				      struct v4l2_mbus_framefmt *mf)
 {
-	mf->width = ov965x_framesizes[0].width;
-	mf->height = ov965x_framesizes[0].height;
-	mf->colorspace = ov965x_formats[0].colorspace;
-	mf->code = ov965x_formats[0].code;
+	mf->width = ov965x->framesizes[0].width;
+	mf->height = ov965x->framesizes[0].height;
+	mf->colorspace = ov965x->formats[0].colorspace;
+	mf->code = ov965x->formats[0].code;
 	mf->field = V4L2_FIELD_NONE;
 }
 
@@ -1103,10 +1119,12 @@ static int ov965x_enum_mbus_code(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_pad_config *cfg,
 				 struct v4l2_subdev_mbus_code_enum *code)
 {
-	if (code->index >= ARRAY_SIZE(ov965x_formats))
+	struct ov965x *ov965x = to_ov965x(sd);
+
+	if (code->index >= ov965x->nb_of_formats)
 		return -EINVAL;
 
-	code->code = ov965x_formats[code->index].code;
+	code->code = ov965x->formats[code->index].code;
 	return 0;
 }
 
@@ -1114,22 +1132,22 @@ static int ov965x_enum_frame_sizes(struct v4l2_subdev *sd,
 				   struct v4l2_subdev_pad_config *cfg,
 				   struct v4l2_subdev_frame_size_enum *fse)
 {
-	int i = ARRAY_SIZE(ov965x_formats);
+	struct ov965x *ov965x = to_ov965x(sd);
+	int i = ov965x->nb_of_formats;
 
-	if (fse->index >= ARRAY_SIZE(ov965x_framesizes))
+	if (fse->index >= ov965x->nb_of_framesizes)
 		return -EINVAL;
 
 	while (--i)
-		if (fse->code == ov965x_formats[i].code)
+		if (fse->code == ov965x->formats[i].code)
 			break;
 
-	fse->code = ov965x_formats[i].code;
+	fse->code = ov965x->formats[i].code;
 
-	fse->min_width  = ov965x_framesizes[fse->index].width;
+	fse->min_width  = ov965x->framesizes[fse->index].width;
 	fse->max_width  = fse->min_width;
-	fse->max_height = ov965x_framesizes[fse->index].height;
+	fse->max_height = ov965x->framesizes[fse->index].height;
 	fse->min_height = fse->max_height;
-
 	return 0;
 }
 
@@ -1138,6 +1156,9 @@ static int ov965x_g_frame_interval(struct v4l2_subdev *sd,
 {
 	struct ov965x *ov965x = to_ov965x(sd);
 
+	if (!ov965x->fiv)
+		return 0;
+
 	mutex_lock(&ov965x->lock);
 	fi->interval = ov965x->fiv->interval;
 	mutex_unlock(&ov965x->lock);
@@ -1146,13 +1167,15 @@ static int ov965x_g_frame_interval(struct v4l2_subdev *sd,
 }
 
 static int __ov965x_set_frame_interval(struct ov965x *ov965x,
-				       struct v4l2_subdev_frame_interval *fi)
+				     struct v4l2_subdev_frame_interval *fi)
 {
 	struct v4l2_mbus_framefmt *mbus_fmt = &ov965x->format;
-	const struct ov965x_interval *fiv = &ov965x_intervals[0];
+	const struct ov965x_interval *fiv = ov965x->intervals;
 	u64 req_int, err, min_err = ~0ULL;
 	unsigned int i;
 
+	if (!fiv)
+		return 0;
 
 	if (fi->interval.denominator == 0)
 		return -EINVAL;
@@ -1160,8 +1183,8 @@ static int __ov965x_set_frame_interval(struct ov965x *ov965x,
 	req_int = (u64)(fi->interval.numerator * 10000) /
 		fi->interval.denominator;
 
-	for (i = 0; i < ARRAY_SIZE(ov965x_intervals); i++) {
-		const struct ov965x_interval *iv = &ov965x_intervals[i];
+	for (i = 0; i < ov965x->nb_of_intervals; i++) {
+		const struct ov965x_interval *iv = ov965x->intervals;
 
 		if (mbus_fmt->width != iv->size.width ||
 		    mbus_fmt->height != iv->size.height)
@@ -1185,13 +1208,15 @@ static int ov965x_s_frame_interval(struct v4l2_subdev *sd,
 				   struct v4l2_subdev_frame_interval *fi)
 {
 	struct ov965x *ov965x = to_ov965x(sd);
-	int ret;
+	int ret = 0;
 
 	v4l2_dbg(1, debug, sd, "Setting %d/%d frame interval\n",
 		 fi->interval.numerator, fi->interval.denominator);
 
 	mutex_lock(&ov965x->lock);
-	ret = __ov965x_set_frame_interval(ov965x, fi);
+	if (ov965x->set_frame_interval)
+		ret = ov965x->set_frame_interval(ov965x, fi);
+
 	ov965x->apply_frame_fmt = 1;
 	mutex_unlock(&ov965x->lock);
 	return ret;
@@ -1216,12 +1241,13 @@ static int ov965x_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_pad_config
 	return 0;
 }
 
-static void __ov965x_try_frame_size(struct v4l2_mbus_framefmt *mf,
+static void __ov965x_try_frame_size(struct ov965x *ov965x,
+				    struct v4l2_mbus_framefmt *mf,
 				    const struct ov965x_framesize **size)
 {
-	const struct ov965x_framesize *fsize = &ov965x_framesizes[0],
+	const struct ov965x_framesize *fsize = &ov965x->framesizes[0],
 		*match = NULL;
-	int i = ARRAY_SIZE(ov965x_framesizes);
+	int i = ov965x->nb_of_framesizes;
 	unsigned int min_err = UINT_MAX;
 
 	while (i--) {
@@ -1234,7 +1260,7 @@ static void __ov965x_try_frame_size(struct v4l2_mbus_framefmt *mf,
 		fsize++;
 	}
 	if (!match)
-		match = &ov965x_framesizes[0];
+		match = &ov965x->framesizes[0];
 	mf->width  = match->width;
 	mf->height = match->height;
 	if (size)
@@ -1244,20 +1270,20 @@ static void __ov965x_try_frame_size(struct v4l2_mbus_framefmt *mf,
 static int ov965x_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_pad_config *cfg,
 			  struct v4l2_subdev_format *fmt)
 {
-	unsigned int index = ARRAY_SIZE(ov965x_formats);
-	struct v4l2_mbus_framefmt *mf = &fmt->format;
 	struct ov965x *ov965x = to_ov965x(sd);
+	unsigned int index = ov965x->nb_of_formats;
+	struct v4l2_mbus_framefmt *mf = &fmt->format;
 	const struct ov965x_framesize *size = NULL;
 	int ret = 0;
 
-	__ov965x_try_frame_size(mf, &size);
+	__ov965x_try_frame_size(ov965x, mf, &size);
 
 	while (--index)
-		if (ov965x_formats[index].code == mf->code)
+		if (ov965x->formats[index].code == mf->code)
 			break;
 
 	mf->colorspace	= V4L2_COLORSPACE_JPEG;
-	mf->code	= ov965x_formats[index].code;
+	mf->code	= ov965x->formats[index].code;
 	mf->field	= V4L2_FIELD_NONE;
 
 	mutex_lock(&ov965x->lock);
@@ -1273,7 +1299,7 @@ static int ov965x_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_pad_config
 		} else {
 			ov965x->frame_size = size;
 			ov965x->format = fmt->format;
-			ov965x->tslb_reg = ov965x_formats[index].tslb_reg;
+			ov965x->tslb_reg = ov965x->formats[index].tslb_reg;
 			ov965x->apply_frame_fmt = 1;
 		}
 	}
@@ -1283,12 +1309,14 @@ static int ov965x_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_pad_config
 			.interval = { 0, 1 }
 		};
 		/* Reset to minimum possible frame interval */
-		__ov965x_set_frame_interval(ov965x, &fiv);
+		if (ov965x->set_frame_interval)
+			ret = ov965x->set_frame_interval(ov965x, &fiv);
 	}
 	mutex_unlock(&ov965x->lock);
 
 	if (!ret)
-		ov965x_update_exposure_ctrl(ov965x);
+		if (ov965x->update_exposure_ctrl)
+			ov965x->update_exposure_ctrl(ov965x);
 
 	return ret;
 }
@@ -1363,7 +1391,8 @@ static int ov965x_s_stream(struct v4l2_subdev *sd, int on)
 	mutex_lock(&ov965x->lock);
 	if (ov965x->streaming == !on) {
 		if (on)
-			ret = __ov965x_set_params(ov965x);
+			if (ov965x->set_params)
+				ret = ov965x->set_params(ov965x);
 
 		if (!ret && ctrls->update) {
 			/*
@@ -1396,8 +1425,9 @@ static int ov965x_s_stream(struct v4l2_subdev *sd, int on)
 static int ov965x_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 {
 	struct v4l2_mbus_framefmt *mf = v4l2_subdev_get_try_format(sd, fh->pad, 0);
+	struct ov965x *ov965x = to_ov965x(sd);
 
-	ov965x_get_default_format(mf);
+	ov965x_get_default_format(ov965x, mf);
 	return 0;
 }
 
@@ -1516,8 +1546,6 @@ static int ov965x_probe(struct i2c_client *client,
 	ov965x = devm_kzalloc(&client->dev, sizeof(*ov965x), GFP_KERNEL);
 	if (!ov965x)
 		return -ENOMEM;
-
-	mutex_init(&ov965x->lock);
 	ov965x->client = client;
 	mutex_init(&ov965x->lock);
 
@@ -1557,38 +1585,52 @@ static int ov965x_probe(struct i2c_client *client,
 	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
 		     V4L2_SUBDEV_FL_HAS_EVENTS;
 
-	ret = ov965x_configure_gpios(ov965x, pdata);
-	if (ret < 0)
-		return ret;
-
 	ov965x->pad.flags = MEDIA_PAD_FL_SOURCE;
 	sd->entity.function = MEDIA_ENT_F_CAM_SENSOR;
 	ret = media_entity_pads_init(&sd->entity, 1, &ov965x->pad);
 	if (ret < 0)
 		return ret;
 
-	ret = ov965x_initialize_controls(ov965x);
+	ret = ov965x_detect_sensor(sd);
 	if (ret < 0)
 		goto err_me;
 
-	ov965x_get_default_format(&ov965x->format);
-	ov965x->frame_size = &ov965x_framesizes[0];
+	ov965x->init_regs = ov965x_init_regs;
+	ov965x->initialize_controls = ov965x_initialize_controls;
+	ov965x->framesizes = ov965x_framesizes;
+	ov965x->nb_of_framesizes = ARRAY_SIZE(ov965x_framesizes);
+	ov965x->formats = ov965x_formats;
+	ov965x->nb_of_formats = ARRAY_SIZE(ov965x_formats);
+	ov965x->intervals = ov965x_intervals;
+	ov965x->nb_of_intervals = ARRAY_SIZE(ov965x_intervals);
 	ov965x->fiv = &ov965x_intervals[0];
+	ov965x->set_frame_interval = __ov965x_set_frame_interval;
+	ov965x->update_exposure_ctrl = ov965x_update_exposure_ctrl;
+	ov965x->set_params = __ov965x_set_params;
 
-	ret = ov965x_detect_sensor(sd);
-	if (ret < 0)
-		goto err_ctrls;
+	ov965x->frame_size = &ov965x->framesizes[0];
+	ov965x_get_default_format(ov965x, &ov965x->format);
+
+	if (ov965x->initialize_controls) {
+		ret = ov965x->initialize_controls(ov965x);
+		if (ret < 0)
+			goto err_ctrls;
+	}
 
 	/* Update exposure time min/max to match frame format */
-	ov965x_update_exposure_ctrl(ov965x);
+	if (ov965x->update_exposure_ctrl)
+		ov965x->update_exposure_ctrl(ov965x);
 
 	ret = v4l2_async_register_subdev(sd);
 	if (ret < 0)
 		goto err_ctrls;
 
+	dev_info(&client->dev, "%s driver probed\n", sd->name);
 	return 0;
+
 err_ctrls:
-	v4l2_ctrl_handler_free(sd->ctrl_handler);
+	if (sd->ctrl_handler)
+		v4l2_ctrl_handler_free(sd->ctrl_handler);
 err_me:
 	media_entity_cleanup(&sd->entity);
 	return ret;
-- 
1.9.1

[PATCH v2 6/7] [media] ov9650: add support of OV9655 variant

[Hugues Fruchet]

Add a first support of OV9655 variant.
Because of register set slightly different from OV9650/9652,
not all of the driver features are supported (controls).
Supported resolutions are limited to VGA, QVGA, QQVGA.
Supported format is limited to RGB565.
Controls are limited to color bar test pattern for test purpose.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
---
 drivers/media/i2c/Kconfig  |   4 +-
 drivers/media/i2c/ov9650.c | 487 ++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 457 insertions(+), 34 deletions(-)

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 168115c..0f7542f 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -614,11 +614,11 @@ config VIDEO_OV7670
 	  controller.
 
 config VIDEO_OV9650
-	tristate "OmniVision OV9650/OV9652 sensor support"
+	tristate "OmniVision OV9650/OV9652/OV9655 sensor support"
 	depends on GPIOLIB && I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
 	---help---
 	  This is a V4L2 sensor-level driver for the Omnivision
-	  OV9650 and OV9652 camera sensors.
+	  OV9650 and OV9652 and OV9655 camera sensors.
 
 config VIDEO_OV13858
 	tristate "OmniVision OV13858 sensor support"
diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c
index 50397e6..9ff0782 100644
--- a/drivers/media/i2c/ov9650.c
+++ b/drivers/media/i2c/ov9650.c
@@ -1,5 +1,5 @@
 /*
- * Omnivision OV9650/OV9652 CMOS Image Sensor driver
+ * Omnivision OV9650/OV9652/OV9655 CMOS Image Sensor driver
  *
  * Copyright (C) 2013, Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
  *
@@ -7,6 +7,15 @@
  * by Vladimir Fonov.
  * Copyright (c) 2010, Vladimir Fonov
  *
+ *
+ * Copyright (C) STMicroelectronics SA 2017
+ * Author: Hugues Fruchet <hugues.fruchet@st.com> for STMicroelectronics.
+ *
+ * OV9655 initial support based on a driver written by H. Nikolaus Schaller:
+ *   http://git.goldelico.com/?p=gta04-kernel.git;a=shortlog;h=refs/heads/work/hns/video/ov9655
+ * OV9655 registers sequence from STM32CubeF7 embedded software, see:
+ *   https://developer.mbed.org/teams/ST/code/BSP_DISCO_F746NG/file/e1d9da7fe856/Drivers/BSP/Components/ov9655/ov9655.c
+ *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
@@ -58,14 +67,21 @@
 #define REG_PID			0x0a	/* Product ID MSB */
 #define REG_VER			0x0b	/* Product ID LSB */
 #define REG_COM3		0x0c
-#define  COM3_SWAP		0x40
+#define  COM3_COLORBAR		0x80
+#define  COM3_RGB565		0x00
+#define  COM3_SWAP		0x40	/* Doesn't work in RGB */
+#define  COM3_RESETB		0x08
 #define  COM3_VARIOPIXEL1	0x04
+#define  OV9655_SINGLEFRAME	0x01
 #define REG_COM4		0x0d	/* Vario Pixels  */
 #define  COM4_VARIOPIXEL2	0x80
+#define  OV9655_TRISTATE		/* seems to have a different function */
 #define REG_COM5		0x0e	/* System clock options */
 #define  COM5_SLAVE_MODE	0x10
-#define  COM5_SYSTEMCLOCK48MHZ	0x80
+#define  COM5_SYSTEMCLOCK48MHZ	0x80	/* not on OV9655 */
+#define  OV9655_EXPOSURESTEP	0x01
 #define REG_COM6		0x0f	/* HREF & ADBLC options */
+#define  COM6_BLC_OPTICAL	0x40	/* Optical black */
 #define REG_AECH		0x10	/* Exposure value, AEC[9:2] */
 #define REG_CLKRC		0x11	/* Clock control */
 #define  CLK_EXT		0x40	/* Use external clock directly */
@@ -74,13 +90,18 @@
 #define  COM7_RESET		0x80
 #define  COM7_FMT_MASK		0x38
 #define  COM7_FMT_VGA		0x40
-#define	 COM7_FMT_CIF		0x20
+#define  COM7_FMT_CIF		0x20
 #define  COM7_FMT_QVGA		0x10
 #define  COM7_FMT_QCIF		0x08
-#define	 COM7_RGB		0x04
-#define	 COM7_YUV		0x00
-#define	 COM7_BAYER		0x01
-#define	 COM7_PBAYER		0x05
+#define  COM7_RGB		0x04
+#define  COM7_YUV		0x00
+#define  COM7_BAYER		0x01
+#define  COM7_PBAYER		0x05
+#define  OV9655_COM7_VGA	0x60
+#define  OV9655_COM7_RAWRGB	0x00	/* different format encoding */
+#define  OV9655_COM7_RAWRGBINT	0x01
+#define  OV9655_COM7_YUV	0x02
+#define  OV9655_COM7_RGB	0x03
 #define REG_COM8		0x13	/* AGC/AEC options */
 #define  COM8_FASTAEC		0x80	/* Enable fast AGC/AEC */
 #define  COM8_AECSTEP		0x40	/* Unlimited AEC step size */
@@ -89,14 +110,23 @@
 #define  COM8_AWB		0x02	/* White balance enable */
 #define  COM8_AEC		0x01	/* Auto exposure enable */
 #define REG_COM9		0x14	/* Gain ceiling */
-#define  COM9_GAIN_CEIL_MASK	0x70	/* */
+#define  COM9_GAIN_CEIL_MASK	0x70
+#define  COM9_GAIN_CEIL_16X	0x30
+#define  OV9655_COM9_EXPTIMING	0x08
+#define  OV9655_COM9_VSYNCDROP	0x04
+#define  OV9655_COM9_AECDROP	0x02
 #define REG_COM10		0x15	/* PCLK, HREF, HSYNC signals polarity */
+#define  OV9655_SLAVE_PIN	0x80	/* SLHS/SLVS instead of RESETB/PWDN */
 #define  COM10_HSYNC		0x40	/* HSYNC instead of HREF */
 #define  COM10_PCLK_HB		0x20	/* Suppress PCLK on horiz blank */
-#define  COM10_HREF_REV		0x08	/* Reverse HREF */
+#define  OV9655_COM10_PCLK_REV		0x10	/* PCLK reverse */
+#define  COM10_HREF_REV	0x08	/* Reverse HREF */
 #define  COM10_VS_LEAD		0x04	/* VSYNC on clock leading edge */
+#define  OV9655_COM10_RESET_OPTION	0x04	/* Reset signal end point */
 #define  COM10_VS_NEG		0x02	/* VSYNC negative */
 #define  COM10_HS_NEG		0x01	/* HSYNC negative */
+#define OV9655_REG16		0x16	/* dummy frame and blanking */
+#define   OV9655_REG16_DUMMY_8	0x20	/* dummy frame when gain > 8 */
 #define REG_HSTART		0x17	/* Horiz start high bits */
 #define REG_HSTOP		0x18	/* Horiz stop high bits */
 #define REG_VSTART		0x19	/* Vert start high bits */
@@ -117,6 +147,7 @@
 #define REG_BBIAS		0x27	/* B channel output bias */
 #define REG_GBBIAS		0x28	/* Gb channel output bias */
 #define REG_GRCOM		0x29	/* Analog BLC & regulator */
+#define OV9655_PREGAIN		0x29
 #define REG_EXHCH		0x2a	/* Dummy pixel insert MSB */
 #define REG_EXHCL		0x2b	/* Dummy pixel insert LSB */
 #define REG_RBIAS		0x2c	/* R channel output bias */
@@ -127,12 +158,30 @@
 #define REG_HSYEN		0x31	/* HSYNC falling edge delay LSB*/
 #define REG_HREF		0x32	/* HREF pieces */
 #define REG_CHLF		0x33	/* reserved */
+#define OV9655_CLKF		0x33	/* Array current control */
+#define OV9655_AREF1		0x34	/* Array reference control */
+#define OV9655_AREF2		0x35	/* Array reference control */
+#define OV9655_AREF3		0x36	/* Array reference control */
 #define REG_ADC			0x37	/* reserved */
+#define OV9655_ADC		0x37	/* ADC Control 1 (Range adjustment) */
 #define REG_ACOM		0x38	/* reserved */
-#define REG_OFON		0x39	/* Power down register */
+#define OV9655_ADC2		0x38	/* ADC Control 2 (Range adjustment) */
+#define REG_OFON		0x39	/* Power down register (ov9650 only) */
 #define  OFON_PWRDN		0x08	/* Power down bit */
+#define OV9655_AREF4		0x39	/* Array reference control */
 #define REG_TSLB		0x3a	/* YUVU format */
+#define  OV9655_PCLKDELAY2NS	0x40
+#define  OV9655_PCLKDELAY4NS	0x80
+#define  OV9655_PCLKDELAY6NS	0xc0
+#define  OV9655_OUTREVERSE	0x20
+#define  OV9655_FIXEDUV	0x10
 #define  TSLB_YUYV_MASK		0x0c	/* UYVY or VYUY - see com13 */
+#define  TSLB_YUYV		0x00
+#define  TSLB_YVYU		0x04
+#define  TSLB_VYUY		0x08
+#define  TSLB_UYVY		0x0c
+#define  OV9655_BANDINGAUTO	0x02
+
 #define REG_COM11		0x3b	/* Night mode, banding filter enable */
 #define  COM11_NIGHT		0x80	/* Night mode enable */
 #define  COM11_NMFR		0x60	/* Two bit NM frame rate */
@@ -142,25 +191,38 @@
 #define  COM12_HREF		0x80	/* HREF always */
 #define REG_COM13		0x3d	/* Gamma selection, Color matrix en. */
 #define  COM13_GAMMA		0x80	/* Gamma enable */
-#define	 COM13_UVSAT		0x40	/* UV saturation auto adjustment */
+#define  COM13_UVSAT		0x40	/* UV saturation auto adjustment */
+#define  COM13_Y_DELAY		0x08	/* Delay Y channel */
 #define  COM13_UVSWAP		0x01	/* V before U - w/TSLB */
 #define REG_COM14		0x3e	/* Edge enhancement options */
 #define  COM14_EDGE_EN		0x02
 #define  COM14_EEF_X2		0x01
+#define OV9655_REG_COM14	0x3e	/* pixel correction/zoom ON/OFF sel. */
+#define  OV9655_COM14_BLACK_PIX	0x08	/* Black pixel correction */
+#define  OV9655_COM14_WHITE_PIX	0x04	/* White pixel correction */
+#define  OV9655_COM14_ZOOM	0x02	/* Zoom function ON */
 #define REG_EDGE		0x3f	/* Edge enhancement factor */
 #define  EDGE_FACTOR_MASK	0x0f
 #define REG_COM15		0x40	/* Output range, RGB 555/565 */
 #define  COM15_R10F0		0x00	/* Data range 10 to F0 */
-#define	 COM15_R01FE		0x80	/* 01 to FE */
+#define  COM15_R01FE		0x80	/* 01 to FE */
 #define  COM15_R00FF		0xc0	/* 00 to FF */
 #define  COM15_RGB565		0x10	/* RGB565 output */
 #define  COM15_RGB555		0x30	/* RGB555 output */
 #define  COM15_SWAPRB		0x04	/* Swap R&B */
 #define REG_COM16		0x41	/* Color matrix coeff options */
 #define REG_COM17		0x42	/* Single frame out, banding filter */
+#define OV9655_REG_COM17	0x42	/* Denoise, edge, auto gain, ... */
+#define   OV9655_COM17_EDGE_AUTO	0x40	/* Edge auto */
+#define   OV9655_COM17_DENOISE_AUTO	0x80	/* Denoise auto */
+#define OV9655_REG_RSVD(__n)	(0x43 + (__n) - 1) /* reserved but used... */
 /* n = 1...9, 0x4f..0x57 */
-#define	REG_MTX(__n)		(0x4f + (__n) - 1)
+#define REG_MTX(__n)		(0x4f + (__n) - 1)
 #define REG_MTXS		0x58
+#define REG_AWBOP(__n)		(0x59 + (__n) - 1) /* AWB control options */
+#define REG_BLMT		0x5F	/* AWB Blue Component Gain Limit */
+#define REG_RLMT		0x60	/* AWB Red Component Gain Limit */
+#define REG_GLMT		0x61	/* AWB Green Component Gain Limit */
 /* Lens Correction Option 1...5, __n = 0...5 */
 #define REG_LCC(__n)		(0x62 + (__n) - 1)
 #define  LCC5_LCC_ENABLE	0x01	/* LCC5, enable lens correction */
@@ -170,10 +232,26 @@
 #define REG_HV			0x69	/* Manual banding filter MSB */
 #define REG_MBD			0x6a	/* Manual banding filter value */
 #define REG_DBLV		0x6b	/* reserved */
+#define OV9655_REG_DBLV		0x6b	/* PLL, DVDD regu bypass, bandgap */
+#define  OV9655_DBLV_BANDGAP	0x0a	/* default value */
+#define  OV9655_DBLV_LDO_BYPASS	0x10
+#define  OV9655_DBLV_PLL_BYPASS	0x00
+#define  OV9655_DBLV_PLL_4X	0x40
+#define  OV9655_DBLV_PLL_6X	0x80
+#define  OV9655_DBLV_PLL_8X	0xc0
 #define REG_GSP			0x6c	/* Gamma curve */
 #define  GSP_LEN		15
+#define OV9655_REG_DNSTH	0x70	/* De-noise Function Threshold Adj. */
+#define OV9655_REG_POIDX	0x72	/* Pixel output index */
+#define OV9655_REG_PCKDV	0x73	/* Pixel Clock Output Selection */
+#define OV9655_REG_XINDX	0x74	/* Horizontal Scaling Down Coeff. */
+#define OV9655_REG_YINDX	0x75	/* Vertical Scaling Down Coeff. */
+#define OV9655_REG_SLOP		0x7A	/* Gamma Curve Highest Segment Slope */
+#define OV9655_REG_GAM(__n)	(0x7B + (__n) - 1)	/* Gamma curve */
 #define REG_GST			0x7c	/* Gamma curve */
 #define  GST_LEN		15
+#define OV9655_REG_COM18	0x8b	/* Zoom mode in VGA */
+#define OV9655_REG_COM19	0x8c	/* UV adjustment */
 #define REG_COM21		0x8b
 #define REG_COM22		0x8c	/* Edge enhancement, denoising */
 #define  COM22_WHTPCOR		0x02	/* White pixel correction enable */
@@ -181,6 +259,8 @@
 #define  COM22_DENOISE		0x10	/* White pixel correction option */
 #define REG_COM23		0x8d	/* Color bar test, color gain */
 #define  COM23_TEST_MODE	0x10
+#define OV9655_REG_COM20	0x8d
+#define  OV9655_COM20_TEST_MODE	0x10
 #define REG_DBLC1		0x8f	/* Digital BLC */
 #define REG_DBLC_B		0x90	/* Digital BLC B channel offset */
 #define REG_DBLC_R		0x91	/* Digital BLC R channel offset */
@@ -193,6 +273,17 @@
 #define REG_AECHM		0xa1	/* Exposure value - bits AEC[15:10] */
 #define REG_BD50ST		0xa2	/* Banding filter value for 50Hz */
 #define REG_BD60ST		0xa3	/* Banding filter value for 60Hz */
+#define OV9655_REG_COM21	0xa4	/* Digital gain */
+#define OV9655_REG_AWB_GREEN	0xa6	/* AWB green */
+#define OV9655_REG_REF_A8	0xa8	/* Analog Reference Control */
+#define OV9655_REG_REF_A9	0xa9	/* Analog Reference Control */
+#define OV9655_REG_BLC(__n)	(0xac + (__n) - 1) /* Black Level Control */
+#define OV9655_REG_CTRLB4	0xb4	/* UV adjustment */
+#define OV9655_REG_ADBOFF	0xbc	/* ADC B channel offset setting */
+#define OV9655_REG_ADROFF	0xbd	/* ADC R channel offset setting */
+#define OV9655_REG_ADGBOFF	0xbe	/* ADC Gb channel offset setting */
+#define OV9655_REG_ADGEOFF	0xbf	/* ADC Gr channel offset setting */
+#define OV9655_REG_COM24	0xc7	/* Pixel clock frequency selection */
 #define REG_NULL		0xff	/* Array end token */
 
 #define DEF_CLKRC		0x80
@@ -200,6 +291,7 @@
 #define OV965X_ID(_msb, _lsb)	((_msb) << 8 | (_lsb))
 #define OV9650_ID		0x9650
 #define OV9652_ID		0x9652
+#define OV9655V5_ID		0x9657
 
 struct ov965x_ctrls {
 	struct v4l2_ctrl_handler handler;
@@ -458,6 +550,291 @@ struct ov965x {
 	{{ 1,   25  }, { QVGA_WIDTH, QVGA_HEIGHT }, 1 },  /* 25 fps */
 };
 
+/* OV9655 */
+static const struct i2c_rv ov9655_init_regs[] = {
+	{ REG_GAIN, 0x00 },
+	{ REG_BLUE, 0x80 },
+	{ REG_RED, 0x80 },
+	{ REG_VREF, 0x02 },
+	{ REG_COM1, 0x03 },
+	{ REG_COM2, 0x01 },/* Output drive x2 */
+	{ REG_COM3, COM3_RGB565 },/* Output drive x2, RGB565 */
+	{ REG_COM5, 0x60 | OV9655_EXPOSURESTEP },/* 0x60 ? */
+	{ REG_COM6, COM6_BLC_OPTICAL },
+	{ REG_CLKRC, 0x01 },/* F(internal clk) = F(input clk) / 2 */
+	{ REG_COM7, OV9655_COM7_VGA | OV9655_COM7_YUV },
+	{ REG_COM8, COM8_FASTAEC | COM8_AECSTEP |
+			COM8_AGC | COM8_AWB | COM8_AEC },
+	{ REG_COM9, COM9_GAIN_CEIL_16X | OV9655_COM9_EXPTIMING |
+			OV9655_COM9_AECDROP },
+	{ OV9655_REG16, OV9655_REG16_DUMMY_8 | 0x4 },
+	{ REG_HSTART, 0x18 },
+	{ REG_HSTOP, 0x04 },
+	{ REG_VSTART, 0x01 },
+	{ REG_VSTOP, 0x81 },
+	{ REG_MVFP, 0x00 },/* No mirror/flip */
+	{ REG_AEW, 0x3c },
+	{ REG_AEB, 0x36 },
+	{ REG_VPT, 0x72 },
+	{ REG_BBIAS, 0x08 },
+	{ REG_GBBIAS, 0x08 },
+	{ OV9655_PREGAIN, 0x15 },
+	{ REG_EXHCH, 0x00 },
+	{ REG_EXHCL, 0x00 },
+	{ REG_RBIAS, 0x08 },
+	{ REG_HREF, 0x12 },/* QVGA */
+	{ REG_CHLF, 0x00 },
+	{ OV9655_AREF1, 0x3f },
+	{ OV9655_AREF2, 0x00 },
+	{ OV9655_AREF3, 0x3a },
+	{ OV9655_ADC2, 0x72 },
+	{ OV9655_AREF4, 0x57 },
+	{ REG_TSLB, OV9655_PCLKDELAY6NS | TSLB_UYVY },
+	{ REG_COM11, 0x04 },/* 0x04 ? */
+	{ REG_COM13, COM13_GAMMA | 0x10 |
+			COM13_Y_DELAY | COM13_UVSWAP },/* 0x10 ? */
+	{OV9655_REG_COM14, OV9655_COM14_ZOOM }, /* QVGA */
+	{ REG_EDGE, 0xc1 },
+	{ REG_COM15, COM15_R00FF },/* Full range output */
+	{ REG_COM16, 0x41 },/* 0x41 ? */
+	{ OV9655_REG_COM17, OV9655_COM17_EDGE_AUTO |
+			OV9655_COM17_DENOISE_AUTO },
+	{ OV9655_REG_RSVD(1), 0x0a },
+	{ OV9655_REG_RSVD(2), 0xf0 },
+	{ OV9655_REG_RSVD(3), 0x46 },
+	{ OV9655_REG_RSVD(4), 0x62 },
+	{ OV9655_REG_RSVD(5), 0x2a },
+	{ OV9655_REG_RSVD(6), 0x3c },
+	{ OV9655_REG_RSVD(7), 0xfc },
+	{ OV9655_REG_RSVD(8), 0xfc },
+	{ OV9655_REG_RSVD(9), 0x7f },
+	{ OV9655_REG_RSVD(10), 0x7f },
+	{ OV9655_REG_RSVD(11), 0x7f },
+	{ REG_MTX(1), 0x98 },
+	{ REG_MTX(2), 0x98 },
+	{ REG_MTX(3), 0x00 },
+	{ REG_MTX(4), 0x28 },
+	{ REG_MTX(5), 0x70 },
+	{ REG_MTX(6), 0x98 },
+	{ REG_MTXS, 0x1a },
+	{ REG_AWBOP(1), 0x85 },
+	{ REG_AWBOP(2), 0xa9 },
+	{ REG_AWBOP(3), 0x64 },
+	{ REG_AWBOP(4), 0x84 },
+	{ REG_AWBOP(5), 0x53 },
+	{ REG_AWBOP(6), 0x0e },
+	{ REG_BLMT, 0xf0 },
+	{ REG_RLMT, 0xf0 },
+	{ REG_GLMT, 0xf0 },
+	{ REG_LCC(1), 0x00 },
+	{ REG_LCC(2), 0x00 },
+	{ REG_LCC(3), 0x02 },
+	{ REG_LCC(4), 0x20 },
+	{ REG_LCC(5), 0x00 },
+	{ 0x69, 0x0a },/* Reserved... */
+	{ OV9655_REG_DBLV, OV9655_DBLV_PLL_4X | OV9655_DBLV_LDO_BYPASS |
+			OV9655_DBLV_BANDGAP },
+	{ 0x6c, 0x04 },/* Reserved... */
+	{ 0x6d, 0x55 },/* Reserved... */
+	{ 0x6e, 0x00 },/* Reserved... */
+	{ 0x6f, 0x9d },/* Reserved... */
+	{ OV9655_REG_DNSTH, 0x21 },
+	{ 0x71, 0x78 },/* Reserved... */
+	{ OV9655_REG_POIDX, 0x11 },/* QVGA */
+	{ OV9655_REG_PCKDV, 0x01 },/* QVGA */
+	{ OV9655_REG_XINDX, 0x10 },
+	{ OV9655_REG_YINDX, 0x10 },
+	{ 0x76, 0x01 },/* Reserved... */
+	{ 0x77, 0x02 },/* Reserved... */
+	{ 0x7A, 0x12 },/* Reserved... */
+	{ OV9655_REG_GAM(1), 0x08 },
+	{ OV9655_REG_GAM(2), 0x16 },
+	{ OV9655_REG_GAM(3), 0x30 },
+	{ OV9655_REG_GAM(4), 0x5e },
+	{ OV9655_REG_GAM(5), 0x72 },
+	{ OV9655_REG_GAM(6), 0x82 },
+	{ OV9655_REG_GAM(7), 0x8e },
+	{ OV9655_REG_GAM(8), 0x9a },
+	{ OV9655_REG_GAM(9), 0xa4 },
+	{ OV9655_REG_GAM(10), 0xac },
+	{ OV9655_REG_GAM(11), 0xb8 },
+	{ OV9655_REG_GAM(12), 0xc3 },
+	{ OV9655_REG_GAM(13), 0xd6 },
+	{ OV9655_REG_GAM(14), 0xe6 },
+	{ OV9655_REG_GAM(15), 0xf2 },
+	{ 0x8a, 0x24 },/* Reserved... */
+	{ OV9655_REG_COM19, 0x80 },
+	{ 0x90, 0x7d },/* Reserved... */
+	{ 0x91, 0x7b },/* Reserved... */
+	{ REG_LCCFB, 0x02 },
+	{ REG_LCCFR, 0x02 },
+	{ REG_DBLC_GB, 0x7a },
+	{ REG_DBLC_GR, 0x79 },
+	{ REG_AECHM, 0x40 },
+	{ OV9655_REG_COM21, 0x50 },
+	{ 0xa5, 0x68 },/* Reserved... */
+	{ OV9655_REG_AWB_GREEN, 0x4a },
+	{ OV9655_REG_REF_A8, 0xc1 },
+	{ OV9655_REG_REF_A9, 0xef },
+	{ 0xaa, 0x92 },/* Reserved... */
+	{ 0xab, 0x04 },/* Reserved... */
+	{ OV9655_REG_BLC(1), 0x80 },
+	{ OV9655_REG_BLC(2), 0x80 },
+	{ OV9655_REG_BLC(3), 0x80 },
+	{ OV9655_REG_BLC(4), 0x80 },
+	{ OV9655_REG_BLC(7), 0xf2 },
+	{ OV9655_REG_BLC(8), 0x20 },
+	{ OV9655_REG_CTRLB4, 0x20 },
+	{ 0xb5, 0x00 },/* Reserved... */
+	{ 0xb6, 0xaf },/* Reserved... */
+	{ 0xb6, 0xaf },/* Reserved... */
+	{ 0xbb, 0xae },/* Reserved... */
+	{ OV9655_REG_ADBOFF, 0x7f },
+	{ OV9655_REG_ADROFF, 0x7f },
+	{ OV9655_REG_ADGBOFF, 0x7f },
+	{ OV9655_REG_ADGEOFF, 0x7f },
+	{ OV9655_REG_ADGEOFF, 0x7f },
+	{ 0xc0, 0xaa },/* Reserved... */
+	{ 0xc1, 0xc0 },/* Reserved... */
+	{ 0xc2, 0x01 },/* Reserved... */
+	{ 0xc3, 0x4e },/* Reserved... */
+	{ 0xc6, 0x05 },/* Reserved... */
+	{ OV9655_REG_COM24, 0x81 },/* QVGA */
+	{ 0xc9, 0xe0 },/* Reserved... */
+	{ 0xca, 0xe8 },/* Reserved... */
+	{ 0xcb, 0xf0 },/* Reserved... */
+	{ 0xcc, 0xd8 },/* Reserved... */
+	{ 0xcd, 0x93 },/* Reserved... */
+	{ REG_COM7, OV9655_COM7_VGA | OV9655_COM7_RGB },
+	{ REG_COM15, COM15_RGB565 },
+	{ REG_NULL, 0}
+};
+
+static const struct i2c_rv ov9655_qvga_regs[] = {
+	{ REG_HREF, 0x12 },
+	{ OV9655_REG_COM14, OV9655_COM14_ZOOM },
+	{ OV9655_REG_POIDX, 0x11 },
+	{ OV9655_REG_PCKDV, 0x01 },
+	{ OV9655_REG_COM24, 0x81 },
+	{ REG_NULL, 0}
+};
+
+static const struct i2c_rv ov9655_qqvga_regs[] = {
+	{ REG_HREF, 0xa4 },
+	{ REG_COM14, OV9655_COM14_BLACK_PIX | OV9655_COM14_WHITE_PIX |
+			OV9655_COM14_ZOOM },
+	{ OV9655_REG_POIDX, 0x22 },
+	{ OV9655_REG_PCKDV, 0x02 },
+	{ OV9655_REG_COM24, 0x82 },
+	{ REG_NULL, 0}
+};
+
+static const struct i2c_rv ov9655_vga_regs[] = {
+	{ REG_GAIN, 0x11 },
+	{ REG_VREF, 0x12 },
+	{ REG_B_AVE, 0x2e },
+	{ REG_GB_AVE, 0x2e },
+	{ REG_GR_AVE, 0x2e },
+	{ REG_R_AVE, 0x2e },
+	{ REG_COM6, 0x48 },
+	{ REG_AECH, 0x7b },
+	{ REG_CLKRC, 0x03 },
+	{ REG_COM8, COM8_FASTAEC | COM8_AECSTEP | COM8_BFILT |
+			COM8_AGC | COM8_AWB | COM8_AEC },
+	{ REG_HSTART, 0x16 },
+	{ REG_HSTOP, 0x02 },
+	{ REG_VSTART, 0x01 },
+	{ REG_VSTOP, 0x3d },
+	{ REG_MVFP, 0x04 },
+	{ REG_YAVE, 0x2e },
+	{ REG_HREF, 0xff },
+	{ OV9655_AREF1, 0x3d },
+	{ OV9655_AREF3, 0xfa },
+	{ REG_TSLB, 0xcc },
+	{ REG_COM11, 0xcc },
+	{ REG_COM14, 0x0c },
+	{ REG_EDGE, 0x82 },
+	{ REG_COM15, COM15_R00FF | COM15_RGB565 },/* full range */
+	{ REG_COM16, 0x40 },
+	{ OV9655_REG_RSVD(1), 0x14 },
+	{ OV9655_REG_RSVD(2), 0xf0 },
+	{ OV9655_REG_RSVD(3), 0x46 },
+	{ OV9655_REG_RSVD(4), 0x62 },
+	{ OV9655_REG_RSVD(5), 0x2a },
+	{ OV9655_REG_RSVD(6), 0x3c },
+	{ OV9655_REG_RSVD(8), 0xe9 },
+	{ OV9655_REG_RSVD(9), 0xdd },
+	{ OV9655_REG_RSVD(10), 0xdd },
+	{ OV9655_REG_RSVD(11), 0xdd },
+	{ OV9655_REG_RSVD(12), 0xdd },
+	{ REG_LCC(1), 0x00 },
+	{ REG_LCC(2), 0x00 },
+	{ REG_LCC(3), 0x02 },
+	{ REG_LCC(4), 0x20 },
+	{ REG_LCC(5), 0x01 },
+	{ REG_GSP, 0x0c },
+	{ 0x6f, 0x9e },/* Reserved... */
+	{ OV9655_REG_DNSTH, 0x06 },
+	{ OV9655_REG_POIDX, 0x00 },
+	{ OV9655_REG_PCKDV, 0x00 },
+	{ OV9655_REG_XINDX, 0x3a },
+	{ OV9655_REG_YINDX, 0x35 },
+	{ OV9655_REG_SLOP, 0x20 },
+	{ OV9655_REG_GAM(1), 0x1c },
+	{ OV9655_REG_GAM(2), 0x28 },
+	{ OV9655_REG_GAM(3), 0x3c },
+	{ OV9655_REG_GAM(4), 0x5a },
+	{ OV9655_REG_GAM(5), 0x68 },
+	{ OV9655_REG_GAM(6), 0x76 },
+	{ OV9655_REG_GAM(7), 0x80 },
+	{ OV9655_REG_GAM(8), 0x88 },
+	{ OV9655_REG_GAM(9), 0x8f },
+	{ OV9655_REG_GAM(10), 0x96 },
+	{ OV9655_REG_GAM(11), 0xa3 },
+	{ OV9655_REG_GAM(12), 0xaf },
+	{ OV9655_REG_GAM(13), 0xc4 },
+	{ OV9655_REG_GAM(14), 0xd7 },
+	{ OV9655_REG_GAM(15), 0xe8 },
+	{ 0x8a, 0x23 },/* Reserved... */
+	{ OV9655_REG_COM19, 0x8d },
+	{ 0x90, 0x92 },/* Reserved... */
+	{ 0x91, 0x92 },/* Reserved... */
+	{ REG_DBLC_GB, 0x90 },
+	{ REG_DBLC_GR, 0x90 },
+	{ OV9655_REG_AWB_GREEN, 0x40 },
+	{ OV9655_REG_ADBOFF, 0x02 },
+	{ OV9655_REG_ADROFF, 0x01 },
+	{ OV9655_REG_ADGBOFF, 0x02 },
+	{ OV9655_REG_ADGEOFF, 0x01 },
+	{ 0xc1, 0xc8 },/* Reserved... */
+	{ 0xc6, 0x85 },/* Reserved... */
+	{ OV9655_REG_COM24, 0x80 },
+	{ REG_NULL, 0}
+};
+
+static const struct ov965x_framesize ov9655_framesizes[] = {
+	{
+		.width		= VGA_WIDTH,
+		.height		= VGA_HEIGHT,
+		.regs		= ov9655_vga_regs,
+		.max_exp_lines	= 498,
+	}, {
+		.width		= QVGA_WIDTH,
+		.height		= QVGA_HEIGHT,
+		.regs		= ov9655_qvga_regs,
+		.max_exp_lines	= 248,
+	}, {
+		.width		= QQVGA_WIDTH,
+		.height		= QQVGA_HEIGHT,
+		.regs		= ov9655_qqvga_regs,
+		.max_exp_lines	= 124,
+	},
+};
+
+static const struct ov965x_pixfmt ov9655_formats[] = {
+	{ MEDIA_BUS_FMT_RGB565_2X8_LE, V4L2_COLORSPACE_SRGB, 0x08},
+};
+
 static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
 {
 	return &container_of(ctrl->handler, struct ov965x, ctrls.handler)->sd;
@@ -894,12 +1271,16 @@ static int ov965x_set_test_pattern(struct ov965x *ov965x, int value)
 {
 	int ret;
 	u8 reg;
+	u8 addr = (ov965x->id == OV9655V5_ID) ?
+			REG_COM3 : REG_COM23;
+	u8 mask = (ov965x->id == OV9655V5_ID) ?
+			COM3_COLORBAR : COM23_TEST_MODE;
 
-	ret = ov965x_read(ov965x->client, REG_COM23, &reg);
+	ret = ov965x_read(ov965x->client, addr, &reg);
 	if (ret < 0)
 		return ret;
-	reg = value ? reg | COM23_TEST_MODE : reg & ~COM23_TEST_MODE;
-	return ov965x_write(ov965x->client, REG_COM23, reg);
+	reg = value ? reg | mask : reg & ~mask;
+	return ov965x_write(ov965x->client, addr, reg);
 }
 
 static int __g_volatile_ctrl(struct ov965x *ov965x, struct v4l2_ctrl *ctrl)
@@ -1102,6 +1483,30 @@ static int ov965x_initialize_controls(struct ov965x *ov965x)
 	return 0;
 }
 
+static int ov9655_initialize_controls(struct ov965x *ov965x)
+{
+	const struct v4l2_ctrl_ops *ops = &ov965x_ctrl_ops;
+	struct ov965x_ctrls *ctrls = &ov965x->ctrls;
+	struct v4l2_ctrl_handler *hdl = &ctrls->handler;
+	int ret;
+
+	ret = v4l2_ctrl_handler_init(hdl, 16);
+	if (ret < 0)
+		return ret;
+
+	v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_TEST_PATTERN,
+				     ARRAY_SIZE(test_pattern_menu) - 1, 0, 0,
+				     test_pattern_menu);
+	if (hdl->error) {
+		ret = hdl->error;
+		v4l2_ctrl_handler_free(hdl);
+		return ret;
+	}
+
+	ov965x->sd.ctrl_handler = hdl;
+	return 0;
+}
+
 /*
  * V4L2 subdev video and pad level operations
  */
@@ -1516,9 +1921,15 @@ static int ov965x_detect_sensor(struct v4l2_subdev *sd)
 
 	if (!ret) {
 		ov965x->id = OV965X_ID(pid, ver);
-		if (ov965x->id == OV9650_ID || ov965x->id == OV9652_ID) {
+		switch (ov965x->id) {
+		case OV9650_ID:
+		case OV9652_ID:
 			v4l2_info(sd, "Found OV%04X sensor\n", ov965x->id);
-		} else {
+			break;
+		case OV9655V5_ID:
+			v4l2_info(sd, "Found OV%04X sensor\n", ov965x->id - 2);
+			break;
+		default:
 			v4l2_err(sd, "Sensor detection failed (%04X, %d)\n",
 				 ov965x->id, ret);
 			ret = -ENODEV;
@@ -1595,18 +2006,28 @@ static int ov965x_probe(struct i2c_client *client,
 	if (ret < 0)
 		goto err_me;
 
-	ov965x->init_regs = ov965x_init_regs;
-	ov965x->initialize_controls = ov965x_initialize_controls;
-	ov965x->framesizes = ov965x_framesizes;
-	ov965x->nb_of_framesizes = ARRAY_SIZE(ov965x_framesizes);
-	ov965x->formats = ov965x_formats;
-	ov965x->nb_of_formats = ARRAY_SIZE(ov965x_formats);
-	ov965x->intervals = ov965x_intervals;
-	ov965x->nb_of_intervals = ARRAY_SIZE(ov965x_intervals);
-	ov965x->fiv = &ov965x_intervals[0];
-	ov965x->set_frame_interval = __ov965x_set_frame_interval;
-	ov965x->update_exposure_ctrl = ov965x_update_exposure_ctrl;
-	ov965x->set_params = __ov965x_set_params;
+	if (ov965x->id != OV9655V5_ID) {
+		ov965x->init_regs = ov965x_init_regs;
+		ov965x->initialize_controls = ov965x_initialize_controls;
+		ov965x->framesizes = ov965x_framesizes;
+		ov965x->nb_of_framesizes = ARRAY_SIZE(ov965x_framesizes);
+		ov965x->formats = ov965x_formats;
+		ov965x->nb_of_formats = ARRAY_SIZE(ov965x_formats);
+		ov965x->intervals = ov965x_intervals;
+		ov965x->nb_of_intervals = ARRAY_SIZE(ov965x_intervals);
+		ov965x->fiv = &ov965x_intervals[0];
+		ov965x->set_frame_interval = __ov965x_set_frame_interval;
+		ov965x->update_exposure_ctrl = ov965x_update_exposure_ctrl;
+		ov965x->set_params = __ov965x_set_params;
+	} else {
+		ov965x->init_regs = ov9655_init_regs;
+		ov965x->initialize_controls = ov9655_initialize_controls;
+		ov965x->framesizes = ov9655_framesizes;
+		ov965x->nb_of_framesizes = ARRAY_SIZE(ov9655_framesizes);
+		ov965x->formats = ov9655_formats;
+		ov965x->nb_of_formats = ARRAY_SIZE(ov9655_formats);
+		ov965x->set_params = ov965x_set_frame_size;
+	}
 
 	ov965x->frame_size = &ov965x->framesizes[0];
 	ov965x_get_default_format(ov965x, &ov965x->format);
@@ -1650,6 +2071,7 @@ static int ov965x_remove(struct i2c_client *client)
 static const struct i2c_device_id ov965x_id[] = {
 	{ "ov9650", 0 },
 	{ "ov9652", 0 },
+	{ "ov9655", 0 },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(i2c, ov965x_id);
@@ -1657,6 +2079,7 @@ static int ov965x_remove(struct i2c_client *client)
 static const struct of_device_id ov965x_of_match[] = {
 	{ .compatible = "ovti,ov9650", },
 	{ .compatible = "ovti,ov9652", },
+	{ .compatible = "ovti,ov9655", },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, ov965x_of_match);
@@ -1674,5 +2097,5 @@ static int ov965x_remove(struct i2c_client *client)
 module_i2c_driver(ov965x_i2c_driver);
 
 MODULE_AUTHOR("Sylwester Nawrocki <sylvester.nawrocki@gmail.com>");
-MODULE_DESCRIPTION("OV9650/OV9652 CMOS Image Sensor driver");
+MODULE_DESCRIPTION("OV9650/OV9652/OV9655 CMOS Image Sensor driver");
 MODULE_LICENSE("GPL");
-- 
1.9.1

[PATCH v2 7/7] [media] ov9650: add analog power supply and clock gating

[Hugues Fruchet]

Add optional analog power supply and clock gating.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
---
 drivers/media/i2c/ov9650.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c
index 9ff0782..56b1eb3 100644
--- a/drivers/media/i2c/ov9650.c
+++ b/drivers/media/i2c/ov9650.c
@@ -29,6 +29,7 @@
 #include <linux/module.h>
 #include <linux/of_gpio.h>
 #include <linux/ratelimit.h>
+#include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/videodev2.h>
@@ -374,6 +375,7 @@ struct ov965x {
 	/* External master clock frequency */
 	unsigned long mclk_frequency;
 	struct clk *clk;
+	struct regulator *avdd;
 
 	/* Protects the struct fields below */
 	struct mutex lock;
@@ -943,13 +945,31 @@ static void ov965x_gpio_set(struct gpio_desc *gpio, int val)
 
 static void __ov965x_set_power(struct ov965x *ov965x, int on)
 {
+	int ret;
+
 	if (on) {
+		/* Bring up the power supply */
+		ret = regulator_enable(ov965x->avdd);
+		if (ret)
+			dev_warn(&ov965x->client->dev,
+				 "Failed to enable analog power (%d)\n", ret);
+		msleep(25);
+		/* Enable clock */
+		ret = clk_prepare_enable(ov965x->clk);
+		if (ret)
+			dev_warn(&ov965x->client->dev,
+				 "Failed to enable clock (%d)\n", ret);
+		msleep(25);
+
 		ov965x_gpio_set(ov965x->gpios[GPIO_PWDN], 0);
 		ov965x_gpio_set(ov965x->gpios[GPIO_RST], 0);
 		msleep(25);
 	} else {
 		ov965x_gpio_set(ov965x->gpios[GPIO_RST], 1);
 		ov965x_gpio_set(ov965x->gpios[GPIO_PWDN], 1);
+
+		clk_disable_unprepare(ov965x->clk);
+		regulator_disable(ov965x->avdd);
 	}
 
 	ov965x->streaming = 0;
@@ -1969,6 +1989,12 @@ static int ov965x_probe(struct i2c_client *client,
 			devm_gpiod_get_optional(&client->dev, "pwdn",
 						GPIOD_OUT_HIGH);
 
+		ov965x->avdd = devm_regulator_get(&client->dev, "avdd");
+		if (IS_ERR(ov965x->avdd)) {
+			dev_err(&client->dev, "Could not get analog regulator\n");
+			return PTR_ERR(ov965x->avdd);
+		}
+
 		ov965x->clk = devm_clk_get(&client->dev, NULL);
 		if (IS_ERR(ov965x->clk)) {
 			dev_err(&client->dev, "Could not get clock\n");
-- 
1.9.1

[PATCH v2 0/7] [PATCH v2 0/7] Add support of OV9655 camera

[Hugues FRUCHET]

Hi Sylwester,

Do you have the possibility to check for non-regression of this patchset 
on 9650/52 camera ?

Best regards,
Hugues.

On 07/03/2017 11:16 AM, Hugues Fruchet wrote:
> This patchset enables OV9655 camera support.
> 
> OV9655 support has been tested using STM32F4DIS-CAM extension board
> plugged on connector P1 of STM32F746G-DISCO board.
> Due to lack of OV9650/52 hardware support, the modified related code
> could not have been checked for non-regression.
> 
> First patches upgrade current support of OV9650/52 to prepare then
> introduction of OV9655 variant patch.
> Because of OV9655 register set slightly different from OV9650/9652,
> not all of the driver features are supported (controls). Supported
> resolutions are limited to VGA, QVGA, QQVGA.
> Supported format is limited to RGB565.
> Controls are limited to color bar test pattern for test purpose.
> 
> OV9655 initial support is based on a driver written by H. Nikolaus Schaller [1].
> OV9655 registers sequences come from STM32CubeF7 embedded software [2].
> 
> [1] http://git.goldelico.com/?p=gta04-kernel.git;a=shortlog;h=refs/heads/work/hns/video/ov9655
> [2] https://developer.mbed.org/teams/ST/code/BSP_DISCO_F746NG/file/e1d9da7fe856/Drivers/BSP/Components/ov9655/ov9655.c
> 
> ===========
> = history =
> ===========
> version 2:
>    - Remove some unneeded semicolons (kbuild test robot):
>        http://www.mail-archive.com/linux-media at vger.kernel.org/msg114616.html
>    - Remove patch [media] ov9650: select the nearest higher resolution:
>      it is up to the application to find the best matching resolution
>      using ENUM_FRAMESIZES/S_FMT/S_SELECTION (S_CROP), see
>        http://www.mail-archive.com/linux-media at vger.kernel.org/msg114667.html
>    - dt-bindings: Fix remarks from Rob Herring about polarity:
>        http://www.mail-archive.com/linux-media at vger.kernel.org/msg114705.html
>    - dt-bindings: Add optional regulators avdd, dvdd, dovdd:
>        http://www.mail-archive.com/linux-media at vger.kernel.org/msg114785.html
>    - fix missing semicolons in if condition:
>        http://www.mail-archive.com/linux-media at vger.kernel.org/msg114611.html
>    - move ov965x_pixfmt relocation in right patch:
>        http://www.mail-archive.com/linux-media at vger.kernel.org/msg114849.html
>    - revisit OV965x renaming to ov965x for device id names and DT compatible strings,
>      drop of_device_id .data device identification
>        http://www.mail-archive.com/linux-media at vger.kernel.org/msg114635.html
>        http://www.mail-archive.com/linux-media at vger.kernel.org/msg114738.html
>    - Add analog power supply and clock gating, needed for GTA04 platform:
>        http://www.mail-archive.com/linux-media at vger.kernel.org/msg114519.html
> 
> version 1:
>    - Initial submission.
> 
> H. Nikolaus Schaller (1):
>    DT bindings: add bindings for ov965x camera module
> 
> Hugues Fruchet (6):
>    [media] ov9650: switch i2c device id to lower case
>    [media] ov9650: add device tree support
>    [media] ov9650: use write_array() for resolution sequences
>    [media] ov9650: add multiple variant support
>    [media] ov9650: add support of OV9655 variant
>    [media] ov9650: add analog power supply and clock gating
> 
>   .../devicetree/bindings/media/i2c/ov965x.txt       |  45 ++
>   drivers/media/i2c/Kconfig                          |   6 +-
>   drivers/media/i2c/ov9650.c                         | 816 +++++++++++++++++----
>   3 files changed, 736 insertions(+), 131 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/media/i2c/ov965x.txt
> 

[PATCH v2 0/7] [PATCH v2 0/7] Add support of OV9655 camera

[Sylwester Nawrocki]

Hi Hugues,

On 07/06/2017 09:51 AM, Hugues FRUCHET wrote:
> Hi Sylwester,
> 
> Do you have the possibility to check for non-regression of this patchset
> on 9650/52 camera ?

I will try to test your patch set once I find the camera module for
my Micro2440SDK board. I've spent already a day on setting up everything 
and fixing multiple regressions in the kernel. I will likely try your 
patch series in coming week.

--
Thanks,
Sylwester

[PATCH v2 0/7] [PATCH v2 0/7] Add support of OV9655 camera

[Sylwester Nawrocki]

Hi Hugues,

On 07/03/2017 11:16 AM, Hugues Fruchet wrote:
> This patchset enables OV9655 camera support.
> 
> OV9655 support has been tested using STM32F4DIS-CAM extension board
> plugged on connector P1 of STM32F746G-DISCO board.
> Due to lack of OV9650/52 hardware support, the modified related code
> could not have been checked for non-regression.
> 
> First patches upgrade current support of OV9650/52 to prepare then
> introduction of OV9655 variant patch.
> Because of OV9655 register set slightly different from OV9650/9652,
> not all of the driver features are supported (controls). Supported
> resolutions are limited to VGA, QVGA, QQVGA.
> Supported format is limited to RGB565.
> Controls are limited to color bar test pattern for test purpose.

I appreciate your efforts towards making a common driver but IMO it would be 
better to create a separate driver for the OV9655 sensor.  The original driver 
is 1576 lines of code, your patch set adds half of that (816).  There are
significant differences in the feature set of both sensors, there are 
differences in the register layout.  I would go for a separate driver, we  
would then have code easier to follow and wouldn't need to worry about possible
regressions.  I'm afraid I have lost the camera module and won't be able 
to test the patch set against regressions.

IMHO from maintenance POV it's better to make a separate driver. In the end 
of the day we wouldn't be adding much more code than it is being done now.

>   .../devicetree/bindings/media/i2c/ov965x.txt       |  45 ++
>   drivers/media/i2c/Kconfig                          |   6 +-
>   drivers/media/i2c/ov9650.c                         | 816 +++++++++++++++++----
>   3 files changed, 736 insertions(+), 131 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/media/i2c/ov965x.txt

--
Thanks,
Sylwester

[PATCH v2 0/7] [PATCH v2 0/7] Add support of OV9655 camera

[Hans Verkuil]

On 12/07/17 22:01, Sylwester Nawrocki wrote:
> Hi Hugues,
> 
> On 07/03/2017 11:16 AM, Hugues Fruchet wrote:
>> This patchset enables OV9655 camera support.
>>
>> OV9655 support has been tested using STM32F4DIS-CAM extension board
>> plugged on connector P1 of STM32F746G-DISCO board.
>> Due to lack of OV9650/52 hardware support, the modified related code
>> could not have been checked for non-regression.
>>
>> First patches upgrade current support of OV9650/52 to prepare then
>> introduction of OV9655 variant patch.
>> Because of OV9655 register set slightly different from OV9650/9652,
>> not all of the driver features are supported (controls). Supported
>> resolutions are limited to VGA, QVGA, QQVGA.
>> Supported format is limited to RGB565.
>> Controls are limited to color bar test pattern for test purpose.
> 
> I appreciate your efforts towards making a common driver but IMO it would be 
> better to create a separate driver for the OV9655 sensor.  The original driver 
> is 1576 lines of code, your patch set adds half of that (816).  There are
> significant differences in the feature set of both sensors, there are 
> differences in the register layout.  I would go for a separate driver, we  
> would then have code easier to follow and wouldn't need to worry about possible
> regressions.  I'm afraid I have lost the camera module and won't be able 
> to test the patch set against regressions.
> 
> IMHO from maintenance POV it's better to make a separate driver. In the end 
> of the day we wouldn't be adding much more code than it is being done now.

I agree. We do not have great experiences in the past with trying to support
multiple variants in a single driver (unless the diffs are truly small).

Regards,

	Hans

> 
>>   .../devicetree/bindings/media/i2c/ov965x.txt       |  45 ++
>>   drivers/media/i2c/Kconfig                          |   6 +-
>>   drivers/media/i2c/ov9650.c                         | 816 +++++++++++++++++----
>>   3 files changed, 736 insertions(+), 131 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/media/i2c/ov965x.txt
> 
> --
> Thanks,
> Sylwester
> 

[PATCH v2 0/7] [PATCH v2 0/7] Add support of OV9655 camera

[H. Nikolaus Schaller]

Hi,

> Am 18.07.2017 um 13:59 schrieb Hans Verkuil <hverkuil@xs4all.nl>:
> 
> On 12/07/17 22:01, Sylwester Nawrocki wrote:
>> Hi Hugues,
>> 
>> On 07/03/2017 11:16 AM, Hugues Fruchet wrote:
>>> This patchset enables OV9655 camera support.
>>> 
>>> OV9655 support has been tested using STM32F4DIS-CAM extension board
>>> plugged on connector P1 of STM32F746G-DISCO board.
>>> Due to lack of OV9650/52 hardware support, the modified related code
>>> could not have been checked for non-regression.
>>> 
>>> First patches upgrade current support of OV9650/52 to prepare then
>>> introduction of OV9655 variant patch.
>>> Because of OV9655 register set slightly different from OV9650/9652,
>>> not all of the driver features are supported (controls). Supported
>>> resolutions are limited to VGA, QVGA, QQVGA.
>>> Supported format is limited to RGB565.
>>> Controls are limited to color bar test pattern for test purpose.
>> 
>> I appreciate your efforts towards making a common driver but IMO it would be 
>> better to create a separate driver for the OV9655 sensor.  The original driver 
>> is 1576 lines of code, your patch set adds half of that (816).  There are
>> significant differences in the feature set of both sensors, there are 
>> differences in the register layout.  I would go for a separate driver, we  
>> would then have code easier to follow and wouldn't need to worry about possible
>> regressions.  I'm afraid I have lost the camera module and won't be able 
>> to test the patch set against regressions.
>> 
>> IMHO from maintenance POV it's better to make a separate driver. In the end 
>> of the day we wouldn't be adding much more code than it is being done now.
> 
> I agree. We do not have great experiences in the past with trying to support
> multiple variants in a single driver (unless the diffs are truly small).

Well,
IMHO the diffs in ov965x are smaller (but untestable because nobody seems
to have an ov9650/52 board) than within the bq27xxx chips, but I can dig out
an old pdata based separate ov9655 driver and extend that to become DT compatible.

I had abandoned that separate approach in favour of extending the ov965x driver.

Have to discuss with Hugues how to proceed.

BR and thanks,
Nikolaus

[PATCH v2 0/7] [PATCH v2 0/7] Add support of OV9655 camera

[Hugues FRUCHET]

On 07/18/2017 02:17 PM, H. Nikolaus Schaller wrote:
> Hi,
> 
>> Am 18.07.2017 um 13:59 schrieb Hans Verkuil <hverkuil@xs4all.nl>:
>>
>> On 12/07/17 22:01, Sylwester Nawrocki wrote:
>>> Hi Hugues,
>>>
>>> On 07/03/2017 11:16 AM, Hugues Fruchet wrote:
>>>> This patchset enables OV9655 camera support.
>>>>
>>>> OV9655 support has been tested using STM32F4DIS-CAM extension board
>>>> plugged on connector P1 of STM32F746G-DISCO board.
>>>> Due to lack of OV9650/52 hardware support, the modified related code
>>>> could not have been checked for non-regression.
>>>>
>>>> First patches upgrade current support of OV9650/52 to prepare then
>>>> introduction of OV9655 variant patch.
>>>> Because of OV9655 register set slightly different from OV9650/9652,
>>>> not all of the driver features are supported (controls). Supported
>>>> resolutions are limited to VGA, QVGA, QQVGA.
>>>> Supported format is limited to RGB565.
>>>> Controls are limited to color bar test pattern for test purpose.
>>>
>>> I appreciate your efforts towards making a common driver but IMO it would be
>>> better to create a separate driver for the OV9655 sensor.  The original driver
>>> is 1576 lines of code, your patch set adds half of that (816).  There are
>>> significant differences in the feature set of both sensors, there are
>>> differences in the register layout.  I would go for a separate driver, we
>>> would then have code easier to follow and wouldn't need to worry about possible
>>> regressions.  I'm afraid I have lost the camera module and won't be able
>>> to test the patch set against regressions.
>>>
>>> IMHO from maintenance POV it's better to make a separate driver. In the end
>>> of the day we wouldn't be adding much more code than it is being done now.
>>
>> I agree. We do not have great experiences in the past with trying to support
>> multiple variants in a single driver (unless the diffs are truly small).
> 
> Well,
> IMHO the diffs in ov965x are smaller (but untestable because nobody seems
> to have an ov9650/52 board) than within the bq27xxx chips, but I can dig out
> an old pdata based separate ov9655 driver and extend that to become DT compatible.
> 
> I had abandoned that separate approach in favour of extending the ov965x driver.
> 
> Have to discuss with Hugues how to proceed.
> 
> BR and thanks,
> Nikolaus
> 

As Sylwester and Hans, I'm also in flavour of a separate driver, the 
fact that register set seems similar but in fact is not and that we 
cannot test for non-regression of 9650/52 are killer for me to continue 
on a single driver.
We can now restart from a new fresh state of the art sensor driver 
getting rid of legacy (pdata, old gpio, etc...).

BR,
Hugues.

[PATCH v2 0/7] [PATCH v2 0/7] Add support of OV9655 camera

[Sakari Ailus]

On Tue, Jul 18, 2017 at 12:53:12PM +0000, Hugues FRUCHET wrote:
> 
> 
> On 07/18/2017 02:17 PM, H. Nikolaus Schaller wrote:
> > Hi,
> > 
> >> Am 18.07.2017 um 13:59 schrieb Hans Verkuil <hverkuil@xs4all.nl>:
> >>
> >> On 12/07/17 22:01, Sylwester Nawrocki wrote:
> >>> Hi Hugues,
> >>>
> >>> On 07/03/2017 11:16 AM, Hugues Fruchet wrote:
> >>>> This patchset enables OV9655 camera support.
> >>>>
> >>>> OV9655 support has been tested using STM32F4DIS-CAM extension board
> >>>> plugged on connector P1 of STM32F746G-DISCO board.
> >>>> Due to lack of OV9650/52 hardware support, the modified related code
> >>>> could not have been checked for non-regression.
> >>>>
> >>>> First patches upgrade current support of OV9650/52 to prepare then
> >>>> introduction of OV9655 variant patch.
> >>>> Because of OV9655 register set slightly different from OV9650/9652,
> >>>> not all of the driver features are supported (controls). Supported
> >>>> resolutions are limited to VGA, QVGA, QQVGA.
> >>>> Supported format is limited to RGB565.
> >>>> Controls are limited to color bar test pattern for test purpose.
> >>>
> >>> I appreciate your efforts towards making a common driver but IMO it would be
> >>> better to create a separate driver for the OV9655 sensor.  The original driver
> >>> is 1576 lines of code, your patch set adds half of that (816).  There are
> >>> significant differences in the feature set of both sensors, there are
> >>> differences in the register layout.  I would go for a separate driver, we
> >>> would then have code easier to follow and wouldn't need to worry about possible
> >>> regressions.  I'm afraid I have lost the camera module and won't be able
> >>> to test the patch set against regressions.
> >>>
> >>> IMHO from maintenance POV it's better to make a separate driver. In the end
> >>> of the day we wouldn't be adding much more code than it is being done now.
> >>
> >> I agree. We do not have great experiences in the past with trying to support
> >> multiple variants in a single driver (unless the diffs are truly small).
> > 
> > Well,
> > IMHO the diffs in ov965x are smaller (but untestable because nobody seems
> > to have an ov9650/52 board) than within the bq27xxx chips, but I can dig out
> > an old pdata based separate ov9655 driver and extend that to become DT compatible.
> > 
> > I had abandoned that separate approach in favour of extending the ov965x driver.
> > 
> > Have to discuss with Hugues how to proceed.
> > 
> > BR and thanks,
> > Nikolaus
> > 
> 
> As Sylwester and Hans, I'm also in flavour of a separate driver, the 
> fact that register set seems similar but in fact is not and that we 
> cannot test for non-regression of 9650/52 are killer for me to continue 
> on a single driver.
> We can now restart from a new fresh state of the art sensor driver 
> getting rid of legacy (pdata, old gpio, etc...).

Agreed. I bet the result will look cleaner indeed although this wasn't one
of the complex drivers.

It'd be nice that someone was able to test the ov9650/2, too, drivers that
are never used tend to break...

-- 
Sakari Ailus
e-mail: sakari.ailus at iki.fi	XMPP: sailus at retiisi.org.uk

[PATCH v2 0/7] Add support of OV9655 camera

[H. Nikolaus Schaller]

Hi,

> Am 18.07.2017 um 21:52 schrieb Sakari Ailus <sakari.ailus@iki.fi>:
> 
> On Tue, Jul 18, 2017 at 12:53:12PM +0000, Hugues FRUCHET wrote:
>> 
>> 
>> On 07/18/2017 02:17 PM, H. Nikolaus Schaller wrote:
>>> Hi,
>>> 
>>>> Am 18.07.2017 um 13:59 schrieb Hans Verkuil <hverkuil@xs4all.nl>:
>>>> 
>>>> On 12/07/17 22:01, Sylwester Nawrocki wrote:
>>>>> Hi Hugues,
>>>>> 
>>>>> On 07/03/2017 11:16 AM, Hugues Fruchet wrote:
>>>>>> This patchset enables OV9655 camera support.
>>>>>> 
>>>>>> OV9655 support has been tested using STM32F4DIS-CAM extension board
>>>>>> plugged on connector P1 of STM32F746G-DISCO board.
>>>>>> Due to lack of OV9650/52 hardware support, the modified related code
>>>>>> could not have been checked for non-regression.
>>>>>> 
>>>>>> First patches upgrade current support of OV9650/52 to prepare then
>>>>>> introduction of OV9655 variant patch.
>>>>>> Because of OV9655 register set slightly different from OV9650/9652,
>>>>>> not all of the driver features are supported (controls). Supported
>>>>>> resolutions are limited to VGA, QVGA, QQVGA.
>>>>>> Supported format is limited to RGB565.
>>>>>> Controls are limited to color bar test pattern for test purpose.
>>>>> 
>>>>> I appreciate your efforts towards making a common driver but IMO it would be
>>>>> better to create a separate driver for the OV9655 sensor.  The original driver
>>>>> is 1576 lines of code, your patch set adds half of that (816).  There are
>>>>> significant differences in the feature set of both sensors, there are
>>>>> differences in the register layout.  I would go for a separate driver, we
>>>>> would then have code easier to follow and wouldn't need to worry about possible
>>>>> regressions.  I'm afraid I have lost the camera module and won't be able
>>>>> to test the patch set against regressions.
>>>>> 
>>>>> IMHO from maintenance POV it's better to make a separate driver. In the end
>>>>> of the day we wouldn't be adding much more code than it is being done now.
>>>> 
>>>> I agree. We do not have great experiences in the past with trying to support
>>>> multiple variants in a single driver (unless the diffs are truly small).
>>> 
>>> Well,
>>> IMHO the diffs in ov965x are smaller (but untestable because nobody seems
>>> to have an ov9650/52 board) than within the bq27xxx chips, but I can dig out
>>> an old pdata based separate ov9655 driver and extend that to become DT compatible.
>>> 
>>> I had abandoned that separate approach in favour of extending the ov965x driver.
>>> 
>>> Have to discuss with Hugues how to proceed.
>>> 
>>> BR and thanks,
>>> Nikolaus
>>> 
>> 
>> As Sylwester and Hans, I'm also in flavour of a separate driver, the 
>> fact that register set seems similar but in fact is not and that we 
>> cannot test for non-regression of 9650/52 are killer for me to continue 
>> on a single driver.
>> We can now restart from a new fresh state of the art sensor driver 
>> getting rid of legacy (pdata, old gpio, etc...).
> 
> Agreed. I bet the result will look cleaner indeed although this wasn't one
> of the complex drivers.

I finally managed to find the bug why mplayer did select-timeout on the GTA04.
Was a bug in pinmux setup of the GTA04 for the omap3isp.

And I have resurrected our years old 3.12 camera driver, which was based on the
MT9P031 code. It was already separate from ov9650/52.

I have extended it to support DT by including some parts of Hugues' work.

It still needs some cleanup and discussion but will be a simple patch (one
for ov9655.c + Kconfig + Makefile) and one for bindings (I hope it includes
all your comments).

I will post v1 in the next days.

BR,
Nikolaus

[PATCH v2 0/7] Add support of OV9655 camera

[Hugues FRUCHET]

On 07/20/2017 10:37 AM, H. Nikolaus Schaller wrote:
> Hi,
> 
>> Am 18.07.2017 um 21:52 schrieb Sakari Ailus <sakari.ailus@iki.fi>:
>>
>> On Tue, Jul 18, 2017 at 12:53:12PM +0000, Hugues FRUCHET wrote:
>>>
>>>
>>> On 07/18/2017 02:17 PM, H. Nikolaus Schaller wrote:
>>>> Hi,
>>>>
>>>>> Am 18.07.2017 um 13:59 schrieb Hans Verkuil <hverkuil@xs4all.nl>:
>>>>>
>>>>> On 12/07/17 22:01, Sylwester Nawrocki wrote:
>>>>>> Hi Hugues,
>>>>>>
>>>>>> On 07/03/2017 11:16 AM, Hugues Fruchet wrote:
>>>>>>> This patchset enables OV9655 camera support.
>>>>>>>
>>>>>>> OV9655 support has been tested using STM32F4DIS-CAM extension board
>>>>>>> plugged on connector P1 of STM32F746G-DISCO board.
>>>>>>> Due to lack of OV9650/52 hardware support, the modified related code
>>>>>>> could not have been checked for non-regression.
>>>>>>>
>>>>>>> First patches upgrade current support of OV9650/52 to prepare then
>>>>>>> introduction of OV9655 variant patch.
>>>>>>> Because of OV9655 register set slightly different from OV9650/9652,
>>>>>>> not all of the driver features are supported (controls). Supported
>>>>>>> resolutions are limited to VGA, QVGA, QQVGA.
>>>>>>> Supported format is limited to RGB565.
>>>>>>> Controls are limited to color bar test pattern for test purpose.
>>>>>>
>>>>>> I appreciate your efforts towards making a common driver but IMO it would be
>>>>>> better to create a separate driver for the OV9655 sensor.  The original driver
>>>>>> is 1576 lines of code, your patch set adds half of that (816).  There are
>>>>>> significant differences in the feature set of both sensors, there are
>>>>>> differences in the register layout.  I would go for a separate driver, we
>>>>>> would then have code easier to follow and wouldn't need to worry about possible
>>>>>> regressions.  I'm afraid I have lost the camera module and won't be able
>>>>>> to test the patch set against regressions.
>>>>>>
>>>>>> IMHO from maintenance POV it's better to make a separate driver. In the end
>>>>>> of the day we wouldn't be adding much more code than it is being done now.
>>>>>
>>>>> I agree. We do not have great experiences in the past with trying to support
>>>>> multiple variants in a single driver (unless the diffs are truly small).
>>>>
>>>> Well,
>>>> IMHO the diffs in ov965x are smaller (but untestable because nobody seems
>>>> to have an ov9650/52 board) than within the bq27xxx chips, but I can dig out
>>>> an old pdata based separate ov9655 driver and extend that to become DT compatible.
>>>>
>>>> I had abandoned that separate approach in favour of extending the ov965x driver.
>>>>
>>>> Have to discuss with Hugues how to proceed.
>>>>
>>>> BR and thanks,
>>>> Nikolaus
>>>>
>>>
>>> As Sylwester and Hans, I'm also in flavour of a separate driver, the
>>> fact that register set seems similar but in fact is not and that we
>>> cannot test for non-regression of 9650/52 are killer for me to continue
>>> on a single driver.
>>> We can now restart from a new fresh state of the art sensor driver
>>> getting rid of legacy (pdata, old gpio, etc...).
>>
>> Agreed. I bet the result will look cleaner indeed although this wasn't one
>> of the complex drivers.
> 
> I finally managed to find the bug why mplayer did select-timeout on the GTA04.
> Was a bug in pinmux setup of the GTA04 for the omap3isp.
> 
> And I have resurrected our years old 3.12 camera driver, which was based on the
> MT9P031 code. It was already separate from ov9650/52.
> 
> I have extended it to support DT by including some parts of Hugues' work.
> 
> It still needs some cleanup and discussion but will be a simple patch (one
> for ov9655.c + Kconfig + Makefile) and one for bindings (I hope it includes
> all your comments).
> 
> I will post v1 in the next days.
> 
> BR,
> Nikolaus
> 

Thanks Nikolaus,

I was ready to push the new version in new file ov9655.c with all 
comments included, but as my version is very minimal and I suspect that 
yours is more complete, let's merge things together.
Can I consider that you now take ownership of this driver upstream ?
If so I'll send to you my current patchset so you can compare, 
double-check review comments and add missing support on your side 
(RGB565 and VGA/QVGA resolution matter on my side).

Thanks again Nikolaus for this work,

BR,
Hugues.