Hello,
On Fri, Oct 07, 2022 at 12:35:12PM +0100, Conor Dooley wrote:
> [...]
> +static u64 mchp_core_pwm_calc_duty(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state, u8 prescale, u8 period_steps)
> +{
> + struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
> + u64 duty_steps, tmp;
> + u16 prescale_val = PREG_TO_VAL(prescale);
> +
> + /*
> + * Calculate the duty cycle in multiples of the prescaled period:
> + * duty_steps = duty_in_ns / step_in_ns
> + * step_in_ns = (prescale * NSEC_PER_SEC) / clk_rate
> + * The code below is rearranged slightly to only divide once.
> + */
> + duty_steps = state->duty_cycle * clk_get_rate(mchp_core_pwm->clk);
> + tmp = prescale_val * NSEC_PER_SEC;
> + return div64_u64(duty_steps, tmp);
The assignment to duty_steps can overflow. So you have to use
mul_u64_u64_div_u64 here, too.
> +}
> +
> [...]
> +
> +static int mchp_core_pwm_apply_locked(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> + struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
> + struct pwm_state current_state = pwm->state;
> + bool period_locked;
> + u64 duty_steps;
> + u16 prescale;
> + u8 period_steps;
> +
> + if (!state->enabled) {
> + mchp_core_pwm_enable(chip, pwm, false, current_state.period);
> + return 0;
> + }
> +
> + /*
> + * If the only thing that has changed is the duty cycle or the polarity,
> + * we can shortcut the calculations and just compute/apply the new duty
> + * cycle pos & neg edges
> + * As all the channels share the same period, do not allow it to be
> + * changed if any other channels are enabled.
> + * If the period is locked, it may not be possible to use a period
> + * less than that requested. In that case, we just abort.
> + */
> + period_locked = mchp_core_pwm->channel_enabled & ~(1 << pwm->hwpwm);
> +
> + if (period_locked) {
> + u16 hw_prescale;
> + u8 hw_period_steps;
> +
> + mchp_core_pwm_calc_period(chip, state, &prescale, &period_steps);
> + hw_prescale = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PRESCALE);
> + hw_period_steps = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_PERIOD);
> +
> + if ((period_steps + 1) * (prescale + 1) <
> + (hw_period_steps + 1) * (hw_prescale + 1))
> + return -EINVAL;
> +
> + /*
> + * It is possible that something could have set the period_steps
> + * register to 0xff, which would prevent us from setting a 100%
> + * or 0% relative duty cycle, as explained above in
> + * mchp_core_pwm_calc_period().
> + * The period is locked and we cannot change this, so we abort.
> + */
> + if (hw_period_steps == MCHPCOREPWM_PERIOD_STEPS_MAX)
> + return -EINVAL;
> +
> + prescale = hw_prescale;
> + period_steps = hw_period_steps;
> + } else {
> + int ret;
> +
> + ret = mchp_core_pwm_calc_period(chip, state, &prescale, &period_steps);
> + if (ret)
> + return ret;
> +
> + mchp_core_pwm_apply_period(mchp_core_pwm, prescale, period_steps);
> + }
> +
> + duty_steps = mchp_core_pwm_calc_duty(chip, pwm, state, prescale, period_steps);
Both mchp_core_pwm_calc_period and mchp_core_pwm_calc_duty call
clk_get_rate(), I suggest call this only once and pass the rate to these
two functions.
Both branches of the if above start with calling
mchp_core_pwm_calc_period, this could be simplified, too. (Hmm, in
exactly one of them you check the return code, wouldn't that be sensible
for both callers?)
> +
> + /*
> + * Because the period is per channel, it is possible that the requested
> + * duty cycle is longer than the period, in which case cap it to the
> + * period, IOW a 100% duty cycle.
> + */
> + if (duty_steps > period_steps)
> + duty_steps = period_steps + 1;
> +
> + mchp_core_pwm_apply_duty(chip, pwm, state, duty_steps, period_steps);
> +
> + mchp_core_pwm_enable(chip, pwm, true, state->period);
> +
> + return 0;
> +}
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
