2020-10-20 11:28:20

by Hugues Fruchet

[permalink] [raw]
Subject: [PATCH v4 2/2] media: dt-bindings: media: st,stm32-dcmi: Add support of BT656

Add support of BT656 parallel bus mode in DCMI.
This mode is enabled when hsync-active & vsync-active
fields are not specified.

Signed-off-by: Hugues Fruchet <[email protected]>
---
.../devicetree/bindings/media/st,stm32-dcmi.yaml | 30 ++++++++++++++++++++++
1 file changed, 30 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
index 3fe778c..1ee521a 100644
--- a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
+++ b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
@@ -44,6 +44,36 @@ properties:
bindings defined in
Documentation/devicetree/bindings/media/video-interfaces.txt.

+ properties:
+ endpoint:
+ type: object
+
+ properties:
+ bus-width: true
+
+ hsync-active:
+ description:
+ If both HSYNC and VSYNC polarities are not specified, BT656
+ embedded synchronization is selected.
+ default: 0
+
+ vsync-active:
+ description:
+ If both HSYNC and VSYNC polarities are not specified, BT656
+ embedded synchronization is selected.
+ default: 0
+
+ pclk-sample: true
+
+ remote-endpoint: true
+
+ required:
+ - remote-endpoint
+
+ additionalProperties: false
+
+ additionalProperties: false
+
required:
- compatible
- reg
--
2.7.4


2020-10-21 14:17:11

by Sakari Ailus

[permalink] [raw]
Subject: Re: [PATCH v4 2/2] media: dt-bindings: media: st,stm32-dcmi: Add support of BT656

Hi Hugues,

On Tue, Oct 20, 2020 at 12:14:49PM +0200, Hugues Fruchet wrote:
> Add support of BT656 parallel bus mode in DCMI.
> This mode is enabled when hsync-active & vsync-active
> fields are not specified.
>
> Signed-off-by: Hugues Fruchet <[email protected]>
> ---
> .../devicetree/bindings/media/st,stm32-dcmi.yaml | 30 ++++++++++++++++++++++
> 1 file changed, 30 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> index 3fe778c..1ee521a 100644
> --- a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> +++ b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> @@ -44,6 +44,36 @@ properties:
> bindings defined in
> Documentation/devicetree/bindings/media/video-interfaces.txt.
>
> + properties:
> + endpoint:
> + type: object
> +
> + properties:
> + bus-width: true
> +
> + hsync-active:
> + description:
> + If both HSYNC and VSYNC polarities are not specified, BT656
> + embedded synchronization is selected.
> + default: 0
> +
> + vsync-active:
> + description:
> + If both HSYNC and VSYNC polarities are not specified, BT656
> + embedded synchronization is selected.
> + default: 0

Should I understand this as if the polarities were not specified, BT.656
will be used? The bindings previously documented BT.601 (parallel) only, so
it was somewhat ambigious to begin with. Is there a risk of interpreting
old BT.601 bindings as BT.656? With bus-type property, I believe you could
avoid at least that risk.

Also not specifying at least one of the default values leads to BT.656
without bus-type. That could be addressed by removing the defaults.

> +
> + pclk-sample: true
> +
> + remote-endpoint: true
> +
> + required:
> + - remote-endpoint
> +
> + additionalProperties: false
> +
> + additionalProperties: false
> +
> required:
> - compatible
> - reg

--
Regards,

Sakari Ailus

2020-10-21 14:49:35

by Hugues Fruchet

[permalink] [raw]
Subject: Re: [PATCH v4 2/2] media: dt-bindings: media: st,stm32-dcmi: Add support of BT656

Hi Sakari,

On 10/21/20 3:00 PM, Sakari Ailus wrote:
> Hi Hugues,
>
> On Tue, Oct 20, 2020 at 12:14:49PM +0200, Hugues Fruchet wrote:
>> Add support of BT656 parallel bus mode in DCMI.
>> This mode is enabled when hsync-active & vsync-active
>> fields are not specified.
>>
>> Signed-off-by: Hugues Fruchet <[email protected]>
>> ---
>> .../devicetree/bindings/media/st,stm32-dcmi.yaml | 30 ++++++++++++++++++++++
>> 1 file changed, 30 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
>> index 3fe778c..1ee521a 100644
>> --- a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
>> +++ b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
>> @@ -44,6 +44,36 @@ properties:
>> bindings defined in
>> Documentation/devicetree/bindings/media/video-interfaces.txt.
>>
>> + properties:
>> + endpoint:
>> + type: object
>> +
>> + properties:
>> + bus-width: true
>> +
>> + hsync-active:
>> + description:
>> + If both HSYNC and VSYNC polarities are not specified, BT656
>> + embedded synchronization is selected.
>> + default: 0
>> +
>> + vsync-active:
>> + description:
>> + If both HSYNC and VSYNC polarities are not specified, BT656
>> + embedded synchronization is selected.
>> + default: 0
>
> Should I understand this as if the polarities were not specified, BT.656
> will be used?

Yes, this is what is documented in video-interfaces.txt:
"
Note, that if HSYNC and VSYNC polarities are not specified, embedded
synchronization may be required, where supported.
"
and
"
/* If hsync-active/vsync-active are missing,
embedded BT.656 sync is used */
hsync-active = <0>; /* Active low */
vsync-active = <0>; /* Active low */
"
and I found also this in
Documentation/devicetree/bindings/media/renesas,vin.yaml
"
hsync-active:
description:
If both HSYNC and VSYNC polarities are not specified,
embedded
synchronization is selected.
default: 1

vsync-active:
description:
If both HSYNC and VSYNC polarities are not specified,
embedded
synchronization is selected.
default: 1
"

In the other hand I've found few occurences of "bus-type"
(marvell,mmp2-ccic.yaml), it is why I asked you if "bus-type" is the new
way to go versus previous way to signal BT656 (without hsync/vsync) ?
As explained previously, I prefer this last way for backward compatibility.


