Re: [PATCH v9 2/2] pwm: sifive: Add a driver for SiFive SoC PWM

[Uwe Kleine-König]

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/  |