Hello Binbin,
On Sat, Jul 06, 2024 at 03:08:30PM +0600, Binbin Zhou wrote:
> Hi Uwe:
>
> Thanks for your reply.
>
> On Sat, Jul 6, 2024 at 5:26 AM Uwe Kleine-König
> <u.kleine-koenig@xxxxxxxxxxxx> wrote:
> >
> > Hello,
> >
> > On Mon, May 27, 2024 at 03:51:12PM +0800, Binbin Zhou wrote:
> > > +static int pwm_loongson_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > + const struct pwm_state *state)
> > > +{
> > > + int ret;
> > > + u64 period, duty_cycle;
> > > + bool enabled = pwm->state.enabled;
> > > +
> > > + period = min(state->period, NANOHZ_PER_HZ);
> > > + duty_cycle = min(state->duty_cycle, NANOHZ_PER_HZ);
> > > +
> > > + if (state->polarity != pwm->state.polarity) {
> > > + if (enabled) {
> > > + pwm_loongson_disable(chip, pwm);
> > > + enabled = false;
> > > + }
> > > +
> > > + ret = pwm_loongson_set_polarity(chip, pwm, state->polarity);
> > > + if (ret)
> > > + return ret;
> > > + }
> > > +
> > > + if (!state->enabled) {
> > > + if (enabled)
> > > + pwm_loongson_disable(chip, pwm);
> > > + return 0;
> > > + }
> >
> > Given that the configured polarity isn't relevant for a disabled PWM, I
> > suggest to swap these two if blocks. However then you have to be a bit
> > more careful for the polarity check because otherwise the following
> > series of commands yields wrong results:
> >
> > pwm_apply_might_sleep(pwm, {.duty_cycle = D, .period = P, .polarity = PWM_POLARITY_NORMAL, .enabled = true});
> > pwm_apply_might_sleep(pwm, {.duty_cycle = D, .period = P, .polarity = PWM_POLARITY_INVERSED, .enabled = false});
> > pwm_apply_might_sleep(pwm, {.duty_cycle = D, .period = P, .polarity = PWM_POLARITY_INVERSED, .enabled = true});
> >
>
> Yes, we'd better make sure pwm is enabled first.
> I will swap the two if blocks.
>
> > > + ret = pwm_loongson_config(chip, pwm, duty_cycle, period);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + if (!enabled)
> > > + ret = pwm_loongson_enable(chip, pwm);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int pwm_loongson_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > > + struct pwm_state *state)
> > > +{
> > > + u32 duty, period, ctrl;
> > > + struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
> > > +
> > > + /* duty_cycle = ddata->duty * NSEC_PER_SEC / ddata->clk_rate */
> > > + duty = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_DUTY);
> > > + state->duty_cycle = mul_u64_u64_div_u64(duty, NSEC_PER_SEC, ddata->clk_rate);
> > > +
> > > + /* period = ddata->period * NSEC_PER_SEC / ddata->clk_rate */
> > > + period = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_PERIOD);
> > > + state->period = mul_u64_u64_div_u64(period, NSEC_PER_SEC, ddata->clk_rate);
> > > +
> > > + ctrl = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_CTRL);
> > > + state->polarity = (ctrl & LOONGSON_PWM_CTRL_INVERT) ? PWM_POLARITY_INVERSED :
> > > + PWM_POLARITY_NORMAL;
> > > + state->enabled = (ctrl & LOONGSON_PWM_CTRL_EN) ? true : false;
> > > +
> > > + return 0;
> >
> > You didn't test extensively with PWM_DEBUG enabled, right? You need to
> > round up the divisions here otherwise you get strange rounding results:
> >
> > Consider ddata->clk_rate = 1594323. When a state with .period = 20000 is
> > applied, LOONGSON_PWM_REG_PERIOD is assigned 31.
> > Calling .get_state() in this situation gives .period = 19443. Reapplying
> > .period = 19443 results in LOONGSON_PWM_REG_PERIOD := 30. Iterating this
> > further yields:
> >
> > - .period = 18816
> > - LOONGSON_PWM_REG_PERIOD := 29
> > - .period = 18189
> > - LOONGSON_PWM_REG_PERIOD := 28
> > - ...
> >
> Yes, I'm very sorry I didn't do extensive testing with PWM_DEBUG enabled.
> Here I do need to use DIV64_U64_ROUND_UP().
>
> Below:
>
> /* duty_cycle = ddata->duty * NSEC_PER_SEC / ddata->clk_rate */
> duty = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_DUTY);
> state->duty_cycle = DIV64_U64_ROUND_UP((u64)duty *
> NSEC_PER_SEC, ddata->clk_rate);
>
> /* period = ddata->period * NSEC_PER_SEC / ddata->clk_rate */
> period = pwm_loongson_readl(ddata, LOONGSON_PWM_REG_PERIOD);
> state->period = DIV64_U64_ROUND_UP((u64)period * NSEC_PER_SEC,
> ddata->clk_rate);
>
>
> I'd also like to ask which tests I still need to do to make sure the
> driver is more complete?
There is no official lower limit on tests. But the goal is that your
driver behaves correctly and some of the typical errors can be catched
by enabling PWM_DEBUG and then doing the following tests for sensible
values of A and B:
# Sequence of increasing periods
for period in A ... B:
configure 0/period
# Sequence of decreasing periods
for period in A ... B:
configure 0/(B + A - period)
for period in some set:
# Sequence of increasing duty length
for duty_cycle in 0 ... period:
configure duty_cycle/period
# Sequence of decreasing duty length
for duty_cycle in 0 ... period:
configure (period - duty_cycle)/period
That should give you a good coverage.
The idea of extensive testing on your side is also: Review capacities
are a scarce resource and you suffer from that because it takes some
patience between you sending a patch and a maintainer coming around to
review it. If your code is well tested, there is less for the
maintainers to find and so I save time because there are less revisions
and they mature quicker. The direct consequence for you and the others
waiting for my attention should be obvious. (This also means: If you
look into other's patch submissions and point out the things you already
learned to them, you also save maintainer time and so might get their
attention earlier yourself.)
I'm working on a bigger change for the pwm subsystem. One of the results
will be a new userspace API. I intend to create a test program that does
the above tests, so in the foreseeable future testing should get easier.
(In return writing a lowlevel driver will become a bit harder, as the
requirements for precision increase.)
Best regards
Uwe
Attachment:
signature.asc
Description: PGP signature
