quoted

+#define FTM_CNTIN_VAL       0x00

Do we really need this?

Maybe not, I think that the initial value maybe modified in the future.
And this can be more easy to ajust it. 

quoted

+	period_cycles = fsl_rate_to_cycles(fpc, period_ns);
+	if (period_cycles > 0xFFFF) {
+		dev_err(chip->dev, "required PWM period cycles(%lu) overflow "
+				"16-bits counter!\n", period_cycles);
+		return -EINVAL;
+	}
+
+	duty_cycles = fsl_rate_to_cycles(fpc, duty_ns);
+	if (duty_cycles >= 0xFFFF) {
+		dev_err(chip->dev, "required PWM duty cycles(%lu) overflow "
+				"16-bits counter!\n", duty_cycles);
+		return -EINVAL;
+	}

I'm not sure the error messages are all that useful. A -EINVAL error
code should make it pretty clear what the problem is.

Yes, these could be absent.

quoted

+	writel(FTMCnSC_MSB | FTMCnSC_ELSB, fpc->base + FTM_CSC(pwm->hwpwm));
+
+	writel(0xF0, fpc->base + FTM_OUTMASK);
+	writel(0x0F, fpc->base + FTM_OUTINIT);

The purpose of this eludes me. These seem to be global (not specific to
channel pwm->hwpwm) registers, so why are they updated whenever a single
channel is reconfigured?

Well, certainly there has one way to update per channel's configuration 
about this. I will revise it then.

quoted

+	writel(period_cycles + cntin - 1, fpc->base + FTM_MOD);
+	writel(duty_cycles + cntin, fpc->base + FTM_CV(pwm->hwpwm));

And these:

	writel(period - 1, fpc->base + FTM_MOD);
	writel(duty, fpc->base + FTM_CV(pwm->hwpwm));

Although now that I think about it, this seems broken. The period is set
in a global register, so I assume it is valid for all channels. What if
you want to use different periods for individual channels? The way I
read this the last one to be configured will win and change the period
to whatever it wants. Other channels won't even notice.

That's right. And all the 8 channels share the same period settings.

Is there a way to set the period per channel?

Not yet. Only could we do is to set the duty value individually for each
channel. So here is a limitation for the cusumers that all the 8 channels'
period values should be the same.

quoted

+static int fsl_counter_clock_enable(struct fsl_pwm_chip *fpc)
+{
+	int ret;
+	unsigned long reg;
+
+	if (fpc->counter_clk_enable++)
+		return 0;

Are you sure this is safe? I think you'll need to use either an atomic
or a mutex to lock this.

Maybe a mutex lock is a good choice.

quoted

+	ret = clk_prepare_enable(fpc->counter_clk);
+	if (ret)
+		return ret;

In case clk_prepare_enable() fails, the counter_clk_enable will need to
be decremented in order to track the state correctly, doesn't it?

Yes, it should be.

quoted

+static int fsl_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct fsl_pwm_chip *fpc;
+
+	fpc = to_fsl_chip(chip);
+
+	fsl_counter_clock_enable(fpc);

This can fail. Should the error be propagated?

That's better.
 

quoted

+static void fsl_pwm_disable(struct pwm_chip *chip, struct pwm_device

*pwm)

quoted

+{
+	struct fsl_pwm_chip *fpc;
+
+	fpc = to_fsl_chip(chip);
+
+	fsl_counter_clock_disable(fpc);
+}

Same here. Since you can't propagate the error, perhaps an error message
would be appropriate here?

Well, in fsl_counter_clock_disable(fpc); only '0' could be returned, maybe
let it a void type value to return is better.
Just like:
static void fsl_counter_clock_disable(struct fsl_pwm_chip *fpc) {}

Also for the locking above, perhaps a good solution would be to acquire
the lock around the calls to fsl_counter_clock_{enable,disable}() so
that they can safely assume that they are called with the lock held,
which will make their implementation a lot simpler.

So what you could do is this:

	static int fsl_pwm_enable(struct pwm_chip *chip, struct pwm_device
*pwm)
	{
		struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
		int ret;

		mutex_lock(&fpc->lock);
		ret = fsl_counter_clock_enable(fpc);
		mutex_unlock(&fpc->lock);

		return ret;
	}

And analogously for fsl_pwm_disable().

I will think about this.

quoted

+static int fsl_pwm_calculate_ps(struct fsl_pwm_chip *fpc)
+{
+	unsigned long long sys_rate, counter_rate, ratio;
+
+	sys_rate = clk_get_rate(fpc->sys_clk);
+	if (!sys_rate)
+		return -EINVAL;
+
+	counter_rate = clk_get_rate(fpc->counter_clk);
+	if (!counter_rate) {
+		fpc->counter_clk = fpc->sys_clk;
+		fpc->counter_clk_select = VF610_CLK_FTM0;
+		dev_warn(fpc->chip.dev,
+				"the counter source clock is a dummy clock, "
+				"so select the system clock as default!\n");
+	}
+
+	switch (fpc->counter_clk_select) {
+	case VF610_CLK_FTM0_FIX_SEL:
+		ratio = 2 * counter_rate - 1;
+		do_div(ratio, sys_rate);
+		fpc->clk_ps = ratio;
+		break;
+	case VF610_CLK_FTM0_EXT_SEL:
+		ratio = 4 * counter_rate - 1;
+		do_div(ratio, sys_rate);
+		fpc->clk_ps = ratio;
+		break;
+	case VF610_CLK_FTM0:
+		fpc->clk_ps = 7;

Even though it doesn't matter here, you should still add a break.
Otherwise if you ever modify the code in the default case, you don't
have to remember to add it in then.

You are right, adding one break is much safer.

--
Best Rwgards,