Re: [PATCH v4 2/2] pwm: Add Loongson PWM controller support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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?

> > +}
> > +
> > +static const struct pwm_ops pwm_loongson_ops = {
> > +     .apply = pwm_loongson_apply,
> > +     .get_state = pwm_loongson_get_state,
> > +};
> > +
> > +static int pwm_loongson_probe(struct platform_device *pdev)
> > +{
> > +     int ret;
> > +     struct pwm_chip *chip;
> > +     struct pwm_loongson_ddata *ddata;
> > +     struct device *dev = &pdev->dev;
> > +
> > +     chip = devm_pwmchip_alloc(dev, 1, sizeof(*ddata));
> > +     if (IS_ERR(chip))
> > +             return PTR_ERR(chip);
> > +     ddata = to_pwm_loongson_ddata(chip);
> > +
> > +     ddata->base = devm_platform_ioremap_resource(pdev, 0);
> > +     if (IS_ERR(ddata->base))
> > +             return PTR_ERR(ddata->base);
> > +
> > +     if (!has_acpi_companion(dev)) {
> > +             ddata->clk = devm_clk_get_enabled(dev, NULL);
> > +             if (IS_ERR(ddata->clk))
> > +                     return dev_err_probe(dev, PTR_ERR(ddata->clk),
> > +                                          "failed to get pwm clock\n");
> > +             ddata->clk_rate = clk_get_rate(ddata->clk);
> > +     } else {
> > +             ddata->clk_rate = LOONGSON_PWM_FREQ_STD;
> > +     }
> > +
> > +     chip->ops = &pwm_loongson_ops;
> > +     dev_set_drvdata(dev, chip);
> > +
> > +     ret = devm_pwmchip_add(dev, chip);
> > +     if (ret < 0) {
> > +             clk_disable_unprepare(ddata->clk);
>
> This is wrong. You aquired the clk using devm_clk_get_enabled(), so you
> don't need (and must not) care for disable.
>
Sorry, I had some misunderstanding about devm_clk_get_enabled(), I will fix it.

Thanks.
Binbin
> > +             return dev_err_probe(dev, ret, "failed to add PWM chip\n");
> > +     }
> > +
> > +     return 0;
> > +}
>
> Best regards
> Uwe





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux