leds-pwm: issue in __led_pwm_set()

leds-pwm: issue in __led_pwm_set()

[Alexandre Belloni]

Hi,

I'm using leds-pwm on an atmel sama5d31 based board. I have one
question/issue. In __led_pwm_set(), if the duty cycle is 0,
pwm_disable() is called. This won't work fine on that platform. What
happens is that pwm_config() correctly sets the duty cycle to 0 which is
behaving correctly with that controller (and in my case, putting the
line low). But the call to pwm_disable() is making the pwm controller
release the line and then it is set to high.

I've tried various configurations, like configuring a pull-down on the
pin but I still observe the same behavior. For now, I have a workaround
in atmel-pwm (I activate the output overdrive when duty cycle is 0). But
I believe, we should find another way to do that.

Any input is appreciated.

Regards,

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: leds-pwm: issue in __led_pwm_set()

[Milo Kim]

Looping Thierry who maintains the PWM subsystem.

I'm thinking your PWM driver is
https://lkml.org/lkml/2013/9/24/776.
Is it correct?

However, I can't find any pin control for the PWM output.
Just clock control and internal register access, PWM_EN and PWM_DIS.
Is the pin controlled via the registers?

Best regards,
Milo

On 09/26/2013 05:00 AM, Alexandre Belloni wrote:
> Hi,
>
> I'm using leds-pwm on an atmel sama5d31 based board. I have one
> question/issue. In __led_pwm_set(), if the duty cycle is 0,
> pwm_disable() is called. This won't work fine on that platform. What
> happens is that pwm_config() correctly sets the duty cycle to 0 which is
> behaving correctly with that controller (and in my case, putting the
> line low). But the call to pwm_disable() is making the pwm controller
> release the line and then it is set to high.
>
> I've tried various configurations, like configuring a pull-down on the
> pin but I still observe the same behavior. For now, I have a workaround
> in atmel-pwm (I activate the output overdrive when duty cycle is 0). But
> I believe, we should find another way to do that.
>
> Any input is appreciated.
>
> Regards,
>

Re: leds-pwm: issue in __led_pwm_set()

[Alexandre Belloni]

On 26/09/2013 02:28, Milo Kim wrote:
> Looping Thierry who maintains the PWM subsystem.
>
> I'm thinking your PWM driver is
> https://lkml.org/lkml/2013/9/24/776.
> Is it correct?
>

That is the one yes.

> However, I can't find any pin control for the PWM output.
> Just clock control and internal register access, PWM_EN and PWM_DIS.
> Is the pin controlled via the registers?
>

Pin control is taken care of by the pinctrl-at91 driver, shouldn't that
be enough ?

> Best regards,
> Milo
>
> On 09/26/2013 05:00 AM, Alexandre Belloni wrote:
>> Hi,
>>
>> I'm using leds-pwm on an atmel sama5d31 based board. I have one
>> question/issue. In __led_pwm_set(), if the duty cycle is 0,
>> pwm_disable() is called. This won't work fine on that platform. What
>> happens is that pwm_config() correctly sets the duty cycle to 0 which is
>> behaving correctly with that controller (and in my case, putting the
>> line low). But the call to pwm_disable() is making the pwm controller
>> release the line and then it is set to high.
>>
>> I've tried various configurations, like configuring a pull-down on the
>> pin but I still observe the same behavior. For now, I have a workaround
>> in atmel-pwm (I activate the output overdrive when duty cycle is 0). But
>> I believe, we should find another way to do that.
>>
>> Any input is appreciated.
>>
>> Regards,
>>
>
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-leds" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: leds-pwm: issue in __led_pwm_set()

[Milo Kim]

Hi Alexandre,

On 09/26/2013 04:13 PM, Alexandre Belloni wrote:
> On 26/09/2013 02:28, Milo Kim wrote:
>> However, I can't find any pin control for the PWM output.
>> Just clock control and internal register access, PWM_EN and PWM_DIS.
>> Is the pin controlled via the registers?
>>
>
> Pin control is taken care of by the pinctrl-at91 driver, shouldn't that
> be enough ?

