[PATCH 0/3] i2c: davinci: Add power management features

[PATCH 0/3] i2c: davinci: Add power management features

[Chaithrika U S]

Add suspend/resume and cpufreq features to DaVinci I2C driver
All patches apply to Linus' kernel tree.
Testing of these features was done on DA850/OMAP-L138 EVM.

Chaithrika U S (3):
  i2c: davinci: Add helper functions
  i2c: davinci: Add suspend/resume support
  i2c: davinci: Add cpufreq support

 drivers/i2c/busses/i2c-davinci.c |  147 ++++++++++++++++++++++++++++++++-----
 1 files changed, 127 insertions(+), 20 deletions(-)

[PATCH 1/3] i2c: davinci: Add helper functions

[Chaithrika U S]

Add i2c reset control and clock divider calculation functions
which will be useful for power management features.

Signed-off-by: Chaithrika U S <chaithrika-l0cyMroinI0@public.gmane.org>
---
 drivers/i2c/busses/i2c-davinci.c |   56 +++++++++++++++++++++++++-------------
 1 files changed, 37 insertions(+), 19 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index c89687a..5f3838c 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -133,12 +133,21 @@ static inline u16 davinci_i2c_read_reg(struct davinci_i2c_dev *i2c_dev, int reg)
 	return __raw_readw(i2c_dev->base + reg);
 }
 
