Re: [PATCH v13 1/2] pwm: add microchip soft ip corePWM driver

[Uwe Kleine-König]

Hello Conor,

On Wed, Jan 11, 2023 at 12:15:29AM +0000, Conor Dooley wrote:
> On Tue, Jan 10, 2023 at 11:48:05PM +0100, Uwe Kleine-König wrote:
> > On Wed, Dec 21, 2022 at 11:29:12AM +0000, Conor Dooley wrote:
> > > From: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
> 
> > > +		delay_us = DIV_ROUND_UP_ULL(remaining_ns, NSEC_PER_USEC);
> > > +		if ((delay_us / 1000) > MAX_UDELAY_MS)
> > > +			msleep(delay_us / 1000 + 1);
> > 
> > Is this better than
> > 
> > 	msleep(DIV_ROUND_UP(delay_us, 1000);
> > 
> > ? Also I wonder about your usage of MAX_UDELAY_MS. This is about
> 
> I probably started hacking on the example you gave and didn't notice
> the U. What I have here is ~what you suggested last time.

A series with (up to now) 13 revisions and long delays between the
review rounds (which are mostly attributed to my time schedule) is
difficult to handle on both sides. Some repetition isn't easy to prevent
in such a case. Sorry for that. 

> > udelay() but you're using usleep_range()?
> > 
> > > +		else
> > > +			usleep_range(delay_us, delay_us * 2);
> > 
> > I wonder if there isn't a function that implements something like
> > 
> > 	wait_until(mchp_core_pwm->update_timestamp);
> > 
> > which would be a bit nicer than doing this by hand. Maybe fsleep()?
> 
> That'd be fsleep(delay_us), but does at least clean up some of the
> messing.
> 
> > > +static void mchp_core_pwm_apply_duty(struct pwm_chip *chip, struct pwm_device *pwm,
> > > +				     const struct pwm_state *state, u64 duty_steps,
> > > +				     u8 period_steps)
> > > +{
> > > +	struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip);
> > > +	u8 posedge, negedge;
> > > +	u8 period_steps_val = PREG_TO_VAL(period_steps);
> > > +
> > > +	/*
> > > +	 * Setting posedge == negedge doesn't yield a constant output,
> > > +	 * so that's an unsuitable setting to model duty_steps = 0.
> > > +	 * In that case set the unwanted edge to a value that never
> > > +	 * triggers.
> > > +	 */
> > > +	if (state->polarity == PWM_POLARITY_INVERSED) {
> > > +		negedge = !duty_steps ? period_steps_val : 0u;
> > 
> > IMHO
> > 
> > 		negedge = duty_steps ? 0 : period_steps_val;
> > 
> > is a bit easier to parse.
> > 
> > > +		posedge = duty_steps;
> > > +	} else {
> > > +		posedge = !duty_steps ? period_steps_val : 0u;
> > > +		negedge = duty_steps;
> > > +	}
> > 
> > The following code is equivalent:
> > 
> > 	u8 first_edge = 0, second_edge = duty_steps;
> > 
> > 	/*
> > 	 * Setting posedge == negedge doesn't yield a constant output,
> > 	 * so that's an unsuitable setting to model duty_steps = 0.
> > 	 * In that case set the unwanted edge to a value that never
> > 	 * triggers.
> > 	 */
> > 	if (duty_steps == 0)
> > 		first_edge = period_steps_val;
> > 
> > 	if (state->polarity == PWM_POLARITY_INVERSED) {
> > 		negedge = first_edge;
> > 		posedge = second_edge;
> > 	} else {
> > 		posedge = first_edge;
> > 		negedge = second_edge;
> > 	}
> > 
> > I'm not sure if it's easier to understand. What do you think?
> 
> Despite having used them, I dislike ternary statements.

My variant is a bit longer and uses more variables, but has less
repetition. I don't expect a relevant change on the generated code. I
slightly prefer my variant, but I let you choose which one you prefer.

> > > +	writel_relaxed(posedge, mchp_core_pwm->base + MCHPCOREPWM_POSEDGE(pwm->hwpwm));
> > > +	writel_relaxed(negedge, mchp_core_pwm->base + MCHPCOREPWM_NEGEDGE(pwm->hwpwm));
> > > +}
> > > +
> > > +static void mchp_core_pwm_calc_period(const struct pwm_state *state, unsigned long clk_rate,
> > > +				      u16 *prescale, u8 *period_steps)
> > > +{
> > > +	u64 tmp;
> > > +
> > > +	/*
> > > +	 * Calculate the period cycles and prescale values.
> > > +	 * The registers are each 8 bits wide & multiplied to compute the period
> > > +	 * using the formula:
> > > +	 * (clock_period) * (prescale + 1) * (period_steps + 1)
> > > +	 * so the maximum period that can be generated is 0x10000 times the
> > > +	 * period of the input clock.
> > > +	 * However, due to the design of the "hardware", it is not possible to
> > > +	 * attain a 100% duty cycle if the full range of period_steps is used.
> > > +	 * Therefore period_steps is restricted to 0xFE and the maximum multiple
> > > +	 * of the clock period attainable is 0xFF00.
> > > +	 */
> > > +	tmp = mul_u64_u64_div_u64(state->period, clk_rate, NSEC_PER_SEC);
> > > +
> > > +	/*
> > > +	 * The hardware adds one to the register value, so decrement by one to
> > > +	 * account for the offset
> > > +	 */
> > > +	if (tmp >= MCHPCOREPWM_PERIOD_MAX) {
> > > +		*prescale = MCHPCOREPWM_PRESCALE_MAX - 1;
> > > +		*period_steps = MCHPCOREPWM_PERIOD_STEPS_MAX - 1;
> > > +
> > > +		return;
> > > +	}
> > > +
> > > +	*prescale = div_u64(tmp, MCHPCOREPWM_PERIOD_STEPS_MAX);
> > > +	/* PREG_TO_VAL() can produce a value larger than UINT8_MAX */
> > > +	*period_steps = div_u64(tmp, PREG_TO_VAL(*prescale)) - 1;
> > 
> > This looks wrong, but I didn't think long about that. Did we discuss
> > this already and/or are you sure this is correct?
> 
> We did discuss it previously AFAICT;
> https://lore.kernel.org/linux-pwm/896d73ac-05af-8673-8379-29011800be83@xxxxxxxxxxxxx/
> 
> [...]
> Unfortunately, I don't think I am seeing what you're seeing.

Well, the calculation lands in the right ballpark for sure, but if my
intuition is right, it's not as exact as it could be. I need some time
with pencil and paper ...

> [...]
> Perhaps I need to watch a lecture on how to write a PWM driver since I
> am clearly no good at it, given the 15 revisions. Do you know of any?

I'm not aware of such a lecture. I'm willing to take the blame for some
of the revisions because I'm very picky and the math involved here isn't
trivial. And I sometimes wonder about myself pointing out an issue in
(say) v5 which was there unnoticed already in v1.

In sum a patch series going through such a high number of revisions is
mostly a good sign. In the end we can be sure that the merged code is
checked deep-rootedly and that both you and me have a certain amount of
endurance. :-)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature