Messages in this thread | | | Date | Tue, 8 Sep 2020 10:12:27 +0200 | From | Uwe Kleine-König <> | Subject | Re: [PATCH v9 06/13] pwm: add support for sl28cpld PWM controller |
| |
Hello,
just a bit of nitpicking left. If Lee is going to apply, I can care for a followup patch if need be.
On Mon, Sep 07, 2020 at 11:37:55PM +0200, Michael Walle wrote: > [..] > +config PWM_SL28CPLD > + tristate "Kontron sl28cpld PWM support" > + depends on MFD_SL28CPLD || COMPILE_TEST
s/ / / (@Lee, maybe fixup during application?)
> + help > + Generic PWM framework driver for board management controller > + found on the Kontron sl28 CPLD. > [...] > +#define SL28CPLD_PWM_CLK 32000 /* 32 kHz */ > +#define SL28CPLD_PWM_MAX_DUTY_CYCLE(prescaler) (1 << (7 - (prescaler))) > +#define SL28CPLD_PWM_PERIOD(prescaler) \ > + (NSEC_PER_SEC / SL28CPLD_PWM_CLK * SL28CPLD_PWM_MAX_DUTY_CYCLE(prescaler)) > + > +/* > + * We calculate the duty cycle like this: > + * duty_cycle_ns = pwm_cycle_reg * max_period_ns / max_duty_cycle > + * > + * With > + * max_period_ns = 1 << (7 - prescaler) / pwm_clk * NSEC_PER_SEC > + * max_duty_cycle = 1 << (7 - prescaler) > + * this then simplifies to: > + * duty_cycle_ns = pwm_cycle_reg / pwm_clk * NSEC_PER_SEC > + * > + * NSEC_PER_SEC is a multiple of SL28CPLD_PWM_CLK, therefore we're not losing > + * precision by doing the divison first.
The division you're talking about is NSEC_PER_SEC / pwm_clk which isn't obvious in the formula in the line above. Maybe:
... this then simplifies to:
duty_cycle_ns = NSEC_PER_SEC / SL28CPLD_PWM_CLK * pwm_cycle_reg
NSEC_PER_SEC is a multiple of SL28CPLD_PWM_CLK, therefor ...
to make it easier to understand the comment.
> + */ > +#define SL28CPLD_PWM_TO_DUTY_CYCLE(reg) \ > + (NSEC_PER_SEC / SL28CPLD_PWM_CLK * (reg)) > +#define SL28CPLD_PWM_FROM_DUTY_CYCLE(duty_cycle) \ > + (DIV_ROUND_DOWN_ULL((duty_cycle), NSEC_PER_SEC / SL28CPLD_PWM_CLK)) > [...] > + /* > + * To avoid glitches when we switch the prescaler, we have to make sure > + * we have a valid duty cycle for the new mode. > + * > + * Take the current prescaler (or the current period length) into > + * account to decide whether we have to write the duty cycle or the new > + * prescaler first. If the period length is decreasing we have to > + * write the duty cycle first. > + */ > + write_duty_cycle_first = pwm->state.period > state->period; > + > + if (write_duty_cycle_first) { > + ret = sl28cpld_pwm_write(priv, SL28CPLD_PWM_CYCLE, cycle); > + if (ret) > + return ret; > + } > + > + ret = sl28cpld_pwm_write(priv, SL28CPLD_PWM_CTRL, ctrl); > + if (ret) > + return ret; > + > + if (!write_duty_cycle_first) { > + ret = sl28cpld_pwm_write(priv, SL28CPLD_PWM_CYCLE, cycle); > + if (ret) > + return ret; > + }
Nice! I didn't spend the necessary brain cycles to confirm this algorithm, but it seems you did :-)
> + > + return 0; > +} > [...]
Best regards Uwe
-- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [unhandled content-type:application/pgp-signature] | |