[PATCH v2 6/7] dt-bindings: media: Add i.MX8 ISI DT bindings

[PATCH v2 6/7] dt-bindings: media: Add i.MX8 ISI DT bindings

[Laurent Pinchart]

The Image Sensing Interface (ISI) combines image processing pipelines
with DMA engines to process and capture frames originating from a
variety of sources. The inputs to the ISI go through Pixel Link
interfaces, and their number and nature is SoC-dependent. They cover
both capture interfaces (MIPI CSI-2 RX, HDMI RX) and memory inputs.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:

- Fix compatible string checks in conditional schema
- Fix interrupts property handling
---
 .../bindings/media/nxp,imx8-isi.yaml          | 148 ++++++++++++++++++
 1 file changed, 148 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/nxp,imx8-isi.yaml

diff --git a/Documentation/devicetree/bindings/media/nxp,imx8-isi.yaml b/Documentation/devicetree/bindings/media/nxp,imx8-isi.yaml
new file mode 100644
index 000000000000..390dfa03026b
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/nxp,imx8-isi.yaml
@@ -0,0 +1,148 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/nxp,imx8-isi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: i.MX8 Image Sensing Interface
+
+maintainers:
+  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
+
+description: |
+  The Image Sensing Interface (ISI) combines image processing pipelines with
+  DMA engines to process and capture frames originating from a variety of
+  sources. The inputs to the ISI go through Pixel Link interfaces, and their
+  number and nature is SoC-dependent. They cover both capture interfaces (MIPI
+  CSI-2 RX, HDMI RX, ...) and display engine outputs for writeback support.
+
+properties:
+  compatible:
+    enum:
+      - fsl,imx8mn-isi
+      - fsl,imx8mp-isi
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: The AXI clock
+      - description: The APB clock
+      # TODO: Check if the per-channel ipg_proc_clk clocks need to be specified
+      # as well, in case some SoCs have the ability to control them separately.
+      # This may be the case of the i.MX8[DQ]X(P)
+
+  clock-names:
+    items:
+      - const: axi
+      - const: apb
+
+  fsl,blk-ctrl:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      A phandle referencing the block control that contains the CSIS to ISI
+      gasket.
+
+  interrupts: true
+
+  power-domains: true
+
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+    description: |
+      Ports represent the Pixel Link inputs to the ISI. Their number and
+      assignment are model-dependent. Each port shall have a single endpoint.
+
+    patternProperties:
+      "^port@[0-9]$":
+        $ref: /schemas/graph.yaml#/properties/port
+        unevaluatedProperties: false
+
+    unevaluatedProperties: false
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - fsl,blk-ctrl
+  - ports
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: fsl,imx8mn-isi
+    then:
+      properties:
+        interrupts:
+          maxItems: 1
+        ports:
+          properties:
+            port@0:
+              description: MIPI CSI-2 RX
+          required:
+            - port@0
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: fsl,imx8mp-isi
+    then:
+      properties:
+        interrupts:
+          maxItems: 2
+        ports:
+          properties:
+            port@0:
+              description: MIPI CSI-2 RX 0
+            port@1:
+              description: MIPI CSI-2 RX 1
+          required:
+            - port@0
+            - port@1
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/imx8mp-clock.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    isi@32e00000 {
+        compatible = "fsl,imx8mp-isi";
+        reg = <0x32e00000 0x4000>;
+        interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>;
+        clocks = <&clk IMX8MP_CLK_MEDIA_AXI_ROOT>,
+                 <&clk IMX8MP_CLK_MEDIA_APB_ROOT>;
+        clock-names = "axi", "apb";
+        fsl,blk-ctrl = <&media_blk_ctrl>;
+        power-domains = <&mediamix_pd>;
+
+        ports {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            port@0 {
+                reg = <0>;
+                isi_in_0: endpoint {
+                    remote-endpoint = <&mipi_csi_0_out>;
+                };
+            };
+
+            port@1 {
+                reg = <1>;
+                isi_in_1: endpoint {
+                    remote-endpoint = <&mipi_csi_1_out>;
+                };
+            };
+        };
+    };
+
+...
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 6/7] dt-bindings: media: Add i.MX8 ISI DT bindings

[Krzysztof Kozlowski]

