[1/4] S3C2443: Move i2s clock definitions to common code
diff mbox

Message ID 201108201801.29541.heiko@sntech.de
State New, archived
Headers show

Commit Message

Heiko Stuebner Aug. 20, 2011, 4:01 p.m. UTC
S3C2416/S3C2450 use the same clocks for their i2s blocks and can therefore
reuse the existing ones.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 arch/arm/mach-s3c2443/clock.c         |   53 ---------------------------------
 arch/arm/plat-s3c24xx/s3c2443-clock.c |   53 +++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 53 deletions(-)

Comments

Russell King - ARM Linux Aug. 21, 2011, 5:13 p.m. UTC | #1
On Sat, Aug 20, 2011 at 06:01:29PM +0200, Heiko Stübner wrote:
> +/* i2s-ref
> + *
> + * i2s bus reference clock, selectable from external, esysclk or epllref
> + *
> + * Note, this used to be two clocks, but was compressed into one.
> +*/
> +
> +struct clk *clk_i2s_srclist[] = {
> +	[0] = &clk_i2s_eplldiv.clk,
> +	[1] = &clk_i2s_ext,
> +	[2] = &clk_epllref.clk,
> +	[3] = &clk_epllref.clk,
> +};

Is there any reason not to make this static (have you run your patch
through checkpatch.pl ?)
Heiko Stuebner Aug. 21, 2011, 5:25 p.m. UTC | #2
Am Sonntag 21 August 2011, 19:13:32 schrieb Russell King - ARM Linux:
> On Sat, Aug 20, 2011 at 06:01:29PM +0200, Heiko Stübner wrote:
> > +/* i2s-ref
> > + *
> > + * i2s bus reference clock, selectable from external, esysclk or epllref
> > + *
> > + * Note, this used to be two clocks, but was compressed into one.
> > +*/
> > +
> > +struct clk *clk_i2s_srclist[] = {
> > +	[0] = &clk_i2s_eplldiv.clk,
> > +	[1] = &clk_i2s_ext,
> > +	[2] = &clk_epllref.clk,
> > +	[3] = &clk_epllref.clk,
> > +};
> 
> Is there any reason not to make this static (have you run your patch
> through checkpatch.pl ?)
Yep I did run all of them through checkpatch (after beeing scolded last time) 
and it didn't report anything.

But for this move of code I simply grabbed the code fragments and put them 
into their new location (i.e. it was this way in mach-s3c2443/clock.c) and 
should have probably taken a closer look at what I'm moving.

So it seems you are right, it should probably be static as everything else is 
also static.

Heiko
Marek Vasut Aug. 21, 2011, 8:26 p.m. UTC | #3
On Sunday, August 21, 2011 07:25:04 PM Heiko Stübner wrote:
> Am Sonntag 21 August 2011, 19:13:32 schrieb Russell King - ARM Linux:
> > On Sat, Aug 20, 2011 at 06:01:29PM +0200, Heiko Stübner wrote:
> > > +/* i2s-ref
> > > + *
> > > + * i2s bus reference clock, selectable from external, esysclk or
> > > epllref + *
> > > + * Note, this used to be two clocks, but was compressed into one.
> > > +*/
> > > +
> > > +struct clk *clk_i2s_srclist[] = {
> > > +	[0] = &clk_i2s_eplldiv.clk,
> > > +	[1] = &clk_i2s_ext,
> > > +	[2] = &clk_epllref.clk,
> > > +	[3] = &clk_epllref.clk,
> > > +};
> > 
> > Is there any reason not to make this static (have you run your patch
> > through checkpatch.pl ?)
> 
> Yep I did run all of them through checkpatch (after beeing scolded last
> time) and it didn't report anything.
> 
> But for this move of code I simply grabbed the code fragments and put them
> into their new location (i.e. it was this way in mach-s3c2443/clock.c) and
> should have probably taken a closer look at what I'm moving.
> 
> So it seems you are right, it should probably be static as everything else
> is also static.

