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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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@st.com>
> >> ---
> >>   .../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