Re: leds-pwm: issue in __led_pwm_set()

[Milo Kim <milo.kim@ti.com>]

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