[PATCH v3 0/5] Add OV5640 parallel interface and RGB565/YUYV support

[PATCH v3 0/5] Add OV5640 parallel interface and RGB565/YUYV support

[Hugues Fruchet]

Enhance OV5640 CSI driver to support also DVP parallel interface.
Add RGB565 (LE & BE) and YUV422 YUYV format in addition to existing
YUV422 UYVY format.
Some other improvements on chip identifier check and removal
of warnings in powering phase around gpio handling.

===========
= history =
===========
version 3:
  - Move chip identifier check at probe according to Fabio Estevam comment:
    https://www.mail-archive.com/linux-media@vger.kernel.org/msg122575.html
  - Use 16 bits register read for this check as per Steve Longerbeam comment:
    https://www.mail-archive.com/linux-media@vger.kernel.org/msg122692.html
  - Update bindings to document parallel mode support as per Fabio Estevam comment:
    https://www.mail-archive.com/linux-media@vger.kernel.org/msg122576.html
  - Enable the whole 10 bits parallel output and document 8/10 bits support
    in ov5640_set_stream_dvp() to answer to Steve Longerbeam comment:
    https://www.mail-archive.com/linux-media@vger.kernel.org/msg122693.html

version 2:
  - Fix comments from Sakari Ailus:
    https://www.mail-archive.com/linux-media@vger.kernel.org/msg122259.html
  - Revisit ov5640_set_stream_dvp() to only configure DVP at streamon
  - Revisit ov5640_set_stream_dvp() implementation with fewer register settings

version 1:
  - Initial submission

Hugues Fruchet (5):
  media: ov5640: switch to gpiod_set_value_cansleep()
  media: ov5640: check chip id
  media: dt-bindings: ov5640: add support of DVP parallel interface
  media: ov5640: add support of DVP parallel interface
  media: ov5640: add support of RGB565 and YUYV formats

 .../devicetree/bindings/media/i2c/ov5640.txt       |  27 +-
 drivers/media/i2c/ov5640.c                         | 291 +++++++++++++++++----
 2 files changed, 271 insertions(+), 47 deletions(-)

-- 
1.9.1

[PATCH v3 1/5] media: ov5640: switch to gpiod_set_value_cansleep()

[Hugues Fruchet]

Switch gpiod_set_value to gpiod_set_value_cansleep to avoid
warnings when powering sensor.

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

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index c89ed66..61071f5 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -1524,7 +1524,7 @@ static int ov5640_restore_mode(struct ov5640_dev *sensor)
 
 static void ov5640_power(struct ov5640_dev *sensor, bool enable)
 {
-	gpiod_set_value(sensor->pwdn_gpio, enable ? 0 : 1);
+	gpiod_set_value_cansleep(sensor->pwdn_gpio, enable ? 0 : 1);
 }
 
 static void ov5640_reset(struct ov5640_dev *sensor)
@@ -1532,7 +1532,7 @@ static void ov5640_reset(struct ov5640_dev *sensor)
 	if (!sensor->reset_gpio)
 		return;
 
-	gpiod_set_value(sensor->reset_gpio, 0);
+	gpiod_set_value_cansleep(sensor->reset_gpio, 0);
 
 	/* camera power cycle */
 	ov5640_power(sensor, false);
@@ -1540,10 +1540,10 @@ static void ov5640_reset(struct ov5640_dev *sensor)
 	ov5640_power(sensor, true);
 	usleep_range(5000, 10000);
 
-	gpiod_set_value(sensor->reset_gpio, 1);
+	gpiod_set_value_cansleep(sensor->reset_gpio, 1);
 	usleep_range(1000, 2000);
 
-	gpiod_set_value(sensor->reset_gpio, 0);
+	gpiod_set_value_cansleep(sensor->reset_gpio, 0);
 	usleep_range(5000, 10000);
 }
 
-- 
1.9.1

[PATCH v3 2/5] media: ov5640: check chip id

[Hugues Fruchet]

Verify that chip identifier is correct when probing.

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

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 61071f5..9f031f3 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -1547,24 +1547,58 @@ static void ov5640_reset(struct ov5640_dev *sensor)
 	usleep_range(5000, 10000);
 }
 
-static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
+static int ov5640_set_power_on(struct ov5640_dev *sensor)
 {
-	int ret = 0;
+	struct i2c_client *client = sensor->i2c_client;
+	int ret;
 
-	if (on) {
-		clk_prepare_enable(sensor->xclk);
+	ret = clk_prepare_enable(sensor->xclk);
+	if (ret) {
+		dev_err(&client->dev, "%s: failed to enable clock\n",
+			__func__);
+		return ret;
+	}
 
-		ret = regulator_bulk_enable(OV5640_NUM_SUPPLIES,
-					    sensor->supplies);
-		if (ret)
-			goto xclk_off;
+	ret = regulator_bulk_enable(OV5640_NUM_SUPPLIES,
+				    sensor->supplies);
+	if (ret) {
+		dev_err(&client->dev, "%s: failed to enable regulators\n",
+			__func__);
+		goto xclk_off;
+	}
+
+	ov5640_reset(sensor);
+	ov5640_power(sensor, true);
+
+	ret = ov5640_init_slave_id(sensor);
+	if (ret)
+		goto power_off;
+
+	return 0;
+
+power_off:
+	ov5640_power(sensor, false);
+	regulator_bulk_disable(OV5640_NUM_SUPPLIES, sensor->supplies);
+xclk_off:
+	clk_disable_unprepare(sensor->xclk);
+	return ret;
+}
+
+static void ov5640_set_power_off(struct ov5640_dev *sensor)
+{
+	ov5640_power(sensor, false);
+	regulator_bulk_disable(OV5640_NUM_SUPPLIES, sensor->supplies);
+	clk_disable_unprepare(sensor->xclk);
+}
 
-		ov5640_reset(sensor);
-		ov5640_power(sensor, true);
+static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
+{
+	int ret = 0;
 
-		ret = ov5640_init_slave_id(sensor);
+	if (on) {
+		ret = ov5640_set_power_on(sensor);
 		if (ret)
-			goto power_off;
+			return ret;
 
 		ret = ov5640_restore_mode(sensor);
 		if (ret)
@@ -1586,10 +1620,7 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
 	}
 
 power_off:
-	ov5640_power(sensor, false);
-	regulator_bulk_disable(OV5640_NUM_SUPPLIES, sensor->supplies);
-xclk_off:
-	clk_disable_unprepare(sensor->xclk);
+	ov5640_set_power_off(sensor);
 	return ret;
 }
 
@@ -2202,6 +2233,34 @@ static int ov5640_get_regulators(struct ov5640_dev *sensor)
 				       sensor->supplies);
 }
 
+static int ov5640_check_chip_id(struct ov5640_dev *sensor)
+{
+	struct i2c_client *client = sensor->i2c_client;
+	int ret = 0;
+	u16 chip_id;
+
+	ret = ov5640_set_power_on(sensor);
+	if (ret)
+		return ret;
+
+	ret = ov5640_read_reg16(sensor, OV5640_REG_CHIP_ID, &chip_id);
+	if (ret) {
+		dev_err(&client->dev, "%s: failed to read chip identifier\n",
+			__func__);
+		goto power_off;
+	}
+
+	if (chip_id != 0x5640) {
+		dev_err(&client->dev, "%s: wrong chip identifier, expected 0x5640, got 0x%x\n",
+			__func__, chip_id);
+		ret = -ENXIO;
+	}
+
+power_off:
+	ov5640_set_power_off(sensor);
+	return ret;
+}
+
 static int ov5640_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
