diff mbox series

[1/3] media: dt-bindings: add MT8188 AIE

Message ID 20240717125426.32660-2-yelian.wang@mediatek.com
State New
Headers show
Series media: mediatek: Add support MT8188 AIE | expand

Commit Message

20220614094956 created July 17, 2024, 12:41 p.m. UTC
From: Yelian Wang <yelian.wang@mediatek.com>

Add YAML device tree binding for MT8188 AIE.

Signed-off-by: Yelian Wang <yelian.wang@mediatek.com>
---
 .../bindings/media/mediatek-aie.yaml          | 99 +++++++++++++++++++
 1 file changed, 99 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/mediatek-aie.yaml

Comments

Krzysztof Kozlowski July 17, 2024, 1:04 p.m. UTC | #1
On 17/07/2024 14:41, 20220614094956 created wrote:
> From: Yelian Wang <yelian.wang@mediatek.com>
> 
> Add YAML device tree binding for MT8188 AIE.
> 
> Signed-off-by: Yelian Wang <yelian.wang@mediatek.com>
> ---
>  .../bindings/media/mediatek-aie.yaml          | 99 +++++++++++++++++++
>  1 file changed, 99 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/mediatek-aie.yaml

Filename matching compatible.

> 
> diff --git a/Documentation/devicetree/bindings/media/mediatek-aie.yaml b/Documentation/devicetree/bindings/media/mediatek-aie.yaml
> new file mode 100644
> index 000000000000..300aef43e02b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/mediatek-aie.yaml
> @@ -0,0 +1,99 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/mediatek-aie.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: The AIE Unit of MediaTek Camera System

What is AIE?

> +
> +maintainers:
> +  - Yelian Wang <yelian.wang@mediatek.com>
> +
> +description:
> +  MediaTek AIE is the ISP unit in MediaTek SoC.


AIE is ISP? Say something more, including unwinding of the acronym.
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: mediatek,mt8188-aie
> +      - enum:
> +          - mediatek,aie-hw3.1
> +          - mediatek,aie-hw3.0

Nope, you cannot have flexible fallbacks. Anyway, drop hardware
versions. Do you see them anywhere in other bindings? Use only SoC
compatible.


> +
> +  "#address-cells":
> +    const: 2

Why?


> +
> +  "#size-cells":
> +    const: 2

Why?

> +
> +  reg:
> +    maxItems: 2

You must list and describe items.


> +
> +  interrupts:
> +    maxItems: 1
> +
> +  mediatek,larb:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      Must contain the local arbiters in the current SoCs, see
> +      Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml
> +      for details.
> +
> +  iommus:
> +    maxItems: 4
> +    description:
> +      Points to the respective IOMMU block with master port as argument, see
> +      Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml for details.
> +      Ports are according to the HW.
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 4
> +    minItems: 4

Drop minItems.

List and describe the items instead.

> +
> +  clock-names:
> +    items:
> +      - const: IMG_IPE
> +      - const: IPE_FDVT
> +      - const: IPE_SMI_LARB12
> +      - const: IPE_TOP

Nope. Please use DTS coding style.

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - iommus
> +  - power-domains
> +  - clocks
> +  - clock-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>

You do not use anything from this header.

> +    #include <dt-bindings/memory/mediatek,mt8188-memory-port.h>
> +    #include <dt-bindings/power/mediatek,mt8188-power.h>
> +    #include <dt-bindings/clock/mediatek,mt8188-clk.h>
> +    aie: aie@15310000 {

Drop unused label.

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


Best regards,
Krzysztof
Krzysztof Kozlowski July 17, 2024, 1:24 p.m. UTC | #2
On 17/07/2024 14:41, 20220614094956 created wrote:
> From: Yelian Wang <yelian.wang@mediatek.com>
> 
> Add YAML device tree binding for MT8188 AIE.
> 
> Signed-off-by: Yelian Wang <yelian.wang@mediatek.com>
> ---
>  .../bindings/media/mediatek-aie.yaml          | 99 +++++++++++++++++++
>  1 file changed, 99 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/mediatek-aie.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/mediatek-aie.yaml b/Documentation/devicetree/bindings/media/mediatek-aie.yaml
> new file mode 100644
> index 000000000000..300aef43e02b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/mediatek-aie.yaml
> @@ -0,0 +1,99 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/mediatek-aie.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: The AIE Unit of MediaTek Camera System
> +
> +maintainers:
> +  - Yelian Wang <yelian.wang@mediatek.com>
> +
> +description:
> +  MediaTek AIE is the ISP unit in MediaTek SoC.
> +

BTW, most likely you miss here ports, although with such poor hardware
description as above, it's tricky to guess what this is and what it does.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/media/mediatek-aie.yaml b/Documentation/devicetree/bindings/media/mediatek-aie.yaml
new file mode 100644
index 000000000000..300aef43e02b
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/mediatek-aie.yaml
@@ -0,0 +1,99 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/mediatek-aie.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: The AIE Unit of MediaTek Camera System
+
+maintainers:
+  - Yelian Wang <yelian.wang@mediatek.com>
+
+description:
+  MediaTek AIE is the ISP unit in MediaTek SoC.
+
+properties:
+  compatible:
+    items:
+      - const: mediatek,mt8188-aie
+      - enum:
+          - mediatek,aie-hw3.1
+          - mediatek,aie-hw3.0
+
+  "#address-cells":
+    const: 2
+
+  "#size-cells":
+    const: 2
+
+  reg:
+    maxItems: 2
+
+  interrupts:
+    maxItems: 1
+
+  mediatek,larb:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      Must contain the local arbiters in the current SoCs, see
+      Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml
+      for details.
+
+  iommus:
+    maxItems: 4
+    description:
+      Points to the respective IOMMU block with master port as argument, see
+      Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml for details.
+      Ports are according to the HW.
+
+  power-domains:
+    maxItems: 1
+
+  clocks:
+    maxItems: 4
+    minItems: 4
+
+  clock-names:
+    items:
+      - const: IMG_IPE
+      - const: IPE_FDVT
+      - const: IPE_SMI_LARB12
+      - const: IPE_TOP
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - iommus
+  - power-domains
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/memory/mediatek,mt8188-memory-port.h>
+    #include <dt-bindings/power/mediatek,mt8188-power.h>
+    #include <dt-bindings/clock/mediatek,mt8188-clk.h>
+    aie: aie@15310000 {
+      compatible = "mediatek,mt8188-aie", "mediatek,aie-hw3.1";
+      reg = <0 0x15310000 0 0x1000>;
+      interrupts = <GIC_SPI 787 IRQ_TYPE_LEVEL_HIGH 0>;
+      mediatek,larb = <&larb12>;
+      iommus = <&vpp_iommu M4U_PORT_L12_FDVT_RDA_0>,
+               <&vpp_iommu M4U_PORT_L12_FDVT_RDB_0>,
+               <&vpp_iommu M4U_PORT_L12_FDVT_WRA_0>,
+               <&vpp_iommu M4U_PORT_L12_FDVT_WRB_0>;
+      power-domains = <&spm MT8188_POWER_DOMAIN_IPE>;
+      clocks = <&imgsys CLK_IMGSYS_MAIN_IPE>,
+               <&ipesys CLK_IPE_FDVT>,
+               <&ipesys CLK_IPE_SMI_LARB12>,
+               <&ipesys CLK_IPESYS_TOP>;
+      clock-names = "IMG_IPE",
+                    "IPE_FDVT",
+                    "IPE_SMI_LARB12",
+                    "IPE_TOP";
+    };