| Message ID | 20190730123229.31839-1-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | pwm: jz4740: document known limitations | expand |
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 >
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
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 >
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/ |
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/ |
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>
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(+)