[PATCH] leds: pwm: Allow changing the pinctrl state

[PATCH] leds: pwm: Allow changing the pinctrl state

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@nxp.com>

Add suspend/resume pm hooks, so that the pinctrl state could be changed.

The motivation for doing this was to solve a problem on a imx6-cubox-i
board, where there is a GPIO controlled via pwm-leds that keeps turned on
when the system goes into suspend. Such behaviour is not what we expect
from a system going into suspend.

In order to solve this problem add the suspend/resume pm hooks where
the pinctrl state can be changed accordingly.

This allows to add an extra 'sleep' entry into the pinctrl node, where
the pinctrl can be changed from PWM to GPIO and pull up/pull down can be
configured to turn off the LED during suspend mode.

Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
---
 drivers/leds/leds-pwm.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index 8d456dc6..ff21aa2 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -14,6 +14,7 @@
 
 #include <linux/module.h>
 #include <linux/kernel.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/of_platform.h>
 #include <linux/fb.h>
@@ -210,6 +211,20 @@ static int led_pwm_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int __maybe_unused led_pwm_suspend(struct device *dev)
+{
+	return pinctrl_pm_select_sleep_state(dev);
+}
+
+static int __maybe_unused led_pwm_resume(struct device *dev)
+{
+	return pinctrl_pm_select_default_state(dev);
+}
+
+static const struct dev_pm_ops led_pwm_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(led_pwm_suspend, led_pwm_resume)
+};
+
 static const struct of_device_id of_pwm_leds_match[] = {
 	{ .compatible = "pwm-leds", },
 	{},
@@ -222,6 +237,7 @@ static struct platform_driver led_pwm_driver = {
 	.driver		= {
 		.name	= "leds_pwm",
 		.of_match_table = of_pwm_leds_match,
+		.pm = &led_pwm_pm_ops,
 	},
 };
 
-- 
2.7.4

Re: [PATCH] leds: pwm: Allow changing the pinctrl state

[Jacek Anaszewski]

Hi Fabio,

Thanks for the patch.

On 11/17/2017 03:32 PM, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@nxp.com>
> 
> Add suspend/resume pm hooks, so that the pinctrl state could be changed.
> 
> The motivation for doing this was to solve a problem on a imx6-cubox-i
> board, where there is a GPIO controlled via pwm-leds that keeps turned on
> when the system goes into suspend. Such behaviour is not what we expect
> from a system going into suspend.
> 
> In order to solve this problem add the suspend/resume pm hooks where
> the pinctrl state can be changed accordingly.
> 
> This allows to add an extra 'sleep' entry into the pinctrl node, where
> the pinctrl can be changed from PWM to GPIO and pull up/pull down can be
> configured to turn off the LED during suspend mode.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
> ---
>  drivers/leds/leds-pwm.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
> index 8d456dc6..ff21aa2 100644
> --- a/drivers/leds/leds-pwm.c
> +++ b/drivers/leds/leds-pwm.c
> @@ -14,6 +14,7 @@
>  
>  #include <linux/module.h>
>  #include <linux/kernel.h>
> +#include <linux/pinctrl/consumer.h>
>  #include <linux/platform_device.h>
>  #include <linux/of_platform.h>
>  #include <linux/fb.h>
> @@ -210,6 +211,20 @@ static int led_pwm_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static int __maybe_unused led_pwm_suspend(struct device *dev)
> +{
> +	return pinctrl_pm_select_sleep_state(dev);
> +}
> +
> +static int __maybe_unused led_pwm_resume(struct device *dev)
> +{
> +	return pinctrl_pm_select_default_state(dev);
> +}
> +
> +static const struct dev_pm_ops led_pwm_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(led_pwm_suspend, led_pwm_resume)
> +};
> +
>  static const struct of_device_id of_pwm_leds_match[] = {
>  	{ .compatible = "pwm-leds", },
>  	{},
> @@ -222,6 +237,7 @@ static struct platform_driver led_pwm_driver = {
>  	.driver		= {
>  		.name	= "leds_pwm",
>  		.of_match_table = of_pwm_leds_match,
> +		.pm = &led_pwm_pm_ops,

LED subsystem has its global pm_ops handler - see
drivers/leds/led-class.c:

static int __init leds_init(void)
{
        leds_class = class_create(THIS_MODULE, "leds");
        if (IS_ERR(leds_class))
                return PTR_ERR(leds_class);
        leds_class->pm = &leds_class_dev_pm_ops; <--------------
        leds_class->dev_groups = led_groups;
        return 0;
}

On power down event brightness is set to 0 on each registered
LED class device that set LED_CORE_SUSPENDRESUME flag (leds-pwm does).

It may be the case that pwm subsystem (or underlaying bus used by
particular controller) is suspended before LED subsystem, or the
culprit is the generic LED subsystem workqueue in which brightness
setting events are queued for drivers that use brightness_set_blocking
op (leds-pwm case).

Probably we will have to get rid of global handling of pm ops in
led-class.c due to possible asynchronous brightness setting.

Could you please verify if the led_pwm_set is called with brightness
0 on suspend or not?

If not, then probably replacing pinctrl_pm_select_sleep_state(dev)
in your patch  with a loop iterating through all LED class devices
registered by the driver and calling pwm_disable(led_dat->pwm) should
fix the problem.

pinctrl_pm_select_sleep_state(dev) would not work for all pwm
devices (think of pwm controllers driven through e.g. I2C bus).

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH] leds: pwm: Allow changing the pinctrl state

[Fabio Estevam]

Hi Jacek,

On Fri, Nov 17, 2017 at 6:51 PM, Jacek Anaszewski
<jacek.anaszewski@gmail.com> wrote:

> On power down event brightness is set to 0 on each registered
> LED class device that set LED_CORE_SUSPENDRESUME flag (leds-pwm does).
>
> It may be the case that pwm subsystem (or underlaying bus used by
> particular controller) is suspended before LED subsystem, or the
> culprit is the generic LED subsystem workqueue in which brightness
> setting events are queued for drivers that use brightness_set_blocking
> op (leds-pwm case).

Thanks for your comments.

I realized that this issue can be fixed inside drivers/pwm/pwm-imx.c :

--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -35,6 +35,7 @@
 #define MX3_PWMSAR                     0x0C    /* PWM Sample Register */
 #define MX3_PWMPR                      0x10    /* PWM Period Register */
 #define MX3_PWMCR_PRESCALER(x)         ((((x) - 1) & 0xFFF) << 4)
+#define MX3_PWMCR_STOPEN               (1 << 25)
 #define MX3_PWMCR_DOZEEN               (1 << 24)
 #define MX3_PWMCR_WAITEN               (1 << 23)
 #define MX3_PWMCR_DBGEN                        (1 << 22)
@@ -209,7 +210,7 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip,
struct pwm_device *pwm,
                writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
                writel(period_cycles, imx->mmio_base + MX3_PWMPR);

-               cr = MX3_PWMCR_PRESCALER(prescale) |
+               cr = MX3_PWMCR_PRESCALER(prescale) | MX3_PWMCR_STOPEN |
                     MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
                     MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH |
                     MX3_PWMCR_EN;

Setting the STOPEN bit, which "keeps PWM functional while in stop
mode" does the trick.

Maybe I will convert this into an optional device tree property to
keep the original behavior.

Thanks