[PATCH] ARM: SAMSUNG: Add suspend/resume support for S3C PWM driver

[PATCH] ARM: SAMSUNG: Add suspend/resume support for S3C PWM driver

[Vasily Khoruzhick]

Reset period_ns and duty_ns values in suspend handler to avoid skip of
configuration if same values passed to pwm_config;
Restore invertion bit in resume handler.

Without this patch PWM works incorrectly after resume from suspend.

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
 arch/arm/plat-samsung/pwm.c |   35 +++++++++++++++++++++++++++++++++++
 1 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-samsung/pwm.c b/arch/arm/plat-samsung/pwm.c
index ef019f2..f2d1139 100644
--- a/arch/arm/plat-samsung/pwm.c
+++ b/arch/arm/plat-samsung/pwm.c
@@ -379,6 +379,39 @@ static int __devexit s3c_pwm_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM
+static int s3c_pwm_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	struct pwm_device *pwm = platform_get_drvdata(pdev);
+
+	/* No one preserve these values during suspend so reset them
+	 * Otherwise driver leaves PWM unconfigured if same values
+	 * passed to pwm_config
+	 */
+	pwm->period_ns = 0;
+	pwm->duty_ns = 0;
+
+	return 0;
+}
+
+static int s3c_pwm_resume(struct platform_device *pdev)
+{
+	struct pwm_device *pwm = platform_get_drvdata(pdev);
+	unsigned long tcon;
+
+	/* Restore invertion */
+	tcon = __raw_readl(S3C2410_TCON);
+	tcon |= pwm_tcon_invert(pwm);
+	__raw_writel(tcon, S3C2410_TCON);
+
+	return 0;
+}
+
+#else
+#define s3c_pwm_suspend NULL
+#define s3c_pwm_resume NULL
+#endif
+
 static struct platform_driver s3c_pwm_driver = {
 	.driver		= {
 		.name	= "s3c24xx-pwm",
@@ -386,6 +419,8 @@ static struct platform_driver s3c_pwm_driver = {
 	},
 	.probe		= s3c_pwm_probe,
 	.remove		= __devexit_p(s3c_pwm_remove),
+	.suspend	= s3c_pwm_suspend,
+	.resume		= s3c_pwm_resume,
 };
 
 static int __init pwm_init(void)
-- 
1.7.0.2

[PATCH] ARM: SAMSUNG: Add suspend/resume support for S3C PWM driver

[Lars-Peter Clausen]

Vasily Khoruzhick wrote:
> Reset period_ns and duty_ns values in suspend handler to avoid skip of
> configuration if same values passed to pwm_config;
> Restore invertion bit in resume handler.
>
> Without this patch PWM works incorrectly after resume from suspend.
>
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
>   
Hi

There is still one big issue left regarding pwm suspend/resume.
If the invert bit is not set, the pwm will generate a HIGH signal when
being inactive, when the bit is set it will generate a HIGH signal when
active.
As a result any pin to which the pwm signal is routed will appear as
active until the pwm resume handler is called. This usually takes a few
100 ms seconds and so for a short period of time we'll get the wrong
signal on pwm pins.
For correct behavior the pwm driver would have to check all pins to
which it's signal might be routed and if it's actually is configure the
pin as LOW output.
Upon resume the pin then has to be reconfigured as a pwm pin, only after
the invert bit has been set.

I know that it previously has been stated, that it is not desired for
the pwm driver to know about gpio pins. But in my opinion to ensure
correct behavior it is unavoidable.

- Lars

> ---
>  arch/arm/plat-samsung/pwm.c |   35 +++++++++++++++++++++++++++++++++++
>  1 files changed, 35 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/plat-samsung/pwm.c b/arch/arm/plat-samsung/pwm.c
> index ef019f2..f2d1139 100644
> --- a/arch/arm/plat-samsung/pwm.c
> +++ b/arch/arm/plat-samsung/pwm.c
> @@ -379,6 +379,39 @@ static int __devexit s3c_pwm_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM
> +static int s3c_pwm_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	struct pwm_device *pwm = platform_get_drvdata(pdev);
> +
> +	/* No one preserve these values during suspend so reset them
> +	 * Otherwise driver leaves PWM unconfigured if same values
> +	 * passed to pwm_config
> +	 */
> +	pwm->period_ns = 0;
> +	pwm->duty_ns = 0;
> +
> +	return 0;
> +}
> +
> +static int s3c_pwm_resume(struct platform_device *pdev)
> +{
> +	struct pwm_device *pwm = platform_get_drvdata(pdev);
> +	unsigned long tcon;
> +
> +	/* Restore invertion */
> +	tcon = __raw_readl(S3C2410_TCON);
> +	tcon |= pwm_tcon_invert(pwm);
> +	__raw_writel(tcon, S3C2410_TCON);
> +
> +	return 0;
> +}
> +
> +#else
> +#define s3c_pwm_suspend NULL
> +#define s3c_pwm_resume NULL
> +#endif
> +
>  static struct platform_driver s3c_pwm_driver = {
>  	.driver		= {
>  		.name	= "s3c24xx-pwm",
> @@ -386,6 +419,8 @@ static struct platform_driver s3c_pwm_driver = {
>  	},
>  	.probe		= s3c_pwm_probe,
>  	.remove		= __devexit_p(s3c_pwm_remove),
> +	.suspend	= s3c_pwm_suspend,
> +	.resume		= s3c_pwm_resume,
>  };
>  
>  static int __init pwm_init(void)
>   

[PATCH] ARM: SAMSUNG: Add suspend/resume support for S3C PWM driver

[Vasily Khoruzhick]

? ????????? ?? 15 ????? 2010 21:36:05 ????? Lars-Peter Clausen ???????:

> Hi
> 
> There is still one big issue left regarding pwm suspend/resume.
> If the invert bit is not set, the pwm will generate a HIGH signal when
> being inactive, when the bit is set it will generate a HIGH signal when
> active.
> As a result any pin to which the pwm signal is routed will appear as
> active until the pwm resume handler is called. This usually takes a few
> 100 ms seconds and so for a short period of time we'll get the wrong
> signal on pwm pins.
> For correct behavior the pwm driver would have to check all pins to
> which it's signal might be routed and if it's actually is configure the
> pin as LOW output.
> Upon resume the pin then has to be reconfigured as a pwm pin, only after
> the invert bit has been set.
> 
> I know that it previously has been stated, that it is not desired for
> the pwm driver to know about gpio pins. But in my opinion to ensure
> correct behavior it is unavoidable.
> 
> - Lars

Hi, why not to leave this job for driver that use PWM? This driver should 
disable PWM before going to suspend and ensure that GPIO pin is in right 
state. (For example, I'm using pwm-bl callbacks to manage gpio pin state when 
PWM is disabled).

Regards
Vasily
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100315/8292d817/attachment.sig>

[PATCH] ARM: SAMSUNG: Add suspend/resume support for S3C PWM driver

[Lars-Peter Clausen]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Vasily Khoruzhick wrote:
> ? ????????? ?? 15 ????? 2010 21:36:05 ????? Lars-Peter Clausen
> ???????:
>
>> Hi
>>
>> There is still one big issue left regarding pwm suspend/resume.
>> If the invert bit is not set, the pwm will generate a HIGH signal
>> when being inactive, when the bit is set it will generate a HIGH
>> signal when active. As a result any pin to which the pwm signal
>> is routed will appear as active until the pwm resume handler is
>> called. This usually takes a few 100 ms seconds and so for a
>> short period of time we'll get the wrong signal on pwm pins. For
>> correct behavior the pwm driver would have to check all pins to
>> which it's signal might be routed and if it's actually is
>> configure the pin as LOW output. Upon resume the pin then has to
>> be reconfigured as a pwm pin, only after the invert bit has been
>> set.
>>
>> I know that it previously has been stated, that it is not desired
>> for the pwm driver to know about gpio pins. But in my opinion to
>> ensure correct behavior it is unavoidable.
>>
>> - Lars
>

> Hi, why not to leave this job for driver that use PWM? This driver
> should disable PWM before going to suspend and ensure that GPIO pin
> is in right state. (For example, I'm using pwm-bl callbacks to
> manage gpio pin state when PWM is disabled).
>
Hi

Because it would require to implement the same functionality over and
over again. Furthermore it would require each generic driver using the
pwm api to provide such callbacks.
And you - as a user of the api - will have to know about this oddity.
In my opinion it is a detail specific to the samsung platform and
should be hidden behind the common interface.

- - Lars
> Regards Vasily
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkuemUUACgkQBX4mSR26RiOU7ACggbRUJLjBOV9XVxlDofceBgcR
jIIAn3mRtLhi0RWwyUS7Q60IbeMAdLEP
=Se19
-----END PGP SIGNATURE-----

[PATCH] ARM: SAMSUNG: Add suspend/resume support for S3C PWM driver

[Vasily Khoruzhick]

? ????????? ?? 15 ????? 2010 22:32:06 ????? Lars-Peter Clausen ???????:

> Hi
> 
> Because it would require to implement the same functionality over and
> over again. Furthermore it would require each generic driver using the
> pwm api to provide such callbacks.
> And you - as a user of the api - will have to know about this oddity.
> In my opinion it is a detail specific to the samsung platform and
> should be hidden behind the common interface.
> 
> - Lars

So, you want to configure corresponding GPIO pin as output and set it to low 
in pwm_disable, re-configure it as PWM output in pwm_enable, and 
disable/enable PWM during suspend/resume cycle? If so - it's easy to 
implement, but I want to hear Ben's opinion.

Regards
Vasily
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100315/a864c9ba/attachment.sig>

[PATCH] ARM: SAMSUNG: Add suspend/resume support for S3C PWM driver

[Lars-Peter Clausen]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Vasily Khoruzhick wrote:
> ? ????????? ?? 15 ????? 2010 22:32:06 ????? Lars-Peter Clausen
> ???????:
>
>> Hi
>>
>> Because it would require to implement the same functionality over
>> and over again. Furthermore it would require each generic driver
>> using the pwm api to provide such callbacks. And you - as a user
>> of the api - will have to know about this oddity. In my opinion
>> it is a detail specific to the samsung platform and should be
>> hidden behind the common interface.
>>
>> - Lars
>
> So, you want to configure corresponding GPIO pin as output and set
> it to low in pwm_disable, re-configure it as PWM output in
> pwm_enable, and disable/enable PWM during suspend/resume cycle? If
> so - it's easy to implement, but I want to hear Ben's opinion.
>
> Regards Vasily
Hi

Yes. That's what I would prefer. The PWM is, if I'm not mistaken,
implicitly turned of during suspend anyway.
One problem that arises is that the PWM pins have different numbers on
different Samsung SoC types, which was iirc the reason why the driver
has been written without pin configuring support.
But if the mapping is needed anyway it might also be desirable to
configure the corresponding pin as PWM pin in the drivers probe
function. Cause this currently has to be done separately as well.

But yes, lets wait for Ben's opinion.

- - Lars
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkuesmYACgkQBX4mSR26RiMd5ACffExKN2W9L6/MzLi7b4wG/xa3
fHUAn02pW+1m1KdDfyyrUtW1GW8Az+9c
=IrGE
-----END PGP SIGNATURE-----