The bindings previously documented BT.601 (parallel) only, so
> it was somewhat ambigious to begin with. Is there a risk of interpreting
> old BT.601 bindings as BT.656?
I don't think so.

With bus-type property, I believe you could
> avoid at least that risk.
yes but as explained, I'll prefer not to amend current boards device
tree files.

>
> Also not specifying at least one of the default values leads to BT.656
> without bus-type. That could be addressed by removing the defaults.
>
I'm new to yaml, I've taken that from renesas,vin.yaml. Should I just
drop the "default: 1" lines ?

>> +
>> + pclk-sample: true
>> +
>> + remote-endpoint: true
>> +
>> + required:
>> + - remote-endpoint
>> +
>> + additionalProperties: false
>> +
>> + additionalProperties: false
>> +
>> required:
>> - compatible
>> - reg
>

BR,
Hugues.

2020-10-22 12:58:05

by Sakari Ailus

[permalink] [raw]
Subject: Re: [PATCH v4 2/2] media: dt-bindings: media: st,stm32-dcmi: Add support of BT656

Hi Hugues,

On Wed, Oct 21, 2020 at 02:24:08PM +0000, Hugues FRUCHET wrote:
> Hi Sakari,
>
> On 10/21/20 3:00 PM, Sakari Ailus wrote:
> > Hi Hugues,
> >
> > On Tue, Oct 20, 2020 at 12:14:49PM +0200, Hugues Fruchet wrote:
> >> Add support of BT656 parallel bus mode in DCMI.
> >> This mode is enabled when hsync-active & vsync-active
> >> fields are not specified.
> >>
> >> Signed-off-by: Hugues Fruchet <[email protected]>
> >> ---
> >> .../devicetree/bindings/media/st,stm32-dcmi.yaml | 30 ++++++++++++++++++++++
> >> 1 file changed, 30 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> >> index 3fe778c..1ee521a 100644
> >> --- a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> >> +++ b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> >> @@ -44,6 +44,36 @@ properties:
> >> bindings defined in
> >> Documentation/devicetree/bindings/media/video-interfaces.txt.
> >>
> >> + properties:
> >> + endpoint:
> >> + type: object
> >> +
> >> + properties:
> >> + bus-width: true
> >> +
> >> + hsync-active:
> >> + description:
> >> + If both HSYNC and VSYNC polarities are not specified, BT656
> >> + embedded synchronization is selected.
> >> + default: 0
> >> +
> >> + vsync-active:
> >> + description:
> >> + If both HSYNC and VSYNC polarities are not specified, BT656
> >> + embedded synchronization is selected.
> >> + default: 0
> >
> > Should I understand this as if the polarities were not specified, BT.656
> > will be used?
>
> Yes, this is what is documented in video-interfaces.txt:
> "
> Note, that if HSYNC and VSYNC polarities are not specified, embedded
> synchronization may be required, where supported.
> "
> and
> "
> /* If hsync-active/vsync-active are missing,
> embedded BT.656 sync is used */
> hsync-active = <0>; /* Active low */
> vsync-active = <0>; /* Active low */
> "
> and I found also this in
> Documentation/devicetree/bindings/media/renesas,vin.yaml
> "
> hsync-active:
> description:
> If both HSYNC and VSYNC polarities are not specified,
> embedded
> synchronization is selected.
> default: 1
>
> vsync-active:
> description:
> If both HSYNC and VSYNC polarities are not specified,
> embedded
> synchronization is selected.
> default: 1

Having the defaults leads to somewhat weird behaviour: specifying the
default value on either property changes the bus type.

> "
>
> In the other hand I've found few occurences of "bus-type"
> (marvell,mmp2-ccic.yaml), it is why I asked you if "bus-type" is the new
> way to go versus previous way to signal BT656 (without hsync/vsync) ?
> As explained previously, I prefer this last way for backward compatibility.

If you have a default for bus-type (BT.601), this won't be a problem.

The old DT bindings were somewhat, well, opportunistic. The v4l2-of
framework-let did its best and sometimes it worked. The behaviour is still
supported but not encouraged in new bindings.

>
>
> The bindings previously documented BT.601 (parallel) only, so
> > it was somewhat ambigious to begin with. Is there a risk of interpreting
> > old BT.601 bindings as BT.656?
> I don't think so.
>
> With bus-type property, I believe you could
> > avoid at least that risk.
> yes but as explained, I'll prefer not to amend current boards device
> tree files.

I don't think it matters from this point of view --- you can have a
default bus-type.

>
> >
> > Also not specifying at least one of the default values leads to BT.656
> > without bus-type. That could be addressed by removing the defaults.
> >
> I'm new to yaml, I've taken that from renesas,vin.yaml. Should I just
> drop the "default: 1" lines ?

That's one option, yes. Then you have to have those for BT.601 and it's no
longer ambiguous.

--
Regards,

Sakari Ailus

2020-10-22 20:26:20

by Hugues Fruchet

[permalink] [raw]
Subject: Re: [PATCH v4 2/2] media: dt-bindings: media: st,stm32-dcmi: Add support of BT656

Hi Sakari,

+ Jacopo for his work on ov772x binding related to BT656

