pwm: jz4740: document known limitations
diff mbox series

Message ID 20190730123229.31839-1-u.kleine-koenig@pengutronix.de
State New
Headers show
Series
  • pwm: jz4740: document known limitations
Related show

Commit Message

Uwe Kleine-König July 30, 2019, 12:32 p.m. UTC
The jz4740 PMW implementation doesn't fulfill the (up to now
insufficiently documented) requirements of the PWM API. At least
document them in the driver.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
I intended to also add a Link to the reference manual, Paul suggested to
use https://zcrc.me/~paul/jz_docs/ in December last year, but this
stopped to work.

The second item is something I noticed when reading through the manual,
but it's not confirmed in practise. A test that this is indeed the case
could be done by configuring a long period (say 5s) and a (in
comparison) small duty-cycle (say 1s). If the pwm output isn't active
when the call returns I'd consider this proven.

@Paul: would you mind doing this test?

Best regards
Uwe

 drivers/pwm/pwm-jz4740.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Paul Cercueil Aug. 7, 2019, 1:42 p.m. UTC | #1
Le mar. 30 juil. 2019 à 14:32, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= 
<u.kleine-koenig@pengutronix.de> a écrit :
> The jz4740 PMW implementation doesn't fulfill the (up to now
> insufficiently documented) requirements of the PWM API. At least
> document them in the driver.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> I intended to also add a Link to the reference manual, Paul suggested 
> to
> use https://zcrc.me/~paul/jz_docs/ in December last year, but this
> stopped to work.
> 
> The second item is something I noticed when reading through the 
> manual,
> but it's not confirmed in practise. A test that this is indeed the 
> case
> could be done by configuring a long period (say 5s) and a (in
> comparison) small duty-cycle (say 1s). If the pwm output isn't active
> when the call returns I'd consider this proven.
> 
> @Paul: would you mind doing this test?

You're correct. I configured it for 4s period and 2s duty. After 
enabling the
PWM, it stays LOW for two seconds then switches HIGH for two seconds.

That can be corrected, though, by inverting the configured polarity 
when the
PWM is running and set "period - duty" as the duty value. I can make a 
patch
for that.

Cheers,
-Paul


> Best regards
> Uwe
> 
>  drivers/pwm/pwm-jz4740.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
> index f901e8a0d33d..9d444d012f92 100644
> --- a/drivers/pwm/pwm-jz4740.c
> +++ b/drivers/pwm/pwm-jz4740.c
> @@ -2,6 +2,11 @@
>  /*
>   *  Copyright (C) 2010, Lars-Peter Clausen <lars@metafoo.de>
>   *  JZ4740 platform PWM support
> + *
> + * Limitations:
> + * - The .apply callback doesn't complete the currently running 
> period before
> + *   reconfiguring the hardware.
> + * - Each period starts with the inactive part.
>   */
> 
>  #include <linux/clk.h>
> --
> 2.20.1
>
Uwe Kleine-König Aug. 7, 2019, 1:49 p.m. UTC | #2
On Wed, Aug 07, 2019 at 03:42:31PM +0200, Paul Cercueil wrote:
> 
> 
> Le mar. 30 juil. 2019 à 14:32, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
> <u.kleine-koenig@pengutronix.de> a écrit :
> > The jz4740 PMW implementation doesn't fulfill the (up to now
> > insufficiently documented) requirements of the PWM API. At least
> > document them in the driver.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> > I intended to also add a Link to the reference manual, Paul suggested to
> > use https://zcrc.me/~paul/jz_docs/ in December last year, but this
> > stopped to work.
> > 
> > The second item is something I noticed when reading through the manual,
> > but it's not confirmed in practise. A test that this is indeed the case
> > could be done by configuring a long period (say 5s) and a (in
> > comparison) small duty-cycle (say 1s). If the pwm output isn't active
> > when the call returns I'd consider this proven.
> > 
> > @Paul: would you mind doing this test?
> 
> You're correct. I configured it for 4s period and 2s duty. After enabling
> the
> PWM, it stays LOW for two seconds then switches HIGH for two seconds.
> 
> That can be corrected, though, by inverting the configured polarity when the
> PWM is running and set "period - duty" as the duty value. I can make a patch
> for that.

OK. Do you care for documenting the first limitation then, too?

Or should we apply my patch as is and you remote the second item when
you fix it?

Best regards
Uwe
Paul Cercueil Aug. 7, 2019, 1:53 p.m. UTC | #3
Le mar. 30 juil. 2019 à 14:32, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= 
<u.kleine-koenig@pengutronix.de> a écrit :
> The jz4740 PMW implementation doesn't fulfill the (up to now
> insufficiently documented) requirements of the PWM API. At least
> document them in the driver.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Reviewed-by: Paul Cercueil <paul@crapouillou.net>

