2022-11-09 03:55:51

by Aakarsh Jain

[permalink] [raw]
Subject: [Patch v2 1/3] arm: exynos: Add new compatible string for Exynos3250 SoC.

Since,MFC v7 support was added for Exynos5420 and Exynos
3250 SoC with same compatible string "samsung,mfc-v7".As
both SoCs having different hardware properties and having
same compatible string for both SoCs doesn't seems to be correct.
New compatible is added for Exynos3250 SOC which will
differentiate the node properties for both SoCs which
support MFC v7.

Reviewed-by: Tommaso Merciai <[email protected]>
Suggested-by: Alim Akhtar <[email protected]>
Signed-off-by: Aakarsh Jain <[email protected]>
---
Documentation/devicetree/bindings/media/s5p-mfc.txt | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)

We are already in process of converting this txt file to yaml.
https://patchwork.kernel.org/project/linux-media/patch/[email protected]/
Modifying this txt binding for completeness.

diff --git a/Documentation/devicetree/bindings/media/s5p-mfc.txt b/Documentation/devicetree/bindings/media/s5p-mfc.txt
index aa54c8159d9f..cb166654fa81 100644
--- a/Documentation/devicetree/bindings/media/s5p-mfc.txt
+++ b/Documentation/devicetree/bindings/media/s5p-mfc.txt
@@ -10,10 +10,11 @@ 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
+ (c) "samsung,exynos3250-mfc" for MFC v7 present in Exynos3250 SoC
+ (d) "samsung,mfc-v7" for MFC v7 present in Exynos5420 SoC
+ (e) "samsung,mfc-v8" for MFC v8 present in Exynos5800 SoC
+ (f) "samsung,exynos5433-mfc" for MFC v8 present in Exynos5433 SoC
+ (g) "samsung,mfc-v10" for MFC v10 present in Exynos7880 SoC

- reg : Physical base address of the IP registers and length of memory
mapped region.
--
2.17.1



2022-11-09 04:16:58

by Aakarsh Jain

[permalink] [raw]
Subject: [Patch v2 3/3] arm: dts: exynos: Rename compatible string property from version to SoC specific

commit "752d3a23d1f68de87e3c" which adds MFC codec device node
for exynos3250 SoC. Since exynos3250.dtsi and exynos5420.dtsi are
using same compatible string as "samsung,mfc-v7" but their
node properties are different.As both SoCs have MFC v7 hardware
module but with different clock hierarchy and complexity.
So renaming compatible string from version specific to SoC based.

Reviewed-by: Tommaso Merciai <[email protected]>
Suggested-by: Alim Akhtar <[email protected]>
Signed-off-by: Aakarsh Jain <[email protected]>
---
arch/arm/boot/dts/exynos3250.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/exynos3250.dtsi b/arch/arm/boot/dts/exynos3250.dtsi
index 326b9e0ed8d3..98105c64f7d9 100644
--- a/arch/arm/boot/dts/exynos3250.dtsi
+++ b/arch/arm/boot/dts/exynos3250.dtsi
@@ -485,7 +485,7 @@
};

mfc: codec@13400000 {
- compatible = "samsung,mfc-v7";
+ compatible = "samsung,exynos3250-mfc";
reg = <0x13400000 0x10000>;
interrupts = <GIC_SPI 102 IRQ_TYPE_LEVEL_HIGH>;
clock-names = "mfc", "sclk_mfc";
--
2.17.1


2022-11-09 09:11:32

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [Patch v2 1/3] arm: exynos: Add new compatible string for Exynos3250 SoC.

On 09/11/2022 04:55, Aakarsh Jain wrote:
> Since,MFC v7 support was added for Exynos5420 and Exynos
> 3250 SoC with same compatible string "samsung,mfc-v7".As
> both SoCs having different hardware properties and having
> same compatible string for both SoCs doesn't seems to be correct.
> New compatible is added for Exynos3250 SOC which will
> differentiate the node properties for both SoCs which
> support MFC v7.
>
> Reviewed-by: Tommaso Merciai <[email protected]>
> Suggested-by: Alim Akhtar <[email protected]>
> Signed-off-by: Aakarsh Jain <[email protected]>
> ---
> Documentation/devicetree/bindings/media/s5p-mfc.txt | 9 +++++----

Use subject prefixes matching the subsystem (git log --oneline -- ...).

This is a friendly reminder during the review process.

It seems my previous comments were not fully addressed. Maybe my
feedback got lost between the quotes, maybe you just forgot to apply it.
Please go back to the previous discussion and either implement all
requested changes or keep discussing them.

Thank you.

Best regards,
Krzysztof


2022-11-09 09:22:28

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [Patch v2 1/3] arm: exynos: Add new compatible string for Exynos3250 SoC.

On 09/11/2022 04:55, Aakarsh Jain wrote:
> Since,MFC v7 support was added for Exynos5420 and Exynos
> 3250 SoC with same compatible string "samsung,mfc-v7".As
> both SoCs having different hardware properties and having
> same compatible string for both SoCs doesn't seems to be correct.
> New compatible is added for Exynos3250 SOC which will
> differentiate the node properties for both SoCs which
> support MFC v7.
>
> Reviewed-by: Tommaso Merciai <[email protected]>
> Suggested-by: Alim Akhtar <[email protected]>
> Signed-off-by: Aakarsh Jain <[email protected]>
> ---
> Documentation/devicetree/bindings/media/s5p-mfc.txt | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>

Beside my previous comment, please include changelog. This is v2, right?

You should consider joining something like:
https://www.linaro.org/events/member-training-upstream-kernel-development/

Best regards,
Krzysztof


2022-11-09 09:39:43

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [Patch v2 3/3] arm: dts: exynos: Rename compatible string property from version to SoC specific

On 09/11/2022 04:55, Aakarsh Jain wrote:
> commit "752d3a23d1f68de87e3c" which adds MFC codec device node
> for exynos3250 SoC. Since exynos3250.dtsi and exynos5420.dtsi are
> using same compatible string as "samsung,mfc-v7" but their
> node properties are different.As both SoCs have MFC v7 hardware
> module but with different clock hierarchy and complexity.
> So renaming compatible string from version specific to SoC based.
>
> Reviewed-by: Tommaso Merciai <[email protected]>
> Suggested-by: Alim Akhtar <[email protected]>
> Signed-off-by: Aakarsh Jain <[email protected]>

Use subject prefixes matching the subsystem (git log --oneline -- ...).

> ---
> arch/arm/boot/dts/exynos3250.dtsi | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/exynos3250.dtsi b/arch/arm/boot/dts/exynos3250.dtsi
> index 326b9e0ed8d3..98105c64f7d9 100644
> --- a/arch/arm/boot/dts/exynos3250.dtsi
> +++ b/arch/arm/boot/dts/exynos3250.dtsi
> @@ -485,7 +485,7 @@
> };
>
> mfc: codec@13400000 {
> - compatible = "samsung,mfc-v7";
> + compatible = "samsung,exynos3250-mfc";

This is a friendly reminder during the review process.

It seems my previous comments were not fully addressed. Maybe my
feedback got lost between the quotes, maybe you just forgot to apply it.
Please go back to the previous discussion and either implement all
requested changes or keep discussing them.

Thank you.

Best regards,
Best regards,
Krzysztof


2022-11-10 05:42:36

by Aakarsh Jain

[permalink] [raw]
Subject: RE: [Patch v2 1/3] arm: exynos: Add new compatible string for Exynos3250 SoC.



> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:[email protected]]
> Sent: 09 November 2022 14:31
> To: Aakarsh Jain <[email protected]>; linux-arm-
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [Patch v2 1/3] arm: exynos: Add new compatible string for
> Exynos3250 SoC.
>
> On 09/11/2022 04:55, Aakarsh Jain wrote:
> > Since,MFC v7 support was added for Exynos5420 and Exynos
> > 3250 SoC with same compatible string "samsung,mfc-v7".As both SoCs
> > having different hardware properties and having same compatible string
> > for both SoCs doesn't seems to be correct.
> > New compatible is added for Exynos3250 SOC which will differentiate
> > the node properties for both SoCs which support MFC v7.
> >
> > Reviewed-by: Tommaso Merciai
> <[email protected]>
> > Suggested-by: Alim Akhtar <[email protected]>
> > Signed-off-by: Aakarsh Jain <[email protected]>
> > ---
> > Documentation/devicetree/bindings/media/s5p-mfc.txt | 9 +++++----
>
> Use subject prefixes matching the subsystem (git log --oneline -- ...).
>
As with recent commits on Documentation/devicetree/bindings/media/s5p-mfc.txt with git log --oneline -- , subject prefix doesn't seems to be consistent.

