[PATCH 00/11] drivers: Add Pinctrl PM support

[PATCH 00/11] drivers: Add Pinctrl PM support

[Hebbar Gururaja]

By optionally putting the pins into sleep state in the suspend [or in
runtime_suspend] callback we can accomplish two things.
- One is to minimize current leakage from pins and thus save power,
- second, we can prevent the IP from driving pins output in an
uncontrolled manner, which may happen if the power domain drops the
domain regulator.

These states can be specified in the DT blob and corresponding driver
can pick these states during probe & set the related values during
idle/suspend.

Not all drivers support/has idle state. Drivers like i2c, spi, mmc has
idle states and hence these drivers are updated to support all the
three states
- default  : during regular operation
- idle : when the module is in idle state
- sleep : when the module is in suspend state

For those drivers which doesn't support/have idle state (at least at
the moment), only default & sleep state is added.

The modification expects DT blob to provide the phandler & the pinctrl
states otherwise it gives a warning message. To remove this warning
message pass default state with null phandler to pinctrl in device node

Kernel Base for the series is
	> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/
	linux-2.6.git
	> tag: v3.10-rc3

Hebbar Gururaja (11):
  pinctrl: single: adopt pinctrl sleep mode management
  leds: leds-gpio: Enhance pinctrl support
  Input: gpio_keys: Adopt pinctrl support
  Input: matrix-keypad: Adopt pinctrl support
  spi: omap2-mcspi: enhance pinctrl support
  usb: musb: dsps: Adopt pinctrl support
  pwm: pwm-tiehrpwm: enhance pinctrl support
  pwm: pwm-tiecap: enhance pinctrl support
  mmc: omap_hsmmc: enhance pinctrl support
  video: da8xx-fb: adopt pinctrl support
  i2c: omap: enhance pinctrl support

 drivers/i2c/busses/i2c-omap.c          |  112 ++++++++++++++++++++++++++++++--
 drivers/input/keyboard/gpio_keys.c     |   47 ++++++++++++++
 drivers/input/keyboard/matrix_keypad.c |   48 ++++++++++++++
 drivers/leds/leds-gpio.c               |   81 +++++++++++++++++++++--
 drivers/mmc/host/omap_hsmmc.c          |   79 ++++++++++++++++++++--
 drivers/pinctrl/pinctrl-single.c       |   27 ++++++++
 drivers/pwm/pwm-tiecap.c               |   48 ++++++++++++--
 drivers/pwm/pwm-tiehrpwm.c             |   49 ++++++++++++--
 drivers/spi/spi-omap2-mcspi.c          |   89 +++++++++++++++++++++++--
 drivers/usb/musb/musb_dsps.c           |   46 +++++++++++++
 drivers/video/da8xx-fb.c               |   48 ++++++++++++++
 11 files changed, 641 insertions(+), 33 deletions(-)

-- 
1.7.9.5

[PATCH 01/11] pinctrl: single: adopt pinctrl sleep mode management

[Hebbar Gururaja]

Make pinctrl-single able to handle suspend/resume events and change
hogged pins states accordingly.

Signed-off-by: Hebbar Gururaja <gururaja.hebbar@ti.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
---
:100644 100644 b9fa046... bfd4f6a... M	drivers/pinctrl/pinctrl-single.c
 drivers/pinctrl/pinctrl-single.c |   27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index b9fa046..bfd4f6a 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -1346,6 +1346,29 @@ static int pcs_add_gpio_func(struct device_node *node, struct pcs_device *pcs)
 	return ret;
 }
 
+static int pinctrl_single_suspend(struct platform_device *pdev,
+					pm_message_t state)
+{
+	struct pcs_device *pcs;
+
+	pcs = platform_get_drvdata(pdev);
+	if (!pcs)
+		return -EINVAL;
+
+	return pinctrl_force_sleep(pcs->pctl);
+}
+
+static int pinctrl_single_resume(struct platform_device *pdev)
+{
+	struct pcs_device *pcs;
+
+	pcs = platform_get_drvdata(pdev);
+	if (!pcs)
+		return -EINVAL;
+
+	return pinctrl_force_default(pcs->pctl);
+}
+
 static int pcs_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -1494,6 +1517,10 @@ static struct platform_driver pcs_driver = {
 		.name		= DRIVER_NAME,
 		.of_match_table	= pcs_of_match,
 	},
+#ifdef CONFIG_PM
+	.suspend = pinctrl_single_suspend,
+	.resume = pinctrl_single_resume,
+#endif
 };
 
 module_platform_driver(pcs_driver);
-- 
1.7.9.5

[PATCH 01/11] pinctrl: single: adopt pinctrl sleep mode management

[Linus Walleij]

On Fri, May 31, 2013 at 12:13 PM, Hebbar Gururaja
<gururaja.hebbar@ti.com> wrote:

> Make pinctrl-single able to handle suspend/resume events and change
> hogged pins states accordingly.
>
> Signed-off-by: Hebbar Gururaja <gururaja.hebbar@ti.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>

Noone said anything about this patch, but it looks very straight-forward
to me and seems sound so I have applied it.

If Tony or Haojian has strong feelings about it I will dequeue it.

Yours,
Linus Walleij

[PATCH 01/11] pinctrl: single: adopt pinctrl sleep mode management

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [130617 04:38]:
> On Fri, May 31, 2013 at 12:13 PM, Hebbar Gururaja
> <gururaja.hebbar@ti.com> wrote:
> 
> > Make pinctrl-single able to handle suspend/resume events and change
> > hogged pins states accordingly.
> >
> > Signed-off-by: Hebbar Gururaja <gururaja.hebbar@ti.com>
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> 
> Noone said anything about this patch, but it looks very straight-forward
> to me and seems sound so I have applied it.
> 
> If Tony or Haojian has strong feelings about it I will dequeue it.

I don't like the hogged pins at all as they prevent me from unloading
pinctrl-single.. But the patch seems fine to me:

Acked-by: Tony Lindgren <tony@atomide.com>

[PATCH 01/11] pinctrl: single: adopt pinctrl sleep mode management

[Linus Walleij]

On Mon, Jun 17, 2013 at 2:03 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Linus Walleij <linus.walleij@linaro.org> [130617 04:38]:
>> On Fri, May 31, 2013 at 12:13 PM, Hebbar Gururaja
>> <gururaja.hebbar@ti.com> wrote:
>>
>> > Make pinctrl-single able to handle suspend/resume events and change
>> > hogged pins states accordingly.
>> >
>> > Signed-off-by: Hebbar Gururaja <gururaja.hebbar@ti.com>
>> > Cc: Linus Walleij <linus.walleij@linaro.org>
>>
>> Noone said anything about this patch, but it looks very straight-forward
>> to me and seems sound so I have applied it.
>>
>> If Tony or Haojian has strong feelings about it I will dequeue it.
>
> I don't like the hogged pins at all as they prevent me from unloading
> pinctrl-single.. But the patch seems fine to me:

Hm? I think the hogged pins are the *only* pins we can
reliably release when unloading the driver, look:

void pinctrl_unregister(struct pinctrl_dev *pctldev)
{
(...)
        if (!IS_ERR(pctldev->p))
                pinctrl_put(pctldev->p);

That releases all hogged pinctrl handles when unloading
a pinctrl driver.

Yours,
Linus Walleij

[PATCH 01/11] pinctrl: single: adopt pinctrl sleep mode management

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [130617 09:14]:
> On Mon, Jun 17, 2013 at 2:03 PM, Tony Lindgren <tony@atomide.com> wrote:
> > * Linus Walleij <linus.walleij@linaro.org> [130617 04:38]:
> >> On Fri, May 31, 2013 at 12:13 PM, Hebbar Gururaja
> >> <gururaja.hebbar@ti.com> wrote:
> >>
> >> > Make pinctrl-single able to handle suspend/resume events and change
> >> > hogged pins states accordingly.
> >> >
> >> > Signed-off-by: Hebbar Gururaja <gururaja.hebbar@ti.com>
> >> > Cc: Linus Walleij <linus.walleij@linaro.org>
> >>
> >> Noone said anything about this patch, but it looks very straight-forward
> >> to me and seems sound so I have applied it.
> >>
> >> If Tony or Haojian has strong feelings about it I will dequeue it.
> >
> > I don't like the hogged pins at all as they prevent me from unloading
> > pinctrl-single.. But the patch seems fine to me:
> 
> Hm? I think the hogged pins are the *only* pins we can
> reliably release when unloading the driver, look:

In most cases all the pins can be released reliably if
the pinctrl driver won't do anything with the pins on
release.

After all that's what bootloaders have been doing for
ages :)

> void pinctrl_unregister(struct pinctrl_dev *pctldev)
> {
> (...)
>         if (!IS_ERR(pctldev->p))
>                 pinctrl_put(pctldev->p);
> 
> That releases all hogged pinctrl handles when unloading
> a pinctrl driver.

Hmm maybe I need to try it again. The last time I tried
I had to comment those out, but that was a while back.
And I probably had also some other hack to release pins.

Regards,

Tony

[PATCH 02/11] leds: leds-gpio: Enhance pinctrl support

[Hebbar Gururaja]

Amend leds-gpio driver to optionally take a pin control handle and set
the state of the pins to:

- "default" on boot, resume
- "sleep" on suspend()

By optionally putting the pins into sleep state in the suspend callback
we can accomplish two things.
- One is to minimize current leakage from pins and thus save power,
- second, we can prevent the IP from driving pins output in an
uncontrolled manner, which may happen if the power domain drops the
domain regulator.

If any of the above pin states are missing in dt, a warning message
about the missing state is displayed.
If certain pin-states are not available, to remove this warning message
pass respective state name with null phandler.

Signed-off-by: Hebbar Gururaja <gururaja.hebbar@ti.com>
Cc: Bryan Wu <cooloney@gmail.com>
Cc: Richard Purdie <rpurdie@rpsys.net>
Cc: linux-leds at vger.kernel.org
---
:100644 100644 b02b679... b094e52... M	drivers/leds/leds-gpio.c
 drivers/leds/leds-gpio.c |   81 ++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 75 insertions(+), 6 deletions(-)

diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index b02b679..b094e52 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -153,6 +153,10 @@ static void delete_gpio_led(struct gpio_led_data *led)
 
 struct gpio_leds_priv {
 	int num_leds;
+	/* Two optional pin states - default & sleep */
+	struct pinctrl		*pinctrl;
+	struct pinctrl_state	*pins_default;
+	struct pinctrl_state	*pins_sleep;
 	struct gpio_led_data leds[];
 };
 
@@ -162,6 +166,43 @@ static inline int sizeof_gpio_leds_priv(int num_leds)
 		(sizeof(struct gpio_led_data) * num_leds);
 }
 
+/* Use pinctrl API for gpio configuration */
+static void gpio_led_get_pinctrl(struct gpio_leds_priv *priv,
+				struct platform_device *pdev)
+{
+	priv->pinctrl = devm_pinctrl_get(&pdev->dev);
+	if (!IS_ERR(priv->pinctrl)) {
+		priv->pins_default = pinctrl_lookup_state(priv->pinctrl,
+						PINCTRL_STATE_DEFAULT);
+		/* enable pins to be muxed in and configured */
+		if (IS_ERR(priv->pins_default))
+			dev_dbg(&pdev->dev,
+				"could not get default pinstate\n");
+		else
+			if (pinctrl_select_state(priv->pinctrl,
+						 priv->pins_default))
+				dev_err(&pdev->dev,
+					"could not set default pins\n");
+
+		priv->pins_sleep = pinctrl_lookup_state(priv->pinctrl,
+						PINCTRL_STATE_SLEEP);
+		if (IS_ERR(priv->pins_sleep))
+			dev_dbg(&pdev->dev,
+				"could not get sleep pinstate\n");
+	} else {
+		/*
+		* Since we continue even when pinctrl node is not found,
+		* Invalidate pins as not available. This is to make sure that
+		* IS_ERR(pins_xxx) results in failure when used.
+		*/
+		priv->pins_default = ERR_PTR(-ENODATA);
+		priv->pins_sleep = ERR_PTR(-ENODATA);
+
+		dev_dbg(&pdev->dev,
+			"pins are not configured from the driver\n");
+	}
+}
+
 /* Code to create from OpenFirmware platform devices */
 #ifdef CONFIG_OF_GPIO
 static struct gpio_leds_priv *gpio_leds_create_of(struct platform_device *pdev)
@@ -184,6 +225,8 @@ static struct gpio_leds_priv *gpio_leds_create_of(struct platform_device *pdev)
 	if (!priv)
 		return ERR_PTR(-ENOMEM);
 