Ah, I didn't mean the pinctrl subsystem. Sorry for confusing you.

I just want to know the reason why 'atmel_pwm_disable()' releases the 
PWM pin.
I can't find it in the driver. Am I missing something?

- Milo

Re: leds-pwm: issue in __led_pwm_set()

[Alexandre Belloni]

On 26/09/2013 09:34, Milo Kim wrote:
>
> Ah, I didn't mean the pinctrl subsystem. Sorry for confusing you.
>
> I just want to know the reason why 'atmel_pwm_disable()' releases the
> PWM pin.
> I can't find it in the driver. Am I missing something?
>

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 ?

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: leds-pwm: issue in __led_pwm_set()

[Milo Kim]

On 09/26/2013 04:48 PM, Alexandre Belloni wrote:
> On 26/09/2013 09:34, Milo Kim wrote:
>>
>> Ah, I didn't mean the pinctrl subsystem. Sorry for confusing you.
>>
>> I just want to know the reason why 'atmel_pwm_disable()' releases the
>> PWM pin.
>> I can't find it in the driver. Am I missing something?
>>
>
> 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().

Best regards,
Milo

Re: leds-pwm: issue in __led_pwm_set()

[Alexandre Belloni]

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.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: leds-pwm: issue in __led_pwm_set()

[Milo Kim]

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

Re: leds-pwm: issue in __led_pwm_set()

[Milo Kim]

Sorry, I meant atmel_pwm_free(), not atmel_pwm_disable().
- copy & paste disaster! :(

> 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)

static void atmel_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)

Re: leds-pwm: issue in __led_pwm_set()

[Alexandre Belloni]

On 27/09/2013 19:33, Milo Kim wrote:
> Sorry, I meant atmel_pwm_free(), not atmel_pwm_disable().
> - copy & paste disaster! :(
>
>> 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)
>
> static void atmel_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
>
>

Ok, I tested that and it seems to be working fine.

Thanks !

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: leds-pwm: issue in __led_pwm_set()

[Alexandre Belloni]

On 27/09/2013 20:34, Alexandre Belloni wrote:
>
> Ok, I tested that and it seems to be working fine.
>

One last question though. What about power management ?

For example, in pwm_bl, pwm_backlight_suspend calls:

    pwm_config(pb->pwm, 0, pb->period);
    pwm_disable(pb->pwm);

Which is fine because it will correctly set up the PWM to a low level.
Then, to disable the pwm itself and its clock, I suppose we would have
to define some pm_ops in atmel_pwm, am I right ?

So we will have to be extra careful to correctly set a low level on the
pin at that time.

Thanks,

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: leds-pwm: issue in __led_pwm_set()

[Milo Kim]

On 09/28/2013 04:46 AM, Alexandre Belloni wrote:
> On 27/09/2013 20:34, Alexandre Belloni wrote:
>>
>> Ok, I tested that and it seems to be working fine.
>>
>
> One last question though. What about power management ?
>
> For example, in pwm_bl, pwm_backlight_suspend calls:
>
>      pwm_config(pb->pwm, 0, pb->period);
>      pwm_disable(pb->pwm);
>
> Which is fine because it will correctly set up the PWM to a low level.

OK.

> Then, to disable the pwm itself and its clock, I suppose we would have
> to define some pm_ops in atmel_pwm, am I right ?

Maybe the suspend/resume() can be handled by the PWM driver.
However, not every PWM consumer of atmel-pwm needs the PM code.
I think it would be better if those are handled by each the PWM consumer.

> So we will have to be extra careful to correctly set a low level on the
> pin at that time.

If you only care about the leds-pwm, then no additional handling is 
required.
The LED subsystem provides a 'LED_CORE_SUSPENDRESUME' flag which enables 
the PM automatically.
The brightness is set to 0 forcedly on the suspend, and the stored 
brightness is recovered on the resume.
The leds-pwm driver enables this flag.
On the suspend mode, pwm_config() and pwm_disable() are called in 
__led_pwm_set(). That's exactly what you need.

Best regards,
Milo