On 10/21/20 11:40 PM, Sakari Ailus wrote:
> Hi Hugues,
>
> On Wed, Oct 21, 2020 at 02:24:08PM +0000, Hugues FRUCHET wrote:
>> Hi Sakari,
>>
>> On 10/21/20 3:00 PM, Sakari Ailus wrote:
>>> Hi Hugues,
>>>
>>> On Tue, Oct 20, 2020 at 12:14:49PM +0200, Hugues Fruchet wrote:
>>>> Add support of BT656 parallel bus mode in DCMI.
>>>> This mode is enabled when hsync-active & vsync-active
>>>> fields are not specified.
>>>>
>>>> Signed-off-by: Hugues Fruchet <[email protected]>
>>>> ---
>>>> .../devicetree/bindings/media/st,stm32-dcmi.yaml | 30 ++++++++++++++++++++++
>>>> 1 file changed, 30 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
>>>> index 3fe778c..1ee521a 100644
>>>> --- a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
>>>> +++ b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
>>>> @@ -44,6 +44,36 @@ properties:
>>>> bindings defined in
>>>> Documentation/devicetree/bindings/media/video-interfaces.txt.
>>>>
>>>> + properties:
>>>> + endpoint:
>>>> + type: object
>>>> +
>>>> + properties:
>>>> + bus-width: true
>>>> +
>>>> + hsync-active:
>>>> + description:
>>>> + If both HSYNC and VSYNC polarities are not specified, BT656
>>>> + embedded synchronization is selected.
>>>> + default: 0
>>>> +
>>>> + vsync-active:
>>>> + description:
>>>> + If both HSYNC and VSYNC polarities are not specified, BT656
>>>> + embedded synchronization is selected.
>>>> + default: 0
>>>
>>> Should I understand this as if the polarities were not specified, BT.656
>>> will be used?
>>
>> Yes, this is what is documented in video-interfaces.txt:
>> "
>> Note, that if HSYNC and VSYNC polarities are not specified, embedded
>> synchronization may be required, where supported.
>> "
>> and
>> "
>> /* If hsync-active/vsync-active are missing,
>> embedded BT.656 sync is used */
>> hsync-active = <0>; /* Active low */
>> vsync-active = <0>; /* Active low */
>> "
>> and I found also this in
>> Documentation/devicetree/bindings/media/renesas,vin.yaml
>> "
>> hsync-active:
>> description:
>> If both HSYNC and VSYNC polarities are not specified,
>> embedded
>> synchronization is selected.
>> default: 1
>>
>> vsync-active:
>> description:
>> If both HSYNC and VSYNC polarities are not specified,
>> embedded
>> synchronization is selected.
>> default: 1
>
> Having the defaults leads to somewhat weird behaviour: specifying the
> default value on either property changes the bus type.
>
>> "
>>
>> In the other hand I've found few occurences of "bus-type"
>> (marvell,mmp2-ccic.yaml), it is why I asked you if "bus-type" is the new
>> way to go versus previous way to signal BT656 (without hsync/vsync) ?
>> As explained previously, I prefer this last way for backward compatibility.
>
> If you have a default for bus-type (BT.601), this won't be a problem.
>
> The old DT bindings were somewhat, well, opportunistic. The v4l2-of
> framework-let did its best and sometimes it worked. The behaviour is still
> supported but not encouraged in new bindings.
>

OK, so let's go for the new way.
I've found an interesting patch from Jacopo that is of great help:
https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]/

Here is a draft proposal before I push a new version, please comment:

properties:
bus-type:
enum: [5, 6]
default: 5

bus-width:
enum: [8, 10, 12, 14]
default: 8

hsync-active:
enum: [0, 1]
default: 0

vsync-active:
enum: [0, 1]
default: 0

pclk-sample:
enum: [0, 1]
default: 0

remote-endpoint: true

allOf:
- if:
properties:
bus-type:
const: 6
then:
properties:
hsync-active: false
vsync-active: false
bus-width:
enum: [8]

required:
- remote-endpoint

unevaluatedProperties: false


Unfortunately, the "default: 5" for bus-type is not working !!
If we don't specify "bus-type" in example, dt_binding_check is failing
as if default was 6, it's hardly understandable (see below) !
port {
dcmi_0: endpoint {
remote-endpoint = <&ov5640_0>;
bus-width = <10>;
hsync-active = <0>;
vsync-active = <0>;
pclk-sample = <1>;
};
=> this should be OK but error claimed:
DTC
Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml
CHECK
Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml
Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml:
dcmi@4c006000: port:endpoint:vsync-active: False schema does not allow [[0]]
dcmi@4c006000: port:endpoint:hsync-active: False schema does not allow [[0]]
dcmi@4c006000: port:endpoint:bus-width:0:0: 10 is not one of [8]
From schema: Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml

=> if "bus-type" is explicitly set to 5, all is fine (see below) !
port {
dcmi_0: endpoint {
remote-endpoint = <&ov5640_0>;
bus-type = <5>;
bus-width = <10>;
hsync-active = <0>;
vsync-active = <0>;
pclk-sample = <1>;
};
};

DTC
Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml
CHECK
Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml
~/.../media_tree$


>>
>>
>> The bindings previously documented BT.601 (parallel) only, so
>>> it was somewhat ambigious to begin with. Is there a risk of interpreting
>>> old BT.601 bindings as BT.656?
>> I don't think so.
>>
>> With bus-type property, I believe you could
>>> avoid at least that risk.
>> yes but as explained, I'll prefer not to amend current boards device
>> tree files.
>
> I don't think it matters from this point of view --- you can have a
> default bus-type.
>
>>
>>>
>>> Also not specifying at least one of the default values leads to BT.656
>>> without bus-type. That could be addressed by removing the defaults.
>>>
>> I'm new to yaml, I've taken that from renesas,vin.yaml. Should I just
>> drop the "default: 1" lines ?
>
> That's one option, yes. Then you have to have those for BT.601 and it's no
> longer ambiguous.
>

BR,
Hugues.

2020-10-26 14:24:59

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH v4 2/2] media: dt-bindings: media: st,stm32-dcmi: Add support of BT656

On Thu, Oct 22, 2020 at 02:56:17PM +0000, Hugues FRUCHET wrote:
> Hi Sakari,
>
> + Jacopo for his work on ov772x binding related to BT656
>
> On 10/21/20 11:40 PM, Sakari Ailus wrote:
> > Hi Hugues,
> >
> > On Wed, Oct 21, 2020 at 02:24:08PM +0000, Hugues FRUCHET wrote:
> >> Hi Sakari,
> >>
> >> On 10/21/20 3:00 PM, Sakari Ailus wrote:
> >>> Hi Hugues,
> >>>
> >>> On Tue, Oct 20, 2020 at 12:14:49PM +0200, Hugues Fruchet wrote:
> >>>> Add support of BT656 parallel bus mode in DCMI.
> >>>> This mode is enabled when hsync-active & vsync-active
> >>>> fields are not specified.
> >>>>
> >>>> Signed-off-by: Hugues Fruchet <[email protected]>
> >>>> ---
> >>>> .../devicetree/bindings/media/st,stm32-dcmi.yaml | 30 ++++++++++++++++++++++
> >>>> 1 file changed, 30 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> >>>> index 3fe778c..1ee521a 100644
> >>>> --- a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> >>>> +++ b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> >>>> @@ -44,6 +44,36 @@ properties:
> >>>> bindings defined in
> >>>> Documentation/devicetree/bindings/media/video-interfaces.txt.
> >>>>
> >>>> + properties:
> >>>> + endpoint:
> >>>> + type: object
> >>>> +
> >>>> + properties:
> >>>> + bus-width: true
> >>>> +
> >>>> + hsync-active:
> >>>> + description:
> >>>> + If both HSYNC and VSYNC polarities are not specified, BT656
> >>>> + embedded synchronization is selected.
> >>>> + default: 0
> >>>> +
> >>>> + vsync-active:
> >>>> + description:
> >>>> + If both HSYNC and VSYNC polarities are not specified, BT656
> >>>> + embedded synchronization is selected.
> >>>> + default: 0
> >>>
> >>> Should I understand this as if the polarities were not specified, BT.656
> >>> will be used?
> >>
> >> Yes, this is what is documented in video-interfaces.txt:
> >> "
> >> Note, that if HSYNC and VSYNC polarities are not specified, embedded
> >> synchronization may be required, where supported.
> >> "
> >> and
> >> "
> >> /* If hsync-active/vsync-active are missing,
> >> embedded BT.656 sync is used */
> >> hsync-active = <0>; /* Active low */
> >> vsync-active = <0>; /* Active low */
> >> "
> >> and I found also this in
> >> Documentation/devicetree/bindings/media/renesas,vin.yaml
> >> "
> >> hsync-active:
> >> description:
> >> If both HSYNC and VSYNC polarities are not specified,
> >> embedded
> >> synchronization is selected.
> >> default: 1
> >>
> >> vsync-active:
> >> description:
> >> If both HSYNC and VSYNC polarities are not specified,
> >> embedded
> >> synchronization is selected.
> >> default: 1
> >
> > Having the defaults leads to somewhat weird behaviour: specifying the
> > default value on either property changes the bus type.
> >
> >> "
> >>
> >> In the other hand I've found few occurences of "bus-type"
> >> (marvell,mmp2-ccic.yaml), it is why I asked you if "bus-type" is the new
> >> way to go versus previous way to signal BT656 (without hsync/vsync) ?
> >> As explained previously, I prefer this last way for backward compatibility.
> >
> > If you have a default for bus-type (BT.601), this won't be a problem.
> >
> > The old DT bindings were somewhat, well, opportunistic. The v4l2-of
> > framework-let did its best and sometimes it worked. The behaviour is still
> > supported but not encouraged in new bindings.
> >
>
> OK, so let's go for the new way.
> I've found an interesting patch from Jacopo that is of great help:
> https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]/
>
> Here is a draft proposal before I push a new version, please comment:
>
> properties:
> bus-type:
> enum: [5, 6]
> default: 5
>
> bus-width:
> enum: [8, 10, 12, 14]
> default: 8
>
> hsync-active:
> enum: [0, 1]

For common properties, you can assume there's a common schema. As 0 and
1 are the only possible values, you don't need to define them here
unless only a subset is valid for this device.

> default: 0
>
> vsync-active:
> enum: [0, 1]
> default: 0
>
> pclk-sample:
> enum: [0, 1]
> default: 0
>
> remote-endpoint: true
>
> allOf:
> - if:
> properties:
> bus-type:
> const: 6

To fix the error, you need:

required:
- bus-type

The problem is the above schema is also true if the property
is not present.