@@ -2284,6 +2343,10 @@ static int ov5640_probe(struct i2c_client *client,
 
 	mutex_init(&sensor->lock);
 
+	ret = ov5640_check_chip_id(sensor);
+	if (ret)
+		goto entity_cleanup;
+
 	ret = ov5640_init_controls(sensor);
 	if (ret)
 		goto entity_cleanup;
-- 
1.9.1

[PATCH v3 3/5] media: dt-bindings: ov5640: add support of DVP parallel interface

[Hugues Fruchet]

Add bindings for OV5640 DVP parallel interface support.

Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
---
 .../devicetree/bindings/media/i2c/ov5640.txt       | 27 ++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.txt b/Documentation/devicetree/bindings/media/i2c/ov5640.txt
index 540b36c..04e2a91 100644
--- a/Documentation/devicetree/bindings/media/i2c/ov5640.txt
+++ b/Documentation/devicetree/bindings/media/i2c/ov5640.txt
@@ -1,4 +1,4 @@
-* Omnivision OV5640 MIPI CSI-2 sensor
+* Omnivision OV5640 MIPI CSI-2 / parallel sensor
 
 Required Properties:
 - compatible: should be "ovti,ov5640"
@@ -18,7 +18,11 @@ 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:
+Parallel or CSI mode is selected according to optional endpoint properties.
+Without properties (or bus properties), parallel mode is selected.
+Specifying any CSI properties such as lanes will enable CSI mode.
+
+Examples:
 
 &i2c1 {
 	ov5640: camera@3c {
@@ -35,6 +39,7 @@ Example:
 		reset-gpios = <&gpio1 20 GPIO_ACTIVE_LOW>;
 
 		port {
+			/* MIPI CSI-2 bus endpoint */
 			ov5640_to_mipi_csi2: endpoint {
 				remote-endpoint = <&mipi_csi2_from_ov5640>;
 				clock-lanes = <0>;
@@ -43,3 +48,21 @@ Example:
 		};
 	};
 };
+
+&i2c1 {
+	ov5640: camera@3c {
+		compatible = "ovti,ov5640";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_ov5640>;
+		reg = <0x3c>;
+		clocks = <&clk_ext_camera>;
+		clock-names = "xclk";
+
+		port {
+			/* Parallel bus endpoint */
+			ov5640_to_parallel: endpoint {
+				remote-endpoint = <&parallel_from_ov5640>;
+			};
+		};
+	};
+};
-- 
1.9.1

Re: [PATCH v3 3/5] media: dt-bindings: ov5640: add support of DVP parallel interface

[Sakari Ailus]

Hi Hugues,

On Thu, Dec 07, 2017 at 01:40:51PM +0100, Hugues Fruchet wrote:
> Add bindings for OV5640 DVP parallel interface support.
> 
> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> ---
>  .../devicetree/bindings/media/i2c/ov5640.txt       | 27 ++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.txt b/Documentation/devicetree/bindings/media/i2c/ov5640.txt
> index 540b36c..04e2a91 100644
> --- a/Documentation/devicetree/bindings/media/i2c/ov5640.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.txt
> @@ -1,4 +1,4 @@
> -* Omnivision OV5640 MIPI CSI-2 sensor
> +* Omnivision OV5640 MIPI CSI-2 / parallel sensor
>  
>  Required Properties:
>  - compatible: should be "ovti,ov5640"
> @@ -18,7 +18,11 @@ 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:
> +Parallel or CSI mode is selected according to optional endpoint properties.
> +Without properties (or bus properties), parallel mode is selected.
> +Specifying any CSI properties such as lanes will enable CSI mode.

These bindings never documented what which endpoint properties were needed.

Beyond that, the sensor supports two CSI-2 lanes. You should explicitly
specify that, in other words, you'll need "data-lanes" property. Could you
add that?

Long time ago when the video-interfaces.txt and the V4L2 OF framework were
written, the bus type selection was made implicit and only later on
explicit. This is still reflected in how the bus type gets set between
CSI-2 D-PHY, parallel and Bt.656.

> +
> +Examples:
>  
>  &i2c1 {
>  	ov5640: camera@3c {
> @@ -35,6 +39,7 @@ Example:
>  		reset-gpios = <&gpio1 20 GPIO_ACTIVE_LOW>;
>  
>  		port {
> +			/* MIPI CSI-2 bus endpoint */
>  			ov5640_to_mipi_csi2: endpoint {
>  				remote-endpoint = <&mipi_csi2_from_ov5640>;
>  				clock-lanes = <0>;
> @@ -43,3 +48,21 @@ Example:
>  		};
>  	};
>  };
> +
> +&i2c1 {
> +	ov5640: camera@3c {
> +		compatible = "ovti,ov5640";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_ov5640>;
> +		reg = <0x3c>;
> +		clocks = <&clk_ext_camera>;
> +		clock-names = "xclk";
> +
> +		port {
> +			/* Parallel bus endpoint */
> +			ov5640_to_parallel: endpoint {
> +				remote-endpoint = <&parallel_from_ov5640>;
> +			};
> +		};
> +	};
> +};
> -- 
> 1.9.1
> 

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH v3 3/5] media: dt-bindings: ov5640: add support of DVP parallel interface

[Hugues FRUCHET]

Hi Sakari,

On 12/07/2017 02:59 PM, Sakari Ailus wrote:
> Hi Hugues,
> 
> On Thu, Dec 07, 2017 at 01:40:51PM +0100, Hugues Fruchet wrote:
>> Add bindings for OV5640 DVP parallel interface support.
>>
>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
>> ---
>>   .../devicetree/bindings/media/i2c/ov5640.txt       | 27 ++++++++++++++++++++--
>>   1 file changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.txt b/Documentation/devicetree/bindings/media/i2c/ov5640.txt
>> index 540b36c..04e2a91 100644
>> --- a/Documentation/devicetree/bindings/media/i2c/ov5640.txt
>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.txt
>> @@ -1,4 +1,4 @@
>> -* Omnivision OV5640 MIPI CSI-2 sensor
>> +* Omnivision OV5640 MIPI CSI-2 / parallel sensor
>>   
>>   Required Properties:
>>   - compatible: should be "ovti,ov5640"
>> @@ -18,7 +18,11 @@ 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:
>> +Parallel or CSI mode is selected according to optional endpoint properties.
>> +Without properties (or bus properties), parallel mode is selected.
>> +Specifying any CSI properties such as lanes will enable CSI mode.
> 
> These bindings never documented what which endpoint properties were needed.

Ok I will add a section related to endpoint properties for both CSI and 
parallel.

> 
> Beyond that, the sensor supports two CSI-2 lanes. You should explicitly
> specify that, in other words, you'll need "data-lanes" property. Could you
> add that?
Ok I will add it to required endpoint property in case of CSI mode.
I will change commit header to reflect changes on parallel but also CSI 
documentation.

> 
> Long time ago when the video-interfaces.txt and the V4L2 OF framework were
> written, the bus type selection was made implicit and only later on
> explicit. This is still reflected in how the bus type gets set between
> CSI-2 D-PHY, parallel and Bt.656.
> 
I'm a little bit confused, must I explicitly add as required property 
"bus-type=0" (autodetect) for both cases ? Or must I require 
"bus-type=1" for CSI and "bus-type=3" for parallel ?


Talking bindings, I feel that it could be of great help to document also
the polarity of control signals (hsync/vsync/pclk), they are currently 
set by ov5640 init sequence and not configurable.
Moreover, should some checks be added in probe sequence to verify that
the defined control signals polarity are aligned with default ones from
init sequence ?


Here is a proposal:

"
The device node must contain one 'port' child node for its digital 
output video port with a single 'endpoint' subnode, in accordance
with the video interface bindings defined in
Documentation/devicetree/bindings/media/video-interfaces.txt.

OV5640 can be connected to a MIPI CSI bus or a parallel bus endpoint:

Endpoint node required properties for CSI connection are:
- remote-endpoint: a phandle to the bus receiver's endpoint node.
- bus-type: should be set to <1> (MIPI CSI-2 C-PHY)
- clock-lanes: should be set to <0> (clock lane on hardware lane 0)
- data-lanes: should be set to <1 2> (two CSI-2 lanes supported)

Endpoint node required properties for parallel connection are:
- remote-endpoint: a phandle to the bus receiver's endpoint node.
- bus-type: should be set to <3> (parallel CCP2)
- bus-width: should be set to <8> for 8 bits parallel bus
              or <10> for 10 bits parallel bus
- data-shift: should be set to <2> for 8 bits parallel bus
               (lines 9:2 are used) or <0> for 10 bits parallel bus
- hsync-active: should be set to <0> (Horizontal synchronization
                 polarity is active low).
- vsync-active: should be set to <1> (active high) (Horizontal
                 synchronization polarity is active low).
- pclk-sample:  should be set to <1> (data are sampled on the rising
                 edge of the pixel clock signal).


>> +
>> +Examples:
>>   
>>   &i2c1 {
>>   	ov5640: camera@3c {
>> @@ -35,6 +39,7 @@ Example:
>>   		reset-gpios = <&gpio1 20 GPIO_ACTIVE_LOW>;
>>   
>>   		port {
>> +			/* MIPI CSI-2 bus endpoint */
>>   			ov5640_to_mipi_csi2: endpoint {
>>   				remote-endpoint = <&mipi_csi2_from_ov5640>;
>>   				clock-lanes = <0>;
>> @@ -43,3 +48,21 @@ Example:
>>   		};
>>   	};
>>   };
>> +
>> +&i2c1 {
>> +	ov5640: camera@3c {
>> +		compatible = "ovti,ov5640";
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pinctrl_ov5640>;
>> +		reg = <0x3c>;
>> +		clocks = <&clk_ext_camera>;
>> +		clock-names = "xclk";
>> +
>> +		port {
>> +			/* Parallel bus endpoint */
>> +			ov5640_to_parallel: endpoint {
>> +				remote-endpoint = <&parallel_from_ov5640>;
>> +			};
>> +		};
>> +	};
>> +};
>> -- 
>> 1.9.1
>>
> 

Best regards,
Hugues.

Re: [PATCH v3 3/5] media: dt-bindings: ov5640: add support of DVP parallel interface

[Sakari Ailus]

Hi Hugues,

Hugues FRUCHET wrote:
> Hi Sakari,
> 
> On 12/07/2017 02:59 PM, Sakari Ailus wrote:
>> Hi Hugues,
>>
>> On Thu, Dec 07, 2017 at 01:40:51PM +0100, Hugues Fruchet wrote:
>>> Add bindings for OV5640 DVP parallel interface support.
>>>
>>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
>>> ---
>>>   .../devicetree/bindings/media/i2c/ov5640.txt       | 27 ++++++++++++++++++++--
>>>   1 file changed, 25 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.txt b/Documentation/devicetree/bindings/media/i2c/ov5640.txt
>>> index 540b36c..04e2a91 100644
>>> --- a/Documentation/devicetree/bindings/media/i2c/ov5640.txt
>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.txt
>>> @@ -1,4 +1,4 @@
>>> -* Omnivision OV5640 MIPI CSI-2 sensor
>>> +* Omnivision OV5640 MIPI CSI-2 / parallel sensor
>>>   
>>>   Required Properties:
>>>   - compatible: should be "ovti,ov5640"
>>> @@ -18,7 +18,11 @@ 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:
>>> +Parallel or CSI mode is selected according to optional endpoint properties.
>>> +Without properties (or bus properties), parallel mode is selected.
>>> +Specifying any CSI properties such as lanes will enable CSI mode.
>>
>> These bindings never documented what which endpoint properties were needed.
> 
> Ok I will add a section related to endpoint properties for both CSI and 
> parallel.
> 
>>
>> Beyond that, the sensor supports two CSI-2 lanes. You should explicitly
>> specify that, in other words, you'll need "data-lanes" property. Could you
>> add that?
> Ok I will add it to required endpoint property in case of CSI mode.
> I will change commit header to reflect changes on parallel but also CSI 
> documentation.
> 
>>
>> Long time ago when the video-interfaces.txt and the V4L2 OF framework were
>> written, the bus type selection was made implicit and only later on
>> explicit. This is still reflected in how the bus type gets set between
>> CSI-2 D-PHY, parallel and Bt.656.
>>
> I'm a little bit confused, must I explicitly add as required property 
> "bus-type=0" (autodetect) for both cases ? Or must I require 
> "bus-type=1" for CSI and "bus-type=3" for parallel ?

Yes, the confusion will stay for some time to come in some way. :-)

What I wanted to say that the original decision to make this implicit
from the bindings wasn't great --- we have here the device that does
both parallel and CSI-2 D-PHY.

But due to data-lanes, you can rely on implicit bus type selection
working. So no bus-type is needed.

> 
> 
> Talking bindings, I feel that it could be of great help to document also
> the polarity of control signals (hsync/vsync/pclk), they are currently 
> set by ov5640 init sequence and not configurable.
> Moreover, should some checks be added in probe sequence to verify that
> the defined control signals polarity are aligned with default ones from
> init sequence ?

Yes, that's a very good idea. It should have been there all along.

> 
> 
> Here is a proposal:
> 
> "
> The device node must contain one 'port' child node for its digital 
> output video port with a single 'endpoint' subnode, in accordance
> with the video interface bindings defined in
> Documentation/devicetree/bindings/media/video-interfaces.txt.
> 
> OV5640 can be connected to a MIPI CSI bus or a parallel bus endpoint:

CSI-2, please.

> 
> Endpoint node required properties for CSI connection are:
> - remote-endpoint: a phandle to the bus receiver's endpoint node.
> - bus-type: should be set to <1> (MIPI CSI-2 C-PHY)

You can omit bus-type. Or is this really C-PHY?? We don't actually have
any other devices that support C-PHY yet AFAIK.

> - clock-lanes: should be set to <0> (clock lane on hardware lane 0)

But clock-lanes isn't relevant for C-PHY. So you have D-PHY, I presume.

> - data-lanes: should be set to <1 2> (two CSI-2 lanes supported)

This should document what the hardware can do, not what the driver
supports. So <1> or <1 2> it should be.

> 
> Endpoint node required properties for parallel connection are:
> - remote-endpoint: a phandle to the bus receiver's endpoint node.
> - bus-type: should be set to <3> (parallel CCP2)

CCP2 is Compact Camera Port 2, an older serial bus (between CSI and CSI-2).

(I actually wonder if we could fix the bus-type property by giving
separate entries for parallel and CSI-2 D-PHY; I'll still need to check
whether it's used somewhere. I think not. Not relevant for this patchset
though.)

> - bus-width: should be set to <8> for 8 bits parallel bus
>               or <10> for 10 bits parallel bus
> - data-shift: should be set to <2> for 8 bits parallel bus
>                (lines 9:2 are used) or <0> for 10 bits parallel bus

s/should/shall/ for both.

> - hsync-active: should be set to <0> (Horizontal synchronization
>                  polarity is active low).
> - vsync-active: should be set to <1> (active high) (Horizontal
>                  synchronization polarity is active low).
> - pclk-sample:  should be set to <1> (data are sampled on the rising
>                  edge of the pixel clock signal).

Is this configurable on hardware? If so, no recommendation should be
made for hardware configuration as it's board specific.

> 
> 
>>> +
>>> +Examples:
>>>   
>>>   &i2c1 {
>>>   	ov5640: camera@3c {
>>> @@ -35,6 +39,7 @@ Example:
>>>   		reset-gpios = <&gpio1 20 GPIO_ACTIVE_LOW>;
>>>   
>>>   		port {
>>> +			/* MIPI CSI-2 bus endpoint */
>>>   			ov5640_to_mipi_csi2: endpoint {
>>>   				remote-endpoint = <&mipi_csi2_from_ov5640>;
>>>   				clock-lanes = <0>;
>>> @@ -43,3 +48,21 @@ Example:
>>>   		};
>>>   	};
>>>   };
>>> +
>>> +&i2c1 {
>>> +	ov5640: camera@3c {
>>> +		compatible = "ovti,ov5640";
>>> +		pinctrl-names = "default";
>>> +		pinctrl-0 = <&pinctrl_ov5640>;
>>> +		reg = <0x3c>;
>>> +		clocks = <&clk_ext_camera>;
>>> +		clock-names = "xclk";
>>> +
>>> +		port {
>>> +			/* Parallel bus endpoint */
>>> +			ov5640_to_parallel: endpoint {
>>> +				remote-endpoint = <&parallel_from_ov5640>;
>>> +			};
>>> +		};
>>> +	};
>>> +};
>>> -- 
>>> 1.9.1
>>>
>>
> 
> Best regards,
> Hugues.
> 


-- 
Kind regards,

Sakari Ailus
sakari.ailus@iki.fi

Re: [PATCH v3 3/5] media: dt-bindings: ov5640: add support of DVP parallel interface

[Hugues FRUCHET]

Hi Sakari,

On 12/13/2017 08:47 PM, Sakari Ailus wrote:
> Hi Hugues,
> 
> Hugues FRUCHET wrote:
>> Hi Sakari,
>>
>> On 12/07/2017 02:59 PM, Sakari Ailus wrote:
>>> Hi Hugues,
>>>
>>> On Thu, Dec 07, 2017 at 01:40:51PM +0100, Hugues Fruchet wrote:
>>>> Add bindings for OV5640 DVP parallel interface support.
>>>>
>>>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
>>>> ---
>>>>    .../devicetree/bindings/media/i2c/ov5640.txt       | 27 ++++++++++++++++++++--
>>>>    1 file changed, 25 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.txt b/Documentation/devicetree/bindings/media/i2c/ov5640.txt
>>>> index 540b36c..04e2a91 100644
>>>> --- a/Documentation/devicetree/bindings/media/i2c/ov5640.txt
>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.txt
>>>> @@ -1,4 +1,4 @@
>>>> -* Omnivision OV5640 MIPI CSI-2 sensor
>>>> +* Omnivision OV5640 MIPI CSI-2 / parallel sensor
>>>>    
>>>>    Required Properties:
>>>>    - compatible: should be "ovti,ov5640"
>>>> @@ -18,7 +18,11 @@ 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:
>>>> +Parallel or CSI mode is selected according to optional endpoint properties.
>>>> +Without properties (or bus properties), parallel mode is selected.
>>>> +Specifying any CSI properties such as lanes will enable CSI mode.
>>>
>>> These bindings never documented what which endpoint properties were needed.
>>
>> Ok I will add a section related to endpoint properties for both CSI and
>> parallel.
>>
>>>
>>> Beyond that, the sensor supports two CSI-2 lanes. You should explicitly
>>> specify that, in other words, you'll need "data-lanes" property. Could you
>>> add that?
>> Ok I will add it to required endpoint property in case of CSI mode.
>> I will change commit header to reflect changes on parallel but also CSI
>> documentation.
>>
>>>
>>> Long time ago when the video-interfaces.txt and the V4L2 OF framework were
>>> written, the bus type selection was made implicit and only later on
>>> explicit. This is still reflected in how the bus type gets set between
>>> CSI-2 D-PHY, parallel and Bt.656.
>>>
>> I'm a little bit confused, must I explicitly add as required property
>> "bus-type=0" (autodetect) for both cases ? Or must I require
>> "bus-type=1" for CSI and "bus-type=3" for parallel ?
> 
> Yes, the confusion will stay for some time to come in some way. :-)
> 
> What I wanted to say that the original decision to make this implicit
> from the bindings wasn't great --- we have here the device that does
> both parallel and CSI-2 D-PHY.
> 
> But due to data-lanes, you can rely on implicit bus type selection
> working. So no bus-type is needed.
> 

OK, got it now:
- "bus-type=0" (autodetect) => V4L2_MBUS_PARALLEL or V4L2_MBUS_BT656 or 
V4L2_MBUS_CSI2 depending on properties
- "bus-type=1" => MIPI CSI-2 C-PHY
- "bus-type=2" => MIPI CSI1
- "bus-type=3" => CCP2

/**
  * enum v4l2_mbus_type - media bus type
  * @V4L2_MBUS_PARALLEL:	parallel interface with hsync and vsync
  * @V4L2_MBUS_BT656:	parallel interface with embedded synchronisation, can
  *			also be used for BT.1120
  * @V4L2_MBUS_CSI1:	MIPI CSI-1 serial interface
  * @V4L2_MBUS_CCP2:	CCP2 (Compact Camera Port 2)
  * @V4L2_MBUS_CSI2:	MIPI CSI-2 serial interface
  */
enum v4l2_mbus_type {
	V4L2_MBUS_PARALLEL,
	V4L2_MBUS_BT656,
	V4L2_MBUS_CSI1,
	V4L2_MBUS_CCP2,
	V4L2_MBUS_CSI2,
};

This explain my confusion on CSI-2 CPHY and CCP2 below...

>>
>>
>> Talking bindings, I feel that it could be of great help to document also
>> the polarity of control signals (hsync/vsync/pclk), they are currently
>> set by ov5640 init sequence and not configurable.
>> Moreover, should some checks be added in probe sequence to verify that
>> the defined control signals polarity are aligned with default ones from
>> init sequence ?
> 
> Yes, that's a very good idea. It should have been there all along.
> 
>>
>>
>> Here is a proposal:
>>
>> "
>> The device node must contain one 'port' child node for its digital
>> output video port with a single 'endpoint' subnode, in accordance
>> with the video interface bindings defined in
>> Documentation/devicetree/bindings/media/video-interfaces.txt.
>>
>> OV5640 can be connected to a MIPI CSI bus or a parallel bus endpoint:
> 
> CSI-2, please.
> 
OK

>>
>> Endpoint node required properties for CSI connection are:
>> - remote-endpoint: a phandle to the bus receiver's endpoint node.
>> - bus-type: should be set to <1> (MIPI CSI-2 C-PHY)
> 
> You can omit bus-type. Or is this really C-PHY?? We don't actually have
> any other devices that support C-PHY yet AFAIK.
> 
No it's D-PHY, confusion on my side...

>> - clock-lanes: should be set to <0> (clock lane on hardware lane 0)
> 
> But clock-lanes isn't relevant for C-PHY. So you have D-PHY, I presume.
> 
OK I'll remove.

>> - data-lanes: should be set to <1 2> (two CSI-2 lanes supported)
> 
> This should document what the hardware can do, not what the driver
> supports. So <1> or <1 2> it should be.
OK

> 
>>
>> Endpoint node required properties for parallel connection are:
>> - remote-endpoint: a phandle to the bus receiver's endpoint node.
>> - bus-type: should be set to <3> (parallel CCP2)
> 
> CCP2 is Compact Camera Port 2, an older serial bus (between CSI and CSI-2).
No it's parallel, confusion on my side...

> 
> (I actually wonder if we could fix the bus-type property by giving
> separate entries for parallel and CSI-2 D-PHY; I'll still need to check
> whether it's used somewhere. I think not. Not relevant for this patchset
> though.)
> 
>> - bus-width: should be set to <8> for 8 bits parallel bus
>>                or <10> for 10 bits parallel bus
>> - data-shift: should be set to <2> for 8 bits parallel bus
>>                 (lines 9:2 are used) or <0> for 10 bits parallel bus
> 
> s/should/shall/ for both.
> 
OK

>> - hsync-active: should be set to <0> (Horizontal synchronization
>>                   polarity is active low).
>> - vsync-active: should be set to <1> (active high) (Horizontal
>>                   synchronization polarity is active low).
>> - pclk-sample:  should be set to <1> (data are sampled on the rising
>>                   edge of the pixel clock signal).
> 
> Is this configurable on hardware? If so, no recommendation should be
> made for hardware configuration as it's board specific.
> 
As explained, above:
 >> Talking bindings, I feel that it could be of great help to document 
 >> also the polarity of control signals (hsync/vsync/pclk), they are
 >> currently set by ov5640 init sequence and not configurable.

So this is configurable by some sensor registers but currently hardcoded 
in sensor init sequence inside driver.
So in order to make it functional, the remote side (ISP endpoint 
"parallel_from_ov5640") have to be configured to match those signal levels.
So here we have 3 options:

1) no recommendation
In this case no information can be found in binding on how to set the 
signal levels on ISP side, information is given in ov5640 source code:

+static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
<...>
+	 * The init sequence initializes control lines as defined below:
+	 * - VSYNC:	active high
+	 * - HREF:	active low
+	 * - PCLK:	active high
+	 */

 From this source code information and schematics study for potential 
inverters in-between, we can deduce the ISP endpoint 
"parallel_from_ov5640" signal levels properties.

2) hsync/vsync/pclk signal levels are optional endpoint properties
They affect ov5640 hardware signals polarity. ov5640 sensor driver code 
must be updated to send the right i2c sequences to program signal 
polarities depending on those devicetree endpoint values (not currently 
done).