+	gpio_led_get_pinctrl(priv, pdev);
+
 	for_each_child_of_node(np, child) {
 		struct gpio_led led = {};
 		enum of_gpio_flags flags;
@@ -236,14 +279,8 @@ static int gpio_led_probe(struct platform_device *pdev)
 {
 	struct gpio_led_platform_data *pdata = pdev->dev.platform_data;
 	struct gpio_leds_priv *priv;
-	struct pinctrl *pinctrl;
 	int i, ret = 0;
 
-	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
-	if (IS_ERR(pinctrl))
-		dev_warn(&pdev->dev,
-			"pins are not configured from the driver\n");
-
 	if (pdata && pdata->num_leds) {
 		priv = devm_kzalloc(&pdev->dev,
 				sizeof_gpio_leds_priv(pdata->num_leds),
@@ -251,6 +288,8 @@ static int gpio_led_probe(struct platform_device *pdev)
 		if (!priv)
 			return -ENOMEM;
 
+		gpio_led_get_pinctrl(priv, pdev);
+
 		priv->num_leds = pdata->num_leds;
 		for (i = 0; i < priv->num_leds; i++) {
 			ret = create_gpio_led(&pdata->leds[i],
@@ -287,6 +326,32 @@ static int gpio_led_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int gpio_led_suspend(struct platform_device *pdev,
+					pm_message_t state)
+{
+	struct gpio_leds_priv *priv = platform_get_drvdata(pdev);
+
+	/* Optionally let pins go into sleep states */
+	if (!IS_ERR(priv->pins_sleep))
+		if (pinctrl_select_state(priv->pinctrl, priv->pins_sleep))
+			dev_err(&pdev->dev,
+				"could not set pins to sleep state\n");
+
+	return 0;
+}
+
+static int gpio_led_resume(struct platform_device *pdev)
+{
+	struct gpio_leds_priv *priv = platform_get_drvdata(pdev);
+
+	/* Optionaly enable pins to be muxed in and configured */
+	if (!IS_ERR(priv->pins_default))
+		if (pinctrl_select_state(priv->pinctrl, priv->pins_default))
+			dev_err(&pdev->dev, "could not set default pins\n");
+
+	return 0;
+}
+
 static struct platform_driver gpio_led_driver = {
 	.probe		= gpio_led_probe,
 	.remove		= gpio_led_remove,
@@ -295,6 +360,10 @@ static struct platform_driver gpio_led_driver = {
 		.owner	= THIS_MODULE,
 		.of_match_table = of_match_ptr(of_gpio_leds_match),
 	},
+#ifdef CONFIG_PM
+	.suspend = gpio_led_suspend,
+	.resume = gpio_led_resume,
+#endif
 };
 
 module_platform_driver(gpio_led_driver);
-- 
1.7.9.5

[PATCH 02/11] leds: leds-gpio: Enhance pinctrl support

[Linus Walleij]

On Fri, May 31, 2013 at 12:13 PM, Hebbar Gururaja
<gururaja.hebbar@ti.com> wrote:

>  struct gpio_leds_priv {
>         int num_leds;
> +       /* Two optional pin states - default & sleep */
> +       struct pinctrl          *pinctrl;
> +       struct pinctrl_state    *pins_default;
> +       struct pinctrl_state    *pins_sleep;
>         struct gpio_led_data leds[];
>  };

Or actually, wait. You're adding quite a lot of these.

Could you try:

- Adding pins_sleep and pins_idle to struct dev_pin_info
  in include/linux/pinctrl/devinfo.h

- Modify drivers/base/pinctrl.c to optionally look up
  sleep and idle states, you can make that code
  #ifdef CONFIG_PM I think.

- Add something like static inline functions to
  include/linux/pinctrl/consumer.h
  with names like pinctrl_select_pm_idle(struct device *)
  pinctrl_select_pm_sleep(struct device *) to switch states
  using the device core containers, and includes
  checking IS_ERR() on the handles etc.

I think this will save a *lot* of identical code in all the
drivers, that will just have to call
pinctrl_select_pm_sleep(), pinctrl_select_pm_default()
pinctrl_select_pm_idle() instead of all the complex code.

This is what I planned to do but never got around to.

Please?

Yours,
Linus Walleij

[PATCH 03/11] Input: gpio_keys: Adopt pinctrl support

[Hebbar Gururaja]

Amend gpio-keys driver to optionally take a pin control handle and set
the state of the pins to:

- "default" on boot, resume
- "sleep" on suspend()

By Optionally putting the pins into sleep state in the suspend callback
we can accomplish two things.
- One is to minimize current leakage from pins and thus save power,
- second, we can prevent the IP from driving pins output in an
uncontrolled manner, which may happen if the power domain drops the
domain regulator.

If any of the above pin states are missing in dt, a warning message
about the missing state is displayed.
If certain pin-states are not available, to remove this warning message
pass respective state name with null phandler.

Todo:
- if an idle state is available for pins, add support for it.

Signed-off-by: Hebbar Gururaja <gururaja.hebbar@ti.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: linux-input at vger.kernel.org
---
:100644 100644 b29ca65... ca615a4... M	drivers/input/keyboard/gpio_keys.c
 drivers/input/keyboard/gpio_keys.c |   47 ++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index b29ca65..ca615a4 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -28,6 +28,7 @@
 #include <linux/gpio.h>
 #include <linux/of_platform.h>
 #include <linux/of_gpio.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/spinlock.h>
 
 struct gpio_button_data {
@@ -47,6 +48,10 @@ struct gpio_keys_drvdata {
 	struct input_dev *input;
 	struct mutex disable_lock;
 	struct gpio_button_data data[0];
+	/* Two optional pin states - default & sleep */
+	struct pinctrl		*pinctrl;
+	struct pinctrl_state	*pins_default;
+	struct pinctrl_state	*pins_sleep;
 };
 
 /*
@@ -709,6 +714,35 @@ static int gpio_keys_probe(struct platform_device *pdev)
 		goto fail1;
 	}
 
+	ddata->pinctrl = devm_pinctrl_get(dev);
+	if (!IS_ERR(ddata->pinctrl)) {
+		ddata->pins_default = pinctrl_lookup_state(ddata->pinctrl,
+						PINCTRL_STATE_DEFAULT);
+		/* enable pins to be muxed in and configured */
+		if (IS_ERR(ddata->pins_default))
+			dev_dbg(dev, "could not get default pinstate\n");
+		else
+			if (pinctrl_select_state(ddata->pinctrl,
+						 ddata->pins_default))
+				dev_err(dev,
+					"could not set default pinstate\n");
+
+		ddata->pins_sleep = pinctrl_lookup_state(ddata->pinctrl,
+						PINCTRL_STATE_SLEEP);
+		if (IS_ERR(ddata->pins_sleep))
+			dev_dbg(dev, "could not get sleep pinstate\n");
+	} else {
+		/*
+		* Since we continue even when pinctrl node is not found,
+		* Invalidate pins as not available. This is to make sure that
+		* IS_ERR(pins_xxx) results in failure when used.
+		*/
+		ddata->pins_default = ERR_PTR(-ENODATA);
+		ddata->pins_sleep = ERR_PTR(-ENODATA);
+
+		dev_dbg(dev, "pins are not configured from the driver\n");
+	}
+
 	ddata->pdata = pdata;
 	ddata->input = input;
 	mutex_init(&ddata->disable_lock);
@@ -816,6 +850,13 @@ static int gpio_keys_suspend(struct device *dev)
 				enable_irq_wake(bdata->irq);
 		}
 	} else {
+		/* Optionally let pins go into sleep states */
+		if (!IS_ERR(ddata->pins_sleep))
+			if (pinctrl_select_state(ddata->pinctrl,
+						 ddata->pins_sleep))
+				dev_err(dev,
+					"could not set pins to sleep state\n");
+
 		mutex_lock(&input->mutex);
 		if (input->users)
 			gpio_keys_close(input);
@@ -839,6 +880,12 @@ static int gpio_keys_resume(struct device *dev)
 				disable_irq_wake(bdata->irq);
 		}
 	} else {
+		/* Optionaly enable pins to be muxed in and configured */
+		if (!IS_ERR(ddata->pins_default))
+			if (pinctrl_select_state(ddata->pinctrl,
+						 ddata->pins_default))
+				dev_err(dev, "could not set default pins\n");
+
 		mutex_lock(&input->mutex);
 		if (input->users)
 			error = gpio_keys_open(input);
-- 
1.7.9.5

[PATCH 04/11] Input: matrix-keypad: Adopt pinctrl support

[Hebbar Gururaja]

Amend matrix-keypad driver to optionally take a pin control handle and
set the state of the pins to:

- "default" on boot, resume
- "sleep" on suspend()

By optionally putting the pins into sleep state in the suspend callback
we can accomplish two things.
- One is to minimize current leakage from pins and thus save power,
- second, we can prevent the IP from driving pins output in an
uncontrolled manner, which may happen if the power domain drops the
domain regulator.

If any of the above pin states are missing in dt, a warning message
about the missing state is displayed.
If certain pin-states are not available, to remove this warning message
pass respective state name with null phandler.

Todo:
- if an idle state is available for pins, add support for it.

Signed-off-by: Hebbar Gururaja <gururaja.hebbar@ti.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: linux-input at vger.kernel.org
---
:100644 100644 71d7719... aed43fb... M	drivers/input/keyboard/matrix_keypad.c
 drivers/input/keyboard/matrix_keypad.c |   48 ++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
index 71d7719..aed43fb 100644
--- a/drivers/input/keyboard/matrix_keypad.c
+++ b/drivers/input/keyboard/matrix_keypad.c
@@ -26,6 +26,7 @@
 #include <linux/of.h>
 #include <linux/of_gpio.h>
 #include <linux/of_platform.h>
+#include <linux/pinctrl/consumer.h>
 
 struct matrix_keypad {
 	const struct matrix_keypad_platform_data *pdata;
@@ -40,6 +41,10 @@ struct matrix_keypad {
 	bool scan_pending;
 	bool stopped;
 	bool gpio_all_disabled;
+	/* Two optional pin states - default & sleep */
+	struct pinctrl		*pinctrl;
+	struct pinctrl_state	*pins_default;
+	struct pinctrl_state	*pins_sleep;
 };
 
 /*
@@ -280,6 +285,13 @@ static int matrix_keypad_suspend(struct device *dev)
 
 	if (device_may_wakeup(&pdev->dev))
 		matrix_keypad_enable_wakeup(keypad);
+	else
+		/* Optionally let pins go into sleep states */
+		if (!IS_ERR(keypad->pins_sleep))
+			if (pinctrl_select_state(keypad->pinctrl,
+						 keypad->pins_sleep))
+				dev_err(dev,
+					"could not set pins to sleep state\n");
 
 	return 0;
 }
@@ -291,6 +303,12 @@ static int matrix_keypad_resume(struct device *dev)
 
 	if (device_may_wakeup(&pdev->dev))
 		matrix_keypad_disable_wakeup(keypad);
+	else
+		/* Optionaly enable pins to be muxed in and configured */
+		if (!IS_ERR(keypad->pins_default))
+			if (pinctrl_select_state(keypad->pinctrl,
+						 keypad->pins_default))
+				dev_err(dev, "could not set default pins\n");
 
 	matrix_keypad_start(keypad->input_dev);
 
@@ -491,6 +509,36 @@ static int matrix_keypad_probe(struct platform_device *pdev)
 		goto err_free_mem;
 	}
 
+	keypad->pinctrl = devm_pinctrl_get(&pdev->dev);
+	if (!IS_ERR(keypad->pinctrl)) {
+		keypad->pins_default = pinctrl_lookup_state(keypad->pinctrl,
+						PINCTRL_STATE_DEFAULT);
+		/* enable pins to be muxed in and configured */
+		if (IS_ERR(keypad->pins_default))
+			dev_dbg(&pdev->dev, "could not get default pinstate\n");
+		else
+			if (pinctrl_select_state(keypad->pinctrl,
+						 keypad->pins_default))
+				dev_err(&pdev->dev,
+					"could not set default pins\n");
+
+		keypad->pins_sleep = pinctrl_lookup_state(keypad->pinctrl,
+						PINCTRL_STATE_SLEEP);
+		if (IS_ERR(keypad->pins_sleep))
+			dev_dbg(&pdev->dev, "could not get sleep pinstate\n");
+	} else {
+		/*
+		* Since we continue even when pinctrl node is not found,
+		* Invalidate pins as not available. This is to make sure that
+		* IS_ERR(pins_xxx) results in failure when used.
+		*/
+		keypad->pins_default = ERR_PTR(-ENODATA);
+		keypad->pins_sleep = ERR_PTR(-ENODATA);
+
+		dev_dbg(&pdev->dev,
+			"pins are not configured from the driver\n");
+	}
+
 	keypad->input_dev = input_dev;
 	keypad->pdata = pdata;
 	keypad->row_shift = get_count_order(pdata->num_col_gpios);
-- 
1.7.9.5

[PATCH 05/11] spi: omap2-mcspi: enhance pinctrl support

[Hebbar Gururaja]

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

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

By optionally putting the pins into sleep state in the suspend callback
we can accomplish two things.
- One is to minimize current leakage from pins and thus save power,
- second, we can prevent the IP from driving pins output in an
uncontrolled manner, which may happen if the power domain drops the
domain regulator.

If any of the above pin states are missing in dt, a warning message
about the missing state is displayed.
If certain pin-states are not available, to remove this warning message
pass respective state name with null phandler.

(Changes based on i2c-nomadik.c & spi-pl022.c)

Note:
A .suspend callback is added which simply puts the pins to sleep state.
They are moved to default & idle state by the .resume callback.

Signed-off-by: Hebbar Gururaja <gururaja.hebbar@ti.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: spi-devel-general at lists.sourceforge.net
---
:100644 100644 86d2158... 146dd16... M	drivers/spi/spi-omap2-mcspi.c
 drivers/spi/spi-omap2-mcspi.c |   89 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 84 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
index 86d2158..146dd16 100644
--- a/drivers/spi/spi-omap2-mcspi.c
+++ b/drivers/spi/spi-omap2-mcspi.c
@@ -130,6 +130,12 @@ struct omap2_mcspi {
 	struct device		*dev;
 	struct omap2_mcspi_regs ctx;
 	unsigned int		pin_dir:1;
+
+	/* Three pin states - default, idle & sleep */
+	struct pinctrl			*pinctrl;
+	struct pinctrl_state		*pins_default;
+	struct pinctrl_state		*pins_idle;
+	struct pinctrl_state		*pins_sleep;
 };
 
 struct omap2_mcspi_cs {
@@ -267,6 +273,12 @@ static int omap2_prepare_transfer(struct spi_master *master)
 	struct omap2_mcspi *mcspi = spi_master_get_devdata(master);
 
 	pm_runtime_get_sync(mcspi->dev);
+
+	/* Optionaly enable pins to be muxed in and configured */
+	if (!IS_ERR(mcspi->pins_default))
+		if (pinctrl_select_state(mcspi->pinctrl, mcspi->pins_default))
+			dev_err(mcspi->dev, "could not set default pins\n");
+
 	return 0;
 }
 
@@ -274,6 +286,12 @@ static int omap2_unprepare_transfer(struct spi_master *master)
 {
 	struct omap2_mcspi *mcspi = spi_master_get_devdata(master);
 
+	/* Optionally let pins go into idle state */
+	if (!IS_ERR(mcspi->pins_idle))
+		if (pinctrl_select_state(mcspi->pinctrl, mcspi->pins_idle))
+			dev_err(mcspi->dev,
+				"could not set pins to idle state\n");
+
 	pm_runtime_mark_last_busy(mcspi->dev);
 	pm_runtime_put_autosuspend(mcspi->dev);
 	return 0;
@@ -1186,7 +1204,6 @@ static int omap2_mcspi_probe(struct platform_device *pdev)
 	static int		bus_num = 1;
 	struct device_node	*node = pdev->dev.of_node;
 	const struct of_device_id *match;
-	struct pinctrl *pinctrl;
 
 	master = spi_alloc_master(&pdev->dev, sizeof *mcspi);
 	if (master == NULL) {
@@ -1284,10 +1301,46 @@ static int omap2_mcspi_probe(struct platform_device *pdev)
 	if (status < 0)
 		goto dma_chnl_free;
 
-	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
-	if (IS_ERR(pinctrl))
-		dev_warn(&pdev->dev,
-				"pins are not configured from the driver\n");
+	mcspi->pinctrl = devm_pinctrl_get(&pdev->dev);
+	if (!IS_ERR(mcspi->pinctrl)) {
+		mcspi->pins_default = pinctrl_lookup_state(mcspi->pinctrl,
+							 PINCTRL_STATE_DEFAULT);
+		if (IS_ERR(mcspi->pins_default))
+			dev_dbg(&pdev->dev, "could not get default pinstate\n");
+		else
+			if (pinctrl_select_state(mcspi->pinctrl,
+						 mcspi->pins_default))
+				dev_err(&pdev->dev,
+					"could not set default pinstate\n");
+
+		mcspi->pins_idle = pinctrl_lookup_state(mcspi->pinctrl,
+						      PINCTRL_STATE_IDLE);
+		if (IS_ERR(mcspi->pins_idle))
+			dev_dbg(&pdev->dev, "could not get idle pinstate\n");
+		else
+			/* If possible, let's idle until the first transfer */
+			if (pinctrl_select_state(mcspi->pinctrl,
+						 mcspi->pins_idle))
+				dev_err(&pdev->dev,
+					"could not set idle pinstate\n");
+
+		mcspi->pins_sleep = pinctrl_lookup_state(mcspi->pinctrl,
+						       PINCTRL_STATE_SLEEP);
+		if (IS_ERR(mcspi->pins_sleep))
+			dev_dbg(&pdev->dev, "could not get sleep pinstate\n");
+	} else {
+		/*
+		* Since we continue even when pinctrl node is not found,
+		* Invalidate pins as not available. This is to make sure that
+		* IS_ERR(pins_xxx) results in failure when used.
+		*/
+		mcspi->pins_default = ERR_PTR(-ENODATA);
+		mcspi->pins_idle = ERR_PTR(-ENODATA);
+		mcspi->pins_sleep = ERR_PTR(-ENODATA);
+
+		dev_dbg(&pdev->dev, "did not get pins for i2c error: %li\n",
+			PTR_ERR(mcspi->pinctrl));
+	}
 
 	pm_runtime_use_autosuspend(&pdev->dev);
 	pm_runtime_set_autosuspend_delay(&pdev->dev, SPI_AUTOSUSPEND_TIMEOUT);
@@ -1335,6 +1388,18 @@ static int omap2_mcspi_remove(struct platform_device *pdev)
 MODULE_ALIAS("platform:omap2_mcspi");
 
 #ifdef	CONFIG_SUSPEND
+static int omap2_mcspi_suspend(struct device *dev)
+{
+	struct spi_master	*master = dev_get_drvdata(dev);
+	struct omap2_mcspi	*mcspi = spi_master_get_devdata(master);
+
+	if (!IS_ERR(mcspi->pins_sleep))
+		if (pinctrl_select_state(mcspi->pinctrl, mcspi->pins_sleep))
+			dev_err(dev, "could not set pins to sleep state\n");
+
+	return 0;
+}
+
 /*
  * When SPI wake up from off-mode, CS is in activate state. If it was in
  * unactive state when driver was suspend, then force it to unactive state at
@@ -1348,6 +1413,17 @@ static int omap2_mcspi_resume(struct device *dev)
 	struct omap2_mcspi_cs	*cs;
 
 	pm_runtime_get_sync(mcspi->dev);
+
+	/* First go to the default state */
+	if (!IS_ERR(mcspi->pins_default))
+		if (pinctrl_select_state(mcspi->pinctrl, mcspi->pins_default))
+			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(mcspi->pins_idle))
+		if (pinctrl_select_state(mcspi->pinctrl, mcspi->pins_idle))
+			dev_err(dev, "could not set pins to idle state\n");
+
 	list_for_each_entry(cs, &ctx->cs, node) {
 		if ((cs->chconf0 & OMAP2_MCSPI_CHCONF_FORCE) == 0) {
 			/*
@@ -1364,12 +1440,15 @@ static int omap2_mcspi_resume(struct device *dev)
 	pm_runtime_put_autosuspend(mcspi->dev);
 	return 0;
 }
+
 #else
+#define	omap2_mcspi_suspend	NULL
 #define	omap2_mcspi_resume	NULL
 #endif
 
 static const struct dev_pm_ops omap2_mcspi_pm_ops = {
 	.resume = omap2_mcspi_resume,
+	.suspend = omap2_mcspi_suspend,
 	.runtime_resume	= omap_mcspi_runtime_resume,
 };
 
-- 
1.7.9.5

[PATCH 05/11] spi: omap2-mcspi: enhance pinctrl support

[Mark Brown]

On Fri, May 31, 2013 at 03:43:05PM +0530, Hebbar Gururaja wrote:
> Amend the spi omap controller to optionally take a pin control
> handle and set the state of the pins to:
> 
> - "default" on boot, resume and before performing an spi transfer
> - "idle" after initial default, after resume default, and after each
> spi xfer
> - "sleep" on suspend()

Looking at this code I can't really see what's OMAP-specific about it -
exactly the same flow should apply to pretty much any SPI controller,
especially given that the code will happily ignore missing states.
We're just setting the idle state when not actively transferring data
which seems sensible and generic.

This suggests to me that we should be adding this code into the core,
probably joined up with the transfer_one_message stuff, so that any
hardware which has an idle state will be able to get the benefit.  Can
anyone think of a reason why we shouldn't do that?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130601/7602bb88/attachment.sig>

[PATCH 05/11] spi: omap2-mcspi: enhance pinctrl support

[Hebbar, Gururaja]

On Sun, Jun 02, 2013 at 00:57:26, Mark Brown wrote:
> On Fri, May 31, 2013 at 03:43:05PM +0530, Hebbar Gururaja wrote:
> > Amend the spi omap controller to optionally take a pin control
> > handle and set the state of the pins to:
> > 
> > - "default" on boot, resume and before performing an spi transfer
> > - "idle" after initial default, after resume default, and after each
> > spi xfer
> > - "sleep" on suspend()
> 
> Looking at this code I can't really see what's OMAP-specific about it -
> exactly the same flow should apply to pretty much any SPI controller,
> especially given that the code will happily ignore missing states.
> We're just setting the idle state when not actively transferring data
> which seems sensible and generic.
> 
> This suggests to me that we should be adding this code into the core,
> probably joined up with the transfer_one_message stuff, so that any
> hardware which has an idle state will be able to get the benefit.  Can
> anyone think of a reason why we shouldn't do that?

Let me pull out some info about these and come back

> 


Regards, 
Gururaja

[PATCH 06/11] usb: musb: dsps: Adopt pinctrl support

[Hebbar Gururaja]

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

- "default" on boot, resume
- "sleep" on suspend()

By optionally putting the pins into sleep state in the suspend callback
we can accomplish two things.
- One is to minimize current leakage from pins and thus save power,
- second, we can prevent the IP from driving pins output in an
uncontrolled manner, which may happen if the power domain drops the
domain regulator.

If any of the above pin states are missing in dt, a warning message
about the missing state is displayed.
If certain pin-states are not available, to remove this warning message
pass respective state name with null phandler.

Todo:
        - if an idle state is available for pins, add support for it.

Signed-off-by: Hebbar Gururaja <gururaja.hebbar@ti.com>
Cc: Felipe Balbi <balbi@ti.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb at vger.kernel.org
---
:100644 100644 e1b661d... a9580fe... M	drivers/usb/musb/musb_dsps.c
 drivers/usb/musb/musb_dsps.c |   46 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
index e1b661d..a9580fe 100644
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -39,6 +39,7 @@
 #include <linux/usb/nop-usb-xceiv.h>
 #include <linux/platform_data/usb-omap.h>
 #include <linux/sizes.h>
+#include <linux/pinctrl/consumer.h>
 
 #include <linux/of.h>
 #include <linux/of_device.h>
@@ -124,6 +125,11 @@ struct dsps_glue {
 	struct timer_list timer[2];	/* otg_workaround timer */
 	unsigned long last_timer[2];    /* last timer data for each instance */
 	u32 __iomem *usb_ctrl[2];
+
+	/* two pin states - default, sleep */
+	struct pinctrl			*pinctrl;
+	struct pinctrl_state		*pins_default;
+	struct pinctrl_state		*pins_sleep;
 };
 
 #define	DSPS_AM33XX_CONTROL_MODULE_PHYS_0	0x44e10620
@@ -636,6 +642,36 @@ static int dsps_probe(struct platform_device *pdev)
 		ret = -ENOMEM;
 		goto err1;
 	}
+
+	glue->pinctrl = devm_pinctrl_get(&pdev->dev);
+	if (!IS_ERR(glue->pinctrl)) {
+		glue->pins_default = pinctrl_lookup_state(glue->pinctrl,
+							 PINCTRL_STATE_DEFAULT);
+		if (IS_ERR(glue->pins_default))
+			dev_dbg(&pdev->dev, "could not get default pinstate\n");
+		else
+			if (pinctrl_select_state(glue->pinctrl,
+						 glue->pins_default))
+				dev_err(&pdev->dev,
+					"could not set default pinstate\n");
+
+		glue->pins_sleep = pinctrl_lookup_state(glue->pinctrl,
+						       PINCTRL_STATE_SLEEP);
+		if (IS_ERR(glue->pins_sleep))
+			dev_dbg(&pdev->dev, "could not get sleep pinstate\n");
+	} else {
+		/*
+		* Since we continue even when pinctrl node is not found,
+		* Invalidate pins as not available. This is to make sure that
+		* IS_ERR(pins_xxx) results in failure when used.
+		*/
+		glue->pins_default = ERR_PTR(-ENODATA);
+		glue->pins_sleep = ERR_PTR(-ENODATA);
+
+		dev_dbg(&pdev->dev, "did not get pins for i2c error: %li\n",
+			PTR_ERR(glue->pinctrl));
+	}
+
 	platform_set_drvdata(pdev, glue);
 
 	/* enable the usbss clocks */
@@ -700,6 +736,11 @@ static int dsps_suspend(struct device *dev)
 	for (i = 0; i < wrp->instances; i++)
 		musb_dsps_phy_control(glue, i, 0);
 
+	/* Optionally let pins go into sleep states */
+	if (!IS_ERR(glue->pins_sleep))
+		if (pinctrl_select_state(glue->pinctrl, glue->pins_sleep))
+			dev_err(dev, "could not set pins to sleep state\n");
+
 	return 0;
 }
 
@@ -710,6 +751,11 @@ static int dsps_resume(struct device *dev)
 	const struct dsps_musb_wrapper *wrp = glue->wrp;
 	int i;
 
+	/* Optionaly enable pins to be muxed in and configured */
+	if (!IS_ERR(glue->pins_default))
+		if (pinctrl_select_state(glue->pinctrl, glue->pins_default))
+			dev_err(dev, "could not set default pins\n");
+
 	for (i = 0; i < wrp->instances; i++)
 		musb_dsps_phy_control(glue, i, 1);
 
-- 
1.7.9.5

[PATCH 07/11] pwm: pwm-tiehrpwm: enhance pinctrl support

[Hebbar Gururaja]

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

- "default" on boot, resume
- "sleep" on suspend()

By optionally putting the pins into sleep state in the suspend callback
we can accomplish two things.
- One is to minimize current leakage from pins and thus save power,
- second, we can prevent the IP from driving pins output in an
uncontrolled manner, which may happen if the power domain drops the
domain regulator.

If any of the above pin states are missing in dt, a warning message
about the missing state is displayed.
If certain pin-states are not available, to remove this warning message
pass respective state name with null phandler.

Todo:
        - if an idle state is available for pins, add support for it.

Signed-off-by: Hebbar Gururaja <gururaja.hebbar@ti.com>
Cc: Thierry Reding <thierry.reding@avionic-design.de>
Cc: Philip, Avinash <avinashphilip@ti.com>
---
:100644 100644 48a485c... ed55460ae. M	drivers/pwm/pwm-tiehrpwm.c
 drivers/pwm/pwm-tiehrpwm.c |   49 +++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 44 insertions(+), 5 deletions(-)

diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
index 48a485c..ed55460ae 100644
--- a/drivers/pwm/pwm-tiehrpwm.c
+++ b/drivers/pwm/pwm-tiehrpwm.c
@@ -132,6 +132,11 @@ struct ehrpwm_pwm_chip {
 	enum pwm_polarity polarity[NUM_PWM_CHANNEL];
 	struct	clk	*tbclk;
 	struct ehrpwm_context ctx;
+
+	/* two pin states - default, sleep */
+	struct pinctrl			*pinctrl;
+	struct pinctrl_state		*pins_default;
+	struct pinctrl_state		*pins_sleep;
 };
 
 static inline struct ehrpwm_pwm_chip *to_ehrpwm_pwm_chip(struct pwm_chip *chip)
@@ -439,11 +444,6 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev)
 	struct clk *clk;
 	struct ehrpwm_pwm_chip *pc;
 	u16 status;
-	struct pinctrl *pinctrl;
-
-	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
-	if (IS_ERR(pinctrl))
-		dev_warn(&pdev->dev, "unable to select pin group\n");
 
 	pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
 	if (!pc) {
@@ -451,6 +451,34 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
+	pc->pinctrl = devm_pinctrl_get(&pdev->dev);
+	if (!IS_ERR(pc->pinctrl)) {
+		pc->pins_default = pinctrl_lookup_state(pc->pinctrl,
+							 PINCTRL_STATE_DEFAULT);
+		if (IS_ERR(pc->pins_default))
+			dev_dbg(&pdev->dev, "could not get default pinstate\n");
+		else
+			if (pinctrl_select_state(pc->pinctrl, pc->pins_default))
+				dev_err(&pdev->dev,
+					"could not set default pinstate\n");
+
+		pc->pins_sleep = pinctrl_lookup_state(pc->pinctrl,
+						       PINCTRL_STATE_SLEEP);
+		if (IS_ERR(pc->pins_sleep))
+			dev_dbg(&pdev->dev, "could not get sleep pinstate\n");
+	} else {
+		/*
+		* Since we continue even when pinctrl node is not found,
+		* Invalidate pins as not available. This is to make sure that
+		* IS_ERR(pins_xxx) results in failure when used.
+		*/
+		pc->pins_default = ERR_PTR(-ENODATA);
+		pc->pins_sleep = ERR_PTR(-ENODATA);
+
+		dev_dbg(&pdev->dev, "did not get pins for i2c error: %li\n",
+			PTR_ERR(pc->pinctrl));
+	}
+
 	clk = devm_clk_get(&pdev->dev, "fck");
 	if (IS_ERR(clk)) {
 		dev_err(&pdev->dev, "failed to get clock\n");
@@ -570,6 +598,12 @@ static int ehrpwm_pwm_suspend(struct device *dev)
 		/* Disable explicitly if PWM is running */
 		pm_runtime_put_sync(dev);
 	}
+
+	/* Optionally let pins go into sleep states */
+	if (!IS_ERR(pc->pins_sleep))
+		if (pinctrl_select_state(pc->pinctrl, pc->pins_sleep))
+			dev_err(dev, "could not set pins to sleep state\n");
+
 	return 0;
 }
 
@@ -578,6 +612,11 @@ static int ehrpwm_pwm_resume(struct device *dev)
 	struct ehrpwm_pwm_chip *pc = dev_get_drvdata(dev);
 	int i;
 
+	/* Optionaly enable pins to be muxed in and configured */
+	if (!IS_ERR(pc->pins_default))
+		if (pinctrl_select_state(pc->pinctrl, pc->pins_default))
+			dev_err(dev, "could not set default pins\n");
+
 	for (i = 0; i < pc->chip.npwm; i++) {
 		struct pwm_device *pwm = &pc->chip.pwms[i];
 
-- 
1.7.9.5

[PATCH 08/11] pwm: pwm-tiecap: enhance pinctrl support

[Hebbar Gururaja]

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

- "default" on boot, resume
- "sleep" on suspend()

By optionally putting the pins into sleep state in the suspend callback
we can accomplish two things.
- One is to minimize current leakage from pins and thus save power,
- second, we can prevent the IP from driving pins output in an
uncontrolled manner, which may happen if the power domain drops the
domain regulator.

If any of the above pin states are missing in dt, a warning message
about the missing state is displayed.
If certain pin-states are not available, to remove this warning message
pass respective state name with null phandler.

Todo:
        - if an idle state is available for pins, add support for it.

Signed-off-by: Hebbar Gururaja <gururaja.hebbar@ti.com>
Cc: Thierry Reding <thierry.reding@avionic-design.de>
Cc: Philip, Avinash <avinashphilip@ti.com>
---
:100644 100644 72ca42d... d02769d... M	drivers/pwm/pwm-tiecap.c
 drivers/pwm/pwm-tiecap.c |   48 +++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 43 insertions(+), 5 deletions(-)

diff --git a/drivers/pwm/pwm-tiecap.c b/drivers/pwm/pwm-tiecap.c
index 72ca42d..d02769d 100644
--- a/drivers/pwm/pwm-tiecap.c
+++ b/drivers/pwm/pwm-tiecap.c
@@ -52,6 +52,11 @@ struct ecap_pwm_chip {
 	unsigned int	clk_rate;
 	void __iomem	*mmio_base;
 	struct ecap_context ctx;
+
+	/* two pin states - default, sleep */
+	struct pinctrl			*pinctrl;
+	struct pinctrl_state		*pins_default;
+	struct pinctrl_state		*pins_sleep;
 };
 
 static inline struct ecap_pwm_chip *to_ecap_pwm_chip(struct pwm_chip *chip)
@@ -208,11 +213,6 @@ static int ecap_pwm_probe(struct platform_device *pdev)
 	struct clk *clk;
 	struct ecap_pwm_chip *pc;
 	u16 status;
-	struct pinctrl *pinctrl;
-
-	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
-	if (IS_ERR(pinctrl))
-		dev_warn(&pdev->dev, "unable to select pin group\n");
 
 	pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
 	if (!pc) {
@@ -220,6 +220,34 @@ static int ecap_pwm_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
+	pc->pinctrl = devm_pinctrl_get(&pdev->dev);
+	if (!IS_ERR(pc->pinctrl)) {
+		pc->pins_default = pinctrl_lookup_state(pc->pinctrl,
+							 PINCTRL_STATE_DEFAULT);
+		if (IS_ERR(pc->pins_default))
+			dev_dbg(&pdev->dev, "could not get default pinstate\n");
+		else
+			if (pinctrl_select_state(pc->pinctrl, pc->pins_default))
+				dev_err(&pdev->dev,
+					"could not set default pinstate\n");
+
+		pc->pins_sleep = pinctrl_lookup_state(pc->pinctrl,
+						       PINCTRL_STATE_SLEEP);
+		if (IS_ERR(pc->pins_sleep))
+			dev_dbg(&pdev->dev, "could not get sleep pinstate\n");
+	} else {
+		/*
+		* Since we continue even when pinctrl node is not found,
+		* Invalidate pins as not available. This is to make sure that
+		* IS_ERR(pins_xxx) results in failure when used.
+		*/
+		pc->pins_default = ERR_PTR(-ENODATA);
+		pc->pins_sleep = ERR_PTR(-ENODATA);
+
+		dev_dbg(&pdev->dev, "did not get pins for i2c error: %li\n",
+			PTR_ERR(pc->pinctrl));
+	}
+
 	clk = devm_clk_get(&pdev->dev, "fck");
 	if (IS_ERR(clk)) {
 		dev_err(&pdev->dev, "failed to get clock\n");
@@ -318,6 +346,11 @@ static int ecap_pwm_suspend(struct device *dev)
 	if (test_bit(PWMF_ENABLED, &pwm->flags))
 		pm_runtime_put_sync(dev);
 
+	/* Optionally let pins go into sleep states */
+	if (!IS_ERR(pc->pins_sleep))
+		if (pinctrl_select_state(pc->pinctrl, pc->pins_sleep))
+			dev_err(dev, "could not set pins to sleep state\n");
+
 	return 0;
 }
 
@@ -326,6 +359,11 @@ static int ecap_pwm_resume(struct device *dev)
 	struct ecap_pwm_chip *pc = dev_get_drvdata(dev);
 	struct pwm_device *pwm = pc->chip.pwms;
 
+	/* Optionaly enable pins to be muxed in and configured */
+	if (!IS_ERR(pc->pins_default))
+		if (pinctrl_select_state(pc->pinctrl, pc->pins_default))
+			dev_err(dev, "could not set default pins\n");
+
 	/* Enable explicitly if PWM was running */
 	if (test_bit(PWMF_ENABLED, &pwm->flags))
 		pm_runtime_get_sync(dev);
-- 
1.7.9.5

[PATCH 09/11] mmc: omap_hsmmc: enhance pinctrl support

[Hebbar Gururaja]

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

- "default" on boot, resume and before performing a mmc transfer
- "idle" after initial default, after resume default, and after each
mmc/sd card access
- "sleep" on suspend()

By optionally putting the pins into sleep state in the suspend callback
we can accomplish two things.
- One is to minimize current leakage from pins and thus save power,
- second, we can prevent the IP from driving pins output in an
uncontrolled manner, which may happen if the power domain drops the
domain regulator.

If any of the above pin states are missing in dt, a warning message
about the missing state is displayed.
If certain pin-states are not available, to remove this warning message
pass respective state name with null phandler.

Signed-off-by: Hebbar Gururaja <gururaja.hebbar@ti.com>
Cc: Balaji T K <balajitk@ti.com>
Cc: Chris Ball <cjb@laptop.org>
Cc: linux-mmc at vger.kernel.org
Cc: linux-omap at vger.kernel.org
---
:100644 100644 6e44025... a2d69aa... M	drivers/mmc/host/omap_hsmmc.c
 drivers/mmc/host/omap_hsmmc.c |   79 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 74 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 6e44025..a2d69aa 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -183,6 +183,12 @@ struct omap_hsmmc_host {
 	struct omap_hsmmc_next	next_data;
 
 	struct	omap_mmc_platform_data	*pdata;
+
+	/* Three pin states - default, idle & sleep */
+	struct pinctrl			*pinctrl;
+	struct pinctrl_state		*pins_default;
+	struct pinctrl_state		*pins_idle;
+	struct pinctrl_state		*pins_sleep;
 };
 
 static int omap_hsmmc_card_detect(struct device *dev, int slot)
@@ -1775,7 +1781,6 @@ static int omap_hsmmc_probe(struct platform_device *pdev)
 	const struct of_device_id *match;
 	dma_cap_mask_t mask;
 	unsigned tx_req, rx_req;
-	struct pinctrl *pinctrl;
 
 	match = of_match_device(of_match_ptr(omap_mmc_of_match), &pdev->dev);
 	if (match) {
@@ -1982,10 +1987,46 @@ static int omap_hsmmc_probe(struct platform_device *pdev)
 
 	omap_hsmmc_disable_irq(host);
 
-	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
-	if (IS_ERR(pinctrl))
-		dev_warn(&pdev->dev,
-			"pins are not configured from the driver\n");
+	host->pinctrl = devm_pinctrl_get(&pdev->dev);
+	if (!IS_ERR(host->pinctrl)) {
+		host->pins_default = pinctrl_lookup_state(host->pinctrl,
+							PINCTRL_STATE_DEFAULT);
+		if (IS_ERR(host->pins_default))
+			dev_dbg(&pdev->dev, "could not get default pinstate\n");
+		else
+			if (pinctrl_select_state(host->pinctrl,
+						 host->pins_default))
+				dev_err(&pdev->dev,
+					"could not set default pinstate\n");
+
+		host->pins_idle = pinctrl_lookup_state(host->pinctrl,
+							PINCTRL_STATE_IDLE);
+		if (IS_ERR(host->pins_idle))
+			dev_dbg(&pdev->dev, "could not get idle pinstate\n");
+		else
+			/* If possible, let's idle until the first transfer */
+			if (pinctrl_select_state(host->pinctrl,
+						 host->pins_idle))
+				dev_err(&pdev->dev,
+					"could not set idle pinstate\n");
+
+		host->pins_sleep = pinctrl_lookup_state(host->pinctrl,
+							PINCTRL_STATE_SLEEP);
+		if (IS_ERR(host->pins_sleep))
+			dev_dbg(&pdev->dev, "could not get sleep pinstate\n");
+	} else {
+		/*
+		* Since we continue even when pinctrl node is not found,
+		* Invalidate pins as not available. This is to make sure that
+		* IS_ERR(pins_xxx) results in failure when used.
+		*/
+		host->pins_default = ERR_PTR(-ENODATA);
+		host->pins_idle = ERR_PTR(-ENODATA);
+		host->pins_sleep = ERR_PTR(-ENODATA);
+
+		dev_dbg(&pdev->dev, "did not get pins for mmc error: %li\n",
+			PTR_ERR(host->pinctrl));
+	}
 
 	omap_hsmmc_protect_card(host);
 
@@ -2135,6 +2176,12 @@ static int omap_hsmmc_suspend(struct device *dev)
 		clk_disable_unprepare(host->dbclk);
 err:
 	pm_runtime_put_sync(host->dev);
+
+	/* Optionally let pins go into sleep states */
+	if (!IS_ERR(host->pins_sleep))
+		if (pinctrl_select_state(host->pinctrl, host->pins_sleep))
+			dev_err(dev, "could not set pins to sleep state\n");
+
 	return ret;
 }
 
@@ -2152,6 +2199,16 @@ static int omap_hsmmc_resume(struct device *dev)
 
 	pm_runtime_get_sync(host->dev);
 
+	/* First go to the default state */
+	if (!IS_ERR(host->pins_default))
+		if (pinctrl_select_state(host->pinctrl, host->pins_default))
+			dev_err(host->dev, "could not set pins to default state\n");
+
+	/* Then let's idle the pins until the next transfer happens */
+	if (!IS_ERR(host->pins_idle))
+		if (pinctrl_select_state(host->pinctrl, host->pins_idle))
+			dev_err(host->dev, "couldn't set pins to idle state\n");
+
 	if (host->dbclk)
 		clk_prepare_enable(host->dbclk);
 
@@ -2185,6 +2242,12 @@ static int omap_hsmmc_runtime_suspend(struct device *dev)
 
 	host = platform_get_drvdata(to_platform_device(dev));
 	omap_hsmmc_context_save(host);
+
+	/* Optionally let pins go into sleep states */
+	if (!IS_ERR(host->pins_idle))
+		if (pinctrl_select_state(host->pinctrl, host->pins_idle))
+			dev_err(dev, "could not set pins to idle state\n");
+
 	dev_dbg(dev, "disabled\n");
 
 	return 0;
@@ -2195,6 +2258,12 @@ static int omap_hsmmc_runtime_resume(struct device *dev)
 	struct omap_hsmmc_host *host;
 
 	host = platform_get_drvdata(to_platform_device(dev));
+
+	/* Optionaly enable pins to be muxed in and configured */
+	if (!IS_ERR(host->pins_default))
+		if (pinctrl_select_state(host->pinctrl, host->pins_default))
+			dev_err(host->dev, "could not set default pins\n");
+
 	omap_hsmmc_context_restore(host);
 	dev_dbg(dev, "enabled\n");
 
-- 
1.7.9.5

[PATCH 09/11] mmc: omap_hsmmc: enhance pinctrl support

[Linus Walleij]

On Fri, May 31, 2013 at 12:13 PM, Hebbar Gururaja
<gururaja.hebbar@ti.com> wrote:

> Amend the hsmmc controller to optionally take a pin control handle and
> set the state of the pins to:
>
> - "default" on boot, resume and before performing a mmc transfer
> - "idle" after initial default, after resume default, and after each
> mmc/sd card access
> - "sleep" on suspend()
>
> By optionally putting the pins into sleep state in the suspend callback
> we can accomplish two things.
> - One is to minimize current leakage from pins and thus save power,
> - second, we can prevent the IP from driving pins output in an
> uncontrolled manner, which may happen if the power domain drops the
> domain regulator.
>
> If any of the above pin states are missing in dt, a warning message
> about the missing state is displayed.
> If certain pin-states are not available, to remove this warning message
> pass respective state name with null phandler.
>
> Signed-off-by: Hebbar Gururaja <gururaja.hebbar@ti.com>
> Cc: Balaji T K <balajitk@ti.com>
> Cc: Chris Ball <cjb@laptop.org>
> Cc: linux-mmc at vger.kernel.org
> Cc: linux-omap at vger.kernel.org

This is perfectly correct.
Acked-by: Linus Walleij <linus.walleij@linaro.org>

As the PM code seems to be similar across platforms I have had
loose plans to move this to the device core as well, but right now
I'm too busy with other things, and it can surely be refactored later.

Yours,
Linus Walleij

[PATCH 09/11] mmc: omap_hsmmc: enhance pinctrl support

[Linus Walleij]

On Tue, Jun 4, 2013 at 9:11 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, May 31, 2013 at 12:13 PM, Hebbar Gururaja
> <gururaja.hebbar@ti.com> wrote:
>
>> Amend the hsmmc controller to optionally take a pin control handle and
>> set the state of the pins to:
>>
>> - "default" on boot, resume and before performing a mmc transfer
>> - "idle" after initial default, after resume default, and after each
>> mmc/sd card access
>> - "sleep" on suspend()
>>
>> By optionally putting the pins into sleep state in the suspend callback
>> we can accomplish two things.
>> - One is to minimize current leakage from pins and thus save power,
>> - second, we can prevent the IP from driving pins output in an
>> uncontrolled manner, which may happen if the power domain drops the
>> domain regulator.
>>
>> If any of the above pin states are missing in dt, a warning message
>> about the missing state is displayed.
>> If certain pin-states are not available, to remove this warning message
>> pass respective state name with null phandler.
>>
>> Signed-off-by: Hebbar Gururaja <gururaja.hebbar@ti.com>
>> Cc: Balaji T K <balajitk@ti.com>
>> Cc: Chris Ball <cjb@laptop.org>
>> Cc: linux-mmc at vger.kernel.org
>> Cc: linux-omap at vger.kernel.org
>
> This is perfectly correct.
> Acked-by: Linus Walleij <linus.walleij@linaro.org>

So please consider my other option given in patch 2 instead.

Yours,
Linus Walleij

[PATCH 09/11] mmc: omap_hsmmc: enhance pinctrl support

[Hebbar, Gururaja]

On Tue, Jun 04, 2013 at 12:49:57, Linus Walleij wrote:
> On Tue, Jun 4, 2013 at 9:11 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Fri, May 31, 2013 at 12:13 PM, Hebbar Gururaja
> > <gururaja.hebbar@ti.com> wrote:
> >
> >> Amend the hsmmc controller to optionally take a pin control handle and
> >> set the state of the pins to:
> >>
> >> - "default" on boot, resume and before performing a mmc transfer
> >> - "idle" after initial default, after resume default, and after each
> >> mmc/sd card access
> >> - "sleep" on suspend()
> >>
> >> By optionally putting the pins into sleep state in the suspend callback
> >> we can accomplish two things.
> >> - One is to minimize current leakage from pins and thus save power,
> >> - second, we can prevent the IP from driving pins output in an
> >> uncontrolled manner, which may happen if the power domain drops the
> >> domain regulator.
> >>
> >> If any of the above pin states are missing in dt, a warning message
> >> about the missing state is displayed.
> >> If certain pin-states are not available, to remove this warning message
> >> pass respective state name with null phandler.
> >>
> >> Signed-off-by: Hebbar Gururaja <gururaja.hebbar@ti.com>
> >> Cc: Balaji T K <balajitk@ti.com>
> >> Cc: Chris Ball <cjb@laptop.org>
> >> Cc: linux-mmc at vger.kernel.org
> >> Cc: linux-omap at vger.kernel.org
> >
> > This is perfectly correct.
> > Acked-by: Linus Walleij <linus.walleij@linaro.org>
> 
> So please consider my other option given in patch 2 instead.

I will check how I can be a part of this implementation

> 
> Yours,
> Linus Walleij
> 


Regards, 
Gururaja

[PATCH 09/11] mmc: omap_hsmmc: enhance pinctrl support

[Tony Lindgren]

* Hebbar Gururaja <gururaja.hebbar@ti.com> [130531 03:19]:
> Amend the hsmmc controller to optionally take a pin control handle and
> set the state of the pins to:
> 
> - "default" on boot, resume and before performing a mmc transfer
> - "idle" after initial default, after resume default, and after each
> mmc/sd card access
> - "sleep" on suspend()
> 
> By optionally putting the pins into sleep state in the suspend callback
> we can accomplish two things.
> - One is to minimize current leakage from pins and thus save power,
> - second, we can prevent the IP from driving pins output in an
> uncontrolled manner, which may happen if the power domain drops the
> domain regulator.
> 
> If any of the above pin states are missing in dt, a warning message
> about the missing state is displayed.
> If certain pin-states are not available, to remove this warning message
> pass respective state name with null phandler.

There's a similar patch in the "[RESEND PATCH v2 1/3] mmc: omap_hsmmc:
Enable SDIO IRQ using a GPIO in idle mode" thread. It also makes the
SDIO interrupts to work, so we need to consider that too.

We can merge the dynamic pinmuxing parts separately, but they should
be in a separate function like I posted later on in the thread above.

Regards,

Tony 

[PATCH 09/11] mmc: omap_hsmmc: enhance pinctrl support

[Balaji T K]

On Tuesday 04 June 2013 08:16 PM, Tony Lindgren wrote:
> * Hebbar Gururaja <gururaja.hebbar@ti.com> [130531 03:19]:
>> Amend the hsmmc controller to optionally take a pin control handle and
>> set the state of the pins to:
>>
>> - "default" on boot, resume and before performing a mmc transfer
>> - "idle" after initial default, after resume default, and after each
>> mmc/sd card access
>> - "sleep" on suspend()
>>
>> By optionally putting the pins into sleep state in the suspend callback
>> we can accomplish two things.
>> - One is to minimize current leakage from pins and thus save power,
>> - second, we can prevent the IP from driving pins output in an
>> uncontrolled manner, which may happen if the power domain drops the
>> domain regulator.
>>
>> If any of the above pin states are missing in dt, a warning message
>> about the missing state is displayed.
>> If certain pin-states are not available, to remove this warning message
>> pass respective state name with null phandler.
>
> There's a similar patch in the "[RESEND PATCH v2 1/3] mmc: omap_hsmmc:
> Enable SDIO IRQ using a GPIO in idle mode" thread. It also makes the
> SDIO interrupts to work, so we need to consider that too.
>

Hi Tony,

I will try to combine both of these and make use of
pinctrl_pm_select_[*]_state helper functions.


> We can merge the dynamic pinmuxing parts separately, but they should
> be in a separate function like I posted later on in the thread above.
>
> Regards,
>
> Tony
>

[PATCH 09/11] mmc: omap_hsmmc: enhance pinctrl support

[Tony Lindgren]

* Balaji T K <balajitk@ti.com> [130607 06:42]:
> On Tuesday 04 June 2013 08:16 PM, Tony Lindgren wrote:
> >* Hebbar Gururaja <gururaja.hebbar@ti.com> [130531 03:19]:
> >>Amend the hsmmc controller to optionally take a pin control handle and
> >>set the state of the pins to:
> >>
> >>- "default" on boot, resume and before performing a mmc transfer
> >>- "idle" after initial default, after resume default, and after each
> >>mmc/sd card access
> >>- "sleep" on suspend()
> >>
> >>By optionally putting the pins into sleep state in the suspend callback
> >>we can accomplish two things.
> >>- One is to minimize current leakage from pins and thus save power,
> >>- second, we can prevent the IP from driving pins output in an
> >>uncontrolled manner, which may happen if the power domain drops the
> >>domain regulator.
> >>
> >>If any of the above pin states are missing in dt, a warning message
> >>about the missing state is displayed.
> >>If certain pin-states are not available, to remove this warning message
> >>pass respective state name with null phandler.
> >
> >There's a similar patch in the "[RESEND PATCH v2 1/3] mmc: omap_hsmmc:
> >Enable SDIO IRQ using a GPIO in idle mode" thread. It also makes the
> >SDIO interrupts to work, so we need to consider that too.
> >
> 
> Hi Tony,
> 
> I will try to combine both of these and make use of
> pinctrl_pm_select_[*]_state helper functions.

OK thanks, I'll separate out the pinctrl parts from Andreas' patch
and repost.

Regards,

Tony

[PATCH 10/11] video: da8xx-fb: adopt pinctrl support

[Hebbar Gururaja]

Amend the da8xx-fb controller to optionally take a pin control handle
and set the state of the pins to:

- "default" on boot, resume
- "sleep" on suspend()

By optionally putting the pins into sleep state in the suspend callback
we can accomplish two things.
- One is to minimize current leakage from pins and thus save power,
- second, we can prevent the IP from driving pins output in an
uncontrolled manner, which may happen if the power domain drops the
domain regulator.

If any of the above pin states are missing in dt, a warning message
about the missing state is displayed.
If certain pin-states are not available, to remove this warning message
pass respective state name with null phandler.

Todo:
        - if an idle state is available for pins, add support for it.

Signed-off-by: Hebbar Gururaja <gururaja.hebbar@ti.com>
Cc: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
Cc: linux-fbdev at vger.kernel.org
---
:100644 100644 0810939... 10c8036... M	drivers/video/da8xx-fb.c
 drivers/video/da8xx-fb.c |   48 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 0810939..10c8036 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -36,6 +36,7 @@
 #include <linux/slab.h>
 #include <linux/delay.h>
 #include <linux/lcm.h>
+#include <linux/pinctrl/consumer.h>
 #include <video/da8xx-fb.h>
 #include <asm/div64.h>
 
@@ -182,6 +183,11 @@ struct da8xx_fb_par {
 #endif
 	void (*panel_power_ctrl)(int);
 	u32 pseudo_palette[16];
+
+	/* two pin states - default, sleep */
+	struct pinctrl			*pinctrl;
+	struct pinctrl_state		*pins_default;
+	struct pinctrl_state		*pins_sleep;
 };
 
 /* Variable Screen Information */
@@ -1306,6 +1312,36 @@ static int fb_probe(struct platform_device *device)
 	par->lcd_fck_rate = clk_get_rate(fb_clk);
 #endif
 	par->pxl_clk = lcdc_info->pixclock;
+
+	par->pinctrl = devm_pinctrl_get(&device->dev);
+	if (!IS_ERR(par->pinctrl)) {
+		par->pins_default = pinctrl_lookup_state(par->pinctrl,
+							 PINCTRL_STATE_DEFAULT);
+		if (IS_ERR(par->pins_default))
+			dev_dbg(&device->dev, "could not get default pinstate\n");
+		else
+			if (pinctrl_select_state(par->pinctrl,
+						 par->pins_default))
+				dev_err(&device->dev,
+					"could not set default pinstate\n");
+
+		par->pins_sleep = pinctrl_lookup_state(par->pinctrl,
+						       PINCTRL_STATE_SLEEP);
+		if (IS_ERR(par->pins_sleep))
+			dev_dbg(&device->dev, "could not get sleep pinstate\n");
+	} else {
+		/*
+		* Since we continue even when pinctrl node is not found,
+		* Invalidate pins as not available. This is to make sure that
+		* IS_ERR(pins_xxx) results in failure when used.
+		*/
+		par->pins_default = ERR_PTR(-ENODATA);
+		par->pins_sleep = ERR_PTR(-ENODATA);
+
+		dev_dbg(&device->dev, "did not get pins for lcd error: %li\n",
+			PTR_ERR(par->pinctrl));
+	}
+
 	if (fb_pdata->panel_power_ctrl) {
 		par->panel_power_ctrl = fb_pdata->panel_power_ctrl;
 		par->panel_power_ctrl(1);
@@ -1551,6 +1587,12 @@ static int fb_suspend(struct platform_device *dev, pm_message_t state)
 	pm_runtime_put_sync(&dev->dev);
 	console_unlock();
 
+	/* Optionally let pins go into sleep states */
+	if (!IS_ERR(par->pins_sleep))
+		if (pinctrl_select_state(par->pinctrl, par->pins_sleep))
+			dev_err(&dev->dev,
+				"could not set pins to sleep state\n");
+
 	return 0;
 }
 static int fb_resume(struct platform_device *dev)
@@ -1560,6 +1602,12 @@ static int fb_resume(struct platform_device *dev)
 
 	console_lock();
 	pm_runtime_get_sync(&dev->dev);
+
+	/* Optionaly enable pins to be muxed in and configured */
+	if (!IS_ERR(par->pins_default))
+		if (pinctrl_select_state(par->pinctrl, par->pins_default))
+			dev_err(&dev->dev, "could not set default pins\n");
+
 	lcd_context_restore();
 	if (par->blank == FB_BLANK_UNBLANK) {
 		lcd_enable_raster();
-- 
1.7.9.5

[PATCH 11/11] i2c: omap: enhance pinctrl support

[Hebbar Gururaja]

Amend the I2C omap 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()

By optionally putting the pins into sleep state in the suspend callback
we can accomplish two things.
- One is to minimize current leakage from pins and thus save power,
- second, we can prevent the IP from driving pins output in an
uncontrolled manner, which may happen if the power domain drops the
domain regulator.

Note:
A .suspend & .resume callback is added which simply puts the pins to sleep
state upon suspend & are moved to default & idle state upon resume.

If any of the above pin states are missing in dt, a warning message
about the missing state is displayed.
If certain pin-states are not available, to remove this warning message
pass respective state name with null phandler.

(Changes based on i2c-nomadik.c)

Signed-off-by: Hebbar Gururaja <gururaja.hebbar@ti.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: linux-omap at vger.kernel.org
Cc: linux-i2c at vger.kernel.org
---
:100644 100644 e02f9e3... 588ba28... M	drivers/i2c/busses/i2c-omap.c
 drivers/i2c/busses/i2c-omap.c |  112 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 105 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index e02f9e3..588ba28 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -214,7 +214,11 @@ struct omap_i2c_dev {
 	u16			westate;
 	u16			errata;
 
-	struct pinctrl		*pins;
+	/* Three pin states - default, idle & sleep */
+	struct pinctrl			*pinctrl;
+	struct pinctrl_state		*pins_default;
+	struct pinctrl_state		*pins_idle;
+	struct pinctrl_state		*pins_sleep;
 };
 
 static const u8 reg_map_ip_v1[] = {
@@ -641,6 +645,11 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	if (IS_ERR_VALUE(r))
 		goto out;
 
+	/* Optionaly enable pins to be muxed in and configured */
+	if (!IS_ERR(dev->pins_default))
+		if (pinctrl_select_state(dev->pinctrl, dev->pins_default))
+			dev_err(dev->dev, "could not set default pins\n");
+
 	r = omap_i2c_wait_for_bb(dev);
 	if (r < 0)
 		goto out;
@@ -664,7 +673,13 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 
 out:
 	pm_runtime_mark_last_busy(dev->dev);
+
 	pm_runtime_put_autosuspend(dev->dev);
+	/* Optionally let pins go into idle state */
+	if (!IS_ERR(dev->pins_idle))
+		if (pinctrl_select_state(dev->pinctrl, dev->pins_idle))
+			dev_err(dev->dev, "could not set pins to idle state\n");
+
 	return r;
 }
 
@@ -1123,14 +1138,47 @@ omap_i2c_probe(struct platform_device *pdev)
 		dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat;
 	}
 
-	dev->pins = devm_pinctrl_get_select_default(&pdev->dev);
-	if (IS_ERR(dev->pins)) {
-		if (PTR_ERR(dev->pins) == -EPROBE_DEFER)
+	dev->pinctrl = devm_pinctrl_get(&pdev->dev);
+	if (!IS_ERR(dev->pinctrl)) {
+		dev->pins_default = pinctrl_lookup_state(dev->pinctrl,
+							 PINCTRL_STATE_DEFAULT);
+		if (IS_ERR(dev->pins_default))
+			dev_dbg(&pdev->dev, "could not get default pinstate\n");
+		else
+			if (pinctrl_select_state(dev->pinctrl,
+						 dev->pins_default))
+				dev_err(&pdev->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(&pdev->dev, "could not get idle pinstate\n");
+		else
+			/* If possible, let's idle until the first transfer */
+			if (pinctrl_select_state(dev->pinctrl, dev->pins_idle))
+				dev_err(&pdev->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(&pdev->dev, "could not get sleep pinstate\n");
+	} else {
+		if (PTR_ERR(dev->pinctrl) == -EPROBE_DEFER)
 			return -EPROBE_DEFER;
 
-		dev_warn(&pdev->dev, "did not get pins for i2c error: %li\n",
-			 PTR_ERR(dev->pins));
-		dev->pins = NULL;
+		/*
+		* Since we continue even when pinctrl node is not found,
+		* Invalidate pins as not available. This is to make sure that
+		* IS_ERR(pins_xxx) results in failure when used.
+		*/
+		dev->pins_default = ERR_PTR(-ENODATA);
+		dev->pins_idle = ERR_PTR(-ENODATA);
+		dev->pins_sleep = ERR_PTR(-ENODATA);
+
+		dev_dbg(&pdev->dev, "did not get pins for i2c error: %li\n",
+			PTR_ERR(dev->pinctrl));
 	}
 
 	dev->dev = &pdev->dev;
@@ -1300,6 +1348,10 @@ static int omap_i2c_runtime_suspend(struct device *dev)
 		omap_i2c_read_reg(_dev, OMAP_I2C_STAT_REG);
 	}
 
+	if (!IS_ERR(_dev->pins_idle))
+		if (pinctrl_select_state(_dev->pinctrl, _dev->pins_idle))
+			dev_err(dev, "could not set pins to idle state\n");
+
 	return 0;
 }
 
@@ -1311,13 +1363,59 @@ static int omap_i2c_runtime_resume(struct device *dev)
 	if (!_dev->regs)
 		return 0;
 
+	/* Optionally place the pins to the default state */
+	if (!IS_ERR(_dev->pins_default))
+		if (pinctrl_select_state(_dev->pinctrl, _dev->pins_default))
+			dev_err(dev, "could not set pins to default state\n");
+
 	__omap_i2c_init(_dev);
 
 	return 0;
 }
 #endif /* CONFIG_PM_RUNTIME */
 
+#ifdef CONFIG_PM_SLEEP
+static int omap_i2c_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
+
+	pm_runtime_get_sync(dev);
+	if (omap_i2c_wait_for_bb(_dev) < 0) {
+		pm_runtime_put_sync(dev);
+		return -EBUSY;
+	}
+	pm_runtime_put_sync(dev);
+
+	if (!IS_ERR(_dev->pins_sleep))
+		if (pinctrl_select_state(_dev->pinctrl, _dev->pins_sleep))
+			dev_err(dev, "could not set pins to sleep state\n");
+
+	return 0;
+}
+
+static int omap_i2c_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
+
+	/* First go to the default state */
+	if (!IS_ERR(_dev->pins_default))
+		if (pinctrl_select_state(_dev->pinctrl, _dev->pins_default))
+			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(_dev->pins_idle))
+		if (pinctrl_select_state(_dev->pinctrl, _dev->pins_idle))
+			dev_err(dev, "could not set pins to idle state\n");
+
+	return 0;
+}
+#endif
+
+
 static struct dev_pm_ops omap_i2c_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(omap_i2c_suspend, omap_i2c_resume)
 	SET_RUNTIME_PM_OPS(omap_i2c_runtime_suspend,
 			   omap_i2c_runtime_resume, NULL)
 };
-- 
1.7.9.5

[PATCH 11/11] i2c: omap: enhance pinctrl support

[Grygorii Strashko]

On 05/31/2013 01:13 PM, Hebbar Gururaja wrote:
> Amend the I2C omap 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()
>
> By optionally putting the pins into sleep state in the suspend callback
> we can accomplish two things.
> - One is to minimize current leakage from pins and thus save power,
> - second, we can prevent the IP from driving pins output in an
> uncontrolled manner, which may happen if the power domain drops the
> domain regulator.
>
> Note:
> A .suspend & .resume callback is added which simply puts the pins to sleep
> state upon suspend & are moved to default & idle state upon resume.
>
> If any of the above pin states are missing in dt, a warning message
> about the missing state is displayed.
> If certain pin-states are not available, to remove this warning message
> pass respective state name with null phandler.
>
> (Changes based on i2c-nomadik.c)
>
> Signed-off-by: Hebbar Gururaja <gururaja.hebbar@ti.com>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: linux-omap at vger.kernel.org
> Cc: linux-i2c at vger.kernel.org
> ---
> :100644 100644 e02f9e3... 588ba28... M	drivers/i2c/busses/i2c-omap.c
>   drivers/i2c/busses/i2c-omap.c |  112 ++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 105 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index e02f9e3..588ba28 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -214,7 +214,11 @@ struct omap_i2c_dev {
>   	u16			westate;
>   	u16			errata;
>   
> -	struct pinctrl		*pins;
> +	/* Three pin states - default, idle & sleep */
> +	struct pinctrl			*pinctrl;
> +	struct pinctrl_state		*pins_default;
> +	struct pinctrl_state		*pins_idle;
> +	struct pinctrl_state		*pins_sleep;
>   };
>   
>   static const u8 reg_map_ip_v1[] = {
> @@ -641,6 +645,11 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>   	if (IS_ERR_VALUE(r))
>   		goto out;
>   
The current HWMOD framework configures PINs to enable state before 
enabling the device and
switch PINs to idle state after disabling the device. Why here its done 
in different order?
> +	/* Optionaly enable pins to be muxed in and configured */
> +	if (!IS_ERR(dev->pins_default))
> +		if (pinctrl_select_state(dev->pinctrl, dev->pins_default))
> +			dev_err(dev->dev, "could not set default pins\n");
> +
>   	r = omap_i2c_wait_for_bb(dev);
>   	if (r < 0)
>   		goto out;
> @@ -664,7 +673,13 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>   
>   out:
>   	pm_runtime_mark_last_busy(dev->dev);
> +
>   	pm_runtime_put_autosuspend(dev->dev);
> +	/* Optionally let pins go into idle state */
> +	if (!IS_ERR(dev->pins_idle))
> +		if (pinctrl_select_state(dev->pinctrl, dev->pins_idle))
> +			dev_err(dev->dev, "could not set pins to idle state\n");
> +
>   	return r;
>   }
>   
> @@ -1123,14 +1138,47 @@ omap_i2c_probe(struct platform_device *pdev)
>   		dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat;
>   	}
>   
> -	dev->pins = devm_pinctrl_get_select_default(&pdev->dev);
> -	if (IS_ERR(dev->pins)) {
> -		if (PTR_ERR(dev->pins) == -EPROBE_DEFER)
> +	dev->pinctrl = devm_pinctrl_get(&pdev->dev);
May be struct device ->pins->p can be used instead of dev->pinctrl?
> +	if (!IS_ERR(dev->pinctrl)) {
> +		dev->pins_default = pinctrl_lookup_state(dev->pinctrl,
> +							 PINCTRL_STATE_DEFAULT);
> +		if (IS_ERR(dev->pins_default))
> +			dev_dbg(&pdev->dev, "could not get default pinstate\n");
> +		else
> +			if (pinctrl_select_state(dev->pinctrl,
> +						 dev->pins_default))
> +				dev_err(&pdev->dev,
> +					"could not set default pinstate\n");
Don't need to set Default pin state
Default pins state is applied by DD framework automatically before 
device probing
and stored in struct device ->pins->default_state
> +
> +		dev->pins_idle = pinctrl_lookup_state(dev->pinctrl,
> +						      PINCTRL_STATE_IDLE);
> +		if (IS_ERR(dev->pins_idle))
> +			dev_dbg(&pdev->dev, "could not get idle pinstate\n");
> +		else
> +			/* If possible, let's idle until the first transfer */
> +			if (pinctrl_select_state(dev->pinctrl, dev->pins_idle))
> +				dev_err(&pdev->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(&pdev->dev, "could not get sleep pinstate\n");
> +	} else {
> +		if (PTR_ERR(dev->pinctrl) == -EPROBE_DEFER)
>   			return -EPROBE_DEFER;
>   
> -		dev_warn(&pdev->dev, "did not get pins for i2c error: %li\n",
> -			 PTR_ERR(dev->pins));
> -		dev->pins = NULL;
> +		/*
> +		* Since we continue even when pinctrl node is not found,
> +		* Invalidate pins as not available. This is to make sure that
> +		* IS_ERR(pins_xxx) results in failure when used.
> +		*/
> +		dev->pins_default = ERR_PTR(-ENODATA);
> +		dev->pins_idle = ERR_PTR(-ENODATA);
> +		dev->pins_sleep = ERR_PTR(-ENODATA);
> +
> +		dev_dbg(&pdev->dev, "did not get pins for i2c error: %li\n",
> +			PTR_ERR(dev->pinctrl));
>   	}
>   
>   	dev->dev = &pdev->dev;
> @@ -1300,6 +1348,10 @@ static int omap_i2c_runtime_suspend(struct device *dev)
>   		omap_i2c_read_reg(_dev, OMAP_I2C_STAT_REG);
>   	}
>   
> +	if (!IS_ERR(_dev->pins_idle))
> +		if (pinctrl_select_state(_dev->pinctrl, _dev->pins_idle))
> +			dev_err(dev, "could not set pins to idle state\n");
> +
It's done in omap_i2c_xfer
>   	return 0;
>   }
>   
> @@ -1311,13 +1363,59 @@ static int omap_i2c_runtime_resume(struct device *dev)
>   	if (!_dev->regs)
>   		return 0;
>   
> +	/* Optionally place the pins to the default state */
> +	if (!IS_ERR(_dev->pins_default))
> +		if (pinctrl_select_state(_dev->pinctrl, _dev->pins_default))
> +			dev_err(dev, "could not set pins to default state\n");
> +
It's done in omap_i2c_xfer
>   	__omap_i2c_init(_dev);
>   
>   	return 0;
>   }
>   #endif /* CONFIG_PM_RUNTIME */
>   
> +#ifdef CONFIG_PM_SLEEP
> +static int omap_i2c_suspend(struct device *dev)
Please, use .suspend_noirq() - it's possible that I2C will be used
even after "device suspend" stage especially on SMP platforms((( ),
but its prohibited to use it after "suspend_noirq" stage (
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
> +
> +	pm_runtime_get_sync(dev);
Here is the better way to detect late I2C activities during suspending:
     i2c_lock_adapter(&_dev->adapter);

     /* Check for active I2C transaction */
     if (atomic_read(&dev->power.usage_count) > 1) {
         dev_info(dev,
              "active I2C transaction detected - suspend aborted\n");
         return -EBUSY;
     }

     i2c_unlock_adapter(&_dev->adapter);
> +	if (omap_i2c_wait_for_bb(_dev) < 0) {
> +		pm_runtime_put_sync(dev);
> +		return -EBUSY;
> +	}
> +	pm_runtime_put_sync(dev);
> +
> +	if (!IS_ERR(_dev->pins_sleep))
> +		if (pinctrl_select_state(_dev->pinctrl, _dev->pins_sleep))
> +			dev_err(dev, "could not set pins to sleep state\n");
> +
> +	return 0;
> +}
Not sure it's right - Lest assume suspend_noirq() is used here.
Then, at this point, OMAP device framework will disable device 
(_od_suspend_noirq()).
But PINs will be switched to IDLE state before that. Is it ok?
Possibly Kevin could clarify this point?
> +
> +static int omap_i2c_resume(struct device *dev)
Please, use .resume_noirq()
> +{
The same is here - OMAP device framework will enable device 
(_od_suspend_noirq()) here.
But PINs are still in IDLE state???
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
> +
> +	/* First go to the default state */
> +	if (!IS_ERR(_dev->pins_default))
> +		if (pinctrl_select_state(_dev->pinctrl, _dev->pins_default))
> +			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(_dev->pins_idle))
> +		if (pinctrl_select_state(_dev->pinctrl, _dev->pins_idle))
> +			dev_err(dev, "could not set pins to idle state\n");
> +
This is wrong - at this moment I2C device can be as in Enabled state
as in Disabled
> +	return 0;
> +}
> +#endif
> +
> +
>   static struct dev_pm_ops omap_i2c_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(omap_i2c_suspend, omap_i2c_resume)
>   	SET_RUNTIME_PM_OPS(omap_i2c_runtime_suspend,
>   			   omap_i2c_runtime_resume, NULL)
>   };