> then:
> properties:
> hsync-active: false
> vsync-active: false
> bus-width:
> enum: [8]
>
> required:
> - remote-endpoint
>
> unevaluatedProperties: false
>
>
> Unfortunately, the "default: 5" for bus-type is not working !!
> If we don't specify "bus-type" in example, dt_binding_check is failing
> as if default was 6, it's hardly understandable (see below) !
> port {
> dcmi_0: endpoint {
> remote-endpoint = <&ov5640_0>;
> bus-width = <10>;
> hsync-active = <0>;
> vsync-active = <0>;
> pclk-sample = <1>;
> };
> => this should be OK but error claimed:
> DTC
> Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml
> CHECK
> Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml
> Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml:
> dcmi@4c006000: port:endpoint:vsync-active: False schema does not allow [[0]]
> dcmi@4c006000: port:endpoint:hsync-active: False schema does not allow [[0]]
> dcmi@4c006000: port:endpoint:bus-width:0:0: 10 is not one of [8]
> From schema: Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
>
> => if "bus-type" is explicitly set to 5, all is fine (see below) !
> port {
> dcmi_0: endpoint {
> remote-endpoint = <&ov5640_0>;
> bus-type = <5>;
> bus-width = <10>;
> hsync-active = <0>;
> vsync-active = <0>;
> pclk-sample = <1>;
> };
> };
>
> DTC
> Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml
> CHECK
> Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml
> ~/.../media_tree$
>
>
> >>
> >>
> >> The bindings previously documented BT.601 (parallel) only, so
> >>> it was somewhat ambigious to begin with. Is there a risk of interpreting
> >>> old BT.601 bindings as BT.656?
> >> I don't think so.
> >>
> >> With bus-type property, I believe you could
> >>> avoid at least that risk.
> >> yes but as explained, I'll prefer not to amend current boards device
> >> tree files.
> >
> > I don't think it matters from this point of view --- you can have a
> > default bus-type.
> >
> >>
> >>>
> >>> Also not specifying at least one of the default values leads to BT.656
> >>> without bus-type. That could be addressed by removing the defaults.
> >>>
> >> I'm new to yaml, I've taken that from renesas,vin.yaml. Should I just
> >> drop the "default: 1" lines ?
> >
> > That's one option, yes. Then you have to have those for BT.601 and it's no
> > longer ambiguous.
> >
>
> BR,
> Hugues.

2020-10-30 17:44:33

by Sakari Ailus

[permalink] [raw]
Subject: Re: [PATCH v4 2/2] media: dt-bindings: media: st,stm32-dcmi: Add support of BT656

Hi Rob,

On Mon, Oct 26, 2020 at 09:17:14AM -0500, Rob Herring wrote:
> On Thu, Oct 22, 2020 at 02:56:17PM +0000, Hugues FRUCHET wrote:
> > Hi Sakari,
> >
> > + Jacopo for his work on ov772x binding related to BT656
> >
> > On 10/21/20 11:40 PM, Sakari Ailus wrote:
> > > Hi Hugues,
> > >
> > > On Wed, Oct 21, 2020 at 02:24:08PM +0000, Hugues FRUCHET wrote:
> > >> Hi Sakari,
> > >>
> > >> On 10/21/20 3:00 PM, Sakari Ailus wrote:
> > >>> Hi Hugues,
> > >>>
> > >>> On Tue, Oct 20, 2020 at 12:14:49PM +0200, Hugues Fruchet wrote:
> > >>>> Add support of BT656 parallel bus mode in DCMI.
> > >>>> This mode is enabled when hsync-active & vsync-active
> > >>>> fields are not specified.
> > >>>>
> > >>>> Signed-off-by: Hugues Fruchet <[email protected]>
> > >>>> ---
> > >>>> .../devicetree/bindings/media/st,stm32-dcmi.yaml | 30 ++++++++++++++++++++++
> > >>>> 1 file changed, 30 insertions(+)
> > >>>>
> > >>>> diff --git a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> > >>>> index 3fe778c..1ee521a 100644
> > >>>> --- a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> > >>>> +++ b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> > >>>> @@ -44,6 +44,36 @@ properties:
> > >>>> bindings defined in
> > >>>> Documentation/devicetree/bindings/media/video-interfaces.txt.
> > >>>>
> > >>>> + properties:
> > >>>> + endpoint:
> > >>>> + type: object
> > >>>> +
> > >>>> + properties:
> > >>>> + bus-width: true
> > >>>> +
> > >>>> + hsync-active:
> > >>>> + description:
> > >>>> + If both HSYNC and VSYNC polarities are not specified, BT656
> > >>>> + embedded synchronization is selected.
> > >>>> + default: 0
> > >>>> +
> > >>>> + vsync-active:
> > >>>> + description:
> > >>>> + If both HSYNC and VSYNC polarities are not specified, BT656
> > >>>> + embedded synchronization is selected.
> > >>>> + default: 0
> > >>>
> > >>> Should I understand this as if the polarities were not specified, BT.656
> > >>> will be used?
> > >>
> > >> Yes, this is what is documented in video-interfaces.txt:
> > >> "
> > >> Note, that if HSYNC and VSYNC polarities are not specified, embedded
> > >> synchronization may be required, where supported.
> > >> "
> > >> and
> > >> "
> > >> /* If hsync-active/vsync-active are missing,
> > >> embedded BT.656 sync is used */
> > >> hsync-active = <0>; /* Active low */
> > >> vsync-active = <0>; /* Active low */
> > >> "
> > >> and I found also this in
> > >> Documentation/devicetree/bindings/media/renesas,vin.yaml
> > >> "
> > >> hsync-active:
> > >> description:
> > >> If both HSYNC and VSYNC polarities are not specified,
> > >> embedded
> > >> synchronization is selected.
> > >> default: 1
> > >>
> > >> vsync-active:
> > >> description:
> > >> If both HSYNC and VSYNC polarities are not specified,
> > >> embedded
> > >> synchronization is selected.
> > >> default: 1
> > >
> > > Having the defaults leads to somewhat weird behaviour: specifying the
> > > default value on either property changes the bus type.
> > >
> > >> "
> > >>
> > >> In the other hand I've found few occurences of "bus-type"
> > >> (marvell,mmp2-ccic.yaml), it is why I asked you if "bus-type" is the new
> > >> way to go versus previous way to signal BT656 (without hsync/vsync) ?
> > >> As explained previously, I prefer this last way for backward compatibility.
> > >
> > > If you have a default for bus-type (BT.601), this won't be a problem.
> > >
> > > The old DT bindings were somewhat, well, opportunistic. The v4l2-of
> > > framework-let did its best and sometimes it worked. The behaviour is still
> > > supported but not encouraged in new bindings.
> > >
> >
> > OK, so let's go for the new way.
> > I've found an interesting patch from Jacopo that is of great help:
> > https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]/

I wonder if Jacopo tested it. The idea seems interesting nonetheless.

> >
> > Here is a draft proposal before I push a new version, please comment:
> >
> > properties:
> > bus-type:
> > enum: [5, 6]
> > default: 5
> >
> > bus-width:
> > enum: [8, 10, 12, 14]
> > default: 8
> >
> > hsync-active:
> > enum: [0, 1]
>
> For common properties, you can assume there's a common schema. As 0 and
> 1 are the only possible values, you don't need to define them here
> unless only a subset is valid for this device.
>
> > default: 0
> >
> > vsync-active:
> > enum: [0, 1]
> > default: 0
> >
> > pclk-sample:
> > enum: [0, 1]
> > default: 0
> >
> > remote-endpoint: true
> >
> > allOf:
> > - if:
> > properties:
> > bus-type:
> > const: 6
>
> To fix the error, you need:
>
> required:
> - bus-type
>
> The problem is the above schema is also true if the property
> is not present.

Hmm. The idea was that we could keep this consistent with old bindings that
only documented parallel mode, and thus didn't need bus-type. This is
actually quite common --- adding support for something that wasn't known or
cared for during the original review.

I guess this could be done in the driver, too, adding a comment that the
bindings earlier did not require it.