With schematics study for potential inverters in-between, we can easily 
set the ISP endpoint "parallel_from_ov5640" signal levels properties.
No source code check is required.


3) hsync/vsync/pclk signal levels are fixed required endpoint properties
This is my current proposal.
They reflect in binding documentation the hardware signal levels of 
ov5640 set by init sequence.
I can update the ov5640 sensor driver code in order to check that those
mandatory properties are set to the right value.

With schematics study for potential inverters in-between, we can easily 
set the ISP endpoint "parallel_from_ov5640" signal levels properties.
No source code check is required.



>>
>>
>>>> +
>>>> +Examples:
>>>>    
>>>>    &i2c1 {
>>>>    	ov5640: camera@3c {
>>>> @@ -35,6 +39,7 @@ Example:
>>>>    		reset-gpios = <&gpio1 20 GPIO_ACTIVE_LOW>;
>>>>    
>>>>    		port {
>>>> +			/* MIPI CSI-2 bus endpoint */
>>>>    			ov5640_to_mipi_csi2: endpoint {
>>>>    				remote-endpoint = <&mipi_csi2_from_ov5640>;
>>>>    				clock-lanes = <0>;
>>>> @@ -43,3 +48,21 @@ Example:
>>>>    		};
>>>>    	};
>>>>    };
>>>> +
>>>> +&i2c1 {
>>>> +	ov5640: camera@3c {
>>>> +		compatible = "ovti,ov5640";
>>>> +		pinctrl-names = "default";
>>>> +		pinctrl-0 = <&pinctrl_ov5640>;
>>>> +		reg = <0x3c>;
>>>> +		clocks = <&clk_ext_camera>;
>>>> +		clock-names = "xclk";
>>>> +
>>>> +		port {
>>>> +			/* Parallel bus endpoint */
>>>> +			ov5640_to_parallel: endpoint {
>>>> +				remote-endpoint = <&parallel_from_ov5640>;
>>>> +			};
>>>> +		};
>>>> +	};
>>>> +};
>>>> -- 
>>>> 1.9.1
>>>>
>>>
>>
>> Best regards,
>> Hugues.
>>
> 
> 

Best regards,
Hugues

Re: [PATCH v3 3/5] media: dt-bindings: ov5640: add support of DVP parallel interface

[Sakari Ailus]

Hi Hugues,

On Mon, Dec 18, 2017 at 10:24:40AM +0000, Hugues FRUCHET wrote:
> Hi Sakari,
> 
> On 12/13/2017 08:47 PM, Sakari Ailus wrote:
> > Hi Hugues,
> > 
> > Hugues FRUCHET wrote:
> >> Hi Sakari,
> >>
> >> On 12/07/2017 02:59 PM, Sakari Ailus wrote:
> >>> Hi Hugues,
> >>>
> >>> On Thu, Dec 07, 2017 at 01:40:51PM +0100, Hugues Fruchet wrote:
> >>>> Add bindings for OV5640 DVP parallel interface support.
> >>>>
> >>>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> >>>> ---
> >>>>    .../devicetree/bindings/media/i2c/ov5640.txt       | 27 ++++++++++++++++++++--
> >>>>    1 file changed, 25 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.txt b/Documentation/devicetree/bindings/media/i2c/ov5640.txt
> >>>> index 540b36c..04e2a91 100644
> >>>> --- a/Documentation/devicetree/bindings/media/i2c/ov5640.txt
> >>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.txt
> >>>> @@ -1,4 +1,4 @@
> >>>> -* Omnivision OV5640 MIPI CSI-2 sensor
> >>>> +* Omnivision OV5640 MIPI CSI-2 / parallel sensor
> >>>>    
> >>>>    Required Properties:
> >>>>    - compatible: should be "ovti,ov5640"
> >>>> @@ -18,7 +18,11 @@ 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:
> >>>> +Parallel or CSI mode is selected according to optional endpoint properties.
> >>>> +Without properties (or bus properties), parallel mode is selected.
> >>>> +Specifying any CSI properties such as lanes will enable CSI mode.
> >>>
> >>> These bindings never documented what which endpoint properties were needed.
> >>
> >> Ok I will add a section related to endpoint properties for both CSI and
> >> parallel.
> >>
> >>>
> >>> Beyond that, the sensor supports two CSI-2 lanes. You should explicitly
> >>> specify that, in other words, you'll need "data-lanes" property. Could you
> >>> add that?
> >> Ok I will add it to required endpoint property in case of CSI mode.
> >> I will change commit header to reflect changes on parallel but also CSI
> >> documentation.
> >>
> >>>
> >>> Long time ago when the video-interfaces.txt and the V4L2 OF framework were
> >>> written, the bus type selection was made implicit and only later on
> >>> explicit. This is still reflected in how the bus type gets set between
> >>> CSI-2 D-PHY, parallel and Bt.656.
> >>>
> >> I'm a little bit confused, must I explicitly add as required property
> >> "bus-type=0" (autodetect) for both cases ? Or must I require
> >> "bus-type=1" for CSI and "bus-type=3" for parallel ?
> > 
> > Yes, the confusion will stay for some time to come in some way. :-)
> > 
> > What I wanted to say that the original decision to make this implicit
> > from the bindings wasn't great --- we have here the device that does
> > both parallel and CSI-2 D-PHY.
> > 
> > But due to data-lanes, you can rely on implicit bus type selection
> > working. So no bus-type is needed.
> > 
> 
> OK, got it now:
> - "bus-type=0" (autodetect) => V4L2_MBUS_PARALLEL or V4L2_MBUS_BT656 or 
> V4L2_MBUS_CSI2 depending on properties
> - "bus-type=1" => MIPI CSI-2 C-PHY
> - "bus-type=2" => MIPI CSI1
> - "bus-type=3" => CCP2
> 
> /**
>   * enum v4l2_mbus_type - media bus type
>   * @V4L2_MBUS_PARALLEL:	parallel interface with hsync and vsync
>   * @V4L2_MBUS_BT656:	parallel interface with embedded synchronisation, can
>   *			also be used for BT.1120
>   * @V4L2_MBUS_CSI1:	MIPI CSI-1 serial interface
>   * @V4L2_MBUS_CCP2:	CCP2 (Compact Camera Port 2)
>   * @V4L2_MBUS_CSI2:	MIPI CSI-2 serial interface
>   */
> enum v4l2_mbus_type {
> 	V4L2_MBUS_PARALLEL,
> 	V4L2_MBUS_BT656,
> 	V4L2_MBUS_CSI1,
> 	V4L2_MBUS_CCP2,
> 	V4L2_MBUS_CSI2,
> };
> 
> This explain my confusion on CSI-2 CPHY and CCP2 below...
> 
> >>
> >>
> >> Talking bindings, I feel that it could be of great help to document also
> >> the polarity of control signals (hsync/vsync/pclk), they are currently
> >> set by ov5640 init sequence and not configurable.
> >> Moreover, should some checks be added in probe sequence to verify that
> >> the defined control signals polarity are aligned with default ones from
> >> init sequence ?
> > 
> > Yes, that's a very good idea. It should have been there all along.
> > 
> >>
> >>
> >> Here is a proposal:
> >>
> >> "
> >> The device node must contain one 'port' child node for its digital
> >> output video port with a single 'endpoint' subnode, in accordance
> >> with the video interface bindings defined in
> >> Documentation/devicetree/bindings/media/video-interfaces.txt.
> >>
> >> OV5640 can be connected to a MIPI CSI bus or a parallel bus endpoint:
> > 
> > CSI-2, please.
> > 
> OK
> 
> >>
> >> Endpoint node required properties for CSI connection are:
> >> - remote-endpoint: a phandle to the bus receiver's endpoint node.
> >> - bus-type: should be set to <1> (MIPI CSI-2 C-PHY)
> > 
> > You can omit bus-type. Or is this really C-PHY?? We don't actually have
> > any other devices that support C-PHY yet AFAIK.
> > 
> No it's D-PHY, confusion on my side...
> 
> >> - clock-lanes: should be set to <0> (clock lane on hardware lane 0)
> > 
> > But clock-lanes isn't relevant for C-PHY. So you have D-PHY, I presume.
> > 
> OK I'll remove.
> 
> >> - data-lanes: should be set to <1 2> (two CSI-2 lanes supported)
> > 
> > This should document what the hardware can do, not what the driver
> > supports. So <1> or <1 2> it should be.
> OK
> 
> > 
> >>
> >> Endpoint node required properties for parallel connection are:
> >> - remote-endpoint: a phandle to the bus receiver's endpoint node.
> >> - bus-type: should be set to <3> (parallel CCP2)
> > 
> > CCP2 is Compact Camera Port 2, an older serial bus (between CSI and CSI-2).
> No it's parallel, confusion on my side...
> 
> > 
> > (I actually wonder if we could fix the bus-type property by giving
> > separate entries for parallel and CSI-2 D-PHY; I'll still need to check
> > whether it's used somewhere. I think not. Not relevant for this patchset
> > though.)
> > 
> >> - bus-width: should be set to <8> for 8 bits parallel bus
> >>                or <10> for 10 bits parallel bus
> >> - data-shift: should be set to <2> for 8 bits parallel bus
> >>                 (lines 9:2 are used) or <0> for 10 bits parallel bus
> > 
> > s/should/shall/ for both.
> > 
> OK
> 
> >> - hsync-active: should be set to <0> (Horizontal synchronization
> >>                   polarity is active low).
> >> - vsync-active: should be set to <1> (active high) (Horizontal
> >>                   synchronization polarity is active low).
> >> - pclk-sample:  should be set to <1> (data are sampled on the rising
> >>                   edge of the pixel clock signal).
> > 
> > Is this configurable on hardware? If so, no recommendation should be
> > made for hardware configuration as it's board specific.
> > 
> As explained, above:
>  >> Talking bindings, I feel that it could be of great help to document 
>  >> also the polarity of control signals (hsync/vsync/pclk), they are
>  >> currently set by ov5640 init sequence and not configurable.
> 
> So this is configurable by some sensor registers but currently hardcoded 
> in sensor init sequence inside driver.
> So in order to make it functional, the remote side (ISP endpoint 
> "parallel_from_ov5640") have to be configured to match those signal levels.
> So here we have 3 options:

