On 09/28/2013 01:15 AM, Alexandre Belloni wrote:
Cc the atmel maintainers that may want to chime in.
On 27/09/2013 17:51, Milo Kim wrote:
On 09/26/2013 04:48 PM, Alexandre Belloni wrote:
Hum, maybe my wording was wrong. What I meant is that when that
disabling the PWM channel by using the PWM_DIS
register, the PWM is not driving the pin anymore. Then, the level goes
from low (which is correct) to high. Also, the datasheet specifies that
the pwm has to be enabled to get the correct level when duty == 0 or
duty == period.
IIRC, this is not the same on TI SoC where duty == 0 don't give you the
expected behavior and I can understand why there is a pwm_disable()
there. Maybe we have to have a way to differentiate both cases ?
Based on your result, PWM_DIS should be updated when the driver is
unloaded - no PWM consumer anymore.
Why don't you move PWM_DIS register access code from
atmel_pwm_disable() to atmel_pwm_remove()?
If it makes sense, the PWM_EN code also needs to be moved to _probe().
If we do what you suggest, I'm afraid we will enable pwm channels that
have no consumers. Isn't pwm_disable() suppose to disable the pwm
channels ? I believe that is what is done on the other platforms.
Ah, I missed that point, thanks.
What about using pwm_request() and pwm_free()?
1) Add pwm_request() and pwm_free ops in the atmel-pwm driver.
static const struct pwm_ops atmel_pwm_ops = {
+ .request = atmel_pwm_request,
+ .free = atmel_pwm_free,
.config = atmel_pwm_config,
.set_polarity = atmel_pwm_set_polarity,
.enable = atmel_pwm_enable,
.disable = atmel_pwm_disable,
.owner = THIS_MODULE,
};
2) Move atmel PWM register code to atmel_pwm_request() and _free()
static int atmel_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
{
struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
atmel_pwm_writel(atmel_pwm, PWM_ENA, 1 << pwm->hwpwm);
return 0;
}
static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
{
struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
atmel_pwm_writel(atmel_pwm, PWM_DIS, 1 << pwm->hwpwm);
}
It still has some issue which you pointed if a PWM consumer doesn't call
pwm_enable() after pwm_get().
However, it's better idea than enabling the PWM channel at initial time.
Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe linux-leds" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