>
> > then:
> > properties:
> > hsync-active: false
> > vsync-active: false
> > bus-width:
> > enum: [8]
> >
> > required:
> > - remote-endpoint
> >
> > unevaluatedProperties: false
> >
> >
> > Unfortunately, the "default: 5" for bus-type is not working !!
> > If we don't specify "bus-type" in example, dt_binding_check is failing
> > as if default was 6, it's hardly understandable (see below) !
> > port {
> > dcmi_0: endpoint {
> > remote-endpoint = <&ov5640_0>;
> > bus-width = <10>;
> > hsync-active = <0>;
> > vsync-active = <0>;
> > pclk-sample = <1>;
> > };
> > => this should be OK but error claimed:
> > DTC
> > Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml
> > CHECK
> > Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml
> > Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml:
> > dcmi@4c006000: port:endpoint:vsync-active: False schema does not allow [[0]]
> > dcmi@4c006000: port:endpoint:hsync-active: False schema does not allow [[0]]
> > dcmi@4c006000: port:endpoint:bus-width:0:0: 10 is not one of [8]
> > From schema: Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> >
> > => if "bus-type" is explicitly set to 5, all is fine (see below) !
> > port {
> > dcmi_0: endpoint {
> > remote-endpoint = <&ov5640_0>;
> > bus-type = <5>;
> > bus-width = <10>;
> > hsync-active = <0>;
> > vsync-active = <0>;
> > pclk-sample = <1>;
> > };
> > };
> >
> > DTC
> > Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml
> > CHECK
> > Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml
> > ~/.../media_tree$
> >
> >
> > >>
> > >>
> > >> The bindings previously documented BT.601 (parallel) only, so
> > >>> it was somewhat ambigious to begin with. Is there a risk of interpreting
> > >>> old BT.601 bindings as BT.656?
> > >> I don't think so.
> > >>
> > >> With bus-type property, I believe you could
> > >>> avoid at least that risk.
> > >> yes but as explained, I'll prefer not to amend current boards device
> > >> tree files.
> > >
> > > I don't think it matters from this point of view --- you can have a
> > > default bus-type.
> > >
> > >>
> > >>>
> > >>> Also not specifying at least one of the default values leads to BT.656
> > >>> without bus-type. That could be addressed by removing the defaults.
> > >>>
> > >> I'm new to yaml, I've taken that from renesas,vin.yaml. Should I just
> > >> drop the "default: 1" lines ?
> > >
> > > That's one option, yes. Then you have to have those for BT.601 and it's no
> > > longer ambiguous.
> > >
> >
> > BR,
> > Hugues.

--
Kind regards,

Sakari Ailus

2020-10-30 18:02:57

by jacopo mondi

[permalink] [raw]
Subject: Re: [PATCH v4 2/2] media: dt-bindings: media: st,stm32-dcmi: Add support of BT656

Hi Sakari,

On Fri, Oct 30, 2020 at 07:42:36PM +0200, Sakari Ailus wrote:
> Hi Rob,
>
> On Mon, Oct 26, 2020 at 09:17:14AM -0500, Rob Herring wrote:
> > On Thu, Oct 22, 2020 at 02:56:17PM +0000, Hugues FRUCHET wrote:
> > > Hi Sakari,
> > >
> > > + Jacopo for his work on ov772x binding related to BT656
> > >
> > > On 10/21/20 11:40 PM, Sakari Ailus wrote:
> > > > Hi Hugues,
> > > >
> > > > On Wed, Oct 21, 2020 at 02:24:08PM +0000, Hugues FRUCHET wrote:
> > > >> Hi Sakari,
> > > >>
> > > >> On 10/21/20 3:00 PM, Sakari Ailus wrote:
> > > >>> Hi Hugues,
> > > >>>
> > > >>> On Tue, Oct 20, 2020 at 12:14:49PM +0200, Hugues Fruchet wrote:
> > > >>>> Add support of BT656 parallel bus mode in DCMI.
> > > >>>> This mode is enabled when hsync-active & vsync-active
> > > >>>> fields are not specified.
> > > >>>>
> > > >>>> Signed-off-by: Hugues Fruchet <[email protected]>
> > > >>>> ---
> > > >>>> .../devicetree/bindings/media/st,stm32-dcmi.yaml | 30 ++++++++++++++++++++++
> > > >>>> 1 file changed, 30 insertions(+)
> > > >>>>
> > > >>>> diff --git a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> > > >>>> index 3fe778c..1ee521a 100644
> > > >>>> --- a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> > > >>>> +++ b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> > > >>>> @@ -44,6 +44,36 @@ properties:
> > > >>>> bindings defined in
> > > >>>> Documentation/devicetree/bindings/media/video-interfaces.txt.
> > > >>>>
> > > >>>> + properties:
> > > >>>> + endpoint:
> > > >>>> + type: object
> > > >>>> +
> > > >>>> + properties:
> > > >>>> + bus-width: true
> > > >>>> +
> > > >>>> + hsync-active:
> > > >>>> + description:
> > > >>>> + If both HSYNC and VSYNC polarities are not specified, BT656
> > > >>>> + embedded synchronization is selected.
> > > >>>> + default: 0
> > > >>>> +
> > > >>>> + vsync-active:
> > > >>>> + description:
> > > >>>> + If both HSYNC and VSYNC polarities are not specified, BT656
> > > >>>> + embedded synchronization is selected.
> > > >>>> + default: 0
> > > >>>
> > > >>> Should I understand this as if the polarities were not specified, BT.656
> > > >>> will be used?
> > > >>
> > > >> Yes, this is what is documented in video-interfaces.txt:
> > > >> "
> > > >> Note, that if HSYNC and VSYNC polarities are not specified, embedded
> > > >> synchronization may be required, where supported.
> > > >> "
> > > >> and
> > > >> "
> > > >> /* If hsync-active/vsync-active are missing,
> > > >> embedded BT.656 sync is used */
> > > >> hsync-active = <0>; /* Active low */
> > > >> vsync-active = <0>; /* Active low */
> > > >> "
> > > >> and I found also this in
> > > >> Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > >> "
> > > >> hsync-active:
> > > >> description:
> > > >> If both HSYNC and VSYNC polarities are not specified,
> > > >> embedded
> > > >> synchronization is selected.
> > > >> default: 1
> > > >>
> > > >> vsync-active:
> > > >> description:
> > > >> If both HSYNC and VSYNC polarities are not specified,
> > > >> embedded
> > > >> synchronization is selected.
> > > >> default: 1
> > > >
> > > > Having the defaults leads to somewhat weird behaviour: specifying the
> > > > default value on either property changes the bus type.
> > > >
> > > >> "
> > > >>
> > > >> In the other hand I've found few occurences of "bus-type"
> > > >> (marvell,mmp2-ccic.yaml), it is why I asked you if "bus-type" is the new
> > > >> way to go versus previous way to signal BT656 (without hsync/vsync) ?
> > > >> As explained previously, I prefer this last way for backward compatibility.
> > > >
> > > > If you have a default for bus-type (BT.601), this won't be a problem.
> > > >
> > > > The old DT bindings were somewhat, well, opportunistic. The v4l2-of
> > > > framework-let did its best and sometimes it worked. The behaviour is still
> > > > supported but not encouraged in new bindings.
> > > >
> > >
> > > OK, so let's go for the new way.
> > > I've found an interesting patch from Jacopo that is of great help:
> > > https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]/
>
> I wonder if Jacopo tested it. The idea seems interesting nonetheless.
>

