Re: [PATCH] pwm: Add CLPS711X PWM support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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?

> > +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��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux