diff mbox series

[05/22] dt-bindings: media: sun6i-a31-csi: Add MIPI CSI-2 input port

Message ID 20210910184147.336618-6-paul.kocialkowski@bootlin.com
State Not Applicable
Headers show
Series Allwinner A31/A83T MIPI CSI-2 Support and A31 ISP Support | expand

Commit Message

Paul Kocialkowski Sept. 10, 2021, 6:41 p.m. UTC
The A31 CSI controller supports two distinct input interfaces:
parallel and an external MIPI CSI-2 bridge. The parallel interface
is often connected to a set of hardware pins while the MIPI CSI-2
bridge is an internal FIFO-ish link. As a result, these two inputs
are distinguished as two different ports.

Note that only one of the two may be present on a controller instance.
For example, the V3s has one controller dedicated to MIPI-CSI2 and one
dedicated to parallel.

Update the binding with an explicit ports node that holds two distinct
port nodes: one for parallel input and one for MIPI CSI-2.

This is backward-compatible with the single-port approach that was
previously taken for representing the parallel interface port, which
stays enumerated as fwnode port 0.

Note that additional ports may be added in the future, especially to
support feeding the CSI controller's output to the ISP.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Reviewed-by: Rob Herring <robh@kernel.org>
Acked-by: Maxime Ripard <mripard@kernel.org>
---
 .../media/allwinner,sun6i-a31-csi.yaml        | 75 +++++++++++++++----
 1 file changed, 62 insertions(+), 13 deletions(-)

Comments

Maxime Ripard Sept. 13, 2021, 8:09 a.m. UTC | #1
On Fri, Sep 10, 2021 at 08:41:30PM +0200, Paul Kocialkowski wrote:
> The A31 CSI controller supports two distinct input interfaces:
> parallel and an external MIPI CSI-2 bridge. The parallel interface
> is often connected to a set of hardware pins while the MIPI CSI-2
> bridge is an internal FIFO-ish link. As a result, these two inputs
> are distinguished as two different ports.
> 
> Note that only one of the two may be present on a controller instance.
> For example, the V3s has one controller dedicated to MIPI-CSI2 and one
> dedicated to parallel.
> 
> Update the binding with an explicit ports node that holds two distinct
> port nodes: one for parallel input and one for MIPI CSI-2.
> 
> This is backward-compatible with the single-port approach that was
> previously taken for representing the parallel interface port, which
> stays enumerated as fwnode port 0.
> 
> Note that additional ports may be added in the future, especially to
> support feeding the CSI controller's output to the ISP.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Acked-by: Maxime Ripard <mripard@kernel.org>
> ---
>  .../media/allwinner,sun6i-a31-csi.yaml        | 75 +++++++++++++++----
>  1 file changed, 62 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-csi.yaml b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-csi.yaml
> index 8b568072a069..f4a686b77a38 100644
> --- a/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-csi.yaml
> +++ b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-csi.yaml
> @@ -61,6 +61,49 @@ properties:
>  
>      additionalProperties: false
>  
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        description: Parallel input port, connect to a parallel sensor
> +
> +        properties:
> +          reg:
> +            const: 0
> +
> +          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

You don't have to duplicate the entire definition there, you can just
reference port:

$ref: #/properties/port

> +      port@1:
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        description: MIPI CSI-2 bridge input port
> +
> +        properties:
> +          reg:
> +            const: 1
> +
> +          endpoint:
> +            $ref: video-interfaces.yaml#
> +            unevaluatedProperties: false
> +
> +        additionalProperties: false
> +

port@0 is required?

And at the top-level, either ports or port are required too

