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});
> + 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
- ...
> +}
> +
> +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.
> + return dev_err_probe(dev, ret, "failed to add PWM chip\n");
> + }
> +
> + return 0;
> +}
Best regards
Uwe
Attachment:
signature.asc
Description: PGP signature
