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

[Rob Herring]

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 <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]

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.