>  required:
>    - compatible
>    - reg
> @@ -89,19 +132,25 @@ examples:
>                        "ram";
>          resets = <&ccu RST_BUS_CSI>;
>  
> -        port {
> -            /* Parallel bus endpoint */
> -            csi1_ep: endpoint {
> -                remote-endpoint = <&adv7611_ep>;
> -                bus-width = <16>;
> -
> -                /*
> -                 * If hsync-active/vsync-active are missing,
> -                 * embedded BT.656 sync is used.
> -                 */
> -                 hsync-active = <0>; /* Active low */
> -                 vsync-active = <0>; /* Active low */
> -                 pclk-sample = <1>;  /* Rising */
> +        ports {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            port@0 {
> +                reg = <0>;
> +                /* Parallel bus endpoint */
> +                csi1_ep: endpoint {
> +                    remote-endpoint = <&adv7611_ep>;
> +                    bus-width = <16>;
> +
> +                    /*
> +                     * If hsync-active/vsync-active are missing,
> +                     * embedded BT.656 sync is used.
> +                     */
> +                     hsync-active = <0>; /* Active low */
> +                     vsync-active = <0>; /* Active low */
> +                     pclk-sample = <1>;  /* Rising */
> +                };
>              };
>          };
>      };

I'd keep the original example and add one with the CSI bridge

Maxime
Paul Kocialkowski Sept. 14, 2021, 7:43 a.m. UTC | #2
Hi,

On Mon 13 Sep 21, 10:09, Maxime Ripard wrote:
> On Fri, Sep 10, 2021 at 08:41:30PM +0200, Paul Kocialkowski wrote:
> > The A31 CSI controller supports two distinct input interfaces:
> > parallel and an external MIPI CSI-2 bridge. The parallel interface
> > is often connected to a set of hardware pins while the MIPI CSI-2
> > bridge is an internal FIFO-ish link. As a result, these two inputs
> > are distinguished as two different ports.
> > 
> > Note that only one of the two may be present on a controller instance.
> > For example, the V3s has one controller dedicated to MIPI-CSI2 and one
> > dedicated to parallel.
> > 
> > Update the binding with an explicit ports node that holds two distinct
> > port nodes: one for parallel input and one for MIPI CSI-2.
> > 
> > This is backward-compatible with the single-port approach that was
> > previously taken for representing the parallel interface port, which
> > stays enumerated as fwnode port 0.
> > 
> > Note that additional ports may be added in the future, especially to
> > support feeding the CSI controller's output to the ISP.
> > 
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > Acked-by: Maxime Ripard <mripard@kernel.org>
> > ---
> >  .../media/allwinner,sun6i-a31-csi.yaml        | 75 +++++++++++++++----
> >  1 file changed, 62 insertions(+), 13 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-csi.yaml b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-csi.yaml
> > index 8b568072a069..f4a686b77a38 100644
> > --- a/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-csi.yaml
> > +++ b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-csi.yaml
> > @@ -61,6 +61,49 @@ properties:
> >  
> >      additionalProperties: false
> >  
> > +  ports:
> > +    $ref: /schemas/graph.yaml#/properties/ports
> > +
> > +    properties:
> > +      port@0:
> > +        $ref: /schemas/graph.yaml#/$defs/port-base
> > +        description: Parallel input port, connect to a parallel sensor
> > +
> > +        properties:
> > +          reg:
> > +            const: 0
> > +
> > +          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
> 
> You don't have to duplicate the entire definition there, you can just
> reference port:
> 
> $ref: #/properties/port

And that would reference the local (previous) definition of the port node?
Sounds like a good thing indeed.

> > +      port@1:
> > +        $ref: /schemas/graph.yaml#/$defs/port-base
> > +        description: MIPI CSI-2 bridge input port
> > +
> > +        properties:
> > +          reg:
> > +            const: 1
> > +
> > +          endpoint:
> > +            $ref: video-interfaces.yaml#
> > +            unevaluatedProperties: false
> > +
> > +        additionalProperties: false
> > +
> 
> port@0 is required?

It shouldn't be required. Does that call for a change here?

> And at the top-level, either ports or port are required too

Yes I guess that is true. Should that be a required+oneOf type of thing?

> >  required:
> >    - compatible
> >    - reg
> > @@ -89,19 +132,25 @@ examples:
> >                        "ram";
> >          resets = <&ccu RST_BUS_CSI>;
> >  
> > -        port {
> > -            /* Parallel bus endpoint */
> > -            csi1_ep: endpoint {
> > -                remote-endpoint = <&adv7611_ep>;
> > -                bus-width = <16>;
> > -
> > -                /*
> > -                 * If hsync-active/vsync-active are missing,
> > -                 * embedded BT.656 sync is used.
> > -                 */
> > -                 hsync-active = <0>; /* Active low */
> > -                 vsync-active = <0>; /* Active low */
> > -                 pclk-sample = <1>;  /* Rising */
> > +        ports {
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +
> > +            port@0 {
> > +                reg = <0>;
> > +                /* Parallel bus endpoint */
> > +                csi1_ep: endpoint {
> > +                    remote-endpoint = <&adv7611_ep>;
> > +                    bus-width = <16>;
> > +
> > +                    /*
> > +                     * If hsync-active/vsync-active are missing,
> > +                     * embedded BT.656 sync is used.
> > +                     */
> > +                     hsync-active = <0>; /* Active low */
> > +                     vsync-active = <0>; /* Active low */
> > +                     pclk-sample = <1>;  /* Rising */
> > +                };
> >              };
> >          };
> >      };
> 
> I'd keep the original example and add one with the CSI bridge

Understood, will do.

Thanks,

Paul
Maxime Ripard Sept. 14, 2021, 12:06 p.m. UTC | #3
On Tue, Sep 14, 2021 at 09:43:08AM +0200, Paul Kocialkowski wrote:
> Hi,
> 
> On Mon 13 Sep 21, 10:09, Maxime Ripard wrote:
> > On Fri, Sep 10, 2021 at 08:41:30PM +0200, Paul Kocialkowski wrote:
> > > The A31 CSI controller supports two distinct input interfaces:
> > > parallel and an external MIPI CSI-2 bridge. The parallel interface
> > > is often connected to a set of hardware pins while the MIPI CSI-2
> > > bridge is an internal FIFO-ish link. As a result, these two inputs
> > > are distinguished as two different ports.
> > > 
> > > Note that only one of the two may be present on a controller instance.
> > > For example, the V3s has one controller dedicated to MIPI-CSI2 and one
> > > dedicated to parallel.
> > > 
> > > Update the binding with an explicit ports node that holds two distinct
> > > port nodes: one for parallel input and one for MIPI CSI-2.
> > > 
> > > This is backward-compatible with the single-port approach that was
> > > previously taken for representing the parallel interface port, which
> > > stays enumerated as fwnode port 0.
> > > 
> > > Note that additional ports may be added in the future, especially to
> > > support feeding the CSI controller's output to the ISP.
> > > 
> > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > Reviewed-by: Rob Herring <robh@kernel.org>
> > > Acked-by: Maxime Ripard <mripard@kernel.org>
> > > ---
> > >  .../media/allwinner,sun6i-a31-csi.yaml        | 75 +++++++++++++++----
> > >  1 file changed, 62 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-csi.yaml b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-csi.yaml
> > > index 8b568072a069..f4a686b77a38 100644
> > > --- a/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-csi.yaml
> > > +++ b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-csi.yaml
> > > @@ -61,6 +61,49 @@ properties:
> > >  
> > >      additionalProperties: false
> > >  
> > > +  ports:
> > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > +
> > > +    properties:
> > > +      port@0:
> > > +        $ref: /schemas/graph.yaml#/$defs/port-base
> > > +        description: Parallel input port, connect to a parallel sensor
> > > +
> > > +        properties:
> > > +          reg:
> > > +            const: 0
> > > +
> > > +          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
> > 
> > You don't have to duplicate the entire definition there, you can just
> > reference port:
> > 
> > $ref: #/properties/port
> 
> And that would reference the local (previous) definition of the port node?

Yep. You can't use additionalProperties anymore though, but only
unevaluatedProperties since additionalProperties is about the current
schema (ie, every below port@0 here), while unevaluatedProperties is
about the sum of all the schemas.

> Sounds like a good thing indeed.

> > > +      port@1:
> > > +        $ref: /schemas/graph.yaml#/$defs/port-base
> > > +        description: MIPI CSI-2 bridge input port
> > > +
> > > +        properties:
> > > +          reg:
> > > +            const: 1
> > > +
> > > +          endpoint:
> > > +            $ref: video-interfaces.yaml#
> > > +            unevaluatedProperties: false
> > > +
> > > +        additionalProperties: false
> > > +
> > 
> > port@0 is required?
> 
> It shouldn't be required. Does that call for a change here?

I guess you'd at least need one of the two?

So something like this should work

anyOf:
  - required:
    - port@0

  - required:
    - port@1

> > And at the top-level, either ports or port are required too
> 
> Yes I guess that is true. Should that be a required+oneOf type of thing?

Yes, anyOf is an OR, oneOf a XOR. I don't think it makes sense to have
ports and port there.

Maxime
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-csi.yaml b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-csi.yaml
index 8b568072a069..f4a686b77a38 100644
--- a/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-csi.yaml
+++ b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-csi.yaml
@@ -61,6 +61,49 @@  properties:
 
     additionalProperties: false
 
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        description: Parallel input port, connect to a parallel sensor
+
+        properties:
+          reg:
+            const: 0
+
+          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
+
+      port@1:
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        description: MIPI CSI-2 bridge input port
+
+        properties:
+          reg:
+            const: 1
+
+          endpoint:
+            $ref: video-interfaces.yaml#
+            unevaluatedProperties: false
+
+        additionalProperties: false
+
 required:
   - compatible
   - reg
@@ -89,19 +132,25 @@  examples:
                       "ram";
         resets = <&ccu RST_BUS_CSI>;
 
-        port {
-            /* Parallel bus endpoint */
-            csi1_ep: endpoint {
-                remote-endpoint = <&adv7611_ep>;
-                bus-width = <16>;
-
-                /*
-                 * If hsync-active/vsync-active are missing,
-                 * embedded BT.656 sync is used.
-                 */
-                 hsync-active = <0>; /* Active low */
-                 vsync-active = <0>; /* Active low */
-                 pclk-sample = <1>;  /* Rising */
+        ports {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            port@0 {
+                reg = <0>;
+                /* Parallel bus endpoint */
+                csi1_ep: endpoint {
+                    remote-endpoint = <&adv7611_ep>;
+                    bus-width = <16>;
+
+                    /*
+                     * If hsync-active/vsync-active are missing,
+                     * embedded BT.656 sync is used.
+                     */
+                     hsync-active = <0>; /* Active low */
+                     vsync-active = <0>; /* Active low */
+                     pclk-sample = <1>;  /* Rising */
+                };
             };
         };
     };