The configuration on both ends simply needs to match (with exceptions, if
there's more than just a direct wire between the signal pins). The DT
specifies the hardware configuration and it is independent of whether the
driver implements all hardware features.

See e.g.:

Documentation/devicetree/bindings/media/i2c/tvp514x.txt

If the driver does not implement the configuration specified in DT, then
presumably the hardware won't work --- which is a driver issue.

> 
> 1) no recommendation
> In this case no information can be found in binding on how to set the 
> signal levels on ISP side, information is given in ov5640 source code:
> 
> +static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
> <...>
> +	 * The init sequence initializes control lines as defined below:
> +	 * - VSYNC:	active high
> +	 * - HREF:	active low
> +	 * - PCLK:	active high
> +	 */
> 
>  From this source code information and schematics study for potential 
> inverters in-between, we can deduce the ISP endpoint 
> "parallel_from_ov5640" signal levels properties.
> 
> 2) hsync/vsync/pclk signal levels are optional endpoint properties
> They affect ov5640 hardware signals polarity. ov5640 sensor driver code 
> must be updated to send the right i2c sequences to program signal 
> polarities depending on those devicetree endpoint values (not currently 
> done).
> 
> With schematics study for potential inverters in-between, we can easily 
> set the ISP endpoint "parallel_from_ov5640" signal levels properties.
> No source code check is required.
> 
> 
> 3) hsync/vsync/pclk signal levels are fixed required endpoint properties
> This is my current proposal.
> They reflect in binding documentation the hardware signal levels of 
> ov5640 set by init sequence.
> I can update the ov5640 sensor driver code in order to check that those
> mandatory properties are set to the right value.
> 
> With schematics study for potential inverters in-between, we can easily 
> set the ISP endpoint "parallel_from_ov5640" signal levels properties.
> No source code check is required.
> 
> 
> 
> >>
> >>
> >>>> +
> >>>> +Examples:
> >>>>    
> >>>>    &i2c1 {
> >>>>    	ov5640: camera@3c {
> >>>> @@ -35,6 +39,7 @@ Example:
> >>>>    		reset-gpios = <&gpio1 20 GPIO_ACTIVE_LOW>;
> >>>>    
> >>>>    		port {
> >>>> +			/* MIPI CSI-2 bus endpoint */
> >>>>    			ov5640_to_mipi_csi2: endpoint {
> >>>>    				remote-endpoint = <&mipi_csi2_from_ov5640>;
> >>>>    				clock-lanes = <0>;
> >>>> @@ -43,3 +48,21 @@ Example:
> >>>>    		};
> >>>>    	};
> >>>>    };
> >>>> +
> >>>> +&i2c1 {
> >>>> +	ov5640: camera@3c {
> >>>> +		compatible = "ovti,ov5640";
> >>>> +		pinctrl-names = "default";
> >>>> +		pinctrl-0 = <&pinctrl_ov5640>;
> >>>> +		reg = <0x3c>;
> >>>> +		clocks = <&clk_ext_camera>;
> >>>> +		clock-names = "xclk";
> >>>> +
> >>>> +		port {
> >>>> +			/* Parallel bus endpoint */
> >>>> +			ov5640_to_parallel: endpoint {
> >>>> +				remote-endpoint = <&parallel_from_ov5640>;
> >>>> +			};
> >>>> +		};
> >>>> +	};
> >>>> +};
> >>>> -- 
> >>>> 1.9.1
> >>>>
> >>>
> >>
> >> Best regards,
> >> Hugues.
> >>
> > 
> > 
> 
> Best regards,
> Hugues

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH v3 3/5] media: dt-bindings: ov5640: add support of DVP parallel interface

