On Mon, Jan 06, 2014 at 08:30:53PM +0400, Alexander Shiyan wrote: > Hello. > > Понедельник, 6 января 2014, 17:12 +01:00 от Thierry Reding > <[email protected]>: > > On Wed, Jan 01, 2014 at 07:43:41AM +0400, Alexander Shiyan wrote: > > > Add a new driver for the PWM controllers on the CLPS711X platform > > > based on the PWM framework. > ... > > static inline to_clps711x(struct pwm_chip *chip) > > { > > return container_of(chip, struct clps711x_chip, chip); > > } > > > > > + unsigned int period, freq = clk_get_rate(priv->clk); > > > + > > > + if (!freq) > > > + return -EINVAL; > > > + > > > + /* Calculate and store constant period value */ > > > + period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, freq); > > > + pwm_set_period(pwm, period); > > > + pwm_set_chip_data(pwm, (void *)(uintptr_t)period); > > > > Why store this in chip data again if it can be retrieved directly from > > the PWM device using pwm_get_period()? > > This is used for compare this value in pwm_config(). > pwm_set_period() potentially can be called from any other place and set > illegal value for us, but we should calculate duty ratio with our (proper) > frequency. > Is not it?
Well, that same argument holds for pwm_set_chip_data(). Nothing should
be calling pwm_set_period() from any other place.
> > > +static int clps711x_pwm_enable(struct pwm_chip *chip, struct pwm_device
> > > *pwm)
> > > +{
> > > + /* Do nothing */
> > > + return 0;
> > > +}
> > > +
> > > +static void clps711x_pwm_disable(struct pwm_chip *chip, struct
> > > pwm_device *pwm)
> > > +{
> > > + /* Do nothing */
> > > +}
> >
> > I think it would be better if this would set the duty field to 0 to stop
> > any potential toggling of the PWM signal. .enable() can later restore
> > the proper value.
> >
> > The reason for this is that pwm_disable() is supposed to stop the PWM
> > output from toggling and if you simply ignore it you don't conform to
> > the API specification.
>
> I agree for pwm_disable(), but which value should be restored by pwm_enable()?
> I think we should keep pwm_enable() as is, i.e. we enable PWM with existing
> value.
Yes, pwm_enable() should restore the previous setting. You should be
able to do that by querying the PWM device using pwm_get_duty_cycle()
and reprogram the channel using that value.
Thierry
pgp2sHXGx4yrv.pgp
Description: PGP signature
