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@xxxxxx>
>> ---
>> .../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 = <¶llel_from_ov5640>;
>> + };
>> + };
>> + };
>> +};
>> --
>> 1.9.1
>>
>
Best regards,
Hugues.