[PATCH 11/11] i2c: omap: enhance pinctrl support

[Hebbar, Gururaja]

On Fri, May 31, 2013 at 20:25:38, Strashko, Grygorii wrote:
> On 05/31/2013 01:13 PM, Hebbar Gururaja wrote:
> > Amend the I2C omap 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()
> >
> > By optionally putting the pins into sleep state in the suspend callback
> > we can accomplish two things.
> > - One is to minimize current leakage from pins and thus save power,
> > - second, we can prevent the IP from driving pins output in an
> > uncontrolled manner, which may happen if the power domain drops the
> > domain regulator.
> >
> > Note:
> > A .suspend & .resume callback is added which simply puts the pins to sleep
> > state upon suspend & are moved to default & idle state upon resume.
> >
> > If any of the above pin states are missing in dt, a warning message
> > about the missing state is displayed.
> > If certain pin-states are not available, to remove this warning message
> > pass respective state name with null phandler.
> >
> > (Changes based on i2c-nomadik.c)
> >
> > Signed-off-by: Hebbar Gururaja <gururaja.hebbar@ti.com>
> > Cc: Tony Lindgren <tony@atomide.com>
> > Cc: Wolfram Sang <wsa@the-dreams.de>
> > Cc: linux-omap at vger.kernel.org
> > Cc: linux-i2c at vger.kernel.org
> > ---
> > :100644 100644 e02f9e3... 588ba28... M	drivers/i2c/busses/i2c-omap.c
> >   drivers/i2c/busses/i2c-omap.c |  112 ++++++++++++++++++++++++++++++++++++++---
> >   1 file changed, 105 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> > index e02f9e3..588ba28 100644
> > --- a/drivers/i2c/busses/i2c-omap.c
> > +++ b/drivers/i2c/busses/i2c-omap.c
> > @@ -214,7 +214,11 @@ struct omap_i2c_dev {
> >   	u16			westate;
> >   	u16			errata;
> >   
> > -	struct pinctrl		*pins;
> > +	/* Three pin states - default, idle & sleep */
> > +	struct pinctrl			*pinctrl;
> > +	struct pinctrl_state		*pins_default;
> > +	struct pinctrl_state		*pins_idle;
> > +	struct pinctrl_state		*pins_sleep;
> >   };
> >   
> >   static const u8 reg_map_ip_v1[] = {
> > @@ -641,6 +645,11 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> >   	if (IS_ERR_VALUE(r))
> >   		goto out;
> >   
> The current HWMOD framework configures PINs to enable state before 
> enabling the device and
> switch PINs to idle state after disabling the device. Why here its done 
> in different order?

AFAIK, in case of DT boot, "oh->mux" will be NULL. So hwmod will not change
Any pins.


> > +	/* Optionaly enable pins to be muxed in and configured */
> > +	if (!IS_ERR(dev->pins_default))
> > +		if (pinctrl_select_state(dev->pinctrl, dev->pins_default))
> > +			dev_err(dev->dev, "could not set default pins\n");
> > +
> >   	r = omap_i2c_wait_for_bb(dev);
> >   	if (r < 0)
> >   		goto out;
> > @@ -664,7 +673,13 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> >   
> >   out:
> >   	pm_runtime_mark_last_busy(dev->dev);
> > +
> >   	pm_runtime_put_autosuspend(dev->dev);
> > +	/* Optionally let pins go into idle state */
> > +	if (!IS_ERR(dev->pins_idle))
> > +		if (pinctrl_select_state(dev->pinctrl, dev->pins_idle))
> > +			dev_err(dev->dev, "could not set pins to idle state\n");
> > +
> >   	return r;
> >   }
> >   
> > @@ -1123,14 +1138,47 @@ omap_i2c_probe(struct platform_device *pdev)
> >   		dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat;
> >   	}
> >   
> > -	dev->pins = devm_pinctrl_get_select_default(&pdev->dev);
> > -	if (IS_ERR(dev->pins)) {
> > -		if (PTR_ERR(dev->pins) == -EPROBE_DEFER)
> > +	dev->pinctrl = devm_pinctrl_get(&pdev->dev);
> May be struct device ->pins->p can be used instead of dev->pinctrl?
> > +	if (!IS_ERR(dev->pinctrl)) {
> > +		dev->pins_default = pinctrl_lookup_state(dev->pinctrl,
> > +							 PINCTRL_STATE_DEFAULT);
> > +		if (IS_ERR(dev->pins_default))
> > +			dev_dbg(&pdev->dev, "could not get default pinstate\n");
> > +		else
> > +			if (pinctrl_select_state(dev->pinctrl,
> > +						 dev->pins_default))
> > +				dev_err(&pdev->dev,
> > +					"could not set default pinstate\n");
> Don't need to set Default pin state
> Default pins state is applied by DD framework automatically before 
> device probing
> and stored in struct device ->pins->default_state

Correct.

> > +
> > +		dev->pins_idle = pinctrl_lookup_state(dev->pinctrl,
> > +						      PINCTRL_STATE_IDLE);
> > +		if (IS_ERR(dev->pins_idle))
> > +			dev_dbg(&pdev->dev, "could not get idle pinstate\n");
> > +		else
> > +			/* If possible, let's idle until the first transfer */
> > +			if (pinctrl_select_state(dev->pinctrl, dev->pins_idle))
> > +				dev_err(&pdev->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(&pdev->dev, "could not get sleep pinstate\n");
> > +	} else {
> > +		if (PTR_ERR(dev->pinctrl) == -EPROBE_DEFER)
> >   			return -EPROBE_DEFER;
> >   
> > -		dev_warn(&pdev->dev, "did not get pins for i2c error: %li\n",
> > -			 PTR_ERR(dev->pins));
> > -		dev->pins = NULL;
> > +		/*
> > +		* Since we continue even when pinctrl node is not found,
> > +		* Invalidate pins as not available. This is to make sure that
> > +		* IS_ERR(pins_xxx) results in failure when used.
> > +		*/
> > +		dev->pins_default = ERR_PTR(-ENODATA);
> > +		dev->pins_idle = ERR_PTR(-ENODATA);
> > +		dev->pins_sleep = ERR_PTR(-ENODATA);
> > +
> > +		dev_dbg(&pdev->dev, "did not get pins for i2c error: %li\n",
> > +			PTR_ERR(dev->pinctrl));
> >   	}
> >   
> >   	dev->dev = &pdev->dev;
> > @@ -1300,6 +1348,10 @@ static int omap_i2c_runtime_suspend(struct device *dev)
> >   		omap_i2c_read_reg(_dev, OMAP_I2C_STAT_REG);
> >   	}
> >   
> > +	if (!IS_ERR(_dev->pins_idle))
> > +		if (pinctrl_select_state(_dev->pinctrl, _dev->pins_idle))
> > +			dev_err(dev, "could not set pins to idle state\n");
> > +
> It's done in omap_i2c_xfer

Correct.

> >   	return 0;
> >   }
> >   
> > @@ -1311,13 +1363,59 @@ static int omap_i2c_runtime_resume(struct device *dev)
> >   	if (!_dev->regs)
> >   		return 0;
> >   
> > +	/* Optionally place the pins to the default state */
> > +	if (!IS_ERR(_dev->pins_default))
> > +		if (pinctrl_select_state(_dev->pinctrl, _dev->pins_default))
> > +			dev_err(dev, "could not set pins to default state\n");
> > +
> It's done in omap_i2c_xfer

Hmmm. What is not a single transaction has taken place. In that case
omap_i2c_xfer() would have not been called.

> >   	__omap_i2c_init(_dev);
> >   
> >   	return 0;
> >   }
> >   #endif /* CONFIG_PM_RUNTIME */
> >   
> > +#ifdef CONFIG_PM_SLEEP
> > +static int omap_i2c_suspend(struct device *dev)
> Please, use .suspend_noirq() - it's possible that I2C will be used
> even after "device suspend" stage especially on SMP platforms((( ),
> but its prohibited to use it after "suspend_noirq" stage (

Ok will check about those implementation.

> > +{
> > +	struct platform_device *pdev = to_platform_device(dev);
> > +	struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
> > +
> > +	pm_runtime_get_sync(dev);
> Here is the better way to detect late I2C activities during suspending:
>      i2c_lock_adapter(&_dev->adapter);
> 
>      /* Check for active I2C transaction */
>      if (atomic_read(&dev->power.usage_count) > 1) {
>          dev_info(dev,
>               "active I2C transaction detected - suspend aborted\n");
>          return -EBUSY;
>      }
> 
>      i2c_unlock_adapter(&_dev->adapter);

Hmm. Correct.

> > +	if (omap_i2c_wait_for_bb(_dev) < 0) {
> > +		pm_runtime_put_sync(dev);
> > +		return -EBUSY;
> > +	}
> > +	pm_runtime_put_sync(dev);
> > +
> > +	if (!IS_ERR(_dev->pins_sleep))
> > +		if (pinctrl_select_state(_dev->pinctrl, _dev->pins_sleep))
> > +			dev_err(dev, "could not set pins to sleep state\n");
> > +
> > +	return 0;
> > +}
> Not sure it's right - Lest assume suspend_noirq() is used here.
> Then, at this point, OMAP device framework will disable device 
> (_od_suspend_noirq()).
> But PINs will be switched to IDLE state before that. Is it ok?
> Possibly Kevin could clarify this point?

Seems he has a entirely different approach for this implementation.

> > +
> > +static int omap_i2c_resume(struct device *dev)
> Please, use .resume_noirq()

Ok.

> > +{
> The same is here - OMAP device framework will enable device 
> (_od_suspend_noirq()) here.
> But PINs are still in IDLE state???

Needs to re-check. I will see how I can overcome this issue.

> > +	struct platform_device *pdev = to_platform_device(dev);
> > +	struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
> > +
> > +	/* First go to the default state */
> > +	if (!IS_ERR(_dev->pins_default))
> > +		if (pinctrl_select_state(_dev->pinctrl, _dev->pins_default))
> > +			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(_dev->pins_idle))
> > +		if (pinctrl_select_state(_dev->pinctrl, _dev->pins_idle))
> > +			dev_err(dev, "could not set pins to idle state\n");
> > +
> This is wrong - at this moment I2C device can be as in Enabled state
> as in Disabled

As above. Needs to re-check. I will see how I can overcome this issue.

> > +	return 0;
> > +}
> > +#endif
> > +
> > +
> >   static struct dev_pm_ops omap_i2c_pm_ops = {
> > +	SET_SYSTEM_SLEEP_PM_OPS(omap_i2c_suspend, omap_i2c_resume)
> >   	SET_RUNTIME_PM_OPS(omap_i2c_runtime_suspend,
> >   			   omap_i2c_runtime_resume, NULL)
> >   };
> 
> 


Thanks for the thorough review.

Regards, 
Gururaja

[PATCH 11/11] i2c: omap: enhance pinctrl support

[Kevin Hilman]

Hebbar Gururaja <gururaja.hebbar@ti.com> writes:

> Amend the I2C omap 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()
>
> By optionally putting the pins into sleep state in the suspend callback
> we can accomplish two things.
> - One is to minimize current leakage from pins and thus save power,
> - second, we can prevent the IP from driving pins output in an
> uncontrolled manner, which may happen if the power domain drops the
> domain regulator.
>
> Note:
> A .suspend & .resume callback is added which simply puts the pins to sleep
> state upon suspend & are moved to default & idle state upon resume.
>
> If any of the above pin states are missing in dt, a warning message
> about the missing state is displayed.
> If certain pin-states are not available, to remove this warning message
> pass respective state name with null phandler.
>
> (Changes based on i2c-nomadik.c)
>
> Signed-off-by: Hebbar Gururaja <gururaja.hebbar@ti.com>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: linux-omap at vger.kernel.org
> Cc: linux-i2c at vger.kernel.org

[...]

> @@ -664,7 +673,13 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  
>  out:
>  	pm_runtime_mark_last_busy(dev->dev);
> +
>  	pm_runtime_put_autosuspend(dev->dev);
> +	/* Optionally let pins go into idle state */
> +	if (!IS_ERR(dev->pins_idle))
> +		if (pinctrl_select_state(dev->pinctrl, dev->pins_idle))
> +			dev_err(dev->dev, "could not set pins to idle state\n");

This is wrong.  You're changing the muxing before the device actually
goes idle.  Anything you want to happen when the device actually idles
for real has to be in runtime PM callbacks.

Looking below, I see it's already in the callbacks, so why is it here also?

[...]

> @@ -1300,6 +1348,10 @@ static int omap_i2c_runtime_suspend(struct device *dev)
>  		omap_i2c_read_reg(_dev, OMAP_I2C_STAT_REG);
>  	}
>  
> +	if (!IS_ERR(_dev->pins_idle))
> +		if (pinctrl_select_state(_dev->pinctrl, _dev->pins_idle))
> +			dev_err(dev, "could not set pins to idle state\n");
> +
>  	return 0;
>  }
>  

