[PATCH 2/2 v3] i2c: nomadik: adopt pinctrl support

[PATCH 2/2 v3] i2c: nomadik: adopt pinctrl support

[Linus Walleij]

From: Patrice Chotard <patrice.chotard-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org>

Amend the I2C nomadik pin controller to optionally take a pin control
handle and set the state of the pins to:

- "default" on boot, resume and before performing an i2c transfer
- "idle" after initial default, after resume default, and after each
   i2c xfer
- "sleep" on suspend()

This should make it possible to optimize energy usage for the pins
both for the suspend/resume cycle, and for runtime cases inbetween
I2C transfers.

Signed-off-by: Patrice Chotard <patrice.chotard-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org>
Signed-off-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
ChangeLog v2->v3:
- Rebase on top of the patch from Philippe/Ulf.
ChangeLog v1->v2:
- We used only two states initially: default and sleep. It turns
  out you can save some energy when idling (between transfers)
  and even more when suspending on our platform, so grab all
  three states and use them as applicable.
---
 drivers/i2c/busses/i2c-nomadik.c | 101 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 101 insertions(+)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 3eeae52..d50b16a 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -24,6 +24,7 @@
 #include <linux/io.h>
 #include <linux/pm_runtime.h>
 #include <linux/platform_data/i2c-nomadik.h>
+#include <linux/pinctrl/consumer.h>
 
 #define DRIVER_NAME "nmk-i2c"
 
