[v2,07/66] dt-bindings: media: sun6i-a31-csi: Add MIPI CSI-2 input port

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        | 60 +++++++++++++++----
 1 file changed, 47 insertions(+), 13 deletions(-)

Hi Paul,

Thank you for the patch.

On Sat, Feb 05, 2022 at 07:53:30PM +0100, 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.

Is it that only one of the two is present, or only one of the two is
connected ? In the latter case I'd make both ports required, but with
only one of them connected.

> 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        | 60 +++++++++++++++----
>  1 file changed, 47 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..3cc61866ea89 100644
> --- a/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-csi.yaml
> +++ b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-csi.yaml
> @@ -61,6 +61,34 @@ properties:
>  
>      additionalProperties: false
>  
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +
> +    properties:
> +      port@0:
> +        $ref: "#/properties/port"
> +        unevaluatedProperties: 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
> +
> +    anyOf:
> +      - required:
> +        - port@0
> +      - required:
> +        - port@1
> +
>  required:
>    - compatible
>    - reg

Shouldn't you specify that either port or ports is required, but not
both ? I'd also add a comment in the port node to tell it's deprecated,
and that ports should be used instead.

> @@ -89,19 +117,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 */

Wrong indentation.

> +                };
>              };
>          };
>      };

Hi Laurent,

Thanks for the review!

On Mon 07 Feb 22, 18:03, Laurent Pinchart wrote:
> Hi Paul,
> 
> Thank you for the patch.
> 
> On Sat, Feb 05, 2022 at 07:53:30PM +0100, 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.
> 
> Is it that only one of the two is present, or only one of the two is
> connected ? In the latter case I'd make both ports required, but with
> only one of them connected.

There are situations where the actual pins for parallel (port@0) are missing
and the controller is dedicated to its mipi csi-2 bridge (port@1), cases where
the two are present and cases where the mipi csi-2 bridge doesn't exist.
So all in all it's really legit that only one port may be defined.

> > 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        | 60 +++++++++++++++----
> >  1 file changed, 47 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..3cc61866ea89 100644
> > --- a/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-csi.yaml
> > +++ b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-csi.yaml
> > @@ -61,6 +61,34 @@ properties:
> >  
> >      additionalProperties: false
> >  
> > +  ports:
> > +    $ref: /schemas/graph.yaml#/properties/ports
> > +
> > +    properties:
> > +      port@0:
> > +        $ref: "#/properties/port"
> > +        unevaluatedProperties: 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
> > +
> > +    anyOf:
> > +      - required:
> > +        - port@0
> > +      - required:
> > +        - port@1
> > +
> >  required:
> >    - compatible
> >    - reg
> 
> Shouldn't you specify that either port or ports is required, but not
> both ? I'd also add a comment in the port node to tell it's deprecated,
> and that ports should be used instead.

Yes I agree on both points. I guess that should be a:

oneOf:
  - required:
    - ports
  - required:
    - port

(but feel free to correct me).

> > @@ -89,19 +117,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 */
> 
> Wrong indentation.

The double-space before /* Rising */ or something with the heading indent?

Thanks,

Paul

> > +                };
> >              };
> >          };
> >      };
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Hi,

On Fri 11 Feb 22, 22:56, Laurent Pinchart wrote:
> On Fri, Feb 11, 2022 at 05:10:06PM +0100, Paul Kocialkowski wrote:
> > Hi Laurent,
> > 
> > Thanks for the review!
> > 
> > On Mon 07 Feb 22, 18:03, Laurent Pinchart wrote:
> > > Hi Paul,
> > > 
> > > Thank you for the patch.
> > > 
> > > On Sat, Feb 05, 2022 at 07:53:30PM +0100, 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.
> > > 
> > > Is it that only one of the two is present, or only one of the two is
> > > connected ? In the latter case I'd make both ports required, but with
> > > only one of them connected.
> > 
> > There are situations where the actual pins for parallel (port@0) are missing
> > and the controller is dedicated to its mipi csi-2 bridge (port@1), cases where
> > the two are present and cases where the mipi csi-2 bridge doesn't exist.
> > So all in all it's really legit that only one port may be defined.
> 
> The port could still exist internally in the IP core though. Of course
> that's hard to tell.

Yes that's true, the bit to switch input to mipi csi-2 is there in all cases
but I don't think it makes sense to expose the port in situations where
no controller is attached.

> > > > 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        | 60 +++++++++++++++----
> > > >  1 file changed, 47 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..3cc61866ea89 100644
> > > > --- a/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-csi.yaml
> > > > +++ b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-csi.yaml
> > > > @@ -61,6 +61,34 @@ properties:
> > > >  
> > > >      additionalProperties: false
> > > >  
> > > > +  ports:
> > > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > > +
> > > > +    properties:
> > > > +      port@0:
> > > > +        $ref: "#/properties/port"
> > > > +        unevaluatedProperties: 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
> > > > +
> > > > +    anyOf:
> > > > +      - required:
> > > > +        - port@0
> > > > +      - required:
> > > > +        - port@1
> > > > +
> > > >  required:
> > > >    - compatible
> > > >    - reg
> > > 
> > > Shouldn't you specify that either port or ports is required, but not
> > > both ? I'd also add a comment in the port node to tell it's deprecated,
> > > and that ports should be used instead.
> > 
> > Yes I agree on both points. I guess that should be a:
> > 
> > oneOf:
> >   - required:
> >     - ports
> >   - required:
> >     - port
> > 
> > (but feel free to correct me).
> > 
> > > > @@ -89,19 +117,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 */
> > > 
> > > Wrong indentation.
> > 
> > The double-space before /* Rising */ or something with the heading indent?
> 
> The heading has one extra space for all three lines, they should be
> aligned to the / of /*, not to the *.

Oh that's true, good catch thanks!

Paul

> > > > +                };
> > > >              };
> > > >          };
> > > >      };
> 
> -- 
> Regards,
> 
> Laurent Pinchart