[Hugues FRUCHET]

Thanks Sakari,

I'll push a patchset based on this discussion.

Best regards,
Hugues.

On 12/19/2017 11:08 AM, Sakari Ailus wrote:
> Hi Hugues,
> 
> On Mon, Dec 18, 2017 at 10:24:40AM +0000, Hugues FRUCHET wrote:
>> Hi Sakari,
>>
>> On 12/13/2017 08:47 PM, Sakari Ailus wrote:
>>> Hi Hugues,
>>>
>>> Hugues FRUCHET wrote:
>>>> Hi Sakari,
>>>>
>>>> On 12/07/2017 02:59 PM, Sakari Ailus wrote:
>>>>> Hi Hugues,
>>>>>
>>>>> On Thu, Dec 07, 2017 at 01:40:51PM +0100, Hugues Fruchet wrote:
>>>>>> Add bindings for OV5640 DVP parallel interface support.
>>>>>>
>>>>>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
>>>>>> ---
>>>>>>     .../devicetree/bindings/media/i2c/ov5640.txt       | 27 ++++++++++++++++++++--
>>>>>>     1 file changed, 25 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.txt b/Documentation/devicetree/bindings/media/i2c/ov5640.txt
>>>>>> index 540b36c..04e2a91 100644
>>>>>> --- a/Documentation/devicetree/bindings/media/i2c/ov5640.txt
>>>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.txt
>>>>>> @@ -1,4 +1,4 @@
>>>>>> -* Omnivision OV5640 MIPI CSI-2 sensor
>>>>>> +* Omnivision OV5640 MIPI CSI-2 / parallel sensor
>>>>>>     
>>>>>>     Required Properties:
>>>>>>     - compatible: should be "ovti,ov5640"
>>>>>> @@ -18,7 +18,11 @@ 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:
>>>>>> +Parallel or CSI mode is selected according to optional endpoint properties.
>>>>>> +Without properties (or bus properties), parallel mode is selected.
>>>>>> +Specifying any CSI properties such as lanes will enable CSI mode.
>>>>>
>>>>> These bindings never documented what which endpoint properties were needed.
>>>>
>>>> Ok I will add a section related to endpoint properties for both CSI and
>>>> parallel.
>>>>
>>>>>
>>>>> Beyond that, the sensor supports two CSI-2 lanes. You should explicitly
>>>>> specify that, in other words, you'll need "data-lanes" property. Could you
>>>>> add that?
>>>> Ok I will add it to required endpoint property in case of CSI mode.
>>>> I will change commit header to reflect changes on parallel but also CSI
>>>> documentation.
>>>>
>>>>>
>>>>> Long time ago when the video-interfaces.txt and the V4L2 OF framework were
>>>>> written, the bus type selection was made implicit and only later on
>>>>> explicit. This is still reflected in how the bus type gets set between
>>>>> CSI-2 D-PHY, parallel and Bt.656.
>>>>>
>>>> I'm a little bit confused, must I explicitly add as required property
>>>> "bus-type=0" (autodetect) for both cases ? Or must I require
>>>> "bus-type=1" for CSI and "bus-type=3" for parallel ?
>>>
>>> Yes, the confusion will stay for some time to come in some way. :-)
>>>
>>> What I wanted to say that the original decision to make this implicit
>>> from the bindings wasn't great --- we have here the device that does
>>> both parallel and CSI-2 D-PHY.
>>>
>>> But due to data-lanes, you can rely on implicit bus type selection
>>> working. So no bus-type is needed.
>>>
>>
>> OK, got it now:
>> - "bus-type=0" (autodetect) => V4L2_MBUS_PARALLEL or V4L2_MBUS_BT656 or
>> V4L2_MBUS_CSI2 depending on properties
>> - "bus-type=1" => MIPI CSI-2 C-PHY
>> - "bus-type=2" => MIPI CSI1
>> - "bus-type=3" => CCP2
>>
>> /**
>>    * enum v4l2_mbus_type - media bus type
>>    * @V4L2_MBUS_PARALLEL:	parallel interface with hsync and vsync
>>    * @V4L2_MBUS_BT656:	parallel interface with embedded synchronisation, can
>>    *			also be used for BT.1120
>>    * @V4L2_MBUS_CSI1:	MIPI CSI-1 serial interface
>>    * @V4L2_MBUS_CCP2:	CCP2 (Compact Camera Port 2)
>>    * @V4L2_MBUS_CSI2:	MIPI CSI-2 serial interface
>>    */
>> enum v4l2_mbus_type {
>> 	V4L2_MBUS_PARALLEL,
>> 	V4L2_MBUS_BT656,
>> 	V4L2_MBUS_CSI1,
>> 	V4L2_MBUS_CCP2,
>> 	V4L2_MBUS_CSI2,
>> };
>>
>> This explain my confusion on CSI-2 CPHY and CCP2 below...
>>
>>>>
>>>>
>>>> Talking bindings, I feel that it could be of great help to document also
>>>> the polarity of control signals (hsync/vsync/pclk), they are currently
>>>> set by ov5640 init sequence and not configurable.
>>>> Moreover, should some checks be added in probe sequence to verify that
>>>> the defined control signals polarity are aligned with default ones from
>>>> init sequence ?
>>>
>>> Yes, that's a very good idea. It should have been there all along.
>>>
>>>>
>>>>
>>>> Here is a proposal:
>>>>
>>>> "
>>>> The device node must contain one 'port' child node for its digital
>>>> output video port with a single 'endpoint' subnode, in accordance
>>>> with the video interface bindings defined in
>>>> Documentation/devicetree/bindings/media/video-interfaces.txt.
>>>>
>>>> OV5640 can be connected to a MIPI CSI bus or a parallel bus endpoint:
>>>
>>> CSI-2, please.
>>>
>> OK
>>
>>>>
>>>> Endpoint node required properties for CSI connection are:
>>>> - remote-endpoint: a phandle to the bus receiver's endpoint node.
>>>> - bus-type: should be set to <1> (MIPI CSI-2 C-PHY)
>>>
>>> You can omit bus-type. Or is this really C-PHY?? We don't actually have
>>> any other devices that support C-PHY yet AFAIK.
>>>
>> No it's D-PHY, confusion on my side...
>>
>>>> - clock-lanes: should be set to <0> (clock lane on hardware lane 0)
>>>
>>> But clock-lanes isn't relevant for C-PHY. So you have D-PHY, I presume.
>>>
>> OK I'll remove.
>>
>>>> - data-lanes: should be set to <1 2> (two CSI-2 lanes supported)
>>>
>>> This should document what the hardware can do, not what the driver
>>> supports. So <1> or <1 2> it should be.
>> OK
>>
>>>
>>>>
>>>> Endpoint node required properties for parallel connection are:
>>>> - remote-endpoint: a phandle to the bus receiver's endpoint node.
>>>> - bus-type: should be set to <3> (parallel CCP2)
>>>
>>> CCP2 is Compact Camera Port 2, an older serial bus (between CSI and CSI-2).
>> No it's parallel, confusion on my side...
>>
>>>
>>> (I actually wonder if we could fix the bus-type property by giving
>>> separate entries for parallel and CSI-2 D-PHY; I'll still need to check
>>> whether it's used somewhere. I think not. Not relevant for this patchset
>>> though.)
>>>
>>>> - bus-width: should be set to <8> for 8 bits parallel bus
>>>>                 or <10> for 10 bits parallel bus
>>>> - data-shift: should be set to <2> for 8 bits parallel bus
>>>>                  (lines 9:2 are used) or <0> for 10 bits parallel bus
>>>
>>> s/should/shall/ for both.
>>>
>> OK
>>
>>>> - hsync-active: should be set to <0> (Horizontal synchronization
>>>>                    polarity is active low).
>>>> - vsync-active: should be set to <1> (active high) (Horizontal
>>>>                    synchronization polarity is active low).
>>>> - pclk-sample:  should be set to <1> (data are sampled on the rising
>>>>                    edge of the pixel clock signal).
>>>
>>> Is this configurable on hardware? If so, no recommendation should be
>>> made for hardware configuration as it's board specific.
>>>
>> As explained, above:
>>   >> Talking bindings, I feel that it could be of great help to document
>>   >> also the polarity of control signals (hsync/vsync/pclk), they are
>>   >> currently set by ov5640 init sequence and not configurable.
>>
>> So this is configurable by some sensor registers but currently hardcoded
>> in sensor init sequence inside driver.
>> So in order to make it functional, the remote side (ISP endpoint
>> "parallel_from_ov5640") have to be configured to match those signal levels.
>> So here we have 3 options:
> 
> The configuration on both ends simply needs to match (with exceptions, if
> there's more than just a direct wire between the signal pins). The DT
> specifies the hardware configuration and it is independent of whether the
> driver implements all hardware features.
> 
> See e.g.:
> 
> Documentation/devicetree/bindings/media/i2c/tvp514x.txt
> 
> If the driver does not implement the configuration specified in DT, then
> presumably the hardware won't work --- which is a driver issue.
> 
>>
>> 1) no recommendation
>> In this case no information can be found in binding on how to set the
>> signal levels on ISP side, information is given in ov5640 source code:
>>
>> +static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
>> <...>
>> +	 * The init sequence initializes control lines as defined below:
>> +	 * - VSYNC:	active high
>> +	 * - HREF:	active low
>> +	 * - PCLK:	active high
>> +	 */
>>
>>   From this source code information and schematics study for potential
>> inverters in-between, we can deduce the ISP endpoint
>> "parallel_from_ov5640" signal levels properties.
>>
>> 2) hsync/vsync/pclk signal levels are optional endpoint properties
>> They affect ov5640 hardware signals polarity. ov5640 sensor driver code
>> must be updated to send the right i2c sequences to program signal
>> polarities depending on those devicetree endpoint values (not currently
>> done).
>>
>> With schematics study for potential inverters in-between, we can easily
>> set the ISP endpoint "parallel_from_ov5640" signal levels properties.
>> No source code check is required.
>>
>>
>> 3) hsync/vsync/pclk signal levels are fixed required endpoint properties
>> This is my current proposal.
>> They reflect in binding documentation the hardware signal levels of
>> ov5640 set by init sequence.
>> I can update the ov5640 sensor driver code in order to check that those
>> mandatory properties are set to the right value.
>>
>> With schematics study for potential inverters in-between, we can easily
>> set the ISP endpoint "parallel_from_ov5640" signal levels properties.
>> No source code check is required.
>>
>>
>>
>>>>
>>>>
>>>>>> +
>>>>>> +Examples:
>>>>>>     
>>>>>>     &i2c1 {
>>>>>>     	ov5640: camera@3c {
>>>>>> @@ -35,6 +39,7 @@ Example:
>>>>>>     		reset-gpios = <&gpio1 20 GPIO_ACTIVE_LOW>;
>>>>>>     
>>>>>>     		port {
>>>>>> +			/* MIPI CSI-2 bus endpoint */
>>>>>>     			ov5640_to_mipi_csi2: endpoint {
>>>>>>     				remote-endpoint = <&mipi_csi2_from_ov5640>;
>>>>>>     				clock-lanes = <0>;
>>>>>> @@ -43,3 +48,21 @@ Example:
>>>>>>     		};
>>>>>>     	};
>>>>>>     };
>>>>>> +
>>>>>> +&i2c1 {
>>>>>> +	ov5640: camera@3c {
>>>>>> +		compatible = "ovti,ov5640";
>>>>>> +		pinctrl-names = "default";
>>>>>> +		pinctrl-0 = <&pinctrl_ov5640>;
>>>>>> +		reg = <0x3c>;
>>>>>> +		clocks = <&clk_ext_camera>;
>>>>>> +		clock-names = "xclk";
>>>>>> +
>>>>>> +		port {
>>>>>> +			/* Parallel bus endpoint */
>>>>>> +			ov5640_to_parallel: endpoint {
>>>>>> +				remote-endpoint = <&parallel_from_ov5640>;
>>>>>> +			};
>>>>>> +		};
>>>>>> +	};
>>>>>> +};
>>>>>> -- 
>>>>>> 1.9.1
>>>>>>
>>>>>
>>>>
>>>> Best regards,
>>>> Hugues.
>>>>
>>>
>>>
>>
>> Best regards,
>> Hugues
> 

[PATCH v3 4/5] media: ov5640: add support of DVP parallel interface

[Hugues Fruchet]

Add support of DVP parallel mode in addition of
existing MIPI CSI mode. The choice between two modes
and configuration is made through device tree.

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

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 9f031f3..c791a67 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -34,13 +34,19 @@
 
 #define OV5640_DEFAULT_SLAVE_ID 0x3c
 
+#define OV5640_REG_SYS_CTRL0		0x3008
 #define OV5640_REG_CHIP_ID		0x300a
+#define OV5640_REG_IO_MIPI_CTRL00	0x300e
+#define OV5640_REG_PAD_OUTPUT_ENABLE01	0x3017
+#define OV5640_REG_PAD_OUTPUT_ENABLE02	0x3018
 #define OV5640_REG_PAD_OUTPUT00		0x3019
+#define OV5640_REG_SYSTEM_CONTROL1	0x302e
 #define OV5640_REG_SC_PLL_CTRL0		0x3034
 #define OV5640_REG_SC_PLL_CTRL1		0x3035
 #define OV5640_REG_SC_PLL_CTRL2		0x3036
 #define OV5640_REG_SC_PLL_CTRL3		0x3037
 #define OV5640_REG_SLAVE_ID		0x3100
+#define OV5640_REG_SCCB_SYS_CTRL1	0x3103
 #define OV5640_REG_SYS_ROOT_DIVIDER	0x3108
 #define OV5640_REG_AWB_R_GAIN		0x3400
 #define OV5640_REG_AWB_G_GAIN		0x3402
@@ -982,7 +988,78 @@ static int ov5640_get_gain(struct ov5640_dev *sensor)
 	return gain & 0x3ff;
 }
 
