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

From: Sakari Ailus
Date: Wed Oct 21 2020 - 17:41:07 EST


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.

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