Kevin

[PATCH 11/11] i2c: omap: enhance pinctrl support

[Grygorii Strashko]

Hi Kevin, Gururaja

On 05/31/2013 08:34 PM, Kevin Hilman wrote:
> Hebbar Gururaja <gururaja.hebbar@ti.com> writes:
>
>> Amend the I2C omap 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()
>>
>> By optionally putting the pins into sleep state in the suspend callback
>> we can accomplish two things.
>> - One is to minimize current leakage from pins and thus save power,
>> - second, we can prevent the IP from driving pins output in an
>> uncontrolled manner, which may happen if the power domain drops the
>> domain regulator.
>>
>> Note:
>> A .suspend & .resume callback is added which simply puts the pins to sleep
>> state upon suspend & are moved to default & idle state upon resume.
>>
>> If any of the above pin states are missing in dt, a warning message
>> about the missing state is displayed.
>> If certain pin-states are not available, to remove this warning message
>> pass respective state name with null phandler.
>>
>> (Changes based on i2c-nomadik.c)
>>
>> Signed-off-by: Hebbar Gururaja <gururaja.hebbar@ti.com>
>> Cc: Tony Lindgren <tony@atomide.com>
>> Cc: Wolfram Sang <wsa@the-dreams.de>
>> Cc: linux-omap at vger.kernel.org
>> Cc: linux-i2c at vger.kernel.org
> [...]
>
>> @@ -664,7 +673,13 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>>   
>>   out:
>>   	pm_runtime_mark_last_busy(dev->dev);
>> +
>>   	pm_runtime_put_autosuspend(dev->dev);
>> +	/* Optionally let pins go into idle state */
>> +	if (!IS_ERR(dev->pins_idle))
>> +		if (pinctrl_select_state(dev->pinctrl, dev->pins_idle))
>> +			dev_err(dev->dev, "could not set pins to idle state\n");
> This is wrong.  You're changing the muxing before the device actually
> goes idle.  Anything you want to happen when the device actually idles
> for real has to be in runtime PM callbacks.
>
> Looking below, I see it's already in the callbacks, so why is it here also?

