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

[Kukjin Kim <kgene.kim@samsung.com>]

Pawel Osciak wrote:
> 
> Hello,
> 
> >Kukjin Kim <kgene.kim@samsung.com> wrote:
> >Sylwester Nawrocki wrote:
> >>
> >Hi Sylwester,
> >
> >Hmm...no comments here?
> >
> >> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >> Signed-off-by: Pawel Osciak <p.osciak@samsung.com>
> >
> >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.
> 
...

> >> ---
> >>  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.

> >> +	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
> >> 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

> >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?
> 
> >> @@ -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?

> >> +	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.
> 
> >> +			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...

> >> +		}
> >> +	}
> >> +	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.

> >> --
> >
> >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 <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.