On 12/07/2022 02:02, Laurent Pinchart wrote:
> The Image Sensing Interface (ISI) combines image processing pipelines
> with DMA engines to process and capture frames originating from a
> variety of sources. The inputs to the ISI go through Pixel Link
> interfaces, and their number and nature is SoC-dependent. They cover
> both capture interfaces (MIPI CSI-2 RX, HDMI RX) and memory inputs.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v1:
> 
> - Fix compatible string checks in conditional schema
> - Fix interrupts property handling
> ---
>  .../bindings/media/nxp,imx8-isi.yaml          | 148 ++++++++++++++++++
>  1 file changed, 148 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/nxp,imx8-isi.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/nxp,imx8-isi.yaml b/Documentation/devicetree/bindings/media/nxp,imx8-isi.yaml
> new file mode 100644
> index 000000000000..390dfa03026b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/nxp,imx8-isi.yaml
> @@ -0,0 +1,148 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/nxp,imx8-isi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: i.MX8 Image Sensing Interface
> +
> +maintainers:
> +  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> +
> +description: |
> +  The Image Sensing Interface (ISI) combines image processing pipelines with
> +  DMA engines to process and capture frames originating from a variety of
> +  sources. The inputs to the ISI go through Pixel Link interfaces, and their
> +  number and nature is SoC-dependent. They cover both capture interfaces (MIPI
> +  CSI-2 RX, HDMI RX, ...) and display engine outputs for writeback support.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - fsl,imx8mn-isi
> +      - fsl,imx8mp-isi
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: The AXI clock
> +      - description: The APB clock
> +      # TODO: Check if the per-channel ipg_proc_clk clocks need to be specified
> +      # as well, in case some SoCs have the ability to control them separately.
> +      # This may be the case of the i.MX8[DQ]X(P)
> +
> +  clock-names:
> +    items:
> +      - const: axi
> +      - const: apb
> +
> +  fsl,blk-ctrl:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      A phandle referencing the block control that contains the CSIS to ISI
> +      gasket.
> +
> +  interrupts: true

Need generic constraints - min/maxItems.

> +
> +  power-domains: true

Ditto.

> +
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +    description: |
> +      Ports represent the Pixel Link inputs to the ISI. Their number and
> +      assignment are model-dependent. Each port shall have a single endpoint.
> +
> +    patternProperties:
> +      "^port@[0-9]$":
> +        $ref: /schemas/graph.yaml#/properties/port
> +        unevaluatedProperties: false
> +
> +    unevaluatedProperties: false

At least one port is always required?


> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - clock-names
> +  - fsl,blk-ctrl
> +  - ports
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: fsl,imx8mn-isi
> +    then:
> +      properties:
> +        interrupts:
> +          maxItems: 1
> +        ports:
> +          properties:
> +            port@0:
> +              description: MIPI CSI-2 RX
> +          required:
> +            - port@0
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: fsl,imx8mp-isi
> +    then:
> +      properties:
> +        interrupts:
> +          maxItems: 2

You need to describe the items.


Best regards,
Krzysztof

Re: [PATCH v2 6/7] dt-bindings: media: Add i.MX8 ISI DT bindings

[Laurent Pinchart]

Hi Krzysztof,

On Tue, Jul 12, 2022 at 09:49:45AM +0200, Krzysztof Kozlowski wrote:
> On 12/07/2022 02:02, Laurent Pinchart wrote:
> > The Image Sensing Interface (ISI) combines image processing pipelines
> > with DMA engines to process and capture frames originating from a
> > variety of sources. The inputs to the ISI go through Pixel Link
> > interfaces, and their number and nature is SoC-dependent. They cover
> > both capture interfaces (MIPI CSI-2 RX, HDMI RX) and memory inputs.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v1:
> > 
> > - Fix compatible string checks in conditional schema
> > - Fix interrupts property handling
> > ---
> >  .../bindings/media/nxp,imx8-isi.yaml          | 148 ++++++++++++++++++
> >  1 file changed, 148 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/nxp,imx8-isi.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/media/nxp,imx8-isi.yaml b/Documentation/devicetree/bindings/media/nxp,imx8-isi.yaml
> > new file mode 100644
> > index 000000000000..390dfa03026b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/nxp,imx8-isi.yaml
> > @@ -0,0 +1,148 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/nxp,imx8-isi.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: i.MX8 Image Sensing Interface
> > +
> > +maintainers:
> > +  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > +
> > +description: |
> > +  The Image Sensing Interface (ISI) combines image processing pipelines with
> > +  DMA engines to process and capture frames originating from a variety of
> > +  sources. The inputs to the ISI go through Pixel Link interfaces, and their
> > +  number and nature is SoC-dependent. They cover both capture interfaces (MIPI
> > +  CSI-2 RX, HDMI RX, ...) and display engine outputs for writeback support.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - fsl,imx8mn-isi
> > +      - fsl,imx8mp-isi
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    items:
> > +      - description: The AXI clock
> > +      - description: The APB clock
> > +      # TODO: Check if the per-channel ipg_proc_clk clocks need to be specified
> > +      # as well, in case some SoCs have the ability to control them separately.
> > +      # This may be the case of the i.MX8[DQ]X(P)
> > +
> > +  clock-names:
> > +    items:
> > +      - const: axi
> > +      - const: apb
> > +
> > +  fsl,blk-ctrl:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description:
> > +      A phandle referencing the block control that contains the CSIS to ISI
> > +      gasket.
> > +
> > +  interrupts: true
> 
> Need generic constraints - min/maxItems.