I have two questions regarding this patch & the whole series:

1) PM runtime suspend/resume
Current sequence:
- resume
   |- omap_hwmod_enable()
      |- _enable()
          |- omap_hwmod_mux(oh->mux, _HWMOD_STATE_ENABLED)
             |- enable module (sysc&clocks)
   |- omap_i2c_runtime_resume()
- suspend
   |- omap_i2c_runtime_suspend()
   |- omap_hwmod_idle()
      |- _idle()
          |- disbale module (sysc&clocks)
          |- omap_hwmod_mux(oh->mux, _HWMOD_STATE_IDLE);

The new order will change this sequence - *Is it safe?*:
- resume
   |- omap_hwmod_enable()
      |- _enable()
         |- enable module (sysc&clocks) <---- Is it valid to enable 
module before PINs?
   |- omap_i2c_runtime_resume()
      |- PINs state set to DEFAULT
- suspend
   |- omap_i2c_runtime_suspend()
      |- PINs state set to IDLE
   |- omap_hwmod_idle()
      |- _idle()
          |- disbale module (sysc&clocks) <---- Is it valid to disable 
module after PINs?

2) System suspend (OFF-mode) with assumption that "noirq" stage will be 
used for PINs cfg
Current implementation: handled in the same way as PM runtime


The new implementation:   -- total mess is here((:
- suspend_noirq - I2C device can be still active because of PM auto-suspend
   |-_od_suspend_noirq
      |- omap_i2c_suspend_noirq
         |- PINs state set to SLEEP
   |- pm_generic_runtime_suspend
      |- omap_i2c_runtime_suspend()
         |- PINs state set to IDLE  <--- *oops* PINs state is IDLE and 
not SLEEP
   |- omap_device_idle()
      |- omap_hwmod_idle()
         |- _idle()
            |- disbale module (sysc&clocks)

- resume_noirq - I2C was active before suspend
   |-_od_resume_noirq
      |- omap_hwmod_enable()
         |- _enable()
            |- enable module (sysc&clocks)
      |- pm_generic_runtime_resume
         |- omap_i2c_runtime_resume()
            |- PINs state set to DEFAULT  <--- !!!!
      |- omap_i2c_resume_noirq
         |- PINs state set to DEFAULT
         |- PINs state set to IDLE        <--- *big oops* we have active 
module and its
                                               PINs state is IDLE

All mentioned above, applicable for most of all patches in series.
And It seems, that this functionality can't be implemented in such way (
- OMAP device FW and Driver core might handle PINs states changing and
   drivers (DT) should provide set of available states only.

Am I wrong?

Regards,
-grygorii


>
> [...]
>
>> @@ -1300,6 +1348,10 @@ static int omap_i2c_runtime_suspend(struct device *dev)
>>   		omap_i2c_read_reg(_dev, OMAP_I2C_STAT_REG);
>>   	}
>>   
>> +	if (!IS_ERR(_dev->pins_idle))
>> +		if (pinctrl_select_state(_dev->pinctrl, _dev->pins_idle))
>> +			dev_err(dev, "could not set pins to idle state\n");
>> +
>>   	return 0;
>>   }
>>   
> Kevin
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 11/11] i2c: omap: enhance pinctrl support

[Hebbar, Gururaja]

