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?
> > +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.
Thanks.
---
N�����r��y����b�X��ǧv�^�){.n�+����{��
���^n�r���z���h�����&�� �G���h�(�階�ݢj"���m������z�ޖ���f���h���~�m�
