Hello,
there are just a few minor things left I commented below.
On Tue, Mar 12, 2019 at 01:41:29PM +0530, Yash Shah wrote:
> +#define div_u64_round(a, b) \
> + ({typeof(b) __b = b; div_u64((a) + __b / 2, __b); })
Parenthesis around b please. I guess I didn't had them in my suggestion
either, sorry for that.
> +static int pwm_sifive_enable(struct pwm_chip *chip, bool enable)
> +{
> + struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> + int ret;
> +
> + if (enable) {
> + ret = clk_enable(pwm->clk);
> + if (ret) {
> + dev_err(pwm->chip.dev, "Enable clk failed:%d\n", ret);
> + return ret;
> + }
> + }
> +
> + if (!enable)
> + clk_disable(pwm->clk);
> +
> + return 0;
> +}
There is only a single caller for this function. I wonder if it is
trivial enough to fold it into pwm_sifive_apply.
> +static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *dev,
> + struct pwm_state *state)
> +{
> + struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> + unsigned int duty_cycle;
> + u32 frac;
> + struct pwm_state cur_state;
> + bool enabled;
> + int ret = 0;
> + unsigned long num;
> +
> + if (state->polarity != PWM_POLARITY_INVERSED)
> + return -EINVAL;
> +
> + ret = clk_enable(pwm->clk);
> + if (ret) {
> + dev_err(pwm->chip.dev, "Enable clk failed:%d\n", ret);
> + return ret;
> + }
> +
> + mutex_lock(&pwm->lock);
> + pwm_get_state(dev, &cur_state);
> +
> + enabled = cur_state.enabled;
> +
> + if (state->period != pwm->approx_period) {
> + if (pwm->user_count != 1) {
> + ret = -EBUSY;
> + goto exit;
> + }
> + pwm->approx_period = state->period;
> + pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk));
> + }
> +
> + duty_cycle = state->duty_cycle;
> + if (!state->enabled)
> + duty_cycle = 0;
> +
> + num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH);
> + frac = div_u64_round(num, state->period);
> + /* The hardware cannot generate a 100% duty cycle */
> + frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
> +
> + writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 +
> + dev->hwpwm * PWM_SIFIVE_SIZE_PWMCMP);
> +
> + if (state->enabled != enabled) {
> + ret = pwm_sifive_enable(chip, state->enabled);
> + if (ret)
> + goto exit;
This goto is a noop.
> + }
> +
> +exit:
> + clk_disable(pwm->clk);
> + mutex_unlock(&pwm->lock);
> + return ret;
> +}
There are a few other things that could be improved, but I think they
could be addressed later. For some of these I don't even know what to
suggest, for some Thierry might not agree it is worth fixing:
- rounding
how to round? When should a request declined, when is rounding ok?
There is still "if (state->period != pwm->approx_period) return -EBUSY"
in this driver. This is better than before, but if state-period ==
pwm->approx_period + 1 the result (if accepted) might be the same as
without the +1 and so returning -EBUSY for one case and accepting the
other is strange.
- don't call PWM API functions designed for consumers (here: pwm_get_state)
- Move div_u64_round to include/linux/math64.h
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
