i2c: s3c2410: dont need CPU_FREQ transitions for exynos series

Submitted by Naveen Krishna Ch on Oct. 11, 2013, 11:26 a.m.

Details

Message ID 1381490814-20890-1-git-send-email-ch.naveen@samsung.com
State Superseded
Headers show

Commit Message

Naveen Krishna Ch Oct. 11, 2013, 11:26 a.m.
The exynos5 i2c clock is based on a fixed 66 MHz peripheral clock, and
therefore is completely independent of the cpu frequency.
Thus, registering for a CPU freq notifier is very wasteful.

This patch modifes the code such that, i2c bus registers to
cpu_freq_transition only for non Exynos SoCs.

This change should save a bunch of cpufreq transitions calls
which does not apply to exynos SoCs.

Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
---
 drivers/i2c/busses/i2c-s3c2410.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Tomasz Figa Oct. 12, 2013, 2:22 a.m.
Hi Naveen,

On Friday 11 of October 2013 16:56:54 Naveen Krishna Chatradhi wrote:
> The exynos5 i2c clock is based on a fixed 66 MHz peripheral clock, and
> therefore is completely independent of the cpu frequency.
> Thus, registering for a CPU freq notifier is very wasteful.
> 
> This patch modifes the code such that, i2c bus registers to
> cpu_freq_transition only for non Exynos SoCs.
> 
> This change should save a bunch of cpufreq transitions calls
> which does not apply to exynos SoCs.

The idea is fine, although...

> Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
> ---
>  drivers/i2c/busses/i2c-s3c2410.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
> index cab1c91..d062fa7 100644
> --- a/drivers/i2c/busses/i2c-s3c2410.c
> +++ b/drivers/i2c/busses/i2c-s3c2410.c
> @@ -123,7 +123,7 @@ struct s3c24xx_i2c {
>  	struct s3c2410_platform_i2c	*pdata;
>  	int			gpios[2];
>  	struct pinctrl          *pctrl;
> -#ifdef CONFIG_CPU_FREQ
> +#if defined(CONFIG_CPU_FREQ) && !defined(CONFIG_ARCH_EXYNOS)

...this is not a good coding practice, especially when already having
multiplatform kernels in sight.

The best way would be to check on which SoC we are running at runtime,
but since this might need changing a lot of code, then at least I would
change this from !defined(EXYNOS) to defined(S3C24XX), so it is not being
compiled in when S3C24XX support is not enabled and if it's enabled then
the notifier will be registered as a safe fallback that will run correctly
on all platforms.

Best regards,
Tomasz

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomasz Figa Oct. 12, 2013, 2:28 a.m.
[Fixing incorrent mail addresses and dropping the old DT ML.]

On Saturday 12 of October 2013 04:22:04 Tomasz Figa wrote:
> Hi Naveen,
> 
> On Friday 11 of October 2013 16:56:54 Naveen Krishna Chatradhi wrote:
> > The exynos5 i2c clock is based on a fixed 66 MHz peripheral clock, and
> > therefore is completely independent of the cpu frequency.
> > Thus, registering for a CPU freq notifier is very wasteful.
> > 
> > This patch modifes the code such that, i2c bus registers to
> > cpu_freq_transition only for non Exynos SoCs.
> > 
> > This change should save a bunch of cpufreq transitions calls
> > which does not apply to exynos SoCs.
> 
> The idea is fine, although...
> 
> > Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
> > ---
> >  drivers/i2c/busses/i2c-s3c2410.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
> > index cab1c91..d062fa7 100644
> > --- a/drivers/i2c/busses/i2c-s3c2410.c
> > +++ b/drivers/i2c/busses/i2c-s3c2410.c
> > @@ -123,7 +123,7 @@ struct s3c24xx_i2c {
> >  	struct s3c2410_platform_i2c	*pdata;
> >  	int			gpios[2];
> >  	struct pinctrl          *pctrl;
> > -#ifdef CONFIG_CPU_FREQ
> > +#if defined(CONFIG_CPU_FREQ) && !defined(CONFIG_ARCH_EXYNOS)
> 
> ...this is not a good coding practice, especially when already having
> multiplatform kernels in sight.
> 
> The best way would be to check on which SoC we are running at runtime,
> but since this might need changing a lot of code, then at least I would
> change this from !defined(EXYNOS) to defined(S3C24XX), so it is not being
> compiled in when S3C24XX support is not enabled and if it's enabled then
> the notifier will be registered as a safe fallback that will run correctly
> on all platforms.
> 
> Best regards,
> Tomasz
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomasz Figa Oct. 12, 2013, 5:42 a.m.
On Saturday 12 of October 2013 04:28:51 Tomasz Figa wrote:
> [Fixing incorrent mail addresses and dropping the old DT ML.]
> 
> On Saturday 12 of October 2013 04:22:04 Tomasz Figa wrote:
> > Hi Naveen,
> > 
> > On Friday 11 of October 2013 16:56:54 Naveen Krishna Chatradhi wrote:
> > > The exynos5 i2c clock is based on a fixed 66 MHz peripheral clock, and
> > > therefore is completely independent of the cpu frequency.
> > > Thus, registering for a CPU freq notifier is very wasteful.
> > > 
> > > This patch modifes the code such that, i2c bus registers to
> > > cpu_freq_transition only for non Exynos SoCs.
> > > 
> > > This change should save a bunch of cpufreq transitions calls
> > > which does not apply to exynos SoCs.
> > 
> > The idea is fine, although...
> > 
> > > Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
> > > ---
> > >  drivers/i2c/busses/i2c-s3c2410.c |    4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
> > > index cab1c91..d062fa7 100644
> > > --- a/drivers/i2c/busses/i2c-s3c2410.c
> > > +++ b/drivers/i2c/busses/i2c-s3c2410.c
> > > @@ -123,7 +123,7 @@ struct s3c24xx_i2c {
> > >  	struct s3c2410_platform_i2c	*pdata;
> > >  	int			gpios[2];
> > >  	struct pinctrl          *pctrl;
> > > -#ifdef CONFIG_CPU_FREQ
> > > +#if defined(CONFIG_CPU_FREQ) && !defined(CONFIG_ARCH_EXYNOS)
> > 
> > ...this is not a good coding practice, especially when already having
> > multiplatform kernels in sight.
> > 
> > The best way would be to check on which SoC we are running at runtime,
> > but since this might need changing a lot of code, then at least I would
> > change this from !defined(EXYNOS) to defined(S3C24XX), so it is not being
> > compiled in when S3C24XX support is not enabled and if it's enabled then
> > the notifier will be registered as a safe fallback that will run correctly
> > on all platforms.

Actually you can simply check for CONFIG_CPU_FREQ_S3C24XX here.

Best regards,
Tomasz

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
naveen krishna chatradhi Oct. 12, 2013, 6:36 a.m.
On 12 October 2013 11:12, Tomasz Figa <t.figa@samsung.com> wrote:
> On Saturday 12 of October 2013 04:28:51 Tomasz Figa wrote:
>> [Fixing incorrent mail addresses and dropping the old DT ML.]
>>
>> On Saturday 12 of October 2013 04:22:04 Tomasz Figa wrote:
>> > Hi Naveen,
>> >
>> > On Friday 11 of October 2013 16:56:54 Naveen Krishna Chatradhi wrote:
>> > > The exynos5 i2c clock is based on a fixed 66 MHz peripheral clock, and
>> > > therefore is completely independent of the cpu frequency.
>> > > Thus, registering for a CPU freq notifier is very wasteful.
>> > >
>> > > This patch modifes the code such that, i2c bus registers to
>> > > cpu_freq_transition only for non Exynos SoCs.
>> > >
>> > > This change should save a bunch of cpufreq transitions calls
>> > > which does not apply to exynos SoCs.
>> >
>> > The idea is fine, although...
>> >
>> > > Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
>> > > ---
>> > >  drivers/i2c/busses/i2c-s3c2410.c |    4 ++--
>> > >  1 file changed, 2 insertions(+), 2 deletions(-)
>> > >
>> > > diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
>> > > index cab1c91..d062fa7 100644
>> > > --- a/drivers/i2c/busses/i2c-s3c2410.c
>> > > +++ b/drivers/i2c/busses/i2c-s3c2410.c
>> > > @@ -123,7 +123,7 @@ struct s3c24xx_i2c {
>> > >   struct s3c2410_platform_i2c     *pdata;
>> > >   int                     gpios[2];
>> > >   struct pinctrl          *pctrl;
>> > > -#ifdef CONFIG_CPU_FREQ
>> > > +#if defined(CONFIG_CPU_FREQ) && !defined(CONFIG_ARCH_EXYNOS)
>> >
>> > ...this is not a good coding practice, especially when already having
>> > multiplatform kernels in sight.
>> >
>> > The best way would be to check on which SoC we are running at runtime,
>> > but since this might need changing a lot of code, then at least I would
>> > change this from !defined(EXYNOS) to defined(S3C24XX), so it is not being
>> > compiled in when S3C24XX support is not enabled and if it's enabled then
>> > the notifier will be registered as a safe fallback that will run correctly
>> > on all platforms.
>
> Actually you can simply check for CONFIG_CPU_FREQ_S3C24XX here.
sure, this makes it more meaningful. will make the modifications and resubmit.
>
> Best regards,
> Tomasz
>

Patch hide | download patch | download mbox

diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index cab1c91..d062fa7 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -123,7 +123,7 @@  struct s3c24xx_i2c {
 	struct s3c2410_platform_i2c	*pdata;
 	int			gpios[2];
 	struct pinctrl          *pctrl;
-#ifdef CONFIG_CPU_FREQ
+#if defined(CONFIG_CPU_FREQ) && !defined(CONFIG_ARCH_EXYNOS)
 	struct notifier_block	freq_transition;
 #endif
 };
@@ -843,7 +843,7 @@  static int s3c24xx_i2c_clockrate(struct s3c24xx_i2c *i2c, unsigned int *got)
 	return 0;
 }
 
-#ifdef CONFIG_CPU_FREQ
+#if defined(CONFIG_CPU_FREQ) && !defined(CONFIG_ARCH_EXYNOS)
 
 #define freq_to_i2c(_n) container_of(_n, struct s3c24xx_i2c, freq_transition)