-static int ov5640_set_stream(struct ov5640_dev *sensor, bool on)
+static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
+{
+	int ret;
+
+	/*
+	 * Note about parallel port configuration.
+	 *
+	 * When configured in parallel mode, the OV5640 will
+	 * output 10 bits data on DVP data lines [9:0].
+	 * If only 8 bits data are wanted, the 8 bits data lines
+	 * of the camera interface must be physically connected
+	 * on the DVP data lines [9:2].
+	 *
+	 * The init sequence initializes control lines as defined below:
+	 * - VSYNC:	active high
+	 * - HREF:	active low
+	 * - PCLK:	active high
+	 */
+
+	if (on) {
+		/*
+		 * reset MIPI PCLK/SERCLK divider
+		 *
+		 * SC PLL CONTRL1 0
+		 * - [3..0]:	MIPI PCLK/SERCLK divider
+		 */
+		ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL1, 0x0f, 0);
+		if (ret)
+			return ret;
+	}
+
+	/*
+	 * powerdown MIPI TX/RX PHY & disable MIPI
+	 *
+	 * MIPI CONTROL 00
+	 * 4:	 PWDN PHY TX
+	 * 3:	 PWDN PHY RX
+	 * 2:	 MIPI enable
+	 */
+	ret = ov5640_write_reg(sensor,
+			       OV5640_REG_IO_MIPI_CTRL00, on ? 0x18 : 0);
+	if (ret)
+		return ret;
+
+	/*
+	 * enable VSYNC/HREF/PCLK DVP control lines
+	 * & D[9:6] DVP data lines
+	 *
+	 * PAD OUTPUT ENABLE 01
+	 * - 6:		VSYNC output enable
+	 * - 5:		HREF output enable
+	 * - 4:		PCLK output enable
+	 * - [3:0]:	D[9:6] output enable
+	 */
+	ret = ov5640_write_reg(sensor,
+			       OV5640_REG_PAD_OUTPUT_ENABLE01,
+			       on ? 0x7f : 0);
+	if (ret)
+		return ret;
+
+	/*
+	 * enable D[5:0] DVP data lines
+	 *
+	 * PAD OUTPUT ENABLE 02
+	 * - [7:2]:	D[5:0] output enable
+	 */
+	return ov5640_write_reg(sensor,
+				OV5640_REG_PAD_OUTPUT_ENABLE02,
+				on ? 0xfc : 0);
+}
+
+static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
 {
 	int ret;
 
@@ -1604,17 +1681,19 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
 		if (ret)
 			goto power_off;
 
-		/*
-		 * start streaming briefly followed by stream off in
-		 * order to coax the clock lane into LP-11 state.
-		 */
-		ret = ov5640_set_stream(sensor, true);
-		if (ret)
-			goto power_off;
-		usleep_range(1000, 2000);
-		ret = ov5640_set_stream(sensor, false);
-		if (ret)
-			goto power_off;
+		if (sensor->ep.bus_type == V4L2_MBUS_CSI2) {
+			/*
+			 * start streaming briefly followed by stream off in
+			 * order to coax the clock lane into LP-11 state.
+			 */
+			ret = ov5640_set_stream_mipi(sensor, true);
+			if (ret)
+				goto power_off;
+			usleep_range(1000, 2000);
+			ret = ov5640_set_stream_mipi(sensor, false);
+			if (ret)
+				goto power_off;
+		}
 
 		return 0;
 	}
