On Mon, Jan 06, 2014 at 08:30:53PM +0400, Alexander Shiyan wrote:
> Hello.
>
> Понедельник, 6 января 2014, 17:12 +01:00 от Thierry Reding <thierry.reding@xxxxxxxxx>:
> > 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
Attachment:
pgpqkTEPqKWlT.pgp
Description: PGP signature