I think I did, but in my series bus-type was required so I guess I don't
get bitten by the bus-type mismatch, if I got the below error right


> > >
> > > Here is a draft proposal before I push a new version, please comment:
> > >
> > > properties:
> > > bus-type:
> > > enum: [5, 6]
> > > default: 5
> > >
> > > bus-width:
> > > enum: [8, 10, 12, 14]
> > > default: 8
> > >
> > > hsync-active:
> > > enum: [0, 1]
> >
> > For common properties, you can assume there's a common schema. As 0 and
> > 1 are the only possible values, you don't need to define them here
> > unless only a subset is valid for this device.
> >
> > > default: 0
> > >
> > > vsync-active:
> > > enum: [0, 1]
> > > default: 0
> > >
> > > pclk-sample:
> > > enum: [0, 1]
> > > default: 0
> > >
> > > remote-endpoint: true
> > >
> > > allOf:
> > > - if:
> > > properties:
> > > bus-type:
> > > const: 6
> >
> > To fix the error, you need:
> >
> > required:
> > - bus-type
> >
> > The problem is the above schema is also true if the property
> > is not present.
>
> Hmm. The idea was that we could keep this consistent with old bindings that
> only documented parallel mode, and thus didn't need bus-type. This is
> actually quite common --- adding support for something that wasn't known or
> cared for during the original review.
>
> I guess this could be done in the driver, too, adding a comment that the
> bindings earlier did not require it.
>
> >
> > > then:
> > > properties:
> > > hsync-active: false
> > > vsync-active: false
> > > bus-width:
> > > enum: [8]
> > >
> > > required:
> > > - remote-endpoint
> > >
> > > unevaluatedProperties: false
> > >
> > >
> > > Unfortunately, the "default: 5" for bus-type is not working !!
> > > If we don't specify "bus-type" in example, dt_binding_check is failing
> > > as if default was 6, it's hardly understandable (see below) !
> > > port {
> > > dcmi_0: endpoint {
> > > remote-endpoint = <&ov5640_0>;
> > > bus-width = <10>;
> > > hsync-active = <0>;
> > > vsync-active = <0>;
> > > pclk-sample = <1>;
> > > };
> > > => this should be OK but error claimed:
> > > DTC
> > > Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml
> > > CHECK
> > > Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml
> > > Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml:
> > > dcmi@4c006000: port:endpoint:vsync-active: False schema does not allow [[0]]
> > > dcmi@4c006000: port:endpoint:hsync-active: False schema does not allow [[0]]
> > > dcmi@4c006000: port:endpoint:bus-width:0:0: 10 is not one of [8]
> > > From schema: Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> > >
> > > => if "bus-type" is explicitly set to 5, all is fine (see below) !
> > > port {
> > > dcmi_0: endpoint {
> > > remote-endpoint = <&ov5640_0>;
> > > bus-type = <5>;
> > > bus-width = <10>;
> > > hsync-active = <0>;
> > > vsync-active = <0>;
> > > pclk-sample = <1>;
> > > };
> > > };
> > >
> > > DTC
> > > Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml
> > > CHECK
> > > Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml
> > > ~/.../media_tree$
> > >
> > >
> > > >>
> > > >>
> > > >> The bindings previously documented BT.601 (parallel) only, so
> > > >>> it was somewhat ambigious to begin with. Is there a risk of interpreting
> > > >>> old BT.601 bindings as BT.656?
> > > >> I don't think so.
> > > >>
> > > >> With bus-type property, I believe you could
> > > >>> avoid at least that risk.
> > > >> yes but as explained, I'll prefer not to amend current boards device
> > > >> tree files.
> > > >
> > > > I don't think it matters from this point of view --- you can have a
> > > > default bus-type.
> > > >
> > > >>
> > > >>>
> > > >>> Also not specifying at least one of the default values leads to BT.656
> > > >>> without bus-type. That could be addressed by removing the defaults.
> > > >>>
> > > >> I'm new to yaml, I've taken that from renesas,vin.yaml. Should I just
> > > >> drop the "default: 1" lines ?
> > > >
> > > > That's one option, yes. Then you have to have those for BT.601 and it's no
> > > > longer ambiguous.
> > > >
> > >
> > > BR,
> > > Hugues.
>
> --
> Kind regards,
>
> Sakari Ailus

2020-10-30 20:13:24

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH v4 2/2] media: dt-bindings: media: st,stm32-dcmi: Add support of BT656

