Hi,
On Wed, Oct 10, 2012 at 01:02:39PM +0200, Linus Walleij wrote:
> From: Patrice Chotard <patrice.chotard@xxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxx>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
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:
signature.asc
Description: Digital signature
