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 <hugues.fruchet@xxxxxx> >>>> --- >>>> .../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/20200910162055.614089-4-jacopo+renesas@xxxxxxxxxx/ 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.
