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.
> 
> Why would it need to be modified?
> 

Well, for the PWM function modifying it make no sense, so I'll remove this. 


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

Yes, if it dosen't match it should fail, or nothing to do with the period
register if it's not the first channel to be configured.


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

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