Re: [PATCH v11 3/4] pwm: add microchip soft ip corePWM driver

[Conor Dooley <conor@kernel.org>]

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.