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  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 16 Dec 2020 19:43:45 +0200
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     Rob Herring <robh@...nel.org>
Cc:     Guennadi Liakhovetski <g.liakhovetski@....de>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Jacopo Mondi <jacopo@...ndi.org>,
        Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-media@...r.kernel.org
Subject: Re: [PATCH v3 2/2] dt-bindings: media: Use graph and
 video-interfaces schemas

Hi Rob,

On Wed, Dec 16, 2020 at 11:38:41AM -0600, Rob Herring wrote:
> On Wed, Dec 16, 2020 at 05:19:55PM +0200, Laurent Pinchart wrote:
> > On Thu, Dec 10, 2020 at 03:16:25PM -0600, Rob Herring wrote:
> > > Now that we have graph and video-interfaces schemas, rework the media
> > > related schemas to use them.
> > > 
> > > Cc: Maxime Ripard <mripard@...nel.org>
> > > Cc: Mauro Carvalho Chehab <mchehab@...nel.org>
> > > Cc: Jacopo Mondi <jacopo@...ndi.org>
> > > Cc: Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>
> > > Cc: linux-media@...r.kernel.org
> > > Signed-off-by: Rob Herring <robh@...nel.org>
> > > ---
> 
> 
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/mipi-ccs.yaml b/Documentation/devicetree/bindings/media/i2c/mipi-ccs.yaml
> > > index d94bd67ccea1..3657f2f41098 100644
> > > --- a/Documentation/devicetree/bindings/media/i2c/mipi-ccs.yaml
> > > +++ b/Documentation/devicetree/bindings/media/i2c/mipi-ccs.yaml
> > > @@ -73,19 +73,16 @@ properties:
> > >      enum: [ 0, 180 ]
> > > 
> > >    port:
> > > -    type: object
> > > +    $ref: /schemas/graph.yaml#/$defs/port-base
> > > +    additionalProperties: false
> > > +
> > >      properties:
> > >        endpoint:
> > > -        type: object
> > > +        $ref: /schemas/media/video-interfaces.yaml#
> > > +        unevaluatedProperties: false
> > > +
> > >          properties:
> > > -          link-frequencies:
> > > -            $ref: /schemas/types.yaml#/definitions/uint64-array
> > > -            description: List of allowed data link frequencies.
> > > -          data-lanes:
> > > -            minItems: 1
> > > -            maxItems: 8
> > 
> > Don't we need
> > 
> >           link-frequencies: true
> >           data-lanes: true
> > 
> > to convey the fact that those properties are applicable for this device
> > ? This applies to a few locations below too.
> 
> Adding them would convey that to the reader, but wouldn't change the 
> schema validation. We'd have to use 'additionalProperties' instead and 
> also add 'remote-endpoint' everywhere (and 'reg' sometimes). I prefer 
> not doing all that, but if we want it just for purposes of documenting 
> the usage, that's fine.

I'd prefer keeping it to document what properties are applicable. If we
can later find a better way to express it in a way that will be taken
into account during validation, that will be best, but not required now.

> > >            bus-type:
> > > -            description: The type of the data bus.
> > >              oneOf:
> > >                - const: 1 # CSI-2 C-PHY
> > >                - const: 3 # CCP2
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5647.yaml b/Documentation/devicetree/bindings/media/i2c/ov5647.yaml
> > > index 280c62afae13..3b1ea9da437a 100644
> > > --- a/Documentation/devicetree/bindings/media/i2c/ov5647.yaml
> > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5647.yaml
> > > @@ -31,27 +31,15 @@ properties:
> > >      maxItems: 1
> > > 
> > >    port:
> > > -    type: object
> > > -    description: |-
> > > -      Should contain one endpoint sub-node used to model connection to the
> > > -      video receiver according to the specification defined in
> > > -      Documentation/devicetree/bindings/media/video-interfaces.txt.
> > > +    $ref: /schemas/graph.yaml#/$defs/port-base
> > > 
> > >      properties:
> > >        endpoint:
> > > -        type: object
> > > +        $ref: /schemas/media/video-interfaces.yaml#
> > > +        unevaluatedProperties: false
> > > 
> > >          properties:
> > > -          remote-endpoint:
> > > -            description: |-
> > > -              phandle to the video receiver input port.
> > > -
> > > -          clock-noncontinuous:
> > > -            type: boolean
> > > -            description: |-
> > > -              Set to true to allow MIPI CSI-2 non-continuous clock operations.
> > > -
> > > -        additionalProperties: false
> > > +          clock-noncontinuous: true
> > > 
> > >      additionalProperties: false
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov8856.yaml b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml
> > > index cde85553fd01..c29b057ae922 100644
> > > --- a/Documentation/devicetree/bindings/media/i2c/ov8856.yaml
> > > +++ b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml
> > > @@ -57,16 +57,13 @@ properties:
> > >        active low.
> > > 
> > >    port:
> > > -    type: object
> > > +    $ref: /schemas/graph.yaml#/$defs/port-base
> > >      additionalProperties: false
> > > -    description:
> > > -      A node containing an output port node with an endpoint definition
> > > -      as documented in
> > > -      Documentation/devicetree/bindings/media/video-interfaces.txt
> > > 
> > >      properties:
> > >        endpoint:
> > > -        type: object
> > > +        $ref: /schemas/media/video-interfaces.yaml#
> > > +        unevaluatedProperties: false
> > > 
> > >          properties:
> > >            data-lanes:
> > > @@ -79,18 +76,13 @@ properties:
> > >                - const: 4
> > > 
> > >            link-frequencies:
> > > -            $ref: /schemas/types.yaml#/definitions/uint64-array
> > > -            description:
> > > -              Allowed data bus frequencies. 360000000, 180000000 Hz or both
> > > -              are supported by the driver.
> > > -
> > > +            maxItems: 2
> > > +            items:
> > > +              enum: [ 360000000, 180000000 ]
> > 
> > This is a limitation of the driver, not the device. Should we keep this
> > information in a comment, to eventually get it fixed and drop the
> > limitation from the bindings ?
> 
> If your dts has anything else, then it won't work. Warning on that seems 
> valuable to me, so I think we should keep it. If someone with a better 
> driver complains, we can drop it.
> 
> I can keep the description with 'Frequencies listed are driver, not h/w 
> limitations'.

I'm fine with the constraint itself, my comment was referring to the
fact that we're dropping the description. Your description proposal
works for me, thanks.

-- 
Regards,

Laurent Pinchart

Powered by blists - more mailing lists