[PATCH v3 4/8] ARM: Samsung: Add common Aquila and GONI code

Pawel Osciak wrote:

Hello,

quoted

Kukjin Kim [off-list ref] wrote:
Sylwester Nawrocki wrote:

quoted

Hi Sylwester,

Hmm...no comments here?

quoted

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Pawel Osciak <redacted>

I wonder we need really above multiple sign for this, just only one c file?

Sylwester is the main author, I am also the author of this, although I agree
it is a small patch. Mr. Park is here as the person that gives the permission
for the patches to be published under GPL.

...

quoted

quoted

---
 arch/arm/mach-s5pv210/Kconfig              |    5 ++++
 arch/arm/mach-s5pv210/Makefile             |    2 +
 arch/arm/mach-s5pv210/common-aquila-goni.c |   35
++++++++++++++++++++++++++++
 arch/arm/mach-s5pv210/common-aquila-goni.h |    2 +
 4 files changed, 44 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-s5pv210/common-aquila-goni.c
 create mode 100644 arch/arm/mach-s5pv210/common-aquila-goni.h

diff --git a/arch/arm/mach-s5pv210/Kconfig b/arch/arm/mach-s5pv210/Kconfig
index 12a2c6b..039ba8c 100644
--- a/arch/arm/mach-s5pv210/Kconfig
+++ b/arch/arm/mach-s5pv210/Kconfig

@@ -90,6 +90,11 @@ config MACH_SMDKC110

 endmenu

+config COMMON_AQUILA_GONI

do we _really_ need this?

The alternative is to have identical code in both Aquila and Goni files.

I mean this is just for fimc clock setting...this is not only for Auqila and GONI machines even though need to update to be used in other machines.

quoted

quoted

+	bool
+	help
+	  Compile common code for Samsung Aquila and Samsung GONI
machines
+
 config S5PC110_DEV_ONENAND
 	bool
 	help

diff --git a/arch/arm/mach-s5pv210/Makefile

b/arch/arm/mach-s5pv210/Makefile

quoted

index 05048c5..a4c0bc9 100644

--- a/arch/arm/mach-s5pv210/Makefile
+++ b/arch/arm/mach-s5pv210/Makefile

@@ -22,6 +22,8 @@ obj-$(CONFIG_MACH_SMDKV210)	+= mach-

smdkv210.o
 obj-$(CONFIG_MACH_SMDKC110)	+= mach-smdkc110.o
 obj-$(CONFIG_MACH_GONI)		+= mach-goni.o

+obj-$(CONFIG_COMMON_AQUILA_GONI)	+= common-aquila-goni.o
+
 # device support

 obj-y				+= dev-audio.o

diff --git a/arch/arm/mach-s5pv210/common-aquila-goni.c b/arch/arm/mach-

s5pv210/common-aquila-goni.c
new file mode 100644
index 0000000..4d72531

--- /dev/null
+++ b/arch/arm/mach-s5pv210/common-aquila-goni.c

Just setup-fimc.c is enough...
I don't know why we need common-aquila-goni now.

The alternative was to have identical clock setup code in both mach-aquila
and mach-goni files.

It's possible in setup-fimc.c

quoted

And your following patches in this patch-set perfectly same except just
name.
[PATCH v3 5/8] ARM: s5pv210: enable FIMC on Aquila
[PATCH v3 6/8] ARM: s5pv210: enable FIMC on Goni

Hmm...

One is for Aquila and one is for Goni, sorry, I don't understand what do
you mean here... Would you prefer them to be combined into one patch?

quoted

quoted

@@ -0,0 +1,35 @@
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <plat/fimc.h>
+
+#include "common-aquila-goni.h"
+
+void __init s5pv210_common_fimc_clk_init(void)
+{
+	int i;
+	struct clk *clk_fimc, *parent;
+
+	struct device *fimc_devs[] = {
+		&s5p_device_fimc0.dev,
+		&s5p_device_fimc1.dev,
+		&s5p_device_fimc2.dev
+	};
+
+	parent = clk_get(NULL, "mout_epll");

Is not there in case of using 'mout_mpll' as parent clock?

quoted

quoted

+	if (IS_ERR(parent))
+		return;
+
+	for (i = 0; i < ARRAY_SIZE(fimc_devs); i++) {
+		if (fimc_devs[i]) {
+			clk_fimc = clk_get(fimc_devs[i], "sclk_fimc");
+
+			if (IS_ERR(clk_fimc))
+				continue;
+
+			clk_set_parent(clk_fimc, parent);
+			clk_set_rate(clk_fimc, 133000000);

Is this for direct fifo mode?

Bootloader is not setting up clocks correctly and defaults do not work.
It's not only for fifo, fimc doesn't work at all without this.

quoted

quoted

+			clk_enable(clk_fimc);

Should being in driver...I don't know why we should enable fimc clock in here.
Basically, each driver clock should be enabled in each drivers...

quoted

quoted

+		}
+	}
+	clk_put(parent);
+}

diff --git a/arch/arm/mach-s5pv210/common-aquila-goni.h b/arch/arm/mach-

s5pv210/common-aquila-goni.h
new file mode 100644
index 0000000..f666462

--- /dev/null
+++ b/arch/arm/mach-s5pv210/common-aquila-goni.h

@@ -0,0 +1,2 @@
+
+extern void s5pv210_common_fimc_clk_init(void);

If this is only for Aquila and GONI, should be common_aquila_goni_fimc_clk_init().
But it's quite long :-( ... anyway, as I said, I don't think this is only for Aquila and GONI.

quoted

quoted

--

I do not think that we need common-aquila-goni.[ch] now to us.

What would you propose as the alternative? Have duplicate code in both machine
files?

setup-fimc.c, but need to sort it out so that all machines can use that.

Thanks.

Best regards,
Kgene.
--
Kukjin Kim [off-list ref], Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.