> ---
> I intended to also add a Link to the reference manual, Paul suggested 
> to
> use https://zcrc.me/~paul/jz_docs/ in December last year, but this
> stopped to work.
> 
> The second item is something I noticed when reading through the 
> manual,
> but it's not confirmed in practise. A test that this is indeed the 
> case
> could be done by configuring a long period (say 5s) and a (in
> comparison) small duty-cycle (say 1s). If the pwm output isn't active
> when the call returns I'd consider this proven.
> 
> @Paul: would you mind doing this test?
> 
> Best regards
> Uwe
> 
>  drivers/pwm/pwm-jz4740.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
> index f901e8a0d33d..9d444d012f92 100644
> --- a/drivers/pwm/pwm-jz4740.c
> +++ b/drivers/pwm/pwm-jz4740.c
> @@ -2,6 +2,11 @@
>  /*
>   *  Copyright (C) 2010, Lars-Peter Clausen <lars@metafoo.de>
>   *  JZ4740 platform PWM support
> + *
> + * Limitations:
> + * - The .apply callback doesn't complete the currently running 
> period before
> + *   reconfiguring the hardware.
> + * - Each period starts with the inactive part.
>   */
> 
>  #include <linux/clk.h>
> --
> 2.20.1
>
Paul Cercueil Aug. 7, 2019, 6:40 p.m. UTC | #4
Le mer. 7 août 2019 à 15:49, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= 
<u.kleine-koenig@pengutronix.de> a écrit :
> On Wed, Aug 07, 2019 at 03:42:31PM +0200, Paul Cercueil wrote:
>> 
>> 
>>  Le mar. 30 juil. 2019 à 14:32, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
>>  <u.kleine-koenig@pengutronix.de> a écrit :
>>  > The jz4740 PMW implementation doesn't fulfill the (up to now
>>  > insufficiently documented) requirements of the PWM API. At least
>>  > document them in the driver.
>>  >
>>  > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>>  > ---
>>  > I intended to also add a Link to the reference manual, Paul 
>> suggested to
>>  > use https://zcrc.me/~paul/jz_docs/ in December last year, but this
>>  > stopped to work.
>>  >
>>  > The second item is something I noticed when reading through the 
>> manual,
>>  > but it's not confirmed in practise. A test that this is indeed 
>> the case
>>  > could be done by configuring a long period (say 5s) and a (in
>>  > comparison) small duty-cycle (say 1s). If the pwm output isn't 
>> active
>>  > when the call returns I'd consider this proven.
>>  >
>>  > @Paul: would you mind doing this test?
>> 
>>  You're correct. I configured it for 4s period and 2s duty. After 
>> enabling
>>  the
>>  PWM, it stays LOW for two seconds then switches HIGH for two 
>> seconds.
>> 
>>  That can be corrected, though, by inverting the configured polarity 
>> when the
>>  PWM is running and set "period - duty" as the duty value. I can 
>> make a patch
>>  for that.
> 
> OK. Do you care for documenting the first limitation then, too?
> 
> Or should we apply my patch as is and you remote the second item when
> you fix it?

Yes, just apply the patch, and I'll remove the second comment when I 
send mine.

-Paul


> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König        
>     |
> Industrial Linux Solutions                 | 
> http://www.pengutronix.de/  |
Paul Cercueil Aug. 9, 2019, 12:08 a.m. UTC | #5
Le mer. 7 août 2019 à 15:49, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= 
<u.kleine-koenig@pengutronix.de> a écrit :
> On Wed, Aug 07, 2019 at 03:42:31PM +0200, Paul Cercueil wrote:
>> 
>> 
>>  Le mar. 30 juil. 2019 à 14:32, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
>>  <u.kleine-koenig@pengutronix.de> a écrit :
>>  > The jz4740 PMW implementation doesn't fulfill the (up to now
>>  > insufficiently documented) requirements of the PWM API. At least
>>  > document them in the driver.
>>  >
>>  > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>>  > ---
>>  > I intended to also add a Link to the reference manual, Paul 
>> suggested to
>>  > use https://zcrc.me/~paul/jz_docs/ in December last year, but this
>>  > stopped to work.
>>  >
>>  > The second item is something I noticed when reading through the 
>> manual,
>>  > but it's not confirmed in practise. A test that this is indeed 
>> the case
>>  > could be done by configuring a long period (say 5s) and a (in
>>  > comparison) small duty-cycle (say 1s). If the pwm output isn't 
>> active
>>  > when the call returns I'd consider this proven.
>>  >
>>  > @Paul: would you mind doing this test?
>> 
>>  You're correct. I configured it for 4s period and 2s duty. After 
>> enabling
>>  the
>>  PWM, it stays LOW for two seconds then switches HIGH for two 
>> seconds.
>> 
>>  That can be corrected, though, by inverting the configured polarity 
>> when the
>>  PWM is running and set "period - duty" as the duty value. I can 
>> make a patch
>>  for that.
> 
> OK. Do you care for documenting the first limitation then, too?
> 
> Or should we apply my patch as is and you remote the second item when
> you fix it?

Actually ignore my previous answer, I'll send a patchset very soon and 
I have the
patch ready, so I'll document the first limit in the patchset.

-Paul


> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König        
>     |
> Industrial Linux Solutions                 | 
> http://www.pengutronix.de/  |

Patch
diff mbox series

diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
index f901e8a0d33d..9d444d012f92 100644
--- a/drivers/pwm/pwm-jz4740.c
+++ b/drivers/pwm/pwm-jz4740.c
@@ -2,6 +2,11 @@ 
 /*
  *  Copyright (C) 2010, Lars-Peter Clausen <lars@metafoo.de>
  *  JZ4740 platform PWM support
+ *
+ * Limitations:
+ * - The .apply callback doesn't complete the currently running period before
+ *   reconfiguring the hardware.
+ * - Each period starts with the inactive part.
  */
 
 #include <linux/clk.h>