@@ -145,6 +146,10 @@ struct i2c_nmk_client {
  * @stop: stop condition.
  * @xfer_complete: acknowledge completion for a I2C message.
  * @result: controller propogated result.
+ * @pinctrl: pinctrl handle.
+ * @pins_default: default state for the pins.
+ * @pins_idle: idle state for the pins.
+ * @pins_sleep: sleep state for the pins.
  * @busy: Busy doing transfer.
  */
 struct nmk_i2c_dev {
@@ -158,6 +163,11 @@ struct nmk_i2c_dev {
 	int				stop;
 	struct completion		xfer_complete;
 	int				result;
+	/* Three pin states - default, idle & sleep */
+	struct pinctrl			*pinctrl;
+	struct pinctrl_state		*pins_default;
+	struct pinctrl_state		*pins_idle;
+	struct pinctrl_state		*pins_sleep;
 	bool				busy;
 };
 
@@ -648,6 +658,15 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
 		goto out_clk;
 	}
 
+	/* Optionaly enable pins to be muxed in and configured */
+	if (!IS_ERR(dev->pins_default)) {
+		status = pinctrl_select_state(dev->pinctrl,
+				dev->pins_default);
+		if (status)
+			dev_err(&dev->adev->dev,
+				"could not set default pins\n");
+	}
+
 	status = init_hw(dev);
 	if (status)
 		goto out;
@@ -675,6 +694,15 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
 out:
 	clk_disable_unprepare(dev->clk);
 out_clk:
+	/* Optionally let pins go into idle state */
+	if (!IS_ERR(dev->pins_idle)) {
+		status = pinctrl_select_state(dev->pinctrl,
+				dev->pins_idle);
+		if (status)
+			dev_err(&dev->adev->dev,
+				"could not set pins to idle state\n");
+	}
+
 	pm_runtime_put_sync(&dev->adev->dev);
 
 	dev->busy = false;
@@ -869,15 +897,44 @@ static int nmk_i2c_suspend(struct device *dev)
 {
 	struct amba_device *adev = to_amba_device(dev);
 	struct nmk_i2c_dev *nmk_i2c = amba_get_drvdata(adev);
+	int ret;
 
 	if (nmk_i2c->busy)
 		return -EBUSY;
 
+	if (!IS_ERR(nmk_i2c->pins_sleep)) {
+		ret = pinctrl_select_state(nmk_i2c->pinctrl,
+				nmk_i2c->pins_sleep);
+		if (ret)
+			dev_err(dev,
+				"could not set pins to sleep state\n");
+	}
+
 	return 0;
 }
 
 static int nmk_i2c_resume(struct device *dev)
 {
+	struct amba_device *adev = to_amba_device(dev);
+	struct nmk_i2c_dev *nmk_i2c = amba_get_drvdata(adev);
+	int ret;
+
+	/* First go to the default state */
+	if (!IS_ERR(nmk_i2c->pins_default)) {
+		ret = pinctrl_select_state(nmk_i2c->pinctrl,
+				nmk_i2c->pins_default);
+		if (ret)
+			dev_err(dev,
+				"could not set pins to default state\n");
+	}
+	/* Then let's idle the pins until the next transfer happens */
+	if (!IS_ERR(nmk_i2c->pins_idle)) {
+		ret = pinctrl_select_state(nmk_i2c->pinctrl,
+				nmk_i2c->pins_idle);
+		if (ret)
+			dev_err(dev,
+				"could not set pins to idle state\n");
+	}
 	return 0;
 }
 #else
@@ -941,6 +998,40 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
 	dev->adev = adev;
 	amba_set_drvdata(adev, dev);
 
+	dev->pinctrl = devm_pinctrl_get(&adev->dev);
+	if (IS_ERR(dev->pinctrl)) {
+		ret = PTR_ERR(dev->pinctrl);
+		goto err_pinctrl;
+	}
+
+	dev->pins_default = pinctrl_lookup_state(dev->pinctrl,
+						 PINCTRL_STATE_DEFAULT);
+	if (IS_ERR(dev->pins_default))
+		dev_err(&adev->dev, "could not get default pinstate\n");
+	else {
+		ret = pinctrl_select_state(dev->pinctrl,
+					   dev->pins_default);
+		if (ret)
+			dev_dbg(&adev->dev, "could not set default pinstate\n");
+	}
+
+	dev->pins_idle = pinctrl_lookup_state(dev->pinctrl,
+					      PINCTRL_STATE_IDLE);
+	if (IS_ERR(dev->pins_idle))
+		dev_dbg(&adev->dev, "could not get idle pinstate\n");
+	else {
+		/* If possible, let's go to idle until the first transfer */
+		ret = pinctrl_select_state(dev->pinctrl,
+					   dev->pins_idle);
+		if (ret)
+			dev_dbg(&adev->dev, "could not set idle pinstate\n");
+	}
+
+	dev->pins_sleep = pinctrl_lookup_state(dev->pinctrl,
+					       PINCTRL_STATE_SLEEP);
+	if (IS_ERR(dev->pins_sleep))
+		dev_dbg(&adev->dev, "could not get sleep pinstate\n");
+
 	dev->virtbase = ioremap(adev->res.start, resource_size(&adev->res));
 	if (!dev->virtbase) {
 		ret = -ENOMEM;
@@ -964,6 +1055,12 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
 		goto err_no_clk;
 	}
 
+	ret = clk_prepare(dev->clk);
+	if (ret) {
+		dev_err(&adev->dev, "can't prepare clock\n");
+		goto err_prep_clk;
+	}
+
 	adap = &dev->adap;
 	adap->dev.parent = &adev->dev;
 	adap->owner	= THIS_MODULE;
@@ -999,6 +1096,8 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
 	return 0;
 
  err_add_adap:
+	clk_unprepare(dev->clk);
+ err_prep_clk:
 	clk_put(dev->clk);
  err_no_clk:
 	free_irq(dev->irq, dev);
@@ -1007,6 +1106,7 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
  err_no_ioremap:
 	amba_set_drvdata(adev, NULL);
 	kfree(dev);
+ err_pinctrl:
  err_no_mem:
 
 	return ret;
@@ -1027,6 +1127,7 @@ static int nmk_i2c_remove(struct amba_device *adev)
 	iounmap(dev->virtbase);
 	if (res)
 		release_mem_region(res->start, resource_size(res));
+	clk_unprepare(dev->clk);
 	clk_put(dev->clk);
 	pm_runtime_disable(&adev->dev);
 	amba_set_drvdata(adev, NULL);
-- 
1.7.11.3

Re: [PATCH 2/2 v3] i2c: nomadik: adopt pinctrl support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 7944 bytes --]

Hi,

On Wed, Oct 10, 2012 at 01:02:39PM +0200, Linus Walleij wrote:
> From: Patrice Chotard <patrice.chotard-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org>
> 
> Amend the I2C nomadik pin controller to optionally take a pin control
> handle and set the state of the pins to:
> 
> - "default" on boot, resume and before performing an i2c transfer
> - "idle" after initial default, after resume default, and after each
>    i2c xfer
> - "sleep" on suspend()
> 
> This should make it possible to optimize energy usage for the pins
> both for the suspend/resume cycle, and for runtime cases inbetween
> I2C transfers.
> 
> Signed-off-by: Patrice Chotard <patrice.chotard-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org>
> Signed-off-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

