Re: [PATCH v3 2/2] dt-bindings: media: Use graph and video-interfaces schemas

From: Sakari Ailus
Date: Wed Dec 16 2020 - 17:01:31 EST


Hi Laurent, Rob,

On Wed, Dec 16, 2020 at 07:43:45PM +0200, Laurent Pinchart wrote:
> 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@xxxxxxxxxx>
> > > > Cc: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
> > > > Cc: Jacopo Mondi <jacopo@xxxxxxxxxx>
> > > > Cc: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> > > > Cc: linux-media@xxxxxxxxxxxxxxx
> > > > Signed-off-by: Rob Herring <robh@xxxxxxxxxx>
> > > > ---
> >
> >
> > > > 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.

I agree.

There properties that are used for linking other devices such as
flash-leds. It could be related to any camera sensor, independently of its
properties. If we went that way, that should be also added to a lot of
places.

...

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

Sounds good.

--
Sakari Ailus