Hi Uwe:
On Sat, Jul 6, 2024 at 9:25 PM Uwe Kleine-König
<u.kleine-koenig@xxxxxxxxxxxx> wrote:
>
> 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.
Thanks for your advice.
I would use this approach to test extensively, trying to make sure
there are no typical errors.
Thanks.
Binbin
>
> 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
