| Message ID | 20190923081348.6843-4-linux@rasmusvillemoes.dk |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pwm: mxs: add support for setting polarity via DT | expand |
On Mon, Sep 23, 2019 at 10:13:47AM +0200, Rasmus Villemoes wrote: > If I'm reading of_pwm_xlate_with_flags() right, existing device trees > that set #pwm-cells = 2 will continue to work. > > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > --- > drivers/pwm/pwm-mxs.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/pwm/pwm-mxs.c b/drivers/pwm/pwm-mxs.c > index 284107784dad..c46697acaf11 100644 > --- a/drivers/pwm/pwm-mxs.c > +++ b/drivers/pwm/pwm-mxs.c > @@ -25,8 +25,11 @@ > #define PERIOD_PERIOD(p) ((p) & 0xffff) > #define PERIOD_PERIOD_MAX 0x10000 > #define PERIOD_ACTIVE_HIGH (3 << 16) > +#define PERIOD_ACTIVE_LOW (2 << 16) > +#define PERIOD_INACTIVE_HIGH (3 << 18) > #define PERIOD_INACTIVE_LOW (2 << 18) > #define PERIOD_POLARITY_NORMAL (PERIOD_ACTIVE_HIGH | PERIOD_INACTIVE_LOW) > +#define PERIOD_POLARITY_INVERSE (PERIOD_ACTIVE_LOW | PERIOD_INACTIVE_HIGH) > #define PERIOD_CDIV(div) (((div) & 0x7) << 20) > #define PERIOD_CDIV_MAX 8 > > @@ -50,9 +53,7 @@ static int mxs_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > unsigned int period_cycles, duty_cycles; > unsigned long rate; > unsigned long long c; > - > - if (state->polarity != PWM_POLARITY_NORMAL) > - return -ENOTSUPP; > + unsigned int pol_bits; > > rate = clk_get_rate(mxs->clk); > while (1) { > @@ -81,9 +82,12 @@ static int mxs_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > return ret; > } > > + pol_bits = state->polarity == PWM_POLARITY_NORMAL ? > + PERIOD_POLARITY_NORMAL : PERIOD_POLARITY_INVERSE; > + > writel(duty_cycles << 16, > mxs->base + PWM_ACTIVE0 + pwm->hwpwm * 0x20); > - writel(PERIOD_PERIOD(period_cycles) | PERIOD_POLARITY_NORMAL | PERIOD_CDIV(div), > + writel(PERIOD_PERIOD(period_cycles) | pol_bits | PERIOD_CDIV(div), When will this affect the output? Only on the next start of a period, or immediatly? Can it happen that this results in a mixed output (i.e. a period that has already the new duty cycle from the line above but not the new polarity (or period)? Best regards Uwe
On 23/09/2019 10.27, Uwe Kleine-König wrote: > On Mon, Sep 23, 2019 at 10:13:47AM +0200, Rasmus Villemoes wrote: >> >> >> + pol_bits = state->polarity == PWM_POLARITY_NORMAL ? >> + PERIOD_POLARITY_NORMAL : PERIOD_POLARITY_INVERSE; >> + >> writel(duty_cycles << 16, >> mxs->base + PWM_ACTIVE0 + pwm->hwpwm * 0x20); >> - writel(PERIOD_PERIOD(period_cycles) | PERIOD_POLARITY_NORMAL | PERIOD_CDIV(div), >> + writel(PERIOD_PERIOD(period_cycles) | pol_bits | PERIOD_CDIV(div), > > When will this affect the output? Only on the next start of a period, or > immediatly? Can it happen that this results in a mixed output (i.e. a > period that has already the new duty cycle from the line above but not > the new polarity (or period)? The data sheet says "Also, when the user reprograms the channel in this manner, the new register values will not take effect until the beginning of a new output period. This eliminates the potential for output glitches that could occur if the registers were updated while the channel was enabled and in the middle of a cycle.". So I think this should be ok. "this manner" refers to the registers being written in the proper order (first ACTIVEn, then PERIODn). Rasmus
On Mon, Sep 23, 2019 at 10:45:56AM +0200, Rasmus Villemoes wrote: > On 23/09/2019 10.27, Uwe Kleine-König wrote: > > On Mon, Sep 23, 2019 at 10:13:47AM +0200, Rasmus Villemoes wrote: > >> > >> > >> + pol_bits = state->polarity == PWM_POLARITY_NORMAL ? > >> + PERIOD_POLARITY_NORMAL : PERIOD_POLARITY_INVERSE; > >> + > >> writel(duty_cycles << 16, > >> mxs->base + PWM_ACTIVE0 + pwm->hwpwm * 0x20); > >> - writel(PERIOD_PERIOD(period_cycles) | PERIOD_POLARITY_NORMAL | PERIOD_CDIV(div), > >> + writel(PERIOD_PERIOD(period_cycles) | pol_bits | PERIOD_CDIV(div), > > > > When will this affect the output? Only on the next start of a period, or > > immediatly? Can it happen that this results in a mixed output (i.e. a > > period that has already the new duty cycle from the line above but not > > the new polarity (or period)? > > The data sheet says "Also, when the user reprograms the channel in this > manner, the new register values will not take effect until the beginning > of a new output period. This eliminates the potential for output > glitches that could occur if the registers were updated while the > channel was enabled and in the middle of a cycle.". So I think this > should be ok. "this manner" refers to the registers being written in the > proper order (first ACTIVEn, then PERIODn). OK. IMHO this is worth a code comment. Best regards Uwe
diff --git a/drivers/pwm/pwm-mxs.c b/drivers/pwm/pwm-mxs.c index 284107784dad..c46697acaf11 100644 --- a/drivers/pwm/pwm-mxs.c +++ b/drivers/pwm/pwm-mxs.c @@ -25,8 +25,11 @@ #define PERIOD_PERIOD(p) ((p) & 0xffff) #define PERIOD_PERIOD_MAX 0x10000 #define PERIOD_ACTIVE_HIGH (3 << 16) +#define PERIOD_ACTIVE_LOW (2 << 16) +#define PERIOD_INACTIVE_HIGH (3 << 18) #define PERIOD_INACTIVE_LOW (2 << 18) #define PERIOD_POLARITY_NORMAL (PERIOD_ACTIVE_HIGH | PERIOD_INACTIVE_LOW) +#define PERIOD_POLARITY_INVERSE (PERIOD_ACTIVE_LOW | PERIOD_INACTIVE_HIGH) #define PERIOD_CDIV(div) (((div) & 0x7) << 20) #define PERIOD_CDIV_MAX 8 @@ -50,9 +53,7 @@ static int mxs_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, unsigned int period_cycles, duty_cycles; unsigned long rate; unsigned long long c; - - if (state->polarity != PWM_POLARITY_NORMAL) - return -ENOTSUPP; + unsigned int pol_bits; rate = clk_get_rate(mxs->clk); while (1) { @@ -81,9 +82,12 @@ static int mxs_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, return ret; } + pol_bits = state->polarity == PWM_POLARITY_NORMAL ? + PERIOD_POLARITY_NORMAL : PERIOD_POLARITY_INVERSE; + writel(duty_cycles << 16, mxs->base + PWM_ACTIVE0 + pwm->hwpwm * 0x20); - writel(PERIOD_PERIOD(period_cycles) | PERIOD_POLARITY_NORMAL | PERIOD_CDIV(div), + writel(PERIOD_PERIOD(period_cycles) | pol_bits | PERIOD_CDIV(div), mxs->base + PWM_PERIOD0 + pwm->hwpwm * 0x20); if (state->enabled) { @@ -129,6 +133,8 @@ static int mxs_pwm_probe(struct platform_device *pdev) mxs->chip.dev = &pdev->dev; mxs->chip.ops = &mxs_pwm_ops; + mxs->chip.of_xlate = of_pwm_xlate_with_flags; + mxs->chip.of_pwm_n_cells = 3; mxs->chip.base = -1; ret = of_property_read_u32(np, "fsl,pwm-number", &mxs->chip.npwm);
If I'm reading of_pwm_xlate_with_flags() right, existing device trees that set #pwm-cells = 2 will continue to work. Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> --- drivers/pwm/pwm-mxs.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)