I can't set maxItems here, as the value depends on the compatible
string. It's set below as part of the "allOf". I could set minItems to
1, but I don't really see a point in doing so.

> > +
> > +  power-domains: true
> 
> Ditto.

I'll fix this one.

> > +
> > +  ports:
> > +    $ref: /schemas/graph.yaml#/properties/ports
> > +    description: |
> > +      Ports represent the Pixel Link inputs to the ISI. Their number and
> > +      assignment are model-dependent. Each port shall have a single endpoint.
> > +
> > +    patternProperties:
> > +      "^port@[0-9]$":
> > +        $ref: /schemas/graph.yaml#/properties/port
> > +        unevaluatedProperties: false
> > +
> > +    unevaluatedProperties: false
> 
> At least one port is always required?

That's a fair assumption I think. How would you express that ? There's
no patternRequired as far as I know. Note that the device-dependent
ports are described in the "allOf" section below, where "required" is
set per device model.

> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - clocks
> > +  - clock-names
> > +  - fsl,blk-ctrl
> > +  - ports
> > +
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: fsl,imx8mn-isi
> > +    then:
> > +      properties:
> > +        interrupts:
> > +          maxItems: 1
> > +        ports:
> > +          properties:
> > +            port@0:
> > +              description: MIPI CSI-2 RX
> > +          required:
> > +            - port@0
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: fsl,imx8mp-isi
> > +    then:
> > +      properties:
> > +        interrupts:
> > +          maxItems: 2
> 
> You need to describe the items.

It's one interrupt per pipeline. Can I add the description to the
generic interrupts property instead of documenting each item
individually ? Something along the lines of

  interrupts:
    description: Processing pipeline interrupts, one per pipeline

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 6/7] dt-bindings: media: Add i.MX8 ISI DT bindings

[Krzysztof Kozlowski]

On 12/07/2022 12:25, Laurent Pinchart wrote:
> Hi Krzysztof,
> 
> On Tue, Jul 12, 2022 at 09:49:45AM +0200, Krzysztof Kozlowski wrote:
>> On 12/07/2022 02:02, Laurent Pinchart wrote:
>>> The Image Sensing Interface (ISI) combines image processing pipelines
>>> with DMA engines to process and capture frames originating from a
>>> variety of sources. The inputs to the ISI go through Pixel Link
>>> interfaces, and their number and nature is SoC-dependent. They cover
>>> both capture interfaces (MIPI CSI-2 RX, HDMI RX) and memory inputs.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> ---
>>> Changes since v1:
>>>
>>> - Fix compatible string checks in conditional schema
>>> - Fix interrupts property handling
>>> ---
>>>  .../bindings/media/nxp,imx8-isi.yaml          | 148 ++++++++++++++++++
>>>  1 file changed, 148 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/media/nxp,imx8-isi.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/nxp,imx8-isi.yaml b/Documentation/devicetree/bindings/media/nxp,imx8-isi.yaml
>>> new file mode 100644
>>> index 000000000000..390dfa03026b
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/media/nxp,imx8-isi.yaml
>>> @@ -0,0 +1,148 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/media/nxp,imx8-isi.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: i.MX8 Image Sensing Interface
>>> +
>>> +maintainers:
>>> +  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> +
>>> +description: |
>>> +  The Image Sensing Interface (ISI) combines image processing pipelines with
>>> +  DMA engines to process and capture frames originating from a variety of
>>> +  sources. The inputs to the ISI go through Pixel Link interfaces, and their
>>> +  number and nature is SoC-dependent. They cover both capture interfaces (MIPI
>>> +  CSI-2 RX, HDMI RX, ...) and display engine outputs for writeback support.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - fsl,imx8mn-isi
>>> +      - fsl,imx8mp-isi
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  clocks:
>>> +    items:
>>> +      - description: The AXI clock
>>> +      - description: The APB clock
>>> +      # TODO: Check if the per-channel ipg_proc_clk clocks need to be specified
>>> +      # as well, in case some SoCs have the ability to control them separately.
>>> +      # This may be the case of the i.MX8[DQ]X(P)
>>> +
>>> +  clock-names:
>>> +    items:
>>> +      - const: axi
>>> +      - const: apb
>>> +
>>> +  fsl,blk-ctrl:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>> +    description:
>>> +      A phandle referencing the block control that contains the CSIS to ISI
>>> +      gasket.
>>> +
>>> +  interrupts: true
>>
>> Need generic constraints - min/maxItems.
> 
> I can't set maxItems here, as the value depends on the compatible
> string. It's set below as part of the "allOf". I could set minItems to
> 1, but I don't really see a point in doing so.