On Fri, Oct 30, 2020 at 12:42 PM Sakari Ailus
<[email protected]> wrote:
>
> Hi Rob,
>
> On Mon, Oct 26, 2020 at 09:17:14AM -0500, Rob Herring wrote:
> > On Thu, Oct 22, 2020 at 02:56:17PM +0000, Hugues FRUCHET wrote:
> > > Hi Sakari,
> > >
> > > + Jacopo for his work on ov772x binding related to BT656
> > >
> > > On 10/21/20 11:40 PM, Sakari Ailus wrote:
> > > > Hi Hugues,
> > > >
> > > > On Wed, Oct 21, 2020 at 02:24:08PM +0000, Hugues FRUCHET wrote:
> > > >> Hi Sakari,
> > > >>
> > > >> On 10/21/20 3:00 PM, Sakari Ailus wrote:
> > > >>> Hi Hugues,
> > > >>>
> > > >>> On Tue, Oct 20, 2020 at 12:14:49PM +0200, Hugues Fruchet wrote:
> > > >>>> Add support of BT656 parallel bus mode in DCMI.
> > > >>>> This mode is enabled when hsync-active & vsync-active
> > > >>>> fields are not specified.
> > > >>>>
> > > >>>> Signed-off-by: Hugues Fruchet <[email protected]>
> > > >>>> ---
> > > >>>> .../devicetree/bindings/media/st,stm32-dcmi.yaml | 30 ++++++++++++++++++++++
> > > >>>> 1 file changed, 30 insertions(+)
> > > >>>>
> > > >>>> diff --git a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> > > >>>> index 3fe778c..1ee521a 100644
> > > >>>> --- a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> > > >>>> +++ b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> > > >>>> @@ -44,6 +44,36 @@ properties:
> > > >>>> bindings defined in
> > > >>>> Documentation/devicetree/bindings/media/video-interfaces.txt.
> > > >>>>
> > > >>>> + properties:
> > > >>>> + endpoint:
> > > >>>> + type: object
> > > >>>> +
> > > >>>> + properties:
> > > >>>> + bus-width: true
> > > >>>> +
> > > >>>> + hsync-active:
> > > >>>> + description:
> > > >>>> + If both HSYNC and VSYNC polarities are not specified, BT656
> > > >>>> + embedded synchronization is selected.
> > > >>>> + default: 0
> > > >>>> +
> > > >>>> + vsync-active:
> > > >>>> + description:
> > > >>>> + If both HSYNC and VSYNC polarities are not specified, BT656
> > > >>>> + embedded synchronization is selected.
> > > >>>> + default: 0
> > > >>>
> > > >>> Should I understand this as if the polarities were not specified, BT.656
> > > >>> will be used?
> > > >>
> > > >> Yes, this is what is documented in video-interfaces.txt:
> > > >> "
> > > >> Note, that if HSYNC and VSYNC polarities are not specified, embedded
> > > >> synchronization may be required, where supported.
> > > >> "
> > > >> and
> > > >> "
> > > >> /* If hsync-active/vsync-active are missing,
> > > >> embedded BT.656 sync is used */
> > > >> hsync-active = <0>; /* Active low */
> > > >> vsync-active = <0>; /* Active low */
> > > >> "
> > > >> and I found also this in
> > > >> Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > >> "
> > > >> hsync-active:
> > > >> description:
> > > >> If both HSYNC and VSYNC polarities are not specified,
> > > >> embedded
> > > >> synchronization is selected.
> > > >> default: 1
> > > >>
> > > >> vsync-active:
> > > >> description:
> > > >> If both HSYNC and VSYNC polarities are not specified,
> > > >> embedded
> > > >> synchronization is selected.
> > > >> default: 1
> > > >
> > > > Having the defaults leads to somewhat weird behaviour: specifying the
> > > > default value on either property changes the bus type.
> > > >
> > > >> "
> > > >>
> > > >> In the other hand I've found few occurences of "bus-type"
> > > >> (marvell,mmp2-ccic.yaml), it is why I asked you if "bus-type" is the new
> > > >> way to go versus previous way to signal BT656 (without hsync/vsync) ?
> > > >> As explained previously, I prefer this last way for backward compatibility.
> > > >
> > > > If you have a default for bus-type (BT.601), this won't be a problem.
> > > >
> > > > The old DT bindings were somewhat, well, opportunistic. The v4l2-of
> > > > framework-let did its best and sometimes it worked. The behaviour is still
> > > > supported but not encouraged in new bindings.
> > > >
> > >
> > > OK, so let's go for the new way.
> > > I've found an interesting patch from Jacopo that is of great help:
> > > https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]/
>
> I wonder if Jacopo tested it. The idea seems interesting nonetheless.
>
> > >
> > > Here is a draft proposal before I push a new version, please comment:
> > >
> > > properties:
> > > bus-type:
> > > enum: [5, 6]
> > > default: 5
> > >
> > > bus-width:
> > > enum: [8, 10, 12, 14]
> > > default: 8
> > >
> > > hsync-active:
> > > enum: [0, 1]
> >
> > For common properties, you can assume there's a common schema. As 0 and
> > 1 are the only possible values, you don't need to define them here
> > unless only a subset is valid for this device.
> >
> > > default: 0
> > >
> > > vsync-active:
> > > enum: [0, 1]
> > > default: 0
> > >
> > > pclk-sample:
> > > enum: [0, 1]
> > > default: 0
> > >
> > > remote-endpoint: true
> > >
> > > allOf:
> > > - if:
> > > properties:
> > > bus-type:
> > > const: 6
> >
> > To fix the error, you need:
> >
> > required:
> > - bus-type
> >
> > The problem is the above schema is also true if the property
> > is not present.
>
> Hmm. The idea was that we could keep this consistent with old bindings that
> only documented parallel mode, and thus didn't need bus-type. This is
> actually quite common --- adding support for something that wasn't known or
> cared for during the original review.

TBC, the 'required' here is required for the 'if' in the if/then
schema to work as you want. It's not making 'bus-type' a required
property.

Rob