Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Aakarsh Jain" <aakarsh.jain@samsung.com>
To:     "'Alim Akhtar'" <alim.akhtar@samsung.com>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-media@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <devicetree@vger.kernel.org>
Cc:     <m.szyprowski@samsung.com>, <andrzej.hajda@intel.com>,
        <mchehab@kernel.org>, <hverkuil-cisco@xs4all.nl>,
        <ezequiel@vanguardiasur.com.ar>, <jernej.skrabec@gmail.com>,
        <benjamin.gaignard@collabora.com>, <stanimir.varbanov@linaro.org>,
        <dillon.minfei@gmail.com>, <david.plowman@raspberrypi.com>,
        <mark.rutland@arm.com>, <robh+dt@kernel.org>, <krzk+dt@kernel.org>,
        <andi@etezian.org>, <aswani.reddy@samsung.com>,
        <pankaj.dubey@samsung.com>, <linux-fsd@tesla.com>,
        <smitha.t@samsung.com>
In-Reply-To: <000c01d8e9f0$764e5ea0$62eb1be0$@samsung.com>
Subject: RE: [Patch v3 01/15] dt-bindings: media: s5p-mfc: Add new DT schema
 for MFC
Date:   Fri, 28 Oct 2022 10:07:33 +0530
Message-ID: <026501d8ea87$024d1400$06e73c00$@samsung.com>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Thread-Index: AQIjXZOHgvokXGuofbg3rWbxTFZEMQGGfBLcASb/h3kBiXArEK1smYlQ
Content-Language: en-in
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 105P
DLP-Filter: Pass
References: <20221011122516.32135-1-aakarsh.jain@samsung.com>
        <CGME20221011125142epcas5p13c858a5f27830fb1de50fa51e9730eca@epcas5p1.samsung.com>
        <20221011122516.32135-2-aakarsh.jain@samsung.com>
        <000c01d8e9f0$764e5ea0$62eb1be0$@samsung.com>
Precedence: bulk