And you don't really understand why everything else is static, right ? ;-)

(don't take it as an offense)
> 
> Heiko
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Heiko Stuebner Aug. 21, 2011, 9:11 p.m. UTC | #4
Am Sonntag 21 August 2011, 22:26:16 schrieb Marek Vasut:
> On Sunday, August 21, 2011 07:25:04 PM Heiko Stübner wrote:
> > Am Sonntag 21 August 2011, 19:13:32 schrieb Russell King - ARM Linux:
> > > On Sat, Aug 20, 2011 at 06:01:29PM +0200, Heiko Stübner wrote:
> > > > +/* i2s-ref
> > > > + *
> > > > + * i2s bus reference clock, selectable from external, esysclk or
> > > > epllref + *
> > > > + * Note, this used to be two clocks, but was compressed into one.
> > > > +*/
> > > > +
> > > > +struct clk *clk_i2s_srclist[] = {
> > > > +	[0] = &clk_i2s_eplldiv.clk,
> > > > +	[1] = &clk_i2s_ext,
> > > > +	[2] = &clk_epllref.clk,
> > > > +	[3] = &clk_epllref.clk,
> > > > +};
> > > 
> > > Is there any reason not to make this static (have you run your patch
> > > through checkpatch.pl ?)
> > 
> > Yep I did run all of them through checkpatch (after beeing scolded last
> > time) and it didn't report anything.
> > 
> > But for this move of code I simply grabbed the code fragments and put
> > them into their new location (i.e. it was this way in
> > mach-s3c2443/clock.c) and should have probably taken a closer look at
> > what I'm moving.
> > 
> > So it seems you are right, it should probably be static as everything
> > else is also static.
> 
> And you don't really understand why everything else is static, right ? ;-)
yep :-) - until now

