In preparation for supporting setting the polarity, switch the driver
to support the ->apply method.
Signed-off-by: Rasmus Villemoes <[email protected]>
---
drivers/pwm/pwm-mxs.c | 62 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 62 insertions(+)
diff --git a/drivers/pwm/pwm-mxs.c b/drivers/pwm/pwm-mxs.c
index 04c0f6b95c1a..c70c26a9ff68 100644
--- a/drivers/pwm/pwm-mxs.c
+++ b/drivers/pwm/pwm-mxs.c
@@ -26,6 +26,7 @@
#define PERIOD_PERIOD_MAX 0x10000
#define PERIOD_ACTIVE_HIGH (3 << 16)
#define PERIOD_INACTIVE_LOW (2 << 18)
+#define PERIOD_POLARITY_NORMAL (PERIOD_ACTIVE_HIGH | PERIOD_INACTIVE_LOW)
#define PERIOD_CDIV(div) (((div) & 0x7) << 20)
#define PERIOD_CDIV_MAX 8
@@ -41,6 +42,66 @@ struct mxs_pwm_chip {
#define to_mxs_pwm_chip(_chip) container_of(_chip, struct mxs_pwm_chip, chip)
+static int mxs_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+ struct pwm_state *state)
+{
+ struct mxs_pwm_chip *mxs = to_mxs_pwm_chip(chip);
+ int ret, div = 0;
+ unsigned int period_cycles, duty_cycles;
+ unsigned long rate;
+ unsigned long long c;
+
+ if (state->polarity != PWM_POLARITY_NORMAL)
+ return -ENOTSUPP;
+
+ rate = clk_get_rate(mxs->clk);
+ while (1) {
+ c = rate / cdiv[div];
+ c = c * state->period;
+ do_div(c, 1000000000);
+ if (c < PERIOD_PERIOD_MAX)
+ break;
+ div++;
+ if (div >= PERIOD_CDIV_MAX)
+ return -EINVAL;
+ }
+
+ period_cycles = c;
+ c *= state->duty_cycle;
+ do_div(c, state->period);
+ duty_cycles = c;
+
+ /*
+ * If the PWM channel is disabled, make sure to turn on the clock
+ * before writing the register. Otherwise, keep it enabled.
+ */
+ if (!pwm_is_enabled(pwm)) {
+ ret = clk_prepare_enable(mxs->clk);
+ if (ret)
+ return ret;
+ }
+
+ writel(duty_cycles << 16,
+ mxs->base + PWM_ACTIVE0 + pwm->hwpwm * 0x20);
+ writel(PERIOD_PERIOD(period_cycles) | PERIOD_POLARITY_NORMAL | PERIOD_CDIV(div),
+ mxs->base + PWM_PERIOD0 + pwm->hwpwm * 0x20);
+
+ if (state->enabled) {
+ if (!pwm_is_enabled(pwm)) {
+ /*
+ * The clock was enabled above. Just enable
+ * the channel in the control register.
+ */
+ writel(1 << pwm->hwpwm, mxs->base + PWM_CTRL + SET);
+ }
+ } else {
+ if (pwm_is_enabled(pwm))
+ writel(1 << pwm->hwpwm, mxs->base + PWM_CTRL + CLR);
+ clk_disable_unprepare(mxs->clk);
+ }
+ return 0;
+}
+
static int mxs_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
int duty_ns, int period_ns)
{
@@ -116,6 +177,7 @@ static void mxs_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
}
static const struct pwm_ops mxs_pwm_ops = {
+ .apply = mxs_pwm_apply,
.config = mxs_pwm_config,
.enable = mxs_pwm_enable,
.disable = mxs_pwm_disable,
--
2.20.1
Hello,
[expanded the recipents to include RMK and the clk list]
On Mon, Sep 23, 2019 at 11:04:39AM +0200, Rasmus Villemoes wrote:
> On 23/09/2019 10.24, Uwe Kleine-K?nig wrote:
> > Also there is a bug already in .config: You are not supposed to call
> > clk_get_rate if the clk might be off.
>
> Interesting, I didn't know that. So the prepare_enable logic needs to be
> moved before we start computing the period/duty cycles. Do you know why
> it has apparently worked so far? I would have thought such a rule would
> be enforced by the clock framework, or at least produced a warning.
FTR: This is documented in the kerneldoc code comment to clk_get_rate in
include/linux/clk.h.
Assuming this is relevant, it might indeed make sense to add a
WARN_ONCE for this.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Hello Rasmus,
On Mon, Sep 23, 2019 at 10:13:45AM +0200, Rasmus Villemoes wrote:
> In preparation for supporting setting the polarity, switch the driver
> to support the ->apply method.
>
> Signed-off-by: Rasmus Villemoes <[email protected]>
> ---
> drivers/pwm/pwm-mxs.c | 62 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 62 insertions(+)
>
> diff --git a/drivers/pwm/pwm-mxs.c b/drivers/pwm/pwm-mxs.c
> index 04c0f6b95c1a..c70c26a9ff68 100644
> --- a/drivers/pwm/pwm-mxs.c
> +++ b/drivers/pwm/pwm-mxs.c
> @@ -26,6 +26,7 @@
> #define PERIOD_PERIOD_MAX 0x10000
> #define PERIOD_ACTIVE_HIGH (3 << 16)
> #define PERIOD_INACTIVE_LOW (2 << 18)
> +#define PERIOD_POLARITY_NORMAL (PERIOD_ACTIVE_HIGH | PERIOD_INACTIVE_LOW)
> #define PERIOD_CDIV(div) (((div) & 0x7) << 20)
> #define PERIOD_CDIV_MAX 8
>
> @@ -41,6 +42,66 @@ struct mxs_pwm_chip {
>
> #define to_mxs_pwm_chip(_chip) container_of(_chip, struct mxs_pwm_chip, chip)
>
> +static int mxs_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct mxs_pwm_chip *mxs = to_mxs_pwm_chip(chip);
> + int ret, div = 0;
> + unsigned int period_cycles, duty_cycles;
> + unsigned long rate;
> + unsigned long long c;
> +
> + if (state->polarity != PWM_POLARITY_NORMAL)
> + return -ENOTSUPP;
> +
> + rate = clk_get_rate(mxs->clk);
> + while (1) {
> + c = rate / cdiv[div];
> + c = c * state->period;
> + do_div(c, 1000000000);
> + if (c < PERIOD_PERIOD_MAX)
> + break;
> + div++;
> + if (div >= PERIOD_CDIV_MAX)
> + return -EINVAL;
> + }
> +
> + period_cycles = c;
> + c *= state->duty_cycle;
> + do_div(c, state->period);
> + duty_cycles = c;
> +
> + /*
> + * If the PWM channel is disabled, make sure to turn on the clock
> + * before writing the register. Otherwise, keep it enabled.
> + */
> + if (!pwm_is_enabled(pwm)) {
> + ret = clk_prepare_enable(mxs->clk);
> + if (ret)
> + return ret;
> + }
> +
> + writel(duty_cycles << 16,
> + mxs->base + PWM_ACTIVE0 + pwm->hwpwm * 0x20);
> + writel(PERIOD_PERIOD(period_cycles) | PERIOD_POLARITY_NORMAL | PERIOD_CDIV(div),
> + mxs->base + PWM_PERIOD0 + pwm->hwpwm * 0x20);
> +
> + if (state->enabled) {
> + if (!pwm_is_enabled(pwm)) {
> + /*
> + * The clock was enabled above. Just enable
> + * the channel in the control register.
> + */
> + writel(1 << pwm->hwpwm, mxs->base + PWM_CTRL + SET);
> + }
> + } else {
> + if (pwm_is_enabled(pwm))
> + writel(1 << pwm->hwpwm, mxs->base + PWM_CTRL + CLR);
> + clk_disable_unprepare(mxs->clk);
> + }
> + return 0;
> +}
Maybe it would be easier to review when converting from .config +
.enable + .disable to .apply in a single step. (Note this "maybe" is
honest, I'm not entirely sure.)
There is a bug: If the PWM is running at (say) period=100ms, duty=0ms
and we call
pwm_apply_state(pwm, { .enabled = false, duty=100000, period=1000000 });
the output might get high which it should not.
Also there is a bug already in .config: You are not supposed to call
clk_get_rate if the clk might be off.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On 23/09/2019 10.24, Uwe Kleine-K?nig wrote:
> Hello Rasmus,
>
> On Mon, Sep 23, 2019 at 10:13:45AM +0200, Rasmus Villemoes wrote:
>> In preparation for supporting setting the polarity, switch the driver
>> to support the ->apply method.
>>
>
> Maybe it would be easier to review when converting from .config +
> .enable + .disable to .apply in a single step. (Note this "maybe" is
> honest, I'm not entirely sure.)
I tried to make .apply do exactly what the old sequence of calls from
the core to the individual methods would do, and for that it seemed a
little easier to keep the old methods around - but yes, I do need to be
more careful than that to provide the atomicity guarantee that the
legacy methods did not. It's also much easier to squash than to split,
so for now I'll leave them separate - if somebody prefers them squashed,
I'll do that.
> There is a bug: If the PWM is running at (say) period=100ms, duty=0ms
> and we call
> pwm_apply_state(pwm, { .enabled = false, duty=100000, period=1000000 });
> the output might get high which it should not.
Ah, yes. So I suppose that if we're changing from enabled to disabled,
we should simply disable it in the CTRL register before changing the
duty/period.
> Also there is a bug already in .config: You are not supposed to call
> clk_get_rate if the clk might be off.
Interesting, I didn't know that. So the prepare_enable logic needs to be
moved before we start computing the period/duty cycles. Do you know why
it has apparently worked so far? I would have thought such a rule would
be enforced by the clock framework, or at least produced a warning.
Thanks for the fast review. I'll wait a day or two to see if there are
other comments before sending out a v2.
Rasmus