[3/3] i2c: nomadik: use pinctrl PM helpers
diff mbox series

Message ID 1370439873-30053-3-git-send-email-linus.walleij@stericsson.com
State New, archived
Headers show
Series
  • [1/3] drivers: pinctrl sleep and idle states in the core
Related show

Commit Message

Linus Walleij June 5, 2013, 1:44 p.m. UTC
From: Linus Walleij <linus.walleij@linaro.org>

This utilize the new pinctrl core PM helpers to transition
the driver to "sleep" and "idle" states, cutting away some
boilerplate code.

Cc: Hebbar Gururaja <gururaja.hebbar@ti.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Kevin Hilman <khilman@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Wolfram Sang <wsa@the-dreams.de>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
I'm seeking Wolfram's ACK on this to take it through the
pinctrl tree in the end.
---
 drivers/i2c/busses/i2c-nomadik.c | 90 +++++-----------------------------------
 1 file changed, 10 insertions(+), 80 deletions(-)

Comments

Kevin Hilman June 5, 2013, 4:34 p.m. UTC | #1
Linus Walleij <linus.walleij@stericsson.com> writes:

> From: Linus Walleij <linus.walleij@linaro.org>
>
> This utilize the new pinctrl core PM helpers to transition
> the driver to "sleep" and "idle" states, cutting away some
> boilerplate code.
>
> Cc: Hebbar Gururaja <gururaja.hebbar@ti.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Kevin Hilman <khilman@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

I have some questions on the interaction with runtime PM here...