On Fri, May 31, 2013 at 23:04:59, Kevin Hilman wrote:
> Hebbar Gururaja <gururaja.hebbar@ti.com> writes:
> 
> > Amend the I2C omap 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()
> >
> > By optionally putting the pins into sleep state in the suspend callback
> > we can accomplish two things.
> > - One is to minimize current leakage from pins and thus save power,
> > - second, we can prevent the IP from driving pins output in an
> > uncontrolled manner, which may happen if the power domain drops the
> > domain regulator.
> >
> > Note:
> > A .suspend & .resume callback is added which simply puts the pins to sleep
> > state upon suspend & are moved to default & idle state upon resume.
> >
> > If any of the above pin states are missing in dt, a warning message
> > about the missing state is displayed.
> > If certain pin-states are not available, to remove this warning message
> > pass respective state name with null phandler.
> >
> > (Changes based on i2c-nomadik.c)
> >
> > Signed-off-by: Hebbar Gururaja <gururaja.hebbar@ti.com>
> > Cc: Tony Lindgren <tony@atomide.com>
> > Cc: Wolfram Sang <wsa@the-dreams.de>
> > Cc: linux-omap at vger.kernel.org
> > Cc: linux-i2c at vger.kernel.org
> 
> [...]
> 
> > @@ -664,7 +673,13 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> >  
> >  out:
> >  	pm_runtime_mark_last_busy(dev->dev);
> > +
> >  	pm_runtime_put_autosuspend(dev->dev);
> > +	/* Optionally let pins go into idle state */
> > +	if (!IS_ERR(dev->pins_idle))
> > +		if (pinctrl_select_state(dev->pinctrl, dev->pins_idle))
> > +			dev_err(dev->dev, "could not set pins to idle state\n");
> 
> This is wrong.  You're changing the muxing before the device actually
> goes idle.  Anything you want to happen when the device actually idles
> for real has to be in runtime PM callbacks.
> 
> Looking below, I see it's already in the callbacks, so why is it here also?

Just to be double sure. Seems it is unwanted.

> 
> [...]
> 
> > @@ -1300,6 +1348,10 @@ static int omap_i2c_runtime_suspend(struct device *dev)
> >  		omap_i2c_read_reg(_dev, OMAP_I2C_STAT_REG);
> >  	}
> >  
> > +	if (!IS_ERR(_dev->pins_idle))
> > +		if (pinctrl_select_state(_dev->pinctrl, _dev->pins_idle))
> > +			dev_err(dev, "could not set pins to idle state\n");
> > +
> >  	return 0;
> >  }
> >  
> 
> Kevin
> 


Regards, 
Gururaja

[PATCH 11/11] i2c: omap: enhance pinctrl support

[Kevin Hilman]

+Linus Walleij (pinctrl maintainer)

Hebbar Gururaja <gururaja.hebbar@ti.com> writes:

> Amend the I2C omap 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()
>
> By optionally putting the pins into sleep state in the suspend callback
> we can accomplish two things.
> - One is to minimize current leakage from pins and thus save power,
> - second, we can prevent the IP from driving pins output in an
> uncontrolled manner, which may happen if the power domain drops the
> domain regulator.
>
> Note:
> A .suspend & .resume callback is added which simply puts the pins to sleep
> state upon suspend & are moved to default & idle state upon resume.
>
> If any of the above pin states are missing in dt, a warning message
> about the missing state is displayed.
> If certain pin-states are not available, to remove this warning message
> pass respective state name with null phandler.
>
> (Changes based on i2c-nomadik.c)
>
> Signed-off-by: Hebbar Gururaja <gururaja.hebbar@ti.com>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: linux-omap at vger.kernel.org
> Cc: linux-i2c at vger.kernel.org

[...]

> @@ -1123,14 +1138,47 @@ omap_i2c_probe(struct platform_device *pdev)
>  		dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat;
>  	}
>  
> -	dev->pins = devm_pinctrl_get_select_default(&pdev->dev);
> -	if (IS_ERR(dev->pins)) {
> -		if (PTR_ERR(dev->pins) == -EPROBE_DEFER)
> +	dev->pinctrl = devm_pinctrl_get(&pdev->dev);
> +	if (!IS_ERR(dev->pinctrl)) {
> +		dev->pins_default = pinctrl_lookup_state(dev->pinctrl,
> +							 PINCTRL_STATE_DEFAULT);

This part is already done by probe in driver core, why does it need to
be done again.  dev->pins->default_state should already have this.
(c.f. pinctrl_bind_pins() in drivers/base/pinctrl.c)

But that brings up a bigger question about whether or not we should be
doing the rest of this (idle/sleep) pin management in the drivers or in
the driver core as well.  I would much prefer it be handled by the
driver core.

In fact, since these are all PM related events, it should probably be
handled by the PM core and seems pretty straight forward to do so.

Kevin

[PATCH 11/11] i2c: omap: enhance pinctrl support

[Linus Walleij]

On Fri, May 31, 2013 at 8:07 PM, Kevin Hilman <khilman@linaro.org> wrote:

> But that brings up a bigger question about whether or not we should be
> doing the rest of this (idle/sleep) pin management in the drivers or in
> the driver core as well.  I would much prefer it be handled by the
> driver core.

Yes. See my suggestion in 2/11.

> In fact, since these are all PM related events, it should probably be
> handled by the PM core and seems pretty straight forward to do so.

I can see a clean interface with three simple functions toward
<pinctrl/consumer.h> for switching between the default, idle and
sleep states, but you may see even further...

Yours,
Linus Walleij

[PATCH 11/11] i2c: omap: enhance pinctrl support

[Hebbar, Gururaja]

On Fri, May 31, 2013 at 23:37:02, Kevin Hilman wrote:
> +Linus Walleij (pinctrl maintainer)
> 
> Hebbar Gururaja <gururaja.hebbar@ti.com> writes:
> 
> > Amend the I2C omap 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()
> >
> > By optionally putting the pins into sleep state in the suspend callback
> > we can accomplish two things.
> > - One is to minimize current leakage from pins and thus save power,
> > - second, we can prevent the IP from driving pins output in an
> > uncontrolled manner, which may happen if the power domain drops the
> > domain regulator.
> >
> > Note:
> > A .suspend & .resume callback is added which simply puts the pins to sleep
> > state upon suspend & are moved to default & idle state upon resume.
> >
> > If any of the above pin states are missing in dt, a warning message
> > about the missing state is displayed.
> > If certain pin-states are not available, to remove this warning message
> > pass respective state name with null phandler.
> >
> > (Changes based on i2c-nomadik.c)
> >
> > Signed-off-by: Hebbar Gururaja <gururaja.hebbar@ti.com>
> > Cc: Tony Lindgren <tony@atomide.com>
> > Cc: Wolfram Sang <wsa@the-dreams.de>
> > Cc: linux-omap at vger.kernel.org
> > Cc: linux-i2c at vger.kernel.org
> 
> [...]
> 
> > @@ -1123,14 +1138,47 @@ omap_i2c_probe(struct platform_device *pdev)
> >  		dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat;
> >  	}
> >  
> > -	dev->pins = devm_pinctrl_get_select_default(&pdev->dev);
> > -	if (IS_ERR(dev->pins)) {
> > -		if (PTR_ERR(dev->pins) == -EPROBE_DEFER)
> > +	dev->pinctrl = devm_pinctrl_get(&pdev->dev);
> > +	if (!IS_ERR(dev->pinctrl)) {
> > +		dev->pins_default = pinctrl_lookup_state(dev->pinctrl,
> > +							 PINCTRL_STATE_DEFAULT);
> 
> This part is already done by probe in driver core, why does it need to
> be done again.  dev->pins->default_state should already have this.
> (c.f. pinctrl_bind_pins() in drivers/base/pinctrl.c)
> 
> But that brings up a bigger question about whether or not we should be
> doing the rest of this (idle/sleep) pin management in the drivers or in
> the driver core as well.  I would much prefer it be handled by the
> driver core.
> 
> In fact, since these are all PM related events, it should probably be
> handled by the PM core and seems pretty straight forward to do so.

Let me pull out some info about these and come back

> 
> Kevin
> 


Regards, 
Gururaja

[PATCH 00/11] drivers: Add Pinctrl PM support

[Dmitry Torokhov]

Hi Hebbar,

On Fri, May 31, 2013 at 03:43:00PM +0530, Hebbar Gururaja wrote:
> By optionally putting the pins into sleep state in the suspend [or in
> runtime_suspend] callback we can accomplish two things.
> - One is to minimize current leakage from pins and thus save power,
> - second, we can prevent the IP from driving pins output in an
> uncontrolled manner, which may happen if the power domain drops the
> domain regulator.
> 
> These states can be specified in the DT blob and corresponding driver
> can pick these states during probe & set the related values during
> idle/suspend.
> 
> Not all drivers support/has idle state. Drivers like i2c, spi, mmc has
> idle states and hence these drivers are updated to support all the
> three states
> - default  : during regular operation
> - idle : when the module is in idle state
> - sleep : when the module is in suspend state
> 
> For those drivers which doesn't support/have idle state (at least at
> the moment), only default & sleep state is added.

As with the original introduction of pinctrl states my question is: "Can
all of this be handled in the driver/bus core instead of adding a lot
of boilerplate code to the individual drivers".

Thanks.

-- 
Dmitry

[PATCH 00/11] drivers: Add Pinctrl PM support

[Kevin Hilman]

+ Linus W. (pinctrl maintainer)

Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:

> Hi Hebbar,
>
> On Fri, May 31, 2013 at 03:43:00PM +0530, Hebbar Gururaja wrote:
>> By optionally putting the pins into sleep state in the suspend [or in
>> runtime_suspend] callback we can accomplish two things.
>> - One is to minimize current leakage from pins and thus save power,
>> - second, we can prevent the IP from driving pins output in an
>> uncontrolled manner, which may happen if the power domain drops the
>> domain regulator.
>> 
>> These states can be specified in the DT blob and corresponding driver
>> can pick these states during probe & set the related values during
>> idle/suspend.
>> 
>> Not all drivers support/has idle state. Drivers like i2c, spi, mmc has
>> idle states and hence these drivers are updated to support all the
>> three states
>> - default  : during regular operation
>> - idle : when the module is in idle state
>> - sleep : when the module is in suspend state
>> 
>> For those drivers which doesn't support/have idle state (at least at
>> the moment), only default & sleep state is added.
>
> As with the original introduction of pinctrl states my question is: "Can
> all of this be handled in the driver/bus core instead of adding a lot
> of boilerplate code to the individual drivers".

Yes, I had the same thought.

What's being handled here are either events related to runtime PM
(runtime suspend, runtime resume) or system PM (suspend/resume) so seems
appropriat to handle them in the PM core.

Kevin

[PATCH 00/11] drivers: Add Pinctrl PM support

[Linus Walleij]

On Fri, May 31, 2013 at 7:04 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:

> As with the original introduction of pinctrl states my question is: "Can
> all of this be handled in the driver/bus core instead of adding a lot
> of boilerplate code to the individual drivers".

It can. Per the suggestion in 2/11:

- Adding pins_sleep and pins_idle to struct dev_pin_info
  in include/linux/pinctrl/devinfo.h

- Modify drivers/base/pinctrl.c to optionally look up
  sleep and idle states, you can make that code
  #ifdef CONFIG_PM I think.

- Add something like static inline functions to
  include/linux/pinctrl/consumer.h
  with names like pinctrl_select_pm_idle(struct device *)
  pinctrl_select_pm_sleep(struct device *) to switch states
  using the device core containers, and includes
  checking IS_ERR() on the handles etc.

I think this will save a *lot* of identical code in all the
drivers, that will just have to call
pinctrl_select_pm_sleep(), pinctrl_select_pm_default()
pinctrl_select_pm_idle() instead of all the complex code.

This is what I planned to do but never got around to.

Yours,
Linus Walleij

[PATCH 00/11] drivers: Add Pinctrl PM support

[Kevin Hilman]

Hi Linus,

Linus Walleij <linus.walleij@linaro.org> writes:

> On Fri, May 31, 2013 at 7:04 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>
>> As with the original introduction of pinctrl states my question is: "Can
>> all of this be handled in the driver/bus core instead of adding a lot
>> of boilerplate code to the individual drivers".
>
> It can. Per the suggestion in 2/11:
>
> - Adding pins_sleep and pins_idle to struct dev_pin_info
>   in include/linux/pinctrl/devinfo.h
>
> - Modify drivers/base/pinctrl.c to optionally look up
>   sleep and idle states, you can make that code
>   #ifdef CONFIG_PM I think.
>
> - Add something like static inline functions to
>   include/linux/pinctrl/consumer.h
>   with names like pinctrl_select_pm_idle(struct device *)
>   pinctrl_select_pm_sleep(struct device *) to switch states
>   using the device core containers, and includes
>   checking IS_ERR() on the handles etc.
>
> I think this will save a *lot* of identical code in all the
> drivers, that will just have to call
> pinctrl_select_pm_sleep(), pinctrl_select_pm_default()
> pinctrl_select_pm_idle() instead of all the complex code.
>
> This is what I planned to do but never got around to.

What about taking it one tiny step further, and having it done by the PM
core itself?

Seems like something that should just be done as part of runtime PM
transitions and the suspend/resume process.

Kevin

[PATCH 00/11] drivers: Add Pinctrl PM support

[Mark Brown]

On Tue, Jun 04, 2013 at 11:15:51AM -0700, Kevin Hilman wrote:
> Linus Walleij <linus.walleij@linaro.org> writes:

> > I think this will save a *lot* of identical code in all the
> > drivers, that will just have to call
> > pinctrl_select_pm_sleep(), pinctrl_select_pm_default()
> > pinctrl_select_pm_idle() instead of all the complex code.

> > This is what I planned to do but never got around to.

> What about taking it one tiny step further, and having it done by the PM
> core itself?

> Seems like something that should just be done as part of runtime PM
> transitions and the suspend/resume process.

Indeed, I remember suggesting this as part of the original discussion of
having the code do the default state setting.  It seems like there ought
to be very few devices which actively manage pinctrl themselves,
otherwise we'll get a lot of boilerplate.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130604/aea822c8/attachment.sig>

[PATCH 00/11] drivers: Add Pinctrl PM support

[Linus Walleij]

On Tue, Jun 4, 2013 at 8:15 PM, Kevin Hilman <khilman@linaro.org> wrote:
> [Me]
>> I think this will save a *lot* of identical code in all the
>> drivers, that will just have to call
>> pinctrl_select_pm_sleep(), pinctrl_select_pm_default()
>> pinctrl_select_pm_idle() instead of all the complex code.
>>
>> This is what I planned to do but never got around to.
>
> What about taking it one tiny step further, and having it done by the PM
> core itself?

Maybe so, but atleast currently we cannot do it
exclusively in the PM core.

We have things like what you find in
drivers/tty/serial/amba-pl011.c
where pl011_shutdown() puts the pins into sleep mode
on unused TTYs.

The same applies to any unused ports like that, we're
thinking of a way to sleep pins for unused I2C hosts
for example.

And that works fine also on systems that does not
enable CONFIG_PM (so the pin control subsystem
is completely orthogonal). But maybe that is
complicating things? I was just thinking it was a bit
thick to require everything and its dog to make a
complete transition to (runtime)PM.

Yours,
Linus Walleij