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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2391 bytes --]

On Fri, Nov 29, 2013 at 06:42:06AM +0000, Li Xiubo wrote:
> > > +#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. 

Why would it need to be modified?

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

That will need to be handled some way. Perhaps the easiest would be to
check whether or not another channel is already running and check that
indeed the period as requested by the new channel matches that of any
running channels. If it doesn't match, then pwm_config() should fail.

When you do that you can also optimize this a bit by only setting the
frequency once. Whenever a new channel is configured it will have the
same period and therefore the register doesn't need to be reprogrammed.

> > > +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) {}

Yes, that sounds good.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]