| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20201026141714.GA83693@bogus>
Date: Mon, 26 Oct 2020 09:17:14 -0500
From: Rob Herring <robh@...nel.org>
To: Hugues FRUCHET <hugues.fruchet@...com>
Cc: Sakari Ailus <sakari.ailus@...ux.intel.com>,
Jacopo Mondi <jacopo@...ndi.org>,
Alexandre TORGUE <alexandre.torgue@...com>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Hans Verkuil <hverkuil@...all.nl>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
"linux-stm32@...md-mailman.stormreply.com"
<linux-stm32@...md-mailman.stormreply.com>,
Alain VOLMAT <alain.volmat@...com>,
Yannick FERTRE <yannick.fertre@...com>,
Philippe CORNU <philippe.cornu@...com>
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 <hugues.fruchet@...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.
> >
>
> 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@jmondi.org/
>
> 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@...06000: port:endpoint:vsync-active: False schema does not allow [[0]]
> dcmi@...06000: port:endpoint:hsync-active: False schema does not allow [[0]]
> dcmi@...06000: 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.
Powered by blists - more mailing lists