Hi Uwe:
Thanks for your review.
On Tue, Sep 10, 2024 at 1:00 PM Uwe Kleine-König
<u.kleine-koenig@xxxxxxxxxxxx> wrote:
>
> Hello,
>
> On Wed, Jul 10, 2024 at 10:04:07AM +0800, Binbin Zhou wrote:
> > diff --git a/drivers/pwm/pwm-loongson.c b/drivers/pwm/pwm-loongson.c
> > new file mode 100644
> > index 000000000000..17ab2a2f48ad
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-loongson.c
> > @@ -0,0 +1,285 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Loongson PWM driver
> > + *
> > + * Author: Juxin Gao <gaojuxin@xxxxxxxxxxx>
> > + * Further cleanup and restructuring by:
> > + * Binbin Zhou <zhoubinbin@xxxxxxxxxxx>
> > + *
> > + * Copyright (C) 2017-2024 Loongson Technology Corporation Limited.
> > + *
> > + * Limitations:
> > + * - The buffer register value should be written before the CTRL register.
> > + * - When disabled the output is driven to 0 independent of the configured
> > + * polarity.
>
> An info about possible glitches and if a period is completed on
> reconfiguration or when the PWM is disabled would be great.
>
> Also if there is a publically available manual, please add a link here.
The descriptions related to our PWM controllers are in the CPU
manuals, e.g. for the Loongson-2K series CPUs. but unfortunately we
only have the Chinese manuals right now, so I don't have a link to
them here.
>
> > + */
> > +
> > [...]
> > +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;
> > +
> > + if (enabled && !state->enabled) {
> > + pwm_loongson_disable(chip, pwm);
> > + return 0;
> > + }
>
> You can also shortcut if !pwm->state.enabled. Something like:
>
> if (!state->enabled) {
> if (enabled)
> pwm_loongson_disable(chip, pwm);
> return 0;
> }
>
> > + if (state->polarity != pwm->state.polarity) {
> > + ret = pwm_loongson_set_polarity(chip, pwm, state->polarity);
> > + if (ret)
> > + return ret;
> > + }
>
> Together with the shortcut above this is buggy. Consider:
>
> pwm_apply_might_sleep(mypwm, &(struct pwm_state){
> .period = A,
> .duty_cycle = B,
> .polarity = PWM_POLARITY_NORMAL,
> .enabled = true});
> pwm_apply_might_sleep(mypwm, &(struct pwm_state){
> .period = A,
> .duty_cycle = B,
> .polarity = PWM_POLARITY_INVERSED,
> .enabled = false});
> pwm_apply_might_sleep(mypwm, &(struct pwm_state){
> .period = A,
> .duty_cycle = B,
> .polarity = PWM_POLARITY_INVERSED,
> .enabled = true});
>
> After the 2nd call you left pwm_loongson_apply() early without writing
> the inversed polarity to the register space. In the 3rd call you have
> state->polarity == pwm->state.polarity and so skip configuring the
> polarity again.
>
> I suggest to just do pwm_loongson_set_polarity() unconditionally if
> state->enabled = true.
I check code and try to do it like:
if (!state->enabled) {
if (enabled)
pwm_loongson_disable(chip, pwm);
return 0;
}
ret = pwm_loongson_set_polarity(chip, pwm, state->polarity);
if (ret)
return ret;
Thanks.
Binbin
>
> > + period = min(state->period, NANOHZ_PER_HZ);
> > + duty_cycle = min(state->duty_cycle, NANOHZ_PER_HZ);
> > +
> > + ret = pwm_loongson_config(chip, pwm, duty_cycle, period);
> > + if (ret)
> > + return ret;
> > +
> > + if (!enabled && state->enabled)
> > + ret = pwm_loongson_enable(chip, pwm);
> > +
> > + return ret;
> > +}
>
> Best regards
> Uwe