> -----Original Message-----
> From: Alim Akhtar =5Bmailto:alim.akhtar=40samsung.com=5D
> Sent: 27 October 2022 16:10
> To: 'aakarsh jain' <aakarsh.jain=40samsung.com>; linux-arm-
> kernel=40lists.infradead.org; linux-media=40vger.kernel.org; linux-
> kernel=40vger.kernel.org; devicetree=40vger.kernel.org
> Cc: m.szyprowski=40samsung.com; andrzej.hajda=40intel.com;
> mchehab=40kernel.org; hverkuil-cisco=40xs4all.nl;
> ezequiel=40vanguardiasur.com.ar; jernej.skrabec=40gmail.com;
> benjamin.gaignard=40collabora.com; stanimir.varbanov=40linaro.org;
> dillon.minfei=40gmail.com; david.plowman=40raspberrypi.com;
> mark.rutland=40arm.com; robh+dt=40kernel.org; krzk+dt=40kernel.org;
> andi=40etezian.org; aswani.reddy=40samsung.com;
> pankaj.dubey=40samsung.com; linux-fsd=40tesla.com; smitha.t=40samsung.com
> Subject: RE: =5BPatch v3 01/15=5D dt-bindings: media: s5p-mfc: Add new DT
> schema for MFC
>=20
> Hi Aakarsh
> Thanks for the patch.
>=20
> >-----Original Message-----
> >From: aakarsh jain =5Bmailto:aakarsh.jain=40samsung.com=5D
> >Sent: Tuesday, October 11, 2022 5:55 PM
> >To: linux-arm-kernel=40lists.infradead.org; linux-media=40vger.kernel.or=
g;
> >linux- kernel=40vger.kernel.org; devicetree=40vger.kernel.org
> >Cc: m.szyprowski=40samsung.com; andrzej.hajda=40intel.com;
> >mchehab=40kernel.org; hverkuil-cisco=40xs4all.nl;
> >ezequiel=40vanguardiasur.com.ar; jernej.skrabec=40gmail.com;
> >benjamin.gaignard=40collabora.com; stanimir.varbanov=40linaro.org;
> >dillon.minfei=40gmail.com; david.plowman=40raspberrypi.com;
> >mark.rutland=40arm.com; robh+dt=40kernel.org; krzk+dt=40kernel.org;
> >andi=40etezian.org; alim.akhtar=40samsung.com;
> aswani.reddy=40samsung.com;
> >pankaj.dubey=40samsung.com; linux-fsd=40tesla.com;
> smitha.t=40samsung.com;
> >aakarsh.jain=40samsung.com
> >Subject: =5BPatch v3 01/15=5D dt-bindings: media: s5p-mfc: Add new DT
> >schema for MFC
> >
> >From: Smitha T Murthy <smitha.t=40samsung.com>
> >
> >Convert DT schema for s5p-mfc in yaml format
> >
> >Cc: linux-fsd=40tesla.com
> >Signed-off-by: Smitha T Murthy <smitha.t=40samsung.com>
> >Signed-off-by: Aakarsh Jain <aakarsh.jain=40samsung.com>
> >---
> > .../devicetree/bindings/media/s5p-mfc.txt     =7C  75 --------
> > .../bindings/media/samsung,s5p-mfc.yaml       =7C 163 +++++++++++++++++=
+
> > 2 files changed, 163 insertions(+), 75 deletions(-)  create mode
> >100644 Documentation/devicetree/bindings/media/samsung,s5p-mfc.yaml
> >
> >diff --git a/Documentation/devicetree/bindings/media/s5p-mfc.txt
> >b/Documentation/devicetree/bindings/media/s5p-mfc.txt
> >index aa54c8159d9f..8b137891791f 100644
> >--- a/Documentation/devicetree/bindings/media/s5p-mfc.txt
> >+++ b/Documentation/devicetree/bindings/media/s5p-mfc.txt
> >=40=40 -1,76 +1 =40=40
> >-* Samsung Multi Format Codec (MFC)
> >
> >-Multi Format Codec (MFC) is the IP present in Samsung SoCs which
> >-supports high resolution decoding and encoding functionalities.
> >-The MFC device driver is a v4l2 driver which can encode/decode -video
> >raw/elementary streams and has support for all popular -video codecs.
> >-
> >-Required properties:
> >-  - compatible : value should be either one among the following
> >-	(a) =22samsung,mfc-v5=22 for MFC v5 present in Exynos4 SoCs
> >-	(b) =22samsung,mfc-v6=22 for MFC v6 present in Exynos5 SoCs
> >-	(c) =22samsung,mfc-v7=22 for MFC v7 present in Exynos5420 SoC
> >-	(d) =22samsung,mfc-v8=22 for MFC v8 present in Exynos5800 SoC
> >-	(e) =22samsung,exynos5433-mfc=22 for MFC v8 present in Exynos5433
> SoC
> >-	(f) =22samsung,mfc-v10=22 for MFC v10 present in Exynos7880 SoC
> >-
> >-  - reg : Physical base address of the IP registers and length of memor=
y
> >-	  mapped region.
> >-
> >-  - interrupts : MFC interrupt number to the CPU.
> >-  - clocks : from common clock binding: handle to mfc clock.
> >-  - clock-names : from common clock binding: must contain =22mfc=22,
> >-		  corresponding to entry in the clocks property.
> >-
> >-Optional properties:
> >-  - power-domains : power-domain property defined with a phandle
> >-			   to respective power domain.
> >-  - memory-region : from reserved memory binding: phandles to two
> >reserved
> >-	memory regions, first is for =22left=22 mfc memory bus interfaces,
> >-	second if for the =22right=22 mfc memory bus, used when no SYSMMU
> >-	support is available; used only by MFC v5 present in Exynos4 SoCs
> >-
> >-Obsolete properties:
> >-  - samsung,mfc-r, samsung,mfc-l : support removed, please use memory-
> >region
> >-	property instead
> >-
> >-
> >-Example:
> >-SoC specific DT entry:
> >-
> >-mfc: codec=4013400000 =7B
> >-	compatible =3D =22samsung,mfc-v5=22;
> >-	reg =3D <0x13400000 0x10000>;
> >-	interrupts =3D <0 94 0>;
> >-	power-domains =3D <&pd_mfc>;
> >-	clocks =3D <&clock 273>;
> >-	clock-names =3D =22mfc=22;
> >-=7D;
> >-
> >-Reserved memory specific DT entry for given board (see reserved memory
> >binding -for more information):
> >-
> >-reserved-memory =7B
> >-	=23address-cells =3D <1>;
> >-	=23size-cells =3D <1>;
> >-	ranges;
> >-
> >-	mfc_left: region=4051000000 =7B
> >-		compatible =3D =22shared-dma-pool=22;
> >-		no-map;
> >-		reg =3D <0x51000000 0x800000>;
> >-	=7D;
> >-
> >-	mfc_right: region=4043000000 =7B
> >-		compatible =3D =22shared-dma-pool=22;
> >-		no-map;
> >-		reg =3D <0x43000000 0x800000>;
> >-	=7D;
> >-=7D;
> >-
> >-Board specific DT entry:
> >-
> >-codec=4013400000 =7B
> >-	memory-region =3D <&mfc_left>, <&mfc_right>;
> >-=7D;
> >diff --git a/Documentation/devicetree/bindings/media/samsung,s5p-
> >mfc.yaml b/Documentation/devicetree/bindings/media/samsung,s5p-
> >mfc.yaml
> >new file mode 100644
> >index 000000000000..ad61b509846f
> >--- /dev/null
> >+++ b/Documentation/devicetree/bindings/media/samsung,s5p-mfc.yaml
> >=40=40 -0,0 +1,163 =40=40
> >+=23 SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause %YAML 1.2
> >+---
> >+=24id: http://devicetree.org/schemas/media/samsung,s5p-mfc.yaml=23
> >+=24schema: http://devicetree.org/meta-schemas/core.yaml=23
> >+
> >+title: Samsung Exynos Multi Format Codec (MFC)
> >+
> >+maintainers:
> >+  - Marek Szyprowski <m.szyprowski=40samsung.com>
> >+  - Smitha T Murthy <smitha.t=40samsung.com>
> >+  - Aakarsh Jain <aakarsh.jain=40samsung.com>
> >+
> >+description:
> >+  Multi Format Codec (MFC) is the IP present in Samsung SoCs which
> >+  supports high resolution decoding and encoding functionalities.
> >+
> >+properties:
> >+  compatible:
> >+    enum:
> >+      - samsung,mfc-v5                  =23 Exynos4
> >+      - samsung,mfc-v6                  =23 Exynos5
> >+      - samsung,mfc-v7                  =23 Exynos5420
> >+      - samsung,mfc-v8                  =23 Exynos5800
> >+      - samsung,exynos5433-mfc          =23 Exynos5433
> >+      - samsung,mfc-v10                 =23 Exynos7880
> >+
> >+  reg:
> >+    maxItems: 1
> >+
> >+  clocks:
> >+    minItems: 1
> >+    maxItems: 3
> >+
> >+  clock-names:
> >+    minItems: 1
> >+    maxItems: 3
> >+
> >+  interrupts:
> >+    maxItems: 1
> >+
> >+  iommus:
> >+    maxItems: 2
> >+
> >+  iommu-names:
> >+    maxItems: 2
> >+
> >+  power-domains:
> >+    maxItems: 1
> >+
> >+  memory-region:
> >+    maxItems: 1
> >+
> >+required:
> >+  - compatible
> >+  - reg
> >+  - clocks
> >+  - clock-names
> >+  - interrupts
> >+
> >+allOf:
> >+  - if:
> >+      properties:
> >+        compatible:
> >+          contains:
> >+            enum:
> >+              - samsung,mfc-v6
> >+              - samsung,mfc-v7
> >+              - samsung,mfc-v8
> >+              - tesla,fsd-mfc
> You have not introduce =22tesla,fsd-mfc=22 compatible yet, so this should=
 be part
