> -----Original Message----- > From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@xxxxxxxxxx] > Sent: Tuesday, May 17, 2022 7:26 PM > To: Smitha T Murthy <smitha.t@xxxxxxxxxxx>; linux-arm- > kernel@xxxxxxxxxxxxxxxxxxx; linux-media@xxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx > Cc: m.szyprowski@xxxxxxxxxxx; andrzej.hajda@xxxxxxxxx; > mchehab@xxxxxxxxxx; hverkuil-cisco@xxxxxxxxx; > ezequiel@xxxxxxxxxxxxxxxxxxxx; jernej.skrabec@xxxxxxxxx; > benjamin.gaignard@xxxxxxxxxxxxx; stanimir.varbanov@xxxxxxxxxx; > dillon.minfei@xxxxxxxxx; david.plowman@xxxxxxxxxxxxxxx; > mark.rutland@xxxxxxx; robh+dt@xxxxxxxxxx; krzk+dt@xxxxxxxxxx; > andi@xxxxxxxxxxx; alim.akhtar@xxxxxxxxxxx; aswani.reddy@xxxxxxxxxxx; > pankaj.dubey@xxxxxxxxxxx; linux-fsd@xxxxxxxxx > Subject: Re: [PATCH 02/20] dt-bindings: media: s5p-mfc: Convert s5p-mfc.txt > to new DT schema > > On 17/05/2022 14:55, Smitha T Murthy wrote: > > Adds DT schema for s5p-mfc in yaml format. > > > > Thank you for your patch. There is something to discuss/improve. > Thank you for the review. > > Cc: linux-fsd@xxxxxxxxx > > Signed-off-by: Smitha T Murthy <smitha.t@xxxxxxxxxxx> > > --- > > .../devicetree/bindings/media/s5p-mfc.txt | 77 +-------------- > > .../devicetree/bindings/media/s5p-mfc.yaml | 98 > +++++++++++++++++++ > > 2 files changed, 99 insertions(+), 76 deletions(-) create mode > > 100644 Documentation/devicetree/bindings/media/s5p-mfc.yaml > > > > diff --git a/Documentation/devicetree/bindings/media/s5p-mfc.txt > > b/Documentation/devicetree/bindings/media/s5p-mfc.txt > > index aa54c8159d9f..f00241ed407f 100644 > > --- a/Documentation/devicetree/bindings/media/s5p-mfc.txt > > +++ b/Documentation/devicetree/bindings/media/s5p-mfc.txt > > @@ -1,76 +1 @@ > > -* 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) "samsung,mfc-v5" for MFC v5 present in Exynos4 SoCs > > - (b) "samsung,mfc-v6" for MFC v6 present in Exynos5 SoCs > > - (c) "samsung,mfc-v7" for MFC v7 present in Exynos5420 SoC > > - (d) "samsung,mfc-v8" for MFC v8 present in Exynos5800 SoC > > - (e) "samsung,exynos5433-mfc" for MFC v8 present in Exynos5433 > SoC > > - (f) "samsung,mfc-v10" for MFC v10 present in Exynos7880 SoC > > - > > - - reg : Physical base address of the IP registers and length of memory > > - 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 "mfc", > > - 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 "left" mfc memory bus interfaces, > > - second if for the "right" 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@13400000 { > > - compatible = "samsung,mfc-v5"; > > - reg = <0x13400000 0x10000>; > > - interrupts = <0 94 0>; > > - power-domains = <&pd_mfc>; > > - clocks = <&clock 273>; > > - clock-names = "mfc"; > > -}; > > - > > -Reserved memory specific DT entry for given board (see reserved > > memory binding -for more information): > > - > > -reserved-memory { > > - #address-cells = <1>; > > - #size-cells = <1>; > > - ranges; > > - > > - mfc_left: region@51000000 { > > - compatible = "shared-dma-pool"; > > - no-map; > > - reg = <0x51000000 0x800000>; > > - }; > > - > > - mfc_right: region@43000000 { > > - compatible = "shared-dma-pool"; > > - no-map; > > - reg = <0x43000000 0x800000>; > > - }; > > -}; > > - > > -Board specific DT entry: > > - > > -codec@13400000 { > > - memory-region = <&mfc_left>, <&mfc_right>; > > -}; > > +This file has moved to s5p-mfc.yaml > > Instead entirely remove the file. > Ok, I will remove this file. > > diff --git a/Documentation/devicetree/bindings/media/s5p-mfc.yaml > > b/Documentation/devicetree/bindings/media/s5p-mfc.yaml > > new file mode 100644 > > index 000000000000..fff7c7e0d575 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/s5p-mfc.yaml > > @@ -0,0 +1,98 @@ > > +# SPDX-License-Identifier: GPL-2.0 > > +%YAML 1.2 > > +--- > > +$id: > > +https://protect2.fireeye.com/v1/url?k=8b32845f-ea492ed7-8b330f10- > 74fe > > +4860018a-302429095d2fce6e&q=1&e=73087855-3649-4be8-a878- > d487e0ae58f4& > > +u=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fmedia%2Fs5p- > mfc.yaml%23 > > Let's convert the name as well, so "samsung,s5p-mfc.yaml" > Ok, I will change the name in next series. > > +$schema: > > +https://protect2.fireeye.com/v1/url?k=a01c09cc-c167a344-a01d8283-74fe > > +4860018a-b8e3ca9e8a791d49&q=1&e=73087855-3649-4be8-a878- > d487e0ae58f4& > > +u=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23 > > + > > +title: Samsung Exynos Multi Format Codec (MFC) > > + > > +maintainers: > > + - Mauro Carvalho Chehab <mchehab@xxxxxxxxxx> > > + - Rob Herring <robh+dt@xxxxxxxxxx> > > + - Mark Rutland <mark.rutland@xxxxxxx> > > + - Smitha T Murthy <smitha.t@xxxxxxxxxxx> > > Only people with access to HW, so you can put here Marek and yourself. > Ok, I will change the authors list. > > + > > +properties: > > + compatible: > > + enum: > > + - samsung,mfc-v5 # Exynos4 > > + - samsung,mfc-v6 # Exynos5 > > + - samsung,mfc-v7 # Exynos5420 > > + - samsung,mfc-v8 # Exynos5800 > > + - samsung,exynos5433-mfc # Exynos5433 > > + - samsung,mfc-v10 # Exynos7880 > > Ugh, how MFCv10 appeared here? Since 5433 we moved from versions to > Soc compatibles as recommended... eh, please follow this convention, don't > reverse it to other way. > > I propose to deprecated this in next patch and instead use SoC-based > compatible. > MFCv10 was already mainlined as mfc-v10, maybe for v10 I will add it post this series. For MFCv12 I will add SoC-based in the next series. > > + > > + reg: > > + maxItems: 1 > > + > > + clocks: > > + description: > > + Phandle to MFC IP clock. > > Here and other places: s/Phandle// > Instead describe what is it, e.g. "MFC IP clock" > > Ok noted. > > + maxItems: 1 > > + > > + clock-names: > > + description: > > + Must contain clock name (mfc) matching phandle in clocks > > + property. > > Skip description, its obvious. Instead list the items. > Ok > > + maxItems: 1 > > No need, list the items. > > > + > > + interrupts: > > + description: > > + MFC interrupt number to the CPU. > > Skip description, it's obvious. > Ok > > + maxItems: 1 > > + > > + memory-region: > > + description: > > + From reserved memory binding phandles to two reserved > > + memory regions, first is for "left" mfc memory bus interfaces, > > + second if for the "right" mfc memory bus, used when no SYSMMU > > + support is available; used only by MFC v5 present in Exynos4 SoCs. > > + minItems: 1 > > + maxItems: 2 > > This needs allOf:if:then restricting two items to specific compatible. > Ok, I will make this change. > > + > > + iommus: > > + description: > > + Include the IOMMU domain MFC belong to. > > Skip description, it's obvious. > Ok > > + maxItems: 2 > > + > > What happened to power domains? You also removed them from the > example... Does this pass dtbs_check? > This file passed the dtbs_check. For MFCv10 and v12 power domains are not required. I will add in the example. > > +required: > > + - compatible > > + - reg > > + - clocks > > + - clock-names > > + - interrupts > > + - iommus > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + /* Reserved memory specific DT entry for given board */ > > + reserved-memory { > > Wrong indentation. Four spaces. See example schema. > Ok > > + #address-cells = <1>; > > + #size-cells = <1>; > > + ranges; > > + > > + mfc_left: region@84000000 { > > + compatible = "shared-dma-pool"; > > + no-map; > > + reg = <0x84000000 0x800000>; > > + }; > > + > > + mfc_right: region@A9000000 { > > lower case hex addresses, everywhere. > Ok > > + compatible = "shared-dma-pool"; > > + no-map; > > + reg = <0xA9000000 0x800000>; > > + }; > > + }; > > + > > + mfc_0: mfc0@12880000 { > > Generic node names, so mfc. > Ok > > + compatible = "samsung,mfc-v12"; > > Does not look like you tested the bindings. Please run `make > dt_binding_check` (see Documentation/devicetree/bindings/writing- > schema.rst for instructions). > Be sure to test your bindings before sending them. > I did do make dtbs and dt_binding_check using v2022.3, I will recheck post these changes. > > + reg = <0x12880000 0x10000>; > > + clock-names = "mfc"; > > + interrupts = <0 137 4>; > > Use interrupt defines. > When I use interrupt defines I get errors as "1. Syntax error: This was due to interrupts field has some macro reference and needed to give absolute value.", hence I gave absolute values. Regards, Smitha > > + clocks = <&clock_mfc 1>; > > + memory-region = <&mfc_left>, <&mfc_right>; > > + /* If IOMMU is present use below instead of memory-region > property */ > > + iommus = <&smmu_isp 0x1000 0x0>, <&smmu_isp 0x1400 0x0>; > > + }; > > > Best regards, > Krzysztof
