| Message ID | 201108201801.29541.heiko@sntech.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 ?)
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
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
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
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
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
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,
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(-)