> ---
> I'm seeking Wolfram's ACK on this to take it through the
> pinctrl tree in the end.
> ---
>  drivers/i2c/busses/i2c-nomadik.c | 90 +++++-----------------------------------
>  1 file changed, 10 insertions(+), 80 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
> index 650293f..c7e3b0c 100644
> --- a/drivers/i2c/busses/i2c-nomadik.c
> +++ b/drivers/i2c/busses/i2c-nomadik.c
> @@ -148,10 +148,6 @@ 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 {
> @@ -165,11 +161,6 @@ 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;
>  };
>  
> @@ -645,13 +636,7 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
>  	}
>  
>  	/* 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");
> -	}
> +	pinctrl_pm_select_default_state(&dev->adev->dev);

Shouldn't this be in the ->runtime_resume() callback of the driver (the
original code should've as well.)

IOW, the pinctrl changes only need to happen when the runtime PM
usecount goes from zero to 1.  For any additional users, the device will
already be active and pins already in default state.

I'm not familiar with this HW, and maybe the driver already prevents
multiple users, but for correctness sake (and because others will copy
this), the (re)muxing should be in the runtime PM callback.

Also, IMO, that's further evidence that the pinctrl stuff could (and
probably should) be handled by the PM core.

>  	status = init_hw(dev);
>  	if (status)
> @@ -681,13 +666,7 @@ 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");
> -	}
> +	pinctrl_pm_select_idle_state(&dev->adev->dev);

Again, if there are multiple users, you're changing the pin states
without knowing if you're the last user or not (again, the problem
existed in the original code as well.)

Runtime PM is doing the usecouning, so this should be in the
->runtime_suspend() callback.

>  	pm_runtime_put_sync(&dev->adev->dev);
>  

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Linus Walleij June 7, 2013, 8:33 a.m. UTC | #2
On Wed, Jun 5, 2013 at 6:34 PM, Kevin Hilman <khilman@linaro.org> wrote:

>> From: Linus Walleij <linus.walleij@linaro.org>
>>
>> This utilize the new pinctrl core PM helpers to transition
>> the driver to "sleep" and "idle" states, cutting away some
>> boilerplate code.
>>
>> Cc: Hebbar Gururaja <gururaja.hebbar@ti.com>
>> Cc: Mark Brown <broonie@kernel.org>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: Kevin Hilman <khilman@linaro.org>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Stephen Warren <swarren@wwwdotorg.org>
>> Cc: Wolfram Sang <wsa@the-dreams.de>
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>
> I have some questions on the interaction with runtime PM here...

OK, to get the basic infrastructure in place I have merged these
patches with the I2C maintainers ACK, since it is doing one thing,
i.e. moving the functionality out of the driver and into the pinctrl
core.

I am considering semantic changes related to runtime PM in
addition to this as a separate patch, so let's start talking about
that here.

It would be inappropriate to try to create a patch that is
changing these two things at once, but let's see where we end
up by the merge window.

>> @@ -645,13 +636,7 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
>>       }
>>
>>       /* 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");
>> -     }
>> +     pinctrl_pm_select_default_state(&dev->adev->dev);
>
> Shouldn't this be in the ->runtime_resume() callback of the driver (the
> original code should've as well.)
>
> IOW, the pinctrl changes only need to happen when the runtime PM
> usecount goes from zero to 1.  For any additional users, the device will
> already be active and pins already in default state.
>
> I'm not familiar with this HW, and maybe the driver already prevents
> multiple users, but for correctness sake (and because others will copy
> this), the (re)muxing should be in the runtime PM callback.

I2C message are serialized/marshalled by nature so it's actually
not causing a concurrency problem: this xfer function will not be
called from two places for the same driver.

What is true however is that we're hammering the pins from
active to idle for every transfer, instead of letting runtime PM
provide some nice hysteresis (autosuspend) around that.

Notice though that:

- This driver has no driver-local runtime PM callbacks, so the
  runtime PM calls are intended to inform the rest of the system,
  such as the bus, that the device is idle.

- The bus used is the AMBA (PrimeCell) bus,
  drivers/amba/bus.c

> Also, IMO, that's further evidence that the pinctrl stuff could (and
> probably should) be handled by the PM core.

So I'm now thinking about how to achieve this.

What happens for this driver when the usecount goes from
1->0 is (the other way is very similar):

drivers/base/power/runtime.c

        if (dev->pm_domain)
                callback = dev->pm_domain->ops.runtime_suspend;
        else if (dev->type && dev->type->pm)
                callback = dev->type->pm->runtime_suspend;
        else if (dev->class && dev->class->pm)
                callback = dev->class->pm->runtime_suspend;
        else if (dev->bus && dev->bus->pm)
                callback = dev->bus->pm->runtime_suspend;
        else
                callback = NULL;

        if (!callback && dev->driver && dev->driver->pm)
                callback = dev->driver->pm->runtime_suspend;

        retval = rpm_callback(callback, dev);

This platform will currently hit dev->bus->pm->runtime_suspend
to drivers/amba/bus.c:

static int amba_pm_runtime_suspend(struct device *dev)
{
        struct amba_device *pcdev = to_amba_device(dev);
        int ret = pm_generic_runtime_suspend(dev);

        if (ret == 0 && dev->driver)
                clk_disable(pcdev->pclk);

        return ret;
}

The pm_generic_runtime_suspend will call the driver callbacks
(none in this case).

Then the bus core proceeds to gate off the block clock and
we're done.

I could make a patch adding runtime PM ops to the
driver itself to set the pinctrl state from there, which would
be a nice improvement in itself.

But we're discussing handling it all in the PM core, so
let's think bigger.

If we're making this all generic, were in this chain do you
suggest that I set the pins to idle?
drivers/base/power/runtime.c?

One thing in particular that worries me here is the ordering
of things, because that has been a severe issue for us.

For example: maybe on a certain platform pins need to
be idled/defaulted *before* calling the PM domain or
bus callbacks are executed, because of transient IRQs
and whatnot. So I put my pinctrl_pm_select_idle_state()
*before* the chain of calls.

But sometimes you may want to execute the
pinctrl_pm_select_idle_state() *after* all other things have
been done, including the calls to the domain/bus/driver.

And this is only for the runtime suspend/resume path.

For the common suspend/resume path things get more
complex still. Users may need to call
pinctrl_pm_select_sleep_state() in the middle of the
code sending the platform done, and will not survive it
being called by the PM core, and we'd need to add a flag
for this etc.

To sum up I am afraid of a can of worms of corner cases
on something that looks simple here. Thus I cannot really
make a patch moving pinctrl state selection to the PM
core, I don't know the business there well enough, I just know
there are tigers in there :-/

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Kevin Hilman June 7, 2013, 2:31 p.m. UTC | #3
Linus Walleij <linus.walleij@linaro.org> writes:

> On Wed, Jun 5, 2013 at 6:34 PM, Kevin Hilman <khilman@linaro.org> wrote:
>
>>> From: Linus Walleij <linus.walleij@linaro.org>
>>>
>>> This utilize the new pinctrl core PM helpers to transition
>>> the driver to "sleep" and "idle" states, cutting away some
>>> boilerplate code.
>>>
>>> Cc: Hebbar Gururaja <gururaja.hebbar@ti.com>
>>> Cc: Mark Brown <broonie@kernel.org>
>>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>> Cc: Kevin Hilman <khilman@linaro.org>
>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> Cc: Stephen Warren <swarren@wwwdotorg.org>
>>> Cc: Wolfram Sang <wsa@the-dreams.de>
>>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>>
>> I have some questions on the interaction with runtime PM here...
>
> OK, to get the basic infrastructure in place I have merged these
> patches with the I2C maintainers ACK, since it is doing one thing,
> i.e. moving the functionality out of the driver and into the pinctrl
> core.
>
> I am considering semantic changes related to runtime PM in
> addition to this as a separate patch, so let's start talking about
> that here.
>
> It would be inappropriate to try to create a patch that is
> changing these two things at once, but let's see where we end
> up by the merge window.
>
>>> @@ -645,13 +636,7 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
>>>       }
>>>
>>>       /* 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");
>>> -     }
>>> +     pinctrl_pm_select_default_state(&dev->adev->dev);
>>
>> Shouldn't this be in the ->runtime_resume() callback of the driver (the
>> original code should've as well.)
>>
>> IOW, the pinctrl changes only need to happen when the runtime PM
>> usecount goes from zero to 1.  For any additional users, the device will
>> already be active and pins already in default state.
>>
>> I'm not familiar with this HW, and maybe the driver already prevents
>> multiple users, but for correctness sake (and because others will copy
>> this), the (re)muxing should be in the runtime PM callback.
>
> I2C message are serialized/marshalled by nature so it's actually
> not causing a concurrency problem: this xfer function will not be
> called from two places for the same driver.
>
> What is true however is that we're hammering the pins from
> active to idle for every transfer, instead of letting runtime PM
> provide some nice hysteresis (autosuspend) around that.
>
> Notice though that:
>
> - This driver has no driver-local runtime PM callbacks, so the
>   runtime PM calls are intended to inform the rest of the system,
>   such as the bus, that the device is idle.
>
> - The bus used is the AMBA (PrimeCell) bus,
>   drivers/amba/bus.c
>
>> Also, IMO, that's further evidence that the pinctrl stuff could (and
>> probably should) be handled by the PM core.
>
> So I'm now thinking about how to achieve this.
>
> What happens for this driver when the usecount goes from
> 1->0 is (the other way is very similar):
>
> drivers/base/power/runtime.c
>
>         if (dev->pm_domain)
>                 callback = dev->pm_domain->ops.runtime_suspend;
>         else if (dev->type && dev->type->pm)
>                 callback = dev->type->pm->runtime_suspend;
>         else if (dev->class && dev->class->pm)
>                 callback = dev->class->pm->runtime_suspend;
>         else if (dev->bus && dev->bus->pm)
>                 callback = dev->bus->pm->runtime_suspend;
>         else
>                 callback = NULL;
>
>         if (!callback && dev->driver && dev->driver->pm)
>                 callback = dev->driver->pm->runtime_suspend;
>
>         retval = rpm_callback(callback, dev);
>
> This platform will currently hit dev->bus->pm->runtime_suspend
> to drivers/amba/bus.c:
>
> static int amba_pm_runtime_suspend(struct device *dev)
> {
>         struct amba_device *pcdev = to_amba_device(dev);
>         int ret = pm_generic_runtime_suspend(dev);
>
>         if (ret == 0 && dev->driver)
>                 clk_disable(pcdev->pclk);
>
>         return ret;
> }
>
> The pm_generic_runtime_suspend will call the driver callbacks
> (none in this case).
>
> Then the bus core proceeds to gate off the block clock and
> we're done.
>
> I could make a patch adding runtime PM ops to the
> driver itself to set the pinctrl state from there, which would
> be a nice improvement in itself.
>
> But we're discussing handling it all in the PM core, so
> let's think bigger.
>
> If we're making this all generic, were in this chain do you
> suggest that I set the pins to idle?
> drivers/base/power/runtime.c?

Yes, that's where I was thinking.

> One thing in particular that worries me here is the ordering
> of things, because that has been a severe issue for us.

In the original series from Hebbar Gururaja, Grygorii Strashko pointed
out that we may have some of the ordering issues on OMAP as well.  I
hadn't thought about that (enough) yet.

> For example: maybe on a certain platform pins need to
> be idled/defaulted *before* calling the PM domain or
> bus callbacks are executed, because of transient IRQs
> and whatnot. So I put my pinctrl_pm_select_idle_state()
> *before* the chain of calls.

Right, that corresponds to the runtime PM core calling the drivers
->runtime_suspend() callback before the subsystem/bus/domain has done
its thing.

> But sometimes you may want to execute the
> pinctrl_pm_select_idle_state() *after* all other things have
> been done, including the calls to the domain/bus/driver.

Whether it's in the PM core or not, with runtime PM today, there is no
easy way to do this from the driver ($SUBJECT patch assumes a single
user, which is not true in general.)

The only way a driver truly knows that the domain/bus/subsystem calls
have been done is when its own callback is called, and for suspend this
only happens *before* the device is actually idled.  This is effectively
a pre-runtime_suspend callback.  We don't currently have a
post-runtime_suspend callback (or a pre-runtime_resume callback.)  

Might be we need something like that to do this in a generic way.

> And this is only for the runtime suspend/resume path.
>
> For the common suspend/resume path things get more
> complex still. Users may need to call
> pinctrl_pm_select_sleep_state() in the middle of the
> code sending the platform done, and will not survive it
> being called by the PM core, and we'd need to add a flag
> for this etc.
>
> To sum up I am afraid of a can of worms of corner cases
> on something that looks simple here. Thus I cannot really
> make a patch moving pinctrl state selection to the PM
> core, I don't know the business there well enough, I just know
> there are tigers in there :-/

Yeah, the static PM case is definitely a can of worms, especially in the
case where many devices are already runtime suspended.  That defnitely
needs some serious thought and testing before handling in the PM core.

So for now, it's probably not a good idea to move things to the PM core
until we see a strong pattern emerging.

That being said, maybe we could handle this in the
subsystem/bus/pm_domain level though (in your case AMBA, in OMAP case
the pm_domain) to still avoid sprinkling this across all the drivers?

Kevin

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Patch
diff mbox series

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 650293f..c7e3b0c 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -148,10 +148,6 @@  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 {
@@ -165,11 +161,6 @@  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;
 };
 
@@ -645,13 +636,7 @@  static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
 	}
 
 	/* 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");
-	}
+	pinctrl_pm_select_default_state(&dev->adev->dev);
 
 	status = init_hw(dev);
 	if (status)
@@ -681,13 +666,7 @@  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");
-	}
+	pinctrl_pm_select_idle_state(&dev->adev->dev);
 
 	pm_runtime_put_sync(&dev->adev->dev);
 
@@ -882,41 +861,22 @@  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");
-	}
+	pinctrl_pm_select_sleep_state(dev);
 
 	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");
-	}
+	pinctrl_pm_select_default_state(dev);
 	/* 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");
-	}
+	pinctrl_pm_select_idle_state(dev);
+
 	return 0;
 }
 #else
@@ -1004,39 +964,10 @@  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");
+	/* Select default pin state */
+	pinctrl_pm_select_default_state(&adev->dev);
+	/* If possible, let's go to idle until the first transfer */
+	pinctrl_pm_select_idle_state(&adev->dev);
 
 	dev->virtbase = ioremap(adev->res.start, resource_size(&adev->res));
 	if (!dev->virtbase) {
@@ -1106,7 +1037,6 @@  static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
 	iounmap(dev->virtbase);
  err_no_ioremap:
 	kfree(dev);
- err_pinctrl:
  err_no_mem:
 
 	return ret;