From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Fabrice Gasnier <fabrice.gasnier@st.com>
Cc: thierry.reding@gmail.com, alexandre.torgue@st.com,
linux-pwm@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org,
linux-stm32@st-md-mailman.stormreply.com
Subject: Re: [PATCH v2] pwm: stm32-lp: add check in case requested period cannot be achieved
Date: Wed, 18 Sep 2019 20:40:39 +0200 [thread overview]
Message-ID: <20190918184039.27dtd3zguawijsqo@pengutronix.de> (raw)
In-Reply-To: <1568818461-19995-1-git-send-email-fabrice.gasnier@st.com>
On Wed, Sep 18, 2019 at 04:54:21PM +0200, Fabrice Gasnier wrote:
> LPTimer can use a 32KHz clock for counting. It depends on clock tree
> configuration. In such a case, PWM output frequency range is limited.
> Although unlikely, nothing prevents user from requesting a PWM frequency
> above counting clock (32KHz for instance):
> - This causes (prd - 1) = 0xffff to be written in ARR register later in
> the apply() routine.
> This results in badly configured PWM period (and also duty_cycle).
> Add a check to report an error is such a case.
>
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
If you are interested to improve the driver further, there are a few
things that I would welcome to see fixed in a tested patch:
- duty calculation uses requested instead of implemented period.
- stm32_pwm_lp_apply calls pwm_get_state
- Calculation of prd could be done without a loop
- A hint about relevant documentation in the driver's header would be
great
- Documentation about behaviour of the hardware is missing:
- Does the hardware complete the currently running period when
STM32_LPTIM_CR is cleared?
- Does the output stop in the inactive output level when the PWM is
stopped?
- clk_get_rate might be called without the clock being enabled.
- The driver does:
ret = regmap_write(priv->regmap, STM32_LPTIM_CMP, prd - (1 + dty));
That looks wrong. (Consider dty == prd.)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
WARNING: multiple messages have this Message-ID (diff)
From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Fabrice Gasnier <fabrice.gasnier@st.com>
Cc: linux-pwm@vger.kernel.org, alexandre.torgue@st.com,
linux-kernel@vger.kernel.org, thierry.reding@gmail.com,
linux-stm32@st-md-mailman.stormreply.com,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2] pwm: stm32-lp: add check in case requested period cannot be achieved
Date: Wed, 18 Sep 2019 20:40:39 +0200 [thread overview]
Message-ID: <20190918184039.27dtd3zguawijsqo@pengutronix.de> (raw)
In-Reply-To: <1568818461-19995-1-git-send-email-fabrice.gasnier@st.com>
On Wed, Sep 18, 2019 at 04:54:21PM +0200, Fabrice Gasnier wrote:
> LPTimer can use a 32KHz clock for counting. It depends on clock tree
> configuration. In such a case, PWM output frequency range is limited.
> Although unlikely, nothing prevents user from requesting a PWM frequency
> above counting clock (32KHz for instance):
> - This causes (prd - 1) = 0xffff to be written in ARR register later in
> the apply() routine.
> This results in badly configured PWM period (and also duty_cycle).
> Add a check to report an error is such a case.
>
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
If you are interested to improve the driver further, there are a few
things that I would welcome to see fixed in a tested patch:
- duty calculation uses requested instead of implemented period.
- stm32_pwm_lp_apply calls pwm_get_state
- Calculation of prd could be done without a loop
- A hint about relevant documentation in the driver's header would be
great
- Documentation about behaviour of the hardware is missing:
- Does the hardware complete the currently running period when
STM32_LPTIM_CR is cleared?
- Does the output stop in the inactive output level when the PWM is
stopped?
- clk_get_rate might be called without the clock being enabled.
- The driver does:
ret = regmap_write(priv->regmap, STM32_LPTIM_CMP, prd - (1 + dty));
That looks wrong. (Consider dty == prd.)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-09-18 18:40 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-18 14:54 [PATCH v2] pwm: stm32-lp: add check in case requested period cannot be achieved Fabrice Gasnier
2019-09-18 14:54 ` Fabrice Gasnier
2019-09-18 14:54 ` Fabrice Gasnier
2019-09-18 18:40 ` Uwe Kleine-König [this message]
2019-09-18 18:40 ` Uwe Kleine-König
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190918184039.27dtd3zguawijsqo@pengutronix.de \
--to=u.kleine-koenig@pengutronix.de \
--cc=alexandre.torgue@st.com \
--cc=fabrice.gasnier@st.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pwm@vger.kernel.org \
--cc=linux-stm32@st-md-mailman.stormreply.com \
--cc=thierry.reding@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.