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@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 ?
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 = <¶llel_from_ov5640>;
>>> + };
>>> + };
>>> + };
>>> +};
>>> --
>>> 1.9.1
>>>
>>
>
> Best regards,
> Hugues.
>
--
Kind regards,
Sakari Ailus
sakari.ailus@xxxxxx