b1394dc151cb media: s5p-mfc: Adding initial support for MFC v10.10
60641e22599a [media] s5p-mfc: Use preallocated block allocator always for MFC v6+
003611334d55 [media] s5p-mfc: Add support for MFC v8 available in Exynos 5433 SoCs
0da658704136 ARM: dts: convert to generic power domain bindings for exynos DT
77634289286a ARM: dts: Update clocks entry in MFC binding documentation
2eae613b95a7 ARM: EXYNOS: Add MFC device tree support

Closest is ARM: dts.
so what is your suggestion on this?

Anyway we are in a process of converting this txt file to yaml .


> This is a friendly reminder during the review process.
>
> It seems my previous comments were not fully addressed. Maybe my
> feedback got lost between the quotes, maybe you just forgot to apply it.
> Please go back to the previous discussion and either implement all requested
> changes or keep discussing them.
>
> Thank you.
>
> Best regards,
> Krzysztof

Thanks for the review.


2022-11-10 09:15:51

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [Patch v2 1/3] arm: exynos: Add new compatible string for Exynos3250 SoC.

On 09/11/2022 15:44, Aakarsh Jain wrote:
>
>
>> -----Original Message-----
>> From: Krzysztof Kozlowski [mailto:[email protected]]
>> Sent: 09 November 2022 14:31
>> To: Aakarsh Jain <[email protected]>; linux-arm-
>> [email protected]; [email protected]; linux-
>> [email protected]; [email protected]
>> Cc: [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]
>> Subject: Re: [Patch v2 1/3] arm: exynos: Add new compatible string for
>> Exynos3250 SoC.
>>
>> On 09/11/2022 04:55, Aakarsh Jain wrote:
>>> Since,MFC v7 support was added for Exynos5420 and Exynos
>>> 3250 SoC with same compatible string "samsung,mfc-v7".As both SoCs
>>> having different hardware properties and having same compatible string
>>> for both SoCs doesn't seems to be correct.
>>> New compatible is added for Exynos3250 SOC which will differentiate
>>> the node properties for both SoCs which support MFC v7.
>>>
>>> Reviewed-by: Tommaso Merciai
>> <[email protected]>
>>> Suggested-by: Alim Akhtar <[email protected]>
>>> Signed-off-by: Aakarsh Jain <[email protected]>
>>> ---
>>> Documentation/devicetree/bindings/media/s5p-mfc.txt | 9 +++++----
>>
>> Use subject prefixes matching the subsystem (git log --oneline -- ...).
>>
> As with recent commits on Documentation/devicetree/bindings/media/s5p-mfc.txt with git log --oneline -- , subject prefix doesn't seems to be consistent.
>
> b1394dc151cb media: s5p-mfc: Adding initial support for MFC v10.10
> 60641e22599a [media] s5p-mfc: Use preallocated block allocator always for MFC v6+
> 003611334d55 [media] s5p-mfc: Add support for MFC v8 available in Exynos 5433 SoCs
> 0da658704136 ARM: dts: convert to generic power domain bindings for exynos DT
> 77634289286a ARM: dts: Update clocks entry in MFC binding documentation
> 2eae613b95a7 ARM: EXYNOS: Add MFC device tree support

s5p-mfc is not a subsystem.

git log --oneline -- Documentation/devicetree/bindings/media/

media: dt-bindings: NAME_OF_FILE:


>
> Closest is ARM: dts.

This is not ARM subsystem and not a DTS file.

> so what is your suggestion on this?
>
> Anyway we are in a process of converting this txt file to yaml .
>

Best regards,
Krzysztof


2022-11-11 04:43:39

by Aakarsh Jain

[permalink] [raw]
Subject: RE: [Patch v2 1/3] arm: exynos: Add new compatible string for Exynos3250 SoC.



> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:[email protected]]
> Sent: 10 November 2022 13:56
> To: Aakarsh Jain <[email protected]>; 'Krzysztof Kozlowski'
> <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [Patch v2 1/3] arm: exynos: Add new compatible string for
> Exynos3250 SoC.
>
> On 09/11/2022 15:44, Aakarsh Jain wrote:
> >
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski [mailto:[email protected]]
> >> Sent: 09 November 2022 14:31
> >> To: Aakarsh Jain <[email protected]>; linux-arm-
> >> [email protected]; [email protected]; linux-
> >> [email protected]; [email protected]
> >> Cc: [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected]
> >> Subject: Re: [Patch v2 1/3] arm: exynos: Add new compatible string
> >> for
> >> Exynos3250 SoC.
> >>
> >> On 09/11/2022 04:55, Aakarsh Jain wrote:
> >>> Since,MFC v7 support was added for Exynos5420 and Exynos
> >>> 3250 SoC with same compatible string "samsung,mfc-v7".As both SoCs
> >>> having different hardware properties and having same compatible
> >>> string for both SoCs doesn't seems to be correct.
> >>> New compatible is added for Exynos3250 SOC which will differentiate
> >>> the node properties for both SoCs which support MFC v7.
> >>>
> >>> Reviewed-by: Tommaso Merciai
> >> <[email protected]>
> >>> Suggested-by: Alim Akhtar <[email protected]>
> >>> Signed-off-by: Aakarsh Jain <[email protected]>
> >>> ---
> >>> Documentation/devicetree/bindings/media/s5p-mfc.txt | 9 +++++----
> >>
> >> Use subject prefixes matching the subsystem (git log --oneline -- ...).
> >>
> > As with recent commits on
> Documentation/devicetree/bindings/media/s5p-mfc.txt with git log --
> oneline -- , subject prefix doesn't seems to be consistent.
> >
> > b1394dc151cb media: s5p-mfc: Adding initial support for MFC v10.10
> > 60641e22599a [media] s5p-mfc: Use preallocated block allocator always
> > for MFC v6+
> > 003611334d55 [media] s5p-mfc: Add support for MFC v8 available in
> > Exynos 5433 SoCs
> > 0da658704136 ARM: dts: convert to generic power domain bindings for
> > exynos DT 77634289286a ARM: dts: Update clocks entry in MFC binding
> > documentation
> > 2eae613b95a7 ARM: EXYNOS: Add MFC device tree support
>
> s5p-mfc is not a subsystem.
>
> git log --oneline -- Documentation/devicetree/bindings/media/
>
> media: dt-bindings: NAME_OF_FILE:
>
Thanks for clarification.
>
> >
> > Closest is ARM: dts.
>
> This is not ARM subsystem and not a DTS file.
>
> > so what is your suggestion on this?
> >
> > Anyway we are in a process of converting this txt file to yaml .
> >
>
> Best regards,
> Krzysztof



2022-11-11 04:44:31

by Aakarsh Jain

[permalink] [raw]
Subject: RE: [Patch v2 1/3] arm: exynos: Add new compatible string for Exynos3250 SoC.



> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:[email protected]]
> Sent: 09 November 2022 14:43
> To: Aakarsh Jain <[email protected]>; linux-arm-
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [Patch v2 1/3] arm: exynos: Add new compatible string for
> Exynos3250 SoC.
>
> On 09/11/2022 04:55, Aakarsh Jain wrote:
> > Since,MFC v7 support was added for Exynos5420 and Exynos
> > 3250 SoC with same compatible string "samsung,mfc-v7".As both SoCs
> > having different hardware properties and having same compatible string
> > for both SoCs doesn't seems to be correct.
> > New compatible is added for Exynos3250 SOC which will differentiate
> > the node properties for both SoCs which support MFC v7.
> >
> > Reviewed-by: Tommaso Merciai
> <[email protected]>
> > Suggested-by: Alim Akhtar <[email protected]>
> > Signed-off-by: Aakarsh Jain <[email protected]>
> > ---
> > Documentation/devicetree/bindings/media/s5p-mfc.txt | 9 +++++----
> > 1 file changed, 5 insertions(+), 4 deletions(-)
> >
>
> Beside my previous comment, please include changelog. This is v2, right?
>
Yes this is v2.
I have sent v3 along with cover letter.

> You should consider joining something like:
> https://protect2.fireeye.com/v1/url?k=bf47b121-dfa52c7c-bf463a6e-
> 000babd9f1ba-202e284cd3057a5d&q=1&e=009abfcf-7d3c-433c-8eea-
> 0f9d92caf6cc&u=https%3A%2F%2Fhttp://www.linaro.org%2Fevents%2Fmember-
> training-upstream-kernel-development%2F
>
> Best regards,
> Krzysztof

Thanks for review.