-/*
- * This functions configures I2C and brings I2C out of reset.
- * This function is called during I2C init function. This function
- * also gets called if I2C encounters any errors.
- */
-static int i2c_davinci_init(struct davinci_i2c_dev *dev)
+static inline void davinci_i2c_reset_ctrl(struct davinci_i2c_dev *i2c_dev,
+								int val)
+{
+	u16 w;
+
+	w = davinci_i2c_read_reg(i2c_dev, DAVINCI_I2C_MDR_REG);
+	if (!val)	/* put I2C into reset */
+		MOD_REG_BIT(w, DAVINCI_I2C_MDR_IRS, 0);
+	else		/* take I2C out of reset */
+		MOD_REG_BIT(w, DAVINCI_I2C_MDR_IRS, 1);
+
+	davinci_i2c_write_reg(i2c_dev, DAVINCI_I2C_MDR_REG, w);
+}
+
+static void i2c_davinci_calc_clk_dividers(struct davinci_i2c_dev *dev)
 {
 	struct davinci_i2c_platform_data *pdata = dev->dev->platform_data;
 	u16 psc;
@@ -147,15 +156,6 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
 	u32 clkh;
 	u32 clkl;
 	u32 input_clock = clk_get_rate(dev->clk);
-	u16 w;
-
-	if (!pdata)
-		pdata = &davinci_i2c_platform_data_default;
-
-	/* put I2C into reset */
-	w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
-	MOD_REG_BIT(w, DAVINCI_I2C_MDR_IRS, 0);
-	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
 
 	/* NOTE: I2C Clock divider programming info
 	 * As per I2C specs the following formulas provide prescaler
@@ -187,12 +187,32 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
 	davinci_i2c_write_reg(dev, DAVINCI_I2C_CLKH_REG, clkh);
 	davinci_i2c_write_reg(dev, DAVINCI_I2C_CLKL_REG, clkl);
 
+	dev_dbg(dev->dev, "input_clock = %d, CLK = %d\n", input_clock, clk);
+}
+
+/*
+ * This functions configures I2C and brings I2C out of reset.
+ * This function is called during I2C init function. This function
+ * also gets called if I2C encounters any errors.
+ */
+static int i2c_davinci_init(struct davinci_i2c_dev *dev)
+{
+	struct davinci_i2c_platform_data *pdata = dev->dev->platform_data;
+
+	if (!pdata)
+		pdata = &davinci_i2c_platform_data_default;
+
+	/* put I2C into reset */
+	davinci_i2c_reset_ctrl(dev, 0);
+
+	/* compute clock dividers */
+	i2c_davinci_calc_clk_dividers(dev);
+
 	/* Respond at reserved "SMBus Host" slave address" (and zero);
 	 * we seem to have no option to not respond...
 	 */
 	davinci_i2c_write_reg(dev, DAVINCI_I2C_OAR_REG, 0x08);
 
-	dev_dbg(dev->dev, "input_clock = %d, CLK = %d\n", input_clock, clk);
 	dev_dbg(dev->dev, "PSC  = %d\n",
 		davinci_i2c_read_reg(dev, DAVINCI_I2C_PSC_REG));
 	dev_dbg(dev->dev, "CLKL = %d\n",
@@ -203,9 +223,7 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
 		pdata->bus_freq, pdata->bus_delay);
 
 	/* Take the I2C module out of reset: */
-	w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
-	MOD_REG_BIT(w, DAVINCI_I2C_MDR_IRS, 1);
-	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
+	davinci_i2c_reset_ctrl(dev, 1);
 
 	/* Enable interrupts */
 	davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, I2C_DAVINCI_INTR_ALL);
-- 
1.5.6

[PATCH 2/3] i2c: davinci: Add suspend/resume support

[Chaithrika U S]

Add suspend and resume callbacks to DaVinci I2C driver.
This has been tested on DA850/OMAP-L138 EVM. The SoC specific
suspend-to-RAM support patch series [1] is needed to test this feature.

[1] http://linux.davincidsp.com/pipermail/davinci-linux-open-source/
2009-November/016958.html

Signed-off-by: Chaithrika U S <chaithrika-l0cyMroinI0@public.gmane.org>
---
 drivers/i2c/busses/i2c-davinci.c |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 5f3838c..5f0888d 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -623,6 +623,36 @@ static int davinci_i2c_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM
+static int davinci_i2c_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	struct davinci_i2c_dev *dev = platform_get_drvdata(pdev);
+
+	/* put I2C into reset */
+	davinci_i2c_reset_ctrl(dev, 0);
+
+	clk_disable(dev->clk);
+
+	return 0;
+}
+
+static int davinci_i2c_resume(struct platform_device *pdev)
+{
+	struct davinci_i2c_dev *dev = platform_get_drvdata(pdev);
+
+	clk_enable(dev->clk);
+
+	/* take I2C out of reset */
+	davinci_i2c_reset_ctrl(dev, 1);
+
+	return 0;
+}
+
+#else
+#define davinci_i2c_suspend NULL
+#define davinci_i2c_resume NULL
+#endif
+
 /* work with hotplug and coldplug */
 MODULE_ALIAS("platform:i2c_davinci");
 
@@ -633,6 +663,8 @@ static struct platform_driver davinci_i2c_driver = {
 		.name	= "i2c_davinci",
 		.owner	= THIS_MODULE,
 	},
+	.suspend	= davinci_i2c_suspend,
+	.resume		= davinci_i2c_resume,
 };
 
 /* I2C may be needed to bring up other drivers */
-- 
1.5.6

[PATCH 3/3] i2c: davinci: Add cpufreq support

[Chaithrika U S]

Add cpufreq support for DaVinci I2C driver.
Tested on DA850/OMAP-L138 EVM. For the purpose of testing, the patches
which add cpufreq support [1] for this SoC are needed.

[1]http://linux.davincidsp.com/pipermail/davinci-linux-open-source/
2009-September/016118.html

Signed-off-by: Chaithrika U S <chaithrika-l0cyMroinI0@public.gmane.org>
---
 drivers/i2c/busses/i2c-davinci.c |   59 +++++++++++++++++++++++++++++++++++++-
 1 files changed, 58 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 5f0888d..0f41da4 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -35,9 +35,9 @@
 #include <linux/interrupt.h>
 #include <linux/platform_device.h>
 #include <linux/io.h>
+#include <linux/cpufreq.h>
 
 #include <mach/hardware.h>
