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@xxxxxxxxxxxxxxxx> > >>> --- > >>> 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@xxxxxxxxxxxxxxxx> > >>> + > >>> +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