Of course you can, just like all other files could.

https://elixir.bootlin.com/linux/v5.19-rc6/source/Documentation/devicetree/bindings/clock/samsung,exynos7-clock.yaml#L57

> 
>>> +
>>> +  power-domains: true
>>
>> Ditto.
> 
> I'll fix this one.
> 
>>> +
>>> +  ports:
>>> +    $ref: /schemas/graph.yaml#/properties/ports
>>> +    description: |
>>> +      Ports represent the Pixel Link inputs to the ISI. Their number and
>>> +      assignment are model-dependent. Each port shall have a single endpoint.
>>> +
>>> +    patternProperties:
>>> +      "^port@[0-9]$":
>>> +        $ref: /schemas/graph.yaml#/properties/port
>>> +        unevaluatedProperties: false
>>> +
>>> +    unevaluatedProperties: false
>>
>> At least one port is always required?
> 
> That's a fair assumption I think. How would you express that ? There's
> no patternRequired as far as I know. Note that the device-dependent
> ports are described in the "allOf" section below, where "required" is
> set per device model.

required:
 - port@0

> 
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - interrupts
>>> +  - clocks
>>> +  - clock-names
>>> +  - fsl,blk-ctrl
>>> +  - ports
>>> +
>>> +allOf:
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            const: fsl,imx8mn-isi
>>> +    then:
>>> +      properties:
>>> +        interrupts:
>>> +          maxItems: 1
>>> +        ports:
>>> +          properties:
>>> +            port@0:
>>> +              description: MIPI CSI-2 RX
>>> +          required:
>>> +            - port@0
>>> +
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            const: fsl,imx8mp-isi
>>> +    then:
>>> +      properties:
>>> +        interrupts:
>>> +          maxItems: 2
>>
>> You need to describe the items.
> 
> It's one interrupt per pipeline. Can I add the description to the
> generic interrupts property instead of documenting each item
> individually ? Something along the lines of
> 
>   interrupts:
>     description: Processing pipeline interrupts, one per pipeline
> 

This sounds good, thanks!


Best regards,
Krzysztof

Re: [PATCH v2 6/7] dt-bindings: media: Add i.MX8 ISI DT bindings

[Laurent Pinchart]

Hi Krzysztof,

On Tue, Jul 12, 2022 at 02:49:06PM +0200, Krzysztof Kozlowski wrote:
> On 12/07/2022 12:25, Laurent Pinchart wrote:
> > On Tue, Jul 12, 2022 at 09:49:45AM +0200, Krzysztof Kozlowski wrote:
> >> On 12/07/2022 02:02, Laurent Pinchart wrote:
> >>> The Image Sensing Interface (ISI) combines image processing pipelines
> >>> with DMA engines to process and capture frames originating from a
> >>> variety of sources. The inputs to the ISI go through Pixel Link
> >>> interfaces, and their number and nature is SoC-dependent. They cover
> >>> both capture interfaces (MIPI CSI-2 RX, HDMI RX) and memory inputs.
> >>>
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> ---
> >>> Changes since v1:
> >>>
> >>> - Fix compatible string checks in conditional schema
> >>> - Fix interrupts property handling
> >>> ---
> >>>  .../bindings/media/nxp,imx8-isi.yaml          | 148 ++++++++++++++++++
> >>>  1 file changed, 148 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/media/nxp,imx8-isi.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/media/nxp,imx8-isi.yaml b/Documentation/devicetree/bindings/media/nxp,imx8-isi.yaml
> >>> new file mode 100644
> >>> index 000000000000..390dfa03026b
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/media/nxp,imx8-isi.yaml
> >>> @@ -0,0 +1,148 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/media/nxp,imx8-isi.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: i.MX8 Image Sensing Interface
> >>> +
> >>> +maintainers:
> >>> +  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> +
> >>> +description: |
> >>> +  The Image Sensing Interface (ISI) combines image processing pipelines with
> >>> +  DMA engines to process and capture frames originating from a variety of
> >>> +  sources. The inputs to the ISI go through Pixel Link interfaces, and their
> >>> +  number and nature is SoC-dependent. They cover both capture interfaces (MIPI
> >>> +  CSI-2 RX, HDMI RX, ...) and display engine outputs for writeback support.
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    enum:
> >>> +      - fsl,imx8mn-isi
> >>> +      - fsl,imx8mp-isi
> >>> +
> >>> +  reg:
> >>> +    maxItems: 1
> >>> +
> >>> +  clocks:
> >>> +    items:
> >>> +      - description: The AXI clock
> >>> +      - description: The APB clock
> >>> +      # TODO: Check if the per-channel ipg_proc_clk clocks need to be specified
> >>> +      # as well, in case some SoCs have the ability to control them separately.
> >>> +      # This may be the case of the i.MX8[DQ]X(P)
> >>> +
> >>> +  clock-names:
> >>> +    items:
> >>> +      - const: axi
> >>> +      - const: apb
> >>> +
> >>> +  fsl,blk-ctrl:
> >>> +    $ref: /schemas/types.yaml#/definitions/phandle
> >>> +    description:
> >>> +      A phandle referencing the block control that contains the CSIS to ISI
> >>> +      gasket.
> >>> +
> >>> +  interrupts: true
> >>
> >> Need generic constraints - min/maxItems.
> > 
> > I can't set maxItems here, as the value depends on the compatible
> > string. It's set below as part of the "allOf". I could set minItems to
> > 1, but I don't really see a point in doing so.
> 
> Of course you can, just like all other files could.
> 
> https://elixir.bootlin.com/linux/v5.19-rc6/source/Documentation/devicetree/bindings/clock/samsung,exynos7-clock.yaml#L57

I meant I can't set a fixed maximum, other than the max of all max. Is
there a point in doing do ?

> >>> +
> >>> +  power-domains: true
> >>
> >> Ditto.
> > 
> > I'll fix this one.
> > 
> >>> +
> >>> +  ports:
> >>> +    $ref: /schemas/graph.yaml#/properties/ports
> >>> +    description: |
> >>> +      Ports represent the Pixel Link inputs to the ISI. Their number and
> >>> +      assignment are model-dependent. Each port shall have a single endpoint.
> >>> +
> >>> +    patternProperties:
> >>> +      "^port@[0-9]$":
> >>> +        $ref: /schemas/graph.yaml#/properties/port
> >>> +        unevaluatedProperties: false
> >>> +
> >>> +    unevaluatedProperties: false
> >>
> >> At least one port is always required?
> > 
> > That's a fair assumption I think. How would you express that ? There's
> > no patternRequired as far as I know. Note that the device-dependent
> > ports are described in the "allOf" section below, where "required" is
> > set per device model.
> 
> required:
>  - port@0

What if an SoC has port@1 only, or another port ? It's likely not a
concern in this binding though, so I could add required: - port@0, but
is there a point in doing so if the per-compatible constraints list the
required ports ?

> >>> +
> >>> +required:
> >>> +  - compatible
> >>> +  - reg
> >>> +  - interrupts
> >>> +  - clocks
> >>> +  - clock-names
> >>> +  - fsl,blk-ctrl
> >>> +  - ports
> >>> +
> >>> +allOf:
> >>> +  - if:
> >>> +      properties:
> >>> +        compatible:
> >>> +          contains:
> >>> +            const: fsl,imx8mn-isi
> >>> +    then:
> >>> +      properties:
> >>> +        interrupts:
> >>> +          maxItems: 1
> >>> +        ports:
> >>> +          properties:
> >>> +            port@0:
> >>> +              description: MIPI CSI-2 RX
> >>> +          required:
> >>> +            - port@0
> >>> +
> >>> +  - if:
> >>> +      properties:
> >>> +        compatible:
> >>> +          contains:
> >>> +            const: fsl,imx8mp-isi
> >>> +    then:
> >>> +      properties:
> >>> +        interrupts:
> >>> +          maxItems: 2
> >>
> >> You need to describe the items.
> > 
> > It's one interrupt per pipeline. Can I add the description to the
> > generic interrupts property instead of documenting each item
> > individually ? Something along the lines of
> > 
> >   interrupts:
> >     description: Processing pipeline interrupts, one per pipeline
> 
> This sounds good, thanks!

Thanks, I'll do that then.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 6/7] dt-bindings: media: Add i.MX8 ISI DT bindings

[Krzysztof Kozlowski]

On 12/07/2022 16:26, Laurent Pinchart wrote:
> Hi Krzysztof,
> 
> On Tue, Jul 12, 2022 at 02:49:06PM +0200, Krzysztof Kozlowski wrote:
>> On 12/07/2022 12:25, Laurent Pinchart wrote:
>>> On Tue, Jul 12, 2022 at 09:49:45AM +0200, Krzysztof Kozlowski wrote:
>>>> On 12/07/2022 02:02, Laurent Pinchart wrote:
>>>>> The Image Sensing Interface (ISI) combines image processing pipelines
>>>>> with DMA engines to process and capture frames originating from a
>>>>> variety of sources. The inputs to the ISI go through Pixel Link
>>>>> interfaces, and their number and nature is SoC-dependent. They cover
>>>>> both capture interfaces (MIPI CSI-2 RX, HDMI RX) and memory inputs.
>>>>>
>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>>> ---
>>>>> Changes since v1:
>>>>>
>>>>> - Fix compatible string checks in conditional schema
>>>>> - Fix interrupts property handling
>>>>> ---
>>>>>  .../bindings/media/nxp,imx8-isi.yaml          | 148 ++++++++++++++++++
>>>>>  1 file changed, 148 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/media/nxp,imx8-isi.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/media/nxp,imx8-isi.yaml b/Documentation/devicetree/bindings/media/nxp,imx8-isi.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..390dfa03026b
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/media/nxp,imx8-isi.yaml
>>>>> @@ -0,0 +1,148 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/media/nxp,imx8-isi.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: i.MX8 Image Sensing Interface
>>>>> +
>>>>> +maintainers:
>>>>> +  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>>> +
>>>>> +description: |
>>>>> +  The Image Sensing Interface (ISI) combines image processing pipelines with
>>>>> +  DMA engines to process and capture frames originating from a variety of
>>>>> +  sources. The inputs to the ISI go through Pixel Link interfaces, and their
>>>>> +  number and nature is SoC-dependent. They cover both capture interfaces (MIPI
>>>>> +  CSI-2 RX, HDMI RX, ...) and display engine outputs for writeback support.
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    enum:
>>>>> +      - fsl,imx8mn-isi
>>>>> +      - fsl,imx8mp-isi
>>>>> +
>>>>> +  reg:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  clocks:
>>>>> +    items:
>>>>> +      - description: The AXI clock
>>>>> +      - description: The APB clock
>>>>> +      # TODO: Check if the per-channel ipg_proc_clk clocks need to be specified
>>>>> +      # as well, in case some SoCs have the ability to control them separately.
>>>>> +      # This may be the case of the i.MX8[DQ]X(P)
>>>>> +
>>>>> +  clock-names:
>>>>> +    items:
>>>>> +      - const: axi
>>>>> +      - const: apb
>>>>> +
>>>>> +  fsl,blk-ctrl:
>>>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>>>> +    description:
>>>>> +      A phandle referencing the block control that contains the CSIS to ISI
>>>>> +      gasket.
>>>>> +
>>>>> +  interrupts: true
>>>>
>>>> Need generic constraints - min/maxItems.
>>>
>>> I can't set maxItems here, as the value depends on the compatible
>>> string. It's set below as part of the "allOf". I could set minItems to
>>> 1, but I don't really see a point in doing so.
>>
>> Of course you can, just like all other files could.
>>
>> https://elixir.bootlin.com/linux/v5.19-rc6/source/Documentation/devicetree/bindings/clock/samsung,exynos7-clock.yaml#L57
> 
> I meant I can't set a fixed maximum, other than the max of all max. Is
> there a point in doing do ?

Yes, that's the practice. makes code easier to read and notice all
constraints.


Best regards,
Krzysztof