Re: [PATCH v3 1/2] media: dt-bindings: Convert video-interfaces.txt properties to schemas

[Rob Herring <robh@kernel.org>]

On Wed, Dec 16, 2020 at 04:52:20PM +0200, Laurent Pinchart wrote:
> Hi Rob,
> 
> Thank you for the patch.
> 
> On Thu, Dec 10, 2020 at 03:16:24PM -0600, Rob Herring wrote:
> > Convert video-interfaces.txt to DT schema. As it contains a mixture of
> > device level and endpoint properties, split it up into 2 schemas.
> > 
> > Binding schemas will need to reference both the graph.yaml and
> > video-interfaces.yaml schemas. The exact schema depends on how many
> > ports and endpoints for the binding. A single port with a single
> > endpoint looks similar to this:
> > 
> >   port:
> >     $ref: /schemas/graph.yaml#/$defs/port-base
> > 
> >     properties:
> >       endpoint:
> >         $ref: video-interfaces.yaml#
> >         unevaluatedProperties: false
> > 
> >         properties:
> >           bus-width:
> >             enum: [ 8, 10, 12, 16 ]
> > 
> >           pclk-sample: true
> >           hsync-active: true
> >           vsync-active: true
> > 
> >         required:
> >           - bus-width
> > 
> >     additionalProperties: false
> > 
> > Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Acked-by: Jacopo Mondi <jacopo@jmondi.org>
> > Signed-off-by: Rob Herring <robh@kernel.org>
> > ---
> > I need acks for dual licensing from the listed maintainers.
> > 
> > v3:
> > - Support up to 9 physical lanes
> > - Set lane-polarities array bounds
> > ---
> >  .../media/video-interface-devices.yaml        | 406 +++++++++++
> >  .../bindings/media/video-interfaces.txt       | 640 +-----------------
> >  .../bindings/media/video-interfaces.yaml      | 346 ++++++++++
> >  3 files changed, 753 insertions(+), 639 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/media/video-interface-devices.yaml
> >  create mode 100644 Documentation/devicetree/bindings/media/video-interfaces.yaml


> > diff --git a/Documentation/devicetree/bindings/media/video-interfaces.yaml b/Documentation/devicetree/bindings/media/video-interfaces.yaml
> > new file mode 100644
> > index 000000000000..fefca7d98718
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/video-interfaces.yaml
> > @@ -0,0 +1,346 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/video-interfaces.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Common bindings for video receiver and transmitter interface endpoints
> > +
> > +maintainers:
> > +  - Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > +  - Sakari Ailus <sakari.ailus@linux.intel.com>
> > +
> > +description: |
> > +  Video data pipelines usually consist of external devices, e.g. camera sensors,
> > +  controlled over an I2C, SPI or UART bus, and SoC internal IP blocks, including
> > +  video DMA engines and video data processors.
> > +
> > +  SoC internal blocks are described by DT nodes, placed similarly to other SoC
> > +  blocks.  External devices are represented as child nodes of their respective
> > +  bus controller nodes, e.g. I2C.
> > +
> > +  Data interfaces on all video devices are described by their child 'port' nodes.
> > +  Configuration of a port depends on other devices participating in the data
> > +  transfer and is described by 'endpoint' subnodes.
> > +
> > +  device {
> > +      ...
> > +      ports {
> > +          #address-cells = <1>;
> > +          #size-cells = <0>;
> > +
> > +          port@0 {
> > +              ...
> > +              endpoint@0 { ... };
> > +              endpoint@1 { ... };
> > +          };
> > +          port@1 { ... };
> > +      };
> > +  };
> > +
> > +  If a port can be configured to work with more than one remote device on the same
> > +  bus, an 'endpoint' child node must be provided for each of them.  If more than
> > +  one port is present in a device node or there is more than one endpoint at a
> > +  port, or port node needs to be associated with a selected hardware interface,
> > +  a common scheme using '#address-cells', '#size-cells' and 'reg' properties is
> > +  used.
> > +
> > +  All 'port' nodes can be grouped under optional 'ports' node, which allows to
> > +  specify #address-cells, #size-cells properties independently for the 'port'
> > +  and 'endpoint' nodes and any child device nodes a device might have.
> > +
> > +  Two 'endpoint' nodes are linked with each other through their 'remote-endpoint'
> > +  phandles.  An endpoint subnode of a device contains all properties needed for
> > +  configuration of this device for data exchange with other device.  In most
> > +  cases properties at the peer 'endpoint' nodes will be identical, however they
> > +  might need to be different when there is any signal modifications on the bus
> > +  between two devices, e.g. there are logic signal inverters on the lines.
> > +
> > +  It is allowed for multiple endpoints at a port to be active simultaneously,
> > +  where supported by a device.  For example, in case where a data interface of
> > +  a device is partitioned into multiple data busses, e.g. 16-bit input port
> > +  divided into two separate ITU-R BT.656 8-bit busses.  In such case bus-width
> > +  and data-shift properties can be used to assign physical data lines to each
> > +  endpoint node (logical bus).
> > +
> > +  Documenting bindings for devices
> > +  --------------------------------
> > +
> > +  All required and optional bindings the device supports shall be explicitly
> > +  documented in device DT binding documentation. This also includes port and
> > +  endpoint nodes for the device, including unit-addresses and reg properties
> > +  where relevant.
> > +
> > +  Please also see Documentation/devicetree/bindings/graph.txt .
> 
> Should this be dropped, or modified to reference the YAML schema for OF
> graph ?

Yeah. A lot of the above I feel is just duplicate of how graphs work, 
but I left it as-is.


> > +  clock-lanes:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    # Assume up to 9 physical lane indices
> > +    maximum: 8
> > +    description:
> > +      Physical clock lane index. Position of an entry determines
> 
> s/index/indexes/ (or indices) as there are potentially multiple entries
> (even if in practice, for all bus types we currently support, only one
> clock lane is supported) ?

The original text did say 'array', but there aren't any cases, I can't 
really see why or how you'd have more than 1, and it seemed silly to 
make it an array type with 'maxItems: 1'. Wouldn't a 2 clock case be 
dual interface like we do for DSI?

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks.

Rob