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