-
 #include <mach/i2c.h>
 
 /* ----- global defines ----------------------------------------------- */
@@ -114,6 +114,10 @@ struct davinci_i2c_dev {
 	int			irq;
 	u8			terminate;
 	struct i2c_adapter	adapter;
+#ifdef CONFIG_CPU_FREQ
+	struct completion	xfr_complete;
+	struct notifier_block	freq_transition;
+#endif
 };
 
 /* default platform data to use if not supplied in the platform_device */
@@ -384,6 +388,11 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 		if (ret < 0)
 			return ret;
 	}
+
+#ifdef CONFIG_CPU_FREQ
+	complete(&dev->xfr_complete);
+#endif
+
 	return num;
 }
 
@@ -508,6 +517,39 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
 	return count ? IRQ_HANDLED : IRQ_NONE;
 }
 
+#ifdef CONFIG_CPU_FREQ
+static int i2c_davinci_cpufreq_transition(struct notifier_block *nb,
+				     unsigned long val, void *data)
+{
+	struct davinci_i2c_dev *dev;
+
+	dev = container_of(nb, struct davinci_i2c_dev, freq_transition);
+	if (val == CPUFREQ_PRECHANGE) {
+		wait_for_completion(&dev->xfr_complete);
+		davinci_i2c_reset_ctrl(dev, 0);
+	} else if (val == CPUFREQ_POSTCHANGE) {
+		i2c_davinci_calc_clk_dividers(dev);
+		davinci_i2c_reset_ctrl(dev, 1);
+	}
+
+	return 0;
+}
+
+static inline int i2c_davinci_cpufreq_register(struct davinci_i2c_dev *dev)
+{
+	dev->freq_transition.notifier_call = i2c_davinci_cpufreq_transition;
+
+	return cpufreq_register_notifier(&dev->freq_transition,
+					 CPUFREQ_TRANSITION_NOTIFIER);
+}
+
+static inline void i2c_davinci_cpufreq_deregister(struct davinci_i2c_dev *dev)
+{
+	cpufreq_unregister_notifier(&dev->freq_transition,
+				    CPUFREQ_TRANSITION_NOTIFIER);
+}
+#endif
+
 static struct i2c_algorithm i2c_davinci_algo = {
 	.master_xfer	= i2c_davinci_xfer,
 	.functionality	= i2c_davinci_func,
@@ -547,6 +589,9 @@ static int davinci_i2c_probe(struct platform_device *pdev)
 	}
 
 	init_completion(&dev->cmd_complete);
+#ifdef CONFIG_CPU_FREQ
+	init_completion(&dev->xfr_complete);
+#endif
 	dev->dev = get_device(&pdev->dev);
 	dev->irq = irq->start;
 	platform_set_drvdata(pdev, dev);
@@ -567,6 +612,14 @@ static int davinci_i2c_probe(struct platform_device *pdev)
 		goto err_unuse_clocks;
 	}
 
+#ifdef CONFIG_CPU_FREQ
+	r = i2c_davinci_cpufreq_register(dev);
+	if (r) {
+		dev_err(&pdev->dev, "failed to register cpufreq\n");
+		goto err_free_irq;
+	}
+#endif
+
 	adap = &dev->adapter;
 	i2c_set_adapdata(adap, dev);
 	adap->owner = THIS_MODULE;
@@ -606,6 +659,10 @@ static int davinci_i2c_remove(struct platform_device *pdev)
 	struct davinci_i2c_dev *dev = platform_get_drvdata(pdev);
 	struct resource *mem;
 
+#ifdef CONFIG_CPU_FREQ
+	i2c_davinci_cpufreq_deregister(dev);
+#endif
+
 	platform_set_drvdata(pdev, NULL);
 	i2c_del_adapter(&dev->adapter);
 	put_device(&pdev->dev);
-- 
1.5.6

Re: [PATCH 3/3] i2c: davinci: Add cpufreq support

[Sergei Shtylyov]

Hello.

Chaithrika U S wrote:

> Add cpufreq support for DaVinci I2C driver.
> Tested on DA850/OMAP-L138 EVM. For the purpose of testing, the patches
> which add cpufreq support [1] for this SoC are needed.
>
> [1]http://linux.davincidsp.com/pipermail/davinci-linux-open-source/
> 2009-September/016118.html
>
> Signed-off-by: Chaithrika U S <chaithrika-l0cyMroinI0@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-davinci.c |   59 +++++++++++++++++++++++++++++++++++++-
>  1 files changed, 58 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> index 5f0888d..0f41da4 100644
> --- a/drivers/i2c/busses/i2c-davinci.c
> +++ b/drivers/i2c/busses/i2c-davinci.c

[...]

> @@ -508,6 +517,39 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
>  	return count ? IRQ_HANDLED : IRQ_NONE;
>  }
>  
> +#ifdef CONFIG_CPU_FREQ
> +static int i2c_davinci_cpufreq_transition(struct notifier_block *nb,
> +				     unsigned long val, void *data)
> +{
> +	struct davinci_i2c_dev *dev;
> +
> +	dev = container_of(nb, struct davinci_i2c_dev, freq_transition);
> +	if (val == CPUFREQ_PRECHANGE) {
> +		wait_for_completion(&dev->xfr_complete);
> +		davinci_i2c_reset_ctrl(dev, 0);
> +	} else if (val == CPUFREQ_POSTCHANGE) {
> +		i2c_davinci_calc_clk_dividers(dev);
> +		davinci_i2c_reset_ctrl(dev, 1);
> +	}
> +
> +	return 0;
> +}
> +
> +static inline int i2c_davinci_cpufreq_register(struct davinci_i2c_dev *dev)
> +{
> +	dev->freq_transition.notifier_call = i2c_davinci_cpufreq_transition;
> +
> +	return cpufreq_register_notifier(&dev->freq_transition,
> +					 CPUFREQ_TRANSITION_NOTIFIER);
> +}
> +
> +static inline void i2c_davinci_cpufreq_deregister(struct davinci_i2c_dev *dev)
> +{
> +	cpufreq_unregister_notifier(&dev->freq_transition,
> +				    CPUFREQ_TRANSITION_NOTIFIER);
> +}
> +#endif
> +
>  static struct i2c_algorithm i2c_davinci_algo = {
>  	.master_xfer	= i2c_davinci_xfer,
>  	.functionality	= i2c_davinci_func,
> @@ -547,6 +589,9 @@ static int davinci_i2c_probe(struct platform_device *pdev)
>  	}
>  
>  	init_completion(&dev->cmd_complete);
> +#ifdef CONFIG_CPU_FREQ
> +	init_completion(&dev->xfr_complete);
> +#endif
>  	dev->dev = get_device(&pdev->dev);
>  	dev->irq = irq->start;
>  	platform_set_drvdata(pdev, dev);
> @@ -567,6 +612,14 @@ static int davinci_i2c_probe(struct platform_device *pdev)
>  		goto err_unuse_clocks;
>  	}
>  
> +#ifdef CONFIG_CPU_FREQ
> +	r = i2c_davinci_cpufreq_register(dev);
> +	if (r) {
> +		dev_err(&pdev->dev, "failed to register cpufreq\n");
> +		goto err_free_irq;
> +	}
> +#endif
>   

   You should instead define this function as just returning 0 in the 
#else branch above, so that #ifdef here can be avoided...

> +
>  	adap = &dev->adapter;
>  	i2c_set_adapdata(adap, dev);
>  	adap->owner = THIS_MODULE;
> @@ -606,6 +659,10 @@ static int davinci_i2c_remove(struct platform_device *pdev)
>  	struct davinci_i2c_dev *dev = platform_get_drvdata(pdev);
>  	struct resource *mem;
>  
> +#ifdef CONFIG_CPU_FREQ
> +	i2c_davinci_cpufreq_deregister(dev);
> +#endif
> +
>   

   Same comment here. #ifdef's in the function code are frowned upon....

