This introduces YAML bindings documentation for the Allwinner A31 Image
Signal Processor (ISP).
Signed-off-by: Paul Kocialkowski <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
.../media/allwinner,sun6i-a31-isp.yaml | 97 +++++++++++++++++++
1 file changed, 97 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/allwinner,sun6i-a31-isp.yaml
diff --git a/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-isp.yaml b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-isp.yaml
new file mode 100644
index 000000000000..2fda6e05e16c
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-isp.yaml
@@ -0,0 +1,97 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/allwinner,sun6i-a31-isp.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Allwinner A31 Image Signal Processor Driver (ISP) Device Tree Bindings
+
+maintainers:
+ - Paul Kocialkowski <[email protected]>
+
+properties:
+ compatible:
+ enum:
+ - allwinner,sun6i-a31-isp
+ - allwinner,sun8i-v3s-isp
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ items:
+ - description: Bus Clock
+ - description: Module Clock
+ - description: DRAM Clock
+
+ clock-names:
+ items:
+ - const: bus
+ - const: mod
+ - const: ram
+
+ resets:
+ maxItems: 1
+
+ ports:
+ $ref: /schemas/graph.yaml#/properties/ports
+
+ properties:
+ port@0:
+ $ref: /schemas/graph.yaml#/properties/port
+ description: CSI0 input port
+
+ port@1:
+ $ref: /schemas/graph.yaml#/properties/port
+ description: CSI1 input port
+
+ anyOf:
+ - required:
+ - port@0
+ - required:
+ - port@1
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+ - clock-names
+ - resets
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ #include <dt-bindings/clock/sun8i-v3s-ccu.h>
+ #include <dt-bindings/reset/sun8i-v3s-ccu.h>
+
+ isp: isp@1cb8000 {
+ compatible = "allwinner,sun8i-v3s-isp";
+ reg = <0x01cb8000 0x1000>;
+ interrupts = <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&ccu CLK_BUS_CSI>,
+ <&ccu CLK_CSI1_SCLK>,
+ <&ccu CLK_DRAM_CSI>;
+ clock-names = "bus", "mod", "ram";
+ resets = <&ccu RST_BUS_CSI>;
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+
+ isp_in_csi0: endpoint {
+ remote-endpoint = <&csi0_out_isp>;
+ };
+ };
+ };
+ };
+
+...
--
2.37.1
Hi Paul,
Thank you for the patch.
On Fri, Aug 26, 2022 at 08:41:39PM +0200, Paul Kocialkowski wrote:
> This introduces YAML bindings documentation for the Allwinner A31 Image
> Signal Processor (ISP).
>
> Signed-off-by: Paul Kocialkowski <[email protected]>
> Reviewed-by: Rob Herring <[email protected]>
> ---
> .../media/allwinner,sun6i-a31-isp.yaml | 97 +++++++++++++++++++
> 1 file changed, 97 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/allwinner,sun6i-a31-isp.yaml
>
> diff --git a/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-isp.yaml b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-isp.yaml
> new file mode 100644
> index 000000000000..2fda6e05e16c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-isp.yaml
> @@ -0,0 +1,97 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/allwinner,sun6i-a31-isp.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Allwinner A31 Image Signal Processor Driver (ISP) Device Tree Bindings
> +
> +maintainers:
> + - Paul Kocialkowski <[email protected]>
> +
> +properties:
> + compatible:
> + enum:
> + - allwinner,sun6i-a31-isp
> + - allwinner,sun8i-v3s-isp
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> +
> + clocks:
> + items:
> + - description: Bus Clock
> + - description: Module Clock
> + - description: DRAM Clock
> +
> + clock-names:
> + items:
> + - const: bus
> + - const: mod
> + - const: ram
> +
> + resets:
> + maxItems: 1
> +
> + ports:
> + $ref: /schemas/graph.yaml#/properties/ports
> +
> + properties:
> + port@0:
> + $ref: /schemas/graph.yaml#/properties/port
> + description: CSI0 input port
> +
> + port@1:
> + $ref: /schemas/graph.yaml#/properties/port
> + description: CSI1 input port
> +
> + anyOf:
> + - required:
> + - port@0
> + - required:
> + - port@1
I'd still like to see all ports that exist in the hardware being
mandatory. I assume at least one of the A31 and V3s has two connected
ports in the SoC or you wouldn't declare them both here :-)
Apart from that, this looks good.
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - clocks
> + - clock-names
> + - resets
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> + #include <dt-bindings/clock/sun8i-v3s-ccu.h>
> + #include <dt-bindings/reset/sun8i-v3s-ccu.h>
> +
> + isp: isp@1cb8000 {
> + compatible = "allwinner,sun8i-v3s-isp";
> + reg = <0x01cb8000 0x1000>;
> + interrupts = <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&ccu CLK_BUS_CSI>,
> + <&ccu CLK_CSI1_SCLK>,
> + <&ccu CLK_DRAM_CSI>;
> + clock-names = "bus", "mod", "ram";
> + resets = <&ccu RST_BUS_CSI>;
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> +
> + isp_in_csi0: endpoint {
> + remote-endpoint = <&csi0_out_isp>;
> + };
> + };
> + };
> + };
> +
> +...
--
Regards,
Laurent Pinchart
Hi Laurent,
On Sat 27 Aug 22, 00:12, Laurent Pinchart wrote:
> Hi Paul,
>
> Thank you for the patch.
Thanks for the review!
> On Fri, Aug 26, 2022 at 08:41:39PM +0200, Paul Kocialkowski wrote:
> > This introduces YAML bindings documentation for the Allwinner A31 Image
> > Signal Processor (ISP).
> >
> > Signed-off-by: Paul Kocialkowski <[email protected]>
> > Reviewed-by: Rob Herring <[email protected]>
> > ---
> > .../media/allwinner,sun6i-a31-isp.yaml | 97 +++++++++++++++++++
> > 1 file changed, 97 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/media/allwinner,sun6i-a31-isp.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-isp.yaml b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-isp.yaml
> > new file mode 100644
> > index 000000000000..2fda6e05e16c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-isp.yaml
> > @@ -0,0 +1,97 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/allwinner,sun6i-a31-isp.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Allwinner A31 Image Signal Processor Driver (ISP) Device Tree Bindings
> > +
> > +maintainers:
> > + - Paul Kocialkowski <[email protected]>
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - allwinner,sun6i-a31-isp
> > + - allwinner,sun8i-v3s-isp
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + interrupts:
> > + maxItems: 1
> > +
> > + clocks:
> > + items:
> > + - description: Bus Clock
> > + - description: Module Clock
> > + - description: DRAM Clock
> > +
> > + clock-names:
> > + items:
> > + - const: bus
> > + - const: mod
> > + - const: ram
> > +
> > + resets:
> > + maxItems: 1
> > +
> > + ports:
> > + $ref: /schemas/graph.yaml#/properties/ports
> > +
> > + properties:
> > + port@0:
> > + $ref: /schemas/graph.yaml#/properties/port
> > + description: CSI0 input port
> > +
> > + port@1:
> > + $ref: /schemas/graph.yaml#/properties/port
> > + description: CSI1 input port
> > +
> > + anyOf:
> > + - required:
> > + - port@0
> > + - required:
> > + - port@1
>
> I'd still like to see all ports that exist in the hardware being
> mandatory. I assume at least one of the A31 and V3s has two connected
> ports in the SoC or you wouldn't declare them both here :-)
Some SoCs (e.g. A83T) only have one CSI controller so we can't require both.
This could be a decision based on the compatible but my personal opinion is
that it's not really worth making this binding so complex.
We can always informally enforce that all possible links should be present
when merging changes to the soc dts.
What do you think?
Paul
> Apart from that, this looks good.
>
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - interrupts
> > + - clocks
> > + - clock-names
> > + - resets
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/interrupt-controller/arm-gic.h>
> > + #include <dt-bindings/clock/sun8i-v3s-ccu.h>
> > + #include <dt-bindings/reset/sun8i-v3s-ccu.h>
> > +
> > + isp: isp@1cb8000 {
> > + compatible = "allwinner,sun8i-v3s-isp";
> > + reg = <0x01cb8000 0x1000>;
> > + interrupts = <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>;
> > + clocks = <&ccu CLK_BUS_CSI>,
> > + <&ccu CLK_CSI1_SCLK>,
> > + <&ccu CLK_DRAM_CSI>;
> > + clock-names = "bus", "mod", "ram";
> > + resets = <&ccu RST_BUS_CSI>;
> > +
> > + ports {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + port@0 {
> > + reg = <0>;
> > +
> > + isp_in_csi0: endpoint {
> > + remote-endpoint = <&csi0_out_isp>;
> > + };
> > + };
> > + };
> > + };
> > +
> > +...
>
> --
> Regards,
>
> Laurent Pinchart
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
Hi Paul,
On Thu, Sep 01, 2022 at 05:03:17PM +0200, Paul Kocialkowski wrote:
> On Sat 27 Aug 22, 00:12, Laurent Pinchart wrote:
> > On Fri, Aug 26, 2022 at 08:41:39PM +0200, Paul Kocialkowski wrote:
> > > This introduces YAML bindings documentation for the Allwinner A31 Image
> > > Signal Processor (ISP).
> > >
> > > Signed-off-by: Paul Kocialkowski <[email protected]>
> > > Reviewed-by: Rob Herring <[email protected]>
> > > ---
> > > .../media/allwinner,sun6i-a31-isp.yaml | 97 +++++++++++++++++++
> > > 1 file changed, 97 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/media/allwinner,sun6i-a31-isp.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-isp.yaml b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-isp.yaml
> > > new file mode 100644
> > > index 000000000000..2fda6e05e16c
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-isp.yaml
> > > @@ -0,0 +1,97 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/media/allwinner,sun6i-a31-isp.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Allwinner A31 Image Signal Processor Driver (ISP) Device Tree Bindings
> > > +
> > > +maintainers:
> > > + - Paul Kocialkowski <[email protected]>
> > > +
> > > +properties:
> > > + compatible:
> > > + enum:
> > > + - allwinner,sun6i-a31-isp
> > > + - allwinner,sun8i-v3s-isp
> > > +
> > > + reg:
> > > + maxItems: 1
> > > +
> > > + interrupts:
> > > + maxItems: 1
> > > +
> > > + clocks:
> > > + items:
> > > + - description: Bus Clock
> > > + - description: Module Clock
> > > + - description: DRAM Clock
> > > +
> > > + clock-names:
> > > + items:
> > > + - const: bus
> > > + - const: mod
> > > + - const: ram
> > > +
> > > + resets:
> > > + maxItems: 1
> > > +
> > > + ports:
> > > + $ref: /schemas/graph.yaml#/properties/ports
> > > +
> > > + properties:
> > > + port@0:
> > > + $ref: /schemas/graph.yaml#/properties/port
> > > + description: CSI0 input port
> > > +
> > > + port@1:
> > > + $ref: /schemas/graph.yaml#/properties/port
> > > + description: CSI1 input port
> > > +
> > > + anyOf:
> > > + - required:
> > > + - port@0
> > > + - required:
> > > + - port@1
> >
> > I'd still like to see all ports that exist in the hardware being
> > mandatory. I assume at least one of the A31 and V3s has two connected
> > ports in the SoC or you wouldn't declare them both here :-)
>
> Some SoCs (e.g. A83T) only have one CSI controller so we can't require both.
> This could be a decision based on the compatible but my personal opinion is
> that it's not really worth making this binding so complex.
>
> We can always informally enforce that all possible links should be present
> when merging changes to the soc dts.
>
> What do you think?
It makes the binding more complex, but it allows catching issues in an
automated way instead of relying on reviews. Lowering the review burden
is something I usually welcome :-) It's probably less of an issue here
as this is all about the SoC integration, not about board files, so I
won't insist too strongly even if I prefer bindings that are more
descriptive.
> > Apart from that, this looks good.
> >
> > > +
> > > +required:
> > > + - compatible
> > > + - reg
> > > + - interrupts
> > > + - clocks
> > > + - clock-names
> > > + - resets
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > + - |
> > > + #include <dt-bindings/interrupt-controller/arm-gic.h>
> > > + #include <dt-bindings/clock/sun8i-v3s-ccu.h>
> > > + #include <dt-bindings/reset/sun8i-v3s-ccu.h>
> > > +
> > > + isp: isp@1cb8000 {
> > > + compatible = "allwinner,sun8i-v3s-isp";
> > > + reg = <0x01cb8000 0x1000>;
> > > + interrupts = <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>;
> > > + clocks = <&ccu CLK_BUS_CSI>,
> > > + <&ccu CLK_CSI1_SCLK>,
> > > + <&ccu CLK_DRAM_CSI>;
> > > + clock-names = "bus", "mod", "ram";
> > > + resets = <&ccu RST_BUS_CSI>;
> > > +
> > > + ports {
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > +
> > > + port@0 {
> > > + reg = <0>;
> > > +
> > > + isp_in_csi0: endpoint {
> > > + remote-endpoint = <&csi0_out_isp>;
> > > + };
> > > + };
> > > + };
> > > + };
> > > +
> > > +...
--
Regards,
Laurent Pinchart