RE: [PATCHv6 1/4] pwm: Add Freescale FTM PWM driver support

[Li Xiubo]

> > +#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. 


> > +	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.

> > +	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.


> > +	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.



> > +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.


> > +	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.


> > +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.
 
> > +static void fsl_pwm_disable(struct pwm_chip *chip, struct pwm_device
> *pwm)
> > +{
> > +	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.

> 
> > +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,


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/