> of the patch which adds fsd-mfc compatible
>=20
ok.
> >+    then:
> >+      properties:
> >+        clocks:
> >+          maxItems: 1
> >+        clock-names:
> >+          items:
> >+            - const: mfc
> >+
> >+  - if:
> >+      properties:
> >+        compatible:
> >+          contains:
> >+            enum:
> >+              - samsung,mfc-v5
> >+    then:
> >+      properties:
> >+        clocks:
> >+          minItems: 2
> >+          maxItems: 2
> >+        clock-names:
> >+          items:
> >+            - const: mfc
> >+            - const: sclk_mfc
> >+
> >+  - if:
> >+      properties:
> >+        compatible:
> >+          contains:
> >+            enum:
> >+              - samsung,exynos5433-mfc
> >+    then:
> >+      properties:
> >+        clocks:
> >+          minItems: 3
> >+          maxItems: 3
> >+        clock-names:
> >+          items:
> >+            - const: pclk
> >+            - const: aclk
> >+            - const: aclk_xiu
> >+
> >+  - if:
> >+      properties:
> >+        compatible:
> >+          contains:
> >+            enum:
> >+              - samsung,mfc-v5
> >+              - samsung,mfc-v6
> >+              - samsung,mfc-v7
> >+              - samsung,mfc-v8
> >+              - samsung,exynos5433-mfc
> >+
> >+    then:
> >+      properties:
> >+        iommus:
> >+          minItems: 2
> >+          maxItems: 2
> >+        iommu-names:
> >+          items:
> >+            - const: left
> >+            - const: right
> >+
> >+  - if:
> >+      properties:
> >+        compatible:
> >+          contains:
> >+            enum:
> >+              - tesla,fsd-mfc
>=20
> Same comments as above.
>=20
> My suggestion, first just convert the current s5p-mfc.txt to yaml format =
and
> then add fsd-mfc support.
> That way it will be easy for reviewer to understand the changes.
>=20
> While reviewing and code walk through, I also noticed that exynos3250.dts=
i
> and exynos5420.dtsi are using same compatible as =22samsung,mfc-v7=22 but
> there =22clocks=22 property is different. That doesn't look correct.
> This will cause issues when you convert s5p-mfc.txt to schema format.
> I don=E2=80=99t=20know=20why=20that=20was=20done=20that=20way,=20but=20su=
rely=20this=20need=20to=20be=20_fixed_=0D=0A>=20first=20as=20h/w=20itself=
=20are=20different.=0D=0A>=20=0D=0A>=20So=20my=20suggestion=20is=20to=20fix=
=20exynos3250.dtsi=20and=20exynos5420.dtsi=20compatible=0D=0A>=20and=20mfc=
=20driver=20for=20it=20first=20(as=20a=20separate=20patch=20series).=0D=0A>=
=20You=20can=20use=20the=20same=20approach=20what=20was=20done=20for=20mfc-=
v8=20and=20mfc-v8-5433.=0D=0A>=20=0D=0A>=20=0D=0Aok=20will=20fix=20this=20i=
ssue=20.=0D=0A=20=0D=0A>=20>+=20=20=20=20then:=0D=0A>=20>+=20=20=20=20=20=
=20properties:=0D=0A>=20>+=20=20=20=20=20=20=20=20memory-region:=0D=0A>=20>=
+=20=20=20=20=20=20=20=20=20=20maxItems:=201=0D=0A>=20>+=0D=0A>=20>+additio=
nalProperties:=20false=0D=0A>=20>+=0D=0A>=20>+examples:=0D=0A>=20>+=20=20-=
=20=7C=0D=0A>=20>+=20=20=20=20=23include=20<dt-bindings/clock/exynos4.h>=0D=
=0A>=20>+=20=20=20=20=23include=20<dt-bindings/clock/exynos-audss-clk.h>=0D=
=0A>=20>+=20=20=20=20=23include=20<dt-bindings/interrupt-controller/arm-gic=
.h>=0D=0A>=20>+=20=20=20=20=23include=20<dt-bindings/interrupt-controller/i=
rq.h>=0D=0A>=20>+=0D=0A>=20>+=20=20=20=20codec=4013400000=20=7B=0D=0A>=20>+=
=20=20=20=20=20=20=20=20=20=20compatible=20=3D=20=22samsung,mfc-v5=22;=0D=
=0A>=20>+=20=20=20=20=20=20=20=20=20=20reg=20=3D=20<0x13400000=200x10000>;=
=0D=0A>=20>+=20=20=20=20=20=20=20=20=20=20interrupts=20=3D=20<GIC_SPI=2094=
=20IRQ_TYPE_LEVEL_HIGH>;=0D=0A>=20>+=20=20=20=20=20=20=20=20=20=20power-dom=
ains=20=3D=20<&pd_mfc>;=0D=0A>=20>+=20=20=20=20=20=20=20=20=20=20clocks=20=
=3D=20<&clock=20CLK_MFC>,=20<&clock=20CLK_SCLK_MFC>;=0D=0A>=20>+=20=20=20=
=20=20=20=20=20=20=20clock-names=20=3D=20=22mfc=22,=20=22sclk_mfc=22;=0D=0A=
>=20>+=20=20=20=20=20=20=20=20=20=20iommus=20=3D=20<&sysmmu_mfc_l>,=20<&sys=
mmu_mfc_r>;=0D=0A>=20>+=20=20=20=20=20=20=20=20=20=20iommu-names=20=3D=20=
=22left=22,=20=22right=22;=0D=0A>=20>+=0D=0A>=20>+=20=20=20=20=7D;=0D=0A>=
=20>--=0D=0A>=20>2.17.1=0D=0A>=20=0D=0A>=20=0D=0A=0D=0AThanks=20for=20the=
=20review.=0D=0A=0D=0A=0D=0A