One minor issue I'd fix myself...

> ---
> ChangeLog v2->v3:
> - Rebase on top of the patch from Philippe/Ulf.

... if this was really true which I am unsure.

> ChangeLog v1->v2:
> - We used only two states initially: default and sleep. It turns
>   out you can save some energy when idling (between transfers)
>   and even more when suspending on our platform, so grab all
>   three states and use them as applicable.
> ---
>  drivers/i2c/busses/i2c-nomadik.c | 101 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 101 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
> index 3eeae52..d50b16a 100644
> --- a/drivers/i2c/busses/i2c-nomadik.c
> +++ b/drivers/i2c/busses/i2c-nomadik.c
> @@ -24,6 +24,7 @@
>  #include <linux/io.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/platform_data/i2c-nomadik.h>
> +#include <linux/pinctrl/consumer.h>
>  
>  #define DRIVER_NAME "nmk-i2c"
>  
> @@ -145,6 +146,10 @@ struct i2c_nmk_client {
>   * @stop: stop condition.
>   * @xfer_complete: acknowledge completion for a I2C message.
>   * @result: controller propogated result.
> + * @pinctrl: pinctrl handle.
> + * @pins_default: default state for the pins.
> + * @pins_idle: idle state for the pins.
> + * @pins_sleep: sleep state for the pins.
>   * @busy: Busy doing transfer.
>   */
>  struct nmk_i2c_dev {
> @@ -158,6 +163,11 @@ struct nmk_i2c_dev {
>  	int				stop;
>  	struct completion		xfer_complete;
>  	int				result;
> +	/* Three pin states - default, idle & sleep */
> +	struct pinctrl			*pinctrl;
> +	struct pinctrl_state		*pins_default;
> +	struct pinctrl_state		*pins_idle;
> +	struct pinctrl_state		*pins_sleep;
>  	bool				busy;
>  };
>  
> @@ -648,6 +658,15 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
>  		goto out_clk;
>  	}
>  
> +	/* Optionaly enable pins to be muxed in and configured */
> +	if (!IS_ERR(dev->pins_default)) {
> +		status = pinctrl_select_state(dev->pinctrl,
> +				dev->pins_default);
> +		if (status)
> +			dev_err(&dev->adev->dev,
> +				"could not set default pins\n");

I am not too strict on the 80 char limit, so if it is just a little
over, you can keep it one line. I don't mind here...

> +	}
> +
>  	status = init_hw(dev);
>  	if (status)
>  		goto out;
> @@ -675,6 +694,15 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
>  out:
>  	clk_disable_unprepare(dev->clk);
>  out_clk:
> +	/* Optionally let pins go into idle state */
> +	if (!IS_ERR(dev->pins_idle)) {
> +		status = pinctrl_select_state(dev->pinctrl,
> +				dev->pins_idle);
> +		if (status)
> +			dev_err(&dev->adev->dev,
> +				"could not set pins to idle state\n");

... and here ...

> +	}
> +
>  	pm_runtime_put_sync(&dev->adev->dev);
>  
>  	dev->busy = false;
> @@ -869,15 +897,44 @@ static int nmk_i2c_suspend(struct device *dev)
>  {
>  	struct amba_device *adev = to_amba_device(dev);
>  	struct nmk_i2c_dev *nmk_i2c = amba_get_drvdata(adev);
> +	int ret;
>  
>  	if (nmk_i2c->busy)
>  		return -EBUSY;
>  
> +	if (!IS_ERR(nmk_i2c->pins_sleep)) {
> +		ret = pinctrl_select_state(nmk_i2c->pinctrl,
> +				nmk_i2c->pins_sleep);
> +		if (ret)
> +			dev_err(dev,
> +				"could not set pins to sleep state\n");

... but I'd think here one line is the better choice.

> +	}
> +
>  	return 0;
>  }
>  
>  static int nmk_i2c_resume(struct device *dev)
>  {
> +	struct amba_device *adev = to_amba_device(dev);
> +	struct nmk_i2c_dev *nmk_i2c = amba_get_drvdata(adev);
> +	int ret;
> +
> +	/* First go to the default state */
> +	if (!IS_ERR(nmk_i2c->pins_default)) {
> +		ret = pinctrl_select_state(nmk_i2c->pinctrl,
> +				nmk_i2c->pins_default);
> +		if (ret)
> +			dev_err(dev,
> +				"could not set pins to default state\n");

ditto

> +	}
> +	/* Then let's idle the pins until the next transfer happens */
> +	if (!IS_ERR(nmk_i2c->pins_idle)) {
> +		ret = pinctrl_select_state(nmk_i2c->pinctrl,
> +				nmk_i2c->pins_idle);
> +		if (ret)
> +			dev_err(dev,
> +				"could not set pins to idle state\n");

ditto

> +	}
>  	return 0;
>  }
>  #else
> @@ -941,6 +998,40 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
>  	dev->adev = adev;
>  	amba_set_drvdata(adev, dev);
>  
> +	dev->pinctrl = devm_pinctrl_get(&adev->dev);
> +	if (IS_ERR(dev->pinctrl)) {
> +		ret = PTR_ERR(dev->pinctrl);
> +		goto err_pinctrl;
> +	}
> +
> +	dev->pins_default = pinctrl_lookup_state(dev->pinctrl,
> +						 PINCTRL_STATE_DEFAULT);
> +	if (IS_ERR(dev->pins_default))
> +		dev_err(&adev->dev, "could not get default pinstate\n");
> +	else {
> +		ret = pinctrl_select_state(dev->pinctrl,
> +					   dev->pins_default);
> +		if (ret)
> +			dev_dbg(&adev->dev, "could not set default pinstate\n");
> +	}
> +
> +	dev->pins_idle = pinctrl_lookup_state(dev->pinctrl,
> +					      PINCTRL_STATE_IDLE);
> +	if (IS_ERR(dev->pins_idle))
> +		dev_dbg(&adev->dev, "could not get idle pinstate\n");
> +	else {
> +		/* If possible, let's go to idle until the first transfer */
> +		ret = pinctrl_select_state(dev->pinctrl,
> +					   dev->pins_idle);
> +		if (ret)
> +			dev_dbg(&adev->dev, "could not set idle pinstate\n");
> +	}
> +
> +	dev->pins_sleep = pinctrl_lookup_state(dev->pinctrl,
> +					       PINCTRL_STATE_SLEEP);
> +	if (IS_ERR(dev->pins_sleep))
> +		dev_dbg(&adev->dev, "could not get sleep pinstate\n");
> +
>  	dev->virtbase = ioremap(adev->res.start, resource_size(&adev->res));
>  	if (!dev->virtbase) {
>  		ret = -ENOMEM;
> @@ -964,6 +1055,12 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
>  		goto err_no_clk;
>  	}
>  
> +	ret = clk_prepare(dev->clk);
> +	if (ret) {
> +		dev_err(&adev->dev, "can't prepare clock\n");
> +		goto err_prep_clk;
> +	}
> +

And here I am not sure if the rebasing worked. I don't think you want
the clock handling here. If you do, it is a seperate patch.

>  	adap = &dev->adap;
>  	adap->dev.parent = &adev->dev;
>  	adap->owner	= THIS_MODULE;
> @@ -999,6 +1096,8 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
>  	return 0;
>  
>   err_add_adap:
> +	clk_unprepare(dev->clk);
> + err_prep_clk:
>  	clk_put(dev->clk);
>   err_no_clk:
>  	free_irq(dev->irq, dev);
> @@ -1007,6 +1106,7 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
>   err_no_ioremap:
>  	amba_set_drvdata(adev, NULL);
>  	kfree(dev);
> + err_pinctrl:
>   err_no_mem:
>  
>  	return ret;
> @@ -1027,6 +1127,7 @@ static int nmk_i2c_remove(struct amba_device *adev)
>  	iounmap(dev->virtbase);
>  	if (res)
>  		release_mem_region(res->start, resource_size(res));
> +	clk_unprepare(dev->clk);
>  	clk_put(dev->clk);
>  	pm_runtime_disable(&adev->dev);
>  	amba_set_drvdata(adev, NULL);
> -- 
> 1.7.11.3
> 

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]