WBR, Sergei

Re: [PATCH 1/3] i2c: davinci: Add helper functions

[Troy Kisky]

Chaithrika U S wrote:
> Add i2c reset control and clock divider calculation functions
> which will be useful for power management features.
> 
> Signed-off-by: Chaithrika U S <chaithrika-l0cyMroinI0@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-davinci.c |   56 +++++++++++++++++++++++++-------------
>  1 files changed, 37 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> index c89687a..5f3838c 100644
> --- a/drivers/i2c/busses/i2c-davinci.c
> +++ b/drivers/i2c/busses/i2c-davinci.c
> @@ -133,12 +133,21 @@ static inline u16 davinci_i2c_read_reg(struct davinci_i2c_dev *i2c_dev, int reg)
>  	return __raw_readw(i2c_dev->base + reg);
>  }
>  
> -/*
> - * This functions configures I2C and brings I2C out of reset.
> - * This function is called during I2C init function. This function
> - * also gets called if I2C encounters any errors.
> - */
> -static int i2c_davinci_init(struct davinci_i2c_dev *dev)
> +static inline void davinci_i2c_reset_ctrl(struct davinci_i2c_dev *i2c_dev,
> +								int val)
> +{
> +	u16 w;
> +
> +	w = davinci_i2c_read_reg(i2c_dev, DAVINCI_I2C_MDR_REG);
> +	if (!val)	/* put I2C into reset */
> +		MOD_REG_BIT(w, DAVINCI_I2C_MDR_IRS, 0);
> +	else		/* take I2C out of reset */
> +		MOD_REG_BIT(w, DAVINCI_I2C_MDR_IRS, 1);
> +


I dislike the MOD_REG_BIT macro. Maybe if you wrote
MOD_REG_BIT(w, DAVINCI_I2C_MDR_IRS, val);

it would be slightly better, but I still wouldn't like it.
Please use w &= ~DAVINCI_I2C_MDR_IRS and
 w |= DAVINCI_I2C_MDR_IRS directly.


I know the code you're replacing uses this obfuscating macro
but this is a good time to remove it.


> +	davinci_i2c_write_reg(i2c_dev, DAVINCI_I2C_MDR_REG, w);
> +}
> +
> +static void i2c_davinci_calc_clk_dividers(struct davinci_i2c_dev *dev)
>  {
>  	struct davinci_i2c_platform_data *pdata = dev->dev->platform_data;
>  	u16 psc;
> @@ -147,15 +156,6 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
>  	u32 clkh;
>  	u32 clkl;
>  	u32 input_clock = clk_get_rate(dev->clk);
> -	u16 w;
> -
> -	if (!pdata)
> -		pdata = &davinci_i2c_platform_data_default;
> -
> -	/* put I2C into reset */
> -	w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
> -	MOD_REG_BIT(w, DAVINCI_I2C_MDR_IRS, 0);
> -	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
>  
>  	/* NOTE: I2C Clock divider programming info
>  	 * As per I2C specs the following formulas provide prescaler
> @@ -187,12 +187,32 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
>  	davinci_i2c_write_reg(dev, DAVINCI_I2C_CLKH_REG, clkh);
>  	davinci_i2c_write_reg(dev, DAVINCI_I2C_CLKL_REG, clkl);
>  
> +	dev_dbg(dev->dev, "input_clock = %d, CLK = %d\n", input_clock, clk);
> +}
> +
> +/*
> + * This functions configures I2C and brings I2C out of reset.
> + * This function is called during I2C init function. This function
> + * also gets called if I2C encounters any errors.
> + */
> +static int i2c_davinci_init(struct davinci_i2c_dev *dev)
> +{
> +	struct davinci_i2c_platform_data *pdata = dev->dev->platform_data;
> +
> +	if (!pdata)
> +		pdata = &davinci_i2c_platform_data_default;
> +
> +	/* put I2C into reset */
> +	davinci_i2c_reset_ctrl(dev, 0);
> +
> +	/* compute clock dividers */
> +	i2c_davinci_calc_clk_dividers(dev);
> +
>  	/* Respond at reserved "SMBus Host" slave address" (and zero);
>  	 * we seem to have no option to not respond...
>  	 */
>  	davinci_i2c_write_reg(dev, DAVINCI_I2C_OAR_REG, 0x08);
>  
> -	dev_dbg(dev->dev, "input_clock = %d, CLK = %d\n", input_clock, clk);
>  	dev_dbg(dev->dev, "PSC  = %d\n",
>  		davinci_i2c_read_reg(dev, DAVINCI_I2C_PSC_REG));
>  	dev_dbg(dev->dev, "CLKL = %d\n",
> @@ -203,9 +223,7 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
>  		pdata->bus_freq, pdata->bus_delay);
>  
>  	/* Take the I2C module out of reset: */
> -	w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
> -	MOD_REG_BIT(w, DAVINCI_I2C_MDR_IRS, 1);
> -	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
> +	davinci_i2c_reset_ctrl(dev, 1);
>  
>  	/* Enable interrupts */
>  	davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, I2C_DAVINCI_INTR_ALL);

RE: [PATCH 3/3] i2c: davinci: Add cpufreq support

[Chaithrika U S]

On Mon, Dec 07, 2009 at 16:30:14, Sergei Shtylyov wrote:
> Hello.
> 
> Chaithrika U S wrote:
> 
> > Add cpufreq support for DaVinci I2C driver.
> > Tested on DA850/OMAP-L138 EVM. For the purpose of testing, the patches
> > which add cpufreq support [1] for this SoC are needed.
> >
> > [1]http://linux.davincidsp.com/pipermail/davinci-linux-open-source/
> > 2009-September/016118.html
> >
> > Signed-off-by: Chaithrika U S <chaithrika-l0cyMroinI0@public.gmane.org>
> > ---
> >  drivers/i2c/busses/i2c-davinci.c |   59 +++++++++++++++++++++++++++++++++++++-
> >  1 files changed, 58 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> > index 5f0888d..0f41da4 100644
> > --- a/drivers/i2c/busses/i2c-davinci.c
> > +++ b/drivers/i2c/busses/i2c-davinci.c
> 
> [...]
> 
> > @@ -508,6 +517,39 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
> >  	return count ? IRQ_HANDLED : IRQ_NONE;
> >  }
> >  
> > +#ifdef CONFIG_CPU_FREQ
> > +static int i2c_davinci_cpufreq_transition(struct notifier_block *nb,
> > +				     unsigned long val, void *data)
> > +{
> > +	struct davinci_i2c_dev *dev;
> > +
> > +	dev = container_of(nb, struct davinci_i2c_dev, freq_transition);
> > +	if (val == CPUFREQ_PRECHANGE) {
> > +		wait_for_completion(&dev->xfr_complete);
> > +		davinci_i2c_reset_ctrl(dev, 0);
> > +	} else if (val == CPUFREQ_POSTCHANGE) {
> > +		i2c_davinci_calc_clk_dividers(dev);
> > +		davinci_i2c_reset_ctrl(dev, 1);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static inline int i2c_davinci_cpufreq_register(struct davinci_i2c_dev *dev)
> > +{
> > +	dev->freq_transition.notifier_call = i2c_davinci_cpufreq_transition;
> > +
> > +	return cpufreq_register_notifier(&dev->freq_transition,
> > +					 CPUFREQ_TRANSITION_NOTIFIER);
> > +}
> > +
> > +static inline void i2c_davinci_cpufreq_deregister(struct davinci_i2c_dev *dev)
> > +{
> > +	cpufreq_unregister_notifier(&dev->freq_transition,
> > +				    CPUFREQ_TRANSITION_NOTIFIER);
> > +}
> > +#endif
> > +
> >  static struct i2c_algorithm i2c_davinci_algo = {
> >  	.master_xfer	= i2c_davinci_xfer,
> >  	.functionality	= i2c_davinci_func,
> > @@ -547,6 +589,9 @@ static int davinci_i2c_probe(struct platform_device *pdev)
> >  	}
> >  
> >  	init_completion(&dev->cmd_complete);
> > +#ifdef CONFIG_CPU_FREQ
> > +	init_completion(&dev->xfr_complete);
> > +#endif
> >  	dev->dev = get_device(&pdev->dev);
> >  	dev->irq = irq->start;
> >  	platform_set_drvdata(pdev, dev);
> > @@ -567,6 +612,14 @@ static int davinci_i2c_probe(struct platform_device *pdev)
> >  		goto err_unuse_clocks;
> >  	}
> >  
> > +#ifdef CONFIG_CPU_FREQ
> > +	r = i2c_davinci_cpufreq_register(dev);
> > +	if (r) {
> > +		dev_err(&pdev->dev, "failed to register cpufreq\n");
> > +		goto err_free_irq;
> > +	}
> > +#endif
> >   
> 
>    You should instead define this function as just returning 0 in the 
> #else branch above, so that #ifdef here can be avoided...
> 
> > +
> >  	adap = &dev->adapter;
> >  	i2c_set_adapdata(adap, dev);
> >  	adap->owner = THIS_MODULE;
> > @@ -606,6 +659,10 @@ static int davinci_i2c_remove(struct platform_device *pdev)
> >  	struct davinci_i2c_dev *dev = platform_get_drvdata(pdev);
> >  	struct resource *mem;
> >  
> > +#ifdef CONFIG_CPU_FREQ
> > +	i2c_davinci_cpufreq_deregister(dev);
> > +#endif
> > +
> >   
> 
>    Same comment here. #ifdef's in the function code are frowned upon....
> 

OK.I will post an updated version of this patch series soon.


Thanks and Regards, 
Chaithrika

> WBR, Sergei
> 
> 

RE: [PATCH 1/3] i2c: davinci: Add helper functions

[Chaithrika U S]

On Mon, Dec 07, 2009 at 23:54:16, Troy Kisky wrote:
> Chaithrika U S wrote:
> > Add i2c reset control and clock divider calculation functions
> > which will be useful for power management features.
> > 
> > Signed-off-by: Chaithrika U S <chaithrika-l0cyMroinI0@public.gmane.org>
> > ---
> >  drivers/i2c/busses/i2c-davinci.c |   56 +++++++++++++++++++++++++-------------
> >  1 files changed, 37 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> > index c89687a..5f3838c 100644
> > --- a/drivers/i2c/busses/i2c-davinci.c
> > +++ b/drivers/i2c/busses/i2c-davinci.c
> > @@ -133,12 +133,21 @@ static inline u16 davinci_i2c_read_reg(struct davinci_i2c_dev *i2c_dev, int reg)
> >  	return __raw_readw(i2c_dev->base + reg);
> >  }
> >  
> > -/*
> > - * This functions configures I2C and brings I2C out of reset.
> > - * This function is called during I2C init function. This function
> > - * also gets called if I2C encounters any errors.
> > - */
> > -static int i2c_davinci_init(struct davinci_i2c_dev *dev)
> > +static inline void davinci_i2c_reset_ctrl(struct davinci_i2c_dev *i2c_dev,
> > +								int val)
> > +{
> > +	u16 w;
> > +
> > +	w = davinci_i2c_read_reg(i2c_dev, DAVINCI_I2C_MDR_REG);
> > +	if (!val)	/* put I2C into reset */
> > +		MOD_REG_BIT(w, DAVINCI_I2C_MDR_IRS, 0);
> > +	else		/* take I2C out of reset */
> > +		MOD_REG_BIT(w, DAVINCI_I2C_MDR_IRS, 1);
> > +
> 
> 
> I dislike the MOD_REG_BIT macro. Maybe if you wrote
> MOD_REG_BIT(w, DAVINCI_I2C_MDR_IRS, val);
> 
> it would be slightly better, but I still wouldn't like it.
> Please use w &= ~DAVINCI_I2C_MDR_IRS and
>  w |= DAVINCI_I2C_MDR_IRS directly.
> 
> 
> I know the code you're replacing uses this obfuscating macro
> but this is a good time to remove it.
> 
> 

Agree. Will include a patch to remove this macro in the next version of this
patch set.

Regards, 
Chaithrika

> > +	davinci_i2c_write_reg(i2c_dev, DAVINCI_I2C_MDR_REG, w);
> > +}
> > +
> > +static void i2c_davinci_calc_clk_dividers(struct davinci_i2c_dev *dev)
> >  {
> >  	struct davinci_i2c_platform_data *pdata = dev->dev->platform_data;
> >  	u16 psc;
> > @@ -147,15 +156,6 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
> >  	u32 clkh;
> >  	u32 clkl;
> >  	u32 input_clock = clk_get_rate(dev->clk);
> > -	u16 w;
> > -
> > -	if (!pdata)
> > -		pdata = &davinci_i2c_platform_data_default;
> > -
> > -	/* put I2C into reset */
> > -	w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
> > -	MOD_REG_BIT(w, DAVINCI_I2C_MDR_IRS, 0);
> > -	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
> >  
> >  	/* NOTE: I2C Clock divider programming info
> >  	 * As per I2C specs the following formulas provide prescaler
> > @@ -187,12 +187,32 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
> >  	davinci_i2c_write_reg(dev, DAVINCI_I2C_CLKH_REG, clkh);
> >  	davinci_i2c_write_reg(dev, DAVINCI_I2C_CLKL_REG, clkl);
> >  
> > +	dev_dbg(dev->dev, "input_clock = %d, CLK = %d\n", input_clock, clk);
> > +}
> > +
> > +/*
> > + * This functions configures I2C and brings I2C out of reset.
> > + * This function is called during I2C init function. This function
> > + * also gets called if I2C encounters any errors.
> > + */
> > +static int i2c_davinci_init(struct davinci_i2c_dev *dev)
> > +{
> > +	struct davinci_i2c_platform_data *pdata = dev->dev->platform_data;
> > +
> > +	if (!pdata)
> > +		pdata = &davinci_i2c_platform_data_default;
> > +
> > +	/* put I2C into reset */
> > +	davinci_i2c_reset_ctrl(dev, 0);
> > +
> > +	/* compute clock dividers */
> > +	i2c_davinci_calc_clk_dividers(dev);
> > +
> >  	/* Respond at reserved "SMBus Host" slave address" (and zero);
> >  	 * we seem to have no option to not respond...
> >  	 */
> >  	davinci_i2c_write_reg(dev, DAVINCI_I2C_OAR_REG, 0x08);
> >  
> > -	dev_dbg(dev->dev, "input_clock = %d, CLK = %d\n", input_clock, clk);
> >  	dev_dbg(dev->dev, "PSC  = %d\n",
> >  		davinci_i2c_read_reg(dev, DAVINCI_I2C_PSC_REG));
> >  	dev_dbg(dev->dev, "CLKL = %d\n",
> > @@ -203,9 +223,7 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
> >  		pdata->bus_freq, pdata->bus_delay);
> >  
> >  	/* Take the I2C module out of reset: */
> > -	w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
> > -	MOD_REG_BIT(w, DAVINCI_I2C_MDR_IRS, 1);
> > -	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
> > +	davinci_i2c_reset_ctrl(dev, 1);
> >  
> >  	/* Enable interrupts */
> >  	davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, I2C_DAVINCI_INTR_ALL);
>