@@ -2188,7 +2267,11 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
 				goto out;
 		}
 
-		ret = ov5640_set_stream(sensor, enable);
+		if (sensor->ep.bus_type == V4L2_MBUS_CSI2)
+			ret = ov5640_set_stream_mipi(sensor, enable);
+		else
+			ret = ov5640_set_stream_dvp(sensor, enable);
+
 		if (!ret)
 			sensor->streaming = enable;
 	}
@@ -2301,11 +2384,6 @@ static int ov5640_probe(struct i2c_client *client,
 		return ret;
 	}
 
-	if (sensor->ep.bus_type != V4L2_MBUS_CSI2) {
-		dev_err(dev, "invalid bus type, must be MIPI CSI2\n");
-		return -EINVAL;
-	}
-
 	/* get system clock (xclk) */
 	sensor->xclk = devm_clk_get(dev, "xclk");
 	if (IS_ERR(sensor->xclk)) {
-- 
1.9.1

[PATCH v3 5/5] media: ov5640: add support of RGB565 and YUYV formats

[Hugues Fruchet]

Add RGB565 (LE & BE) and YUV422 YUYV format in addition
to existing YUV422 UYVY format.

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

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index c791a67..4206f43 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -76,8 +76,10 @@
 #define OV5640_REG_HZ5060_CTRL01	0x3c01
 #define OV5640_REG_SIGMADELTA_CTRL0C	0x3c0c
 #define OV5640_REG_FRAME_CTRL01		0x4202
+#define OV5640_REG_FORMAT_CONTROL00	0x4300
 #define OV5640_REG_MIPI_CTRL00		0x4800
 #define OV5640_REG_DEBUG_MODE		0x4814
+#define OV5640_REG_ISP_FORMAT_MUX_CTRL	0x501f
 #define OV5640_REG_PRE_ISP_TEST_SET1	0x503d
 #define OV5640_REG_SDE_CTRL0		0x5580
 #define OV5640_REG_SDE_CTRL1		0x5581
@@ -105,6 +107,18 @@ enum ov5640_frame_rate {
 	OV5640_NUM_FRAMERATES,
 };
 
+struct ov5640_pixfmt {
+	u32 code;
+	u32 colorspace;
+};
+
+static const struct ov5640_pixfmt ov5640_formats[] = {
+	{ MEDIA_BUS_FMT_UYVY8_2X8, V4L2_COLORSPACE_SRGB, },
+	{ MEDIA_BUS_FMT_YUYV8_2X8, V4L2_COLORSPACE_SRGB, },
+	{ MEDIA_BUS_FMT_RGB565_2X8_LE, V4L2_COLORSPACE_SRGB, },
+	{ MEDIA_BUS_FMT_RGB565_2X8_BE, V4L2_COLORSPACE_SRGB, },
+};
+
 /*
  * FIXME: remove this when a subdev API becomes available
  * to set the MIPI CSI-2 virtual channel.
@@ -1803,17 +1817,23 @@ static int ov5640_try_fmt_internal(struct v4l2_subdev *sd,
 {
 	struct ov5640_dev *sensor = to_ov5640_dev(sd);
 	const struct ov5640_mode_info *mode;
+	int i;
 
 	mode = ov5640_find_mode(sensor, fr, fmt->width, fmt->height, true);
 	if (!mode)
 		return -EINVAL;
-
 	fmt->width = mode->width;
 	fmt->height = mode->height;
-	fmt->code = sensor->fmt.code;
 
 	if (new_mode)
 		*new_mode = mode;
+
+	for (i = 0; i < ARRAY_SIZE(ov5640_formats); i++)
+		if (ov5640_formats[i].code == fmt->code)
+			break;
+	if (i >= ARRAY_SIZE(ov5640_formats))
+		fmt->code = ov5640_formats[0].code;
+
 	return 0;
 }
 
@@ -1856,6 +1876,45 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
 	return ret;
 }
 
+static int ov5640_set_framefmt(struct ov5640_dev *sensor,
+			       struct v4l2_mbus_framefmt *format)
+{
+	int ret = 0;
+	bool is_rgb = false;
+	u8 val;
+
+	switch (format->code) {
+	case MEDIA_BUS_FMT_UYVY8_2X8:
+		/* YUV422, UYVY */
+		val = 0x3f;
+		break;
+	case MEDIA_BUS_FMT_YUYV8_2X8:
+		/* YUV422, YUYV */
+		val = 0x30;
+		break;
+	case MEDIA_BUS_FMT_RGB565_2X8_LE:
+		/* RGB565 {g[2:0],b[4:0]},{r[4:0],g[5:3]} */
+		val = 0x6F;
+		is_rgb = true;
+		break;
+	case MEDIA_BUS_FMT_RGB565_2X8_BE:
+		/* RGB565 {r[4:0],g[5:3]},{g[2:0],b[4:0]} */
+		val = 0x61;
+		is_rgb = true;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* FORMAT CONTROL00: YUV and RGB formatting */
+	ret = ov5640_write_reg(sensor, OV5640_REG_FORMAT_CONTROL00, val);
+	if (ret)
+		return ret;
+
+	/* FORMAT MUX CONTROL: ISP YUV or RGB */
+	return ov5640_write_reg(sensor, OV5640_REG_ISP_FORMAT_MUX_CTRL,
+				is_rgb ? 0x01 : 0x00);
+}
 
 /*
  * Sensor Controls.
@@ -2241,15 +2300,12 @@ static int ov5640_enum_mbus_code(struct v4l2_subdev *sd,
 				  struct v4l2_subdev_pad_config *cfg,
 				  struct v4l2_subdev_mbus_code_enum *code)
 {
-	struct ov5640_dev *sensor = to_ov5640_dev(sd);
-
 	if (code->pad != 0)
 		return -EINVAL;
-	if (code->index != 0)
+	if (code->index >= ARRAY_SIZE(ov5640_formats))
 		return -EINVAL;
 
-	code->code = sensor->fmt.code;
-
+	code->code = ov5640_formats[code->index].code;
 	return 0;
 }
 
@@ -2265,6 +2321,10 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
 			ret = ov5640_set_mode(sensor, sensor->current_mode);
 			if (ret)
 				goto out;
+
+			ret = ov5640_set_framefmt(sensor, &sensor->fmt);
+			if (ret)
+				goto out;
 		}
 
 		if (sensor->ep.bus_type == V4L2_MBUS_CSI2)
-- 
1.9.1