But a quick lookup did help [lifetime of the variable extends across the 
entire run of the program, i.e. a global variable and local to the source file 
it's defined in]. 

> (don't take it as an offense)
none taken ... in fact it was a welcome push to reread variable storage 
classes and lifetime again [last time was some years ago] :-).

Heiko
Kim Kukjin Aug. 23, 2011, 3:42 a.m. UTC | #5
Heiko Stübner wrote:
> 
> Am Sonntag 21 August 2011, 19:13:32 schrieb Russell King - ARM Linux:
> > On Sat, Aug 20, 2011 at 06:01:29PM +0200, Heiko Stübner wrote:
> > > +/* i2s-ref
> > > + *
> > > + * i2s bus reference clock, selectable from external, esysclk or
epllref
> > > + *
> > > + * Note, this used to be two clocks, but was compressed into one.
> > > +*/
> > > +
> > > +struct clk *clk_i2s_srclist[] = {
> > > +	[0] = &clk_i2s_eplldiv.clk,
> > > +	[1] = &clk_i2s_ext,
> > > +	[2] = &clk_epllref.clk,
> > > +	[3] = &clk_epllref.clk,
> > > +};
> >
> > Is there any reason not to make this static (have you run your patch
> > through checkpatch.pl ?)
> Yep I did run all of them through checkpatch (after beeing scolded last
time)
> and it didn't report anything.
> 

Hmm...as a note, happened following with checkpatch.pl :(

ERROR: need consistent spacing around '*' (ctx:WxV)
#35: FILE: arch/arm/mach-s3c2416/clock.c:57:
+               .sources = (struct clk *[]) {
                                       ^

total: 1 errors, 0 warnings, 38 lines checked

PATCH 34 S3C2416 Add HSSPI clock sourced from EPLL.txt has style problems,
please review.

Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.


> But for this move of code I simply grabbed the code fragments and put them
> into their new location (i.e. it was this way in mach-s3c2443/clock.c) and
> should have probably taken a closer look at what I'm moving.
> 
> So it seems you are right, it should probably be static as everything else
is
> also static.
> 
> Heiko
Heiko Stuebner Aug. 23, 2011, 8:28 p.m. UTC | #6
Am Dienstag 23 August 2011, 05:42:47 schrieb Kukjin Kim:
> Heiko Stübner wrote:
> > Am Sonntag 21 August 2011, 19:13:32 schrieb Russell King - ARM Linux:
> > > On Sat, Aug 20, 2011 at 06:01:29PM +0200, Heiko Stübner wrote:
> > > > +/* i2s-ref
> > > > + *
> > > > + * i2s bus reference clock, selectable from external, esysclk or
> 
> epllref
> 
> > > > + *
> > > > + * Note, this used to be two clocks, but was compressed into one.
> > > > +*/
> > > > +
> > > > +struct clk *clk_i2s_srclist[] = {
> > > > +	[0] = &clk_i2s_eplldiv.clk,
> > > > +	[1] = &clk_i2s_ext,
> > > > +	[2] = &clk_epllref.clk,
> > > > +	[3] = &clk_epllref.clk,
> > > > +};
> > > 
> > > Is there any reason not to make this static (have you run your patch
> > > through checkpatch.pl ?)
> > 
> > Yep I did run all of them through checkpatch (after beeing scolded last
> 
> time)
> 
> > and it didn't report anything.
> 
> Hmm...as a note, happened following with checkpatch.pl :(
> 
> ERROR: need consistent spacing around '*' (ctx:WxV)
> #35: FILE: arch/arm/mach-s3c2416/clock.c:57:
> +               .sources = (struct clk *[]) {
>                                        ^
> 
> total: 1 errors, 0 warnings, 38 lines checked
> 
> PATCH 34 S3C2416 Add HSSPI clock sourced from EPLL.txt has style problems,
> please review.
will do.

Today I also saw, that my definition of the hsspi pclk would have caused a 
conflict with the hsspi sclk definition of s3c2443.

So, as nobody uses the s3c2443's hsspi controller yet, I would also rename it 
to hsspi-if with the next patch iteration.


Heiko

Patch
diff mbox

diff --git a/arch/arm/mach-s3c2443/clock.c b/arch/arm/mach-s3c2443/clock.c
index a1a7176..966bde5 100644
--- a/arch/arm/mach-s3c2443/clock.c
+++ b/arch/arm/mach-s3c2443/clock.c
@@ -57,10 +57,6 @@ 
 
 /* clock selections */
 
-static struct clk clk_i2s_ext = {
-	.name		= "i2s-ext",
-};
-
 /* armdiv
  *
  * this clock is sourced from msysclk and can have a number of
@@ -235,48 +231,6 @@  static struct clk clk_hsmmc = {
 	},
 };
 
-/* i2s_eplldiv
- *
- * This clock is the output from the I2S divisor of ESYSCLK, and is separate
- * from the mux that comes after it (cannot merge into one single clock)
-*/
-
-static struct clksrc_clk clk_i2s_eplldiv = {
-	.clk	= {
-		.name		= "i2s-eplldiv",
-		.parent		= &clk_esysclk.clk,
-	},
-	.reg_div = { .reg = S3C2443_CLKDIV1, .size = 4, .shift = 12, },
-};
-
-/* i2s-ref
- *
- * i2s bus reference clock, selectable from external, esysclk or epllref
- *
- * Note, this used to be two clocks, but was compressed into one.
-*/
-
-struct clk *clk_i2s_srclist[] = {
-	[0] = &clk_i2s_eplldiv.clk,
-	[1] = &clk_i2s_ext,
-	[2] = &clk_epllref.clk,
-	[3] = &clk_epllref.clk,
-};
-
-static struct clksrc_clk clk_i2s = {
-	.clk	= {
-		.name		= "i2s-if",
-		.ctrlbit	= S3C2443_SCLKCON_I2SCLK,
-		.enable		= s3c2443_clkcon_enable_s,
-
-	},
-	.sources = &(struct clksrc_sources) {
-		.sources = clk_i2s_srclist,
-		.nr_sources = ARRAY_SIZE(clk_i2s_srclist),
-	},
-	.reg_src = { .reg = S3C2443_CLKSRC, .size = 2, .shift = 14 },
-};
-
 /* standard clock definitions */
 
 static struct clk init_clocks_off[] = {
@@ -286,11 +240,6 @@  static struct clk init_clocks_off[] = {
 		.enable		= s3c2443_clkcon_enable_p,
 		.ctrlbit	= S3C2443_PCLKCON_SDI,
 	}, {
-		.name		= "iis",
-		.parent		= &clk_p,
-		.enable		= s3c2443_clkcon_enable_p,
-		.ctrlbit	= S3C2443_PCLKCON_IIS,
-	}, {
 		.name		= "spi",
 		.devname	= "s3c2410-spi.0",
 		.parent		= &clk_p,
@@ -312,8 +261,6 @@  static struct clk init_clocks[] = {
 
 static struct clksrc_clk *clksrcs[] __initdata = {
 	&clk_arm,
-	&clk_i2s_eplldiv,
-	&clk_i2s,
 	&clk_hsspi,
 	&clk_hsmmc_div,
 };
diff --git a/arch/arm/plat-s3c24xx/s3c2443-clock.c b/arch/arm/plat-s3c24xx/s3c2443-clock.c
index 59552c0..e2807d9 100644
--- a/arch/arm/plat-s3c24xx/s3c2443-clock.c
+++ b/arch/arm/plat-s3c24xx/s3c2443-clock.c
@@ -205,9 +205,60 @@  static struct clksrc_clk clksrc_clks[] = {
 	},
 };
 
+static struct clk clk_i2s_ext = {
+	.name		= "i2s-ext",
+};
+
+/* i2s_eplldiv
+ *
+ * This clock is the output from the I2S divisor of ESYSCLK, and is separate
+ * from the mux that comes after it (cannot merge into one single clock)
+*/
+
+static struct clksrc_clk clk_i2s_eplldiv = {
+	.clk	= {
+		.name		= "i2s-eplldiv",
+		.parent		= &clk_esysclk.clk,
+	},
+	.reg_div = { .reg = S3C2443_CLKDIV1, .size = 4, .shift = 12, },
+};
+
+/* i2s-ref
+ *
+ * i2s bus reference clock, selectable from external, esysclk or epllref
+ *
+ * Note, this used to be two clocks, but was compressed into one.
+*/
+
+struct clk *clk_i2s_srclist[] = {
+	[0] = &clk_i2s_eplldiv.clk,
+	[1] = &clk_i2s_ext,
+	[2] = &clk_epllref.clk,
+	[3] = &clk_epllref.clk,
+};
+
+
+static struct clksrc_clk clk_i2s = {
+	.clk	= {
+		.name		= "i2s-if",
+		.ctrlbit	= S3C2443_SCLKCON_I2SCLK,
+		.enable		= s3c2443_clkcon_enable_s,
+
+	},
+	.sources = &(struct clksrc_sources) {
+		.sources = clk_i2s_srclist,
+		.nr_sources = ARRAY_SIZE(clk_i2s_srclist),
+	},
+	.reg_src = { .reg = S3C2443_CLKSRC, .size = 2, .shift = 14 },
+};
 
 static struct clk init_clocks_off[] = {
 	{
+		.name		= "iis",
+		.parent		= &clk_p,
+		.enable		= s3c2443_clkcon_enable_p,
+		.ctrlbit	= S3C2443_PCLKCON_IIS,
+	}, {
 		.name		= "adc",
 		.parent		= &clk_p,
 		.enable		= s3c2443_clkcon_enable_p,
@@ -406,6 +457,8 @@  static struct clk *clks[] __initdata = {
 };
 
 static struct clksrc_clk *clksrcs[] __initdata = {
+	&clk_i2s_eplldiv,
+	&clk_i2s,
 	&clk_usb_bus_host,
 	&clk_epllref,
 	&clk_esysclk,