On Tue, Nov 08, 2022 at 04:50:41PM +0100, Uwe Kleine-König wrote:
> Hello,
Hello! Thanks for the review Uwe :)
> On Fri, Oct 07, 2022 at 12:35:12PM +0100, Conor Dooley wrote:
> > +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.
Sure. I think the signatures of both of those functions could be reduced
in the process which would be nice.
> Both branches of the if above start with calling
> mchp_core_pwm_calc_period, this could be simplified, too.
ret = mchp_core_pwm_calc_period(chip, state, &prescale, &period_steps);
if (ret)
return ret;
period_locked = mchp_core_pwm->channel_enabled & ~(1 << pwm->hwpwm);
if (period_locked) {
u16 hw_prescale;
u8 hw_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 {
mchp_core_pwm_apply_period(mchp_core_pwm, prescale, period_steps);
}
duty_steps = mchp_core_pwm_calc_duty(chip, pwm, state, prescale, period_steps);
I'll aim for something like the (absolutely untested) above then when I
respin.
> (Hmm, in
> exactly one of them you check the return code, wouldn't that be sensible
> for both callers?)
Been messing with rust a bit of late, I love the #[must_use] attribute.
Looks to be an oversight since it's only going to return an error if the
clock rate exceeds what the FPGA is actually capable of.
Thanks again,
Conor.
