| 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: <CAL_Jsq+po4grPDJH6=ayFWrO5J=GzmSHNsgRjQ=ERsVCYzVXQg@mail.gmail.com>
Date: Fri, 30 Oct 2020 15:09:10 -0500
From: Rob Herring <robh@...nel.org>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: Hugues FRUCHET <hugues.fruchet@...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 Fri, Oct 30, 2020 at 12:42 PM Sakari Ailus
<sakari.ailus@...ux.intel.com> wrote:
>
> Hi Rob,
>
> On Mon, Oct 26, 2020 at 09:17:14AM -0500, Rob Herring wrote:
> > 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/
>
> I wonder if Jacopo tested it. The idea seems interesting nonetheless.
>
> > >
> > > 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.
>
> Hmm. The idea was that we could keep this consistent with old bindings that
> only documented parallel mode, and thus didn't need bus-type. This is
> actually quite common --- adding support for something that wasn't known or
> cared for during the original review.
TBC, the 'required' here is required for the 'if' in the if/then
schema to work as you want. It's not making 'bus-type' a required
property.
Rob
Powered by blists - more mailing lists