leds-pwm: issue in __led_pwm_set()

DISCUSSION Alexandre Belloni 2 participants 12 messages Sep 25, 2013 – Sep 28, 2013 LED

Share & tools

Alexandre Belloni

author

2013-09-25 20:00 UTC #0 lore raw

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,

Signature · 3 lines

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

Milo Kim

2013-09-26 00:28 UTC #1 lore raw

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

Quoted 19 lines · 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

author

2013-09-26 07:13 UTC #2 lore raw

Quoted 6 lines · 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?

That is the one yes.

Quoted 4 lines

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 ?

Quoted 28 lines

Best regards,
Milo
Quoted 19 lines · 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,
Signature · 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-leds" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Signature · 3 lines

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

Milo Kim

2013-09-26 07:34 UTC #3 lore raw

Hi Alexandre,

Quoted 8 lines · Alexandre Belloni

Quoted 4 lines · Milo Kim
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

Alexandre Belloni

author

2013-09-26 07:48 UTC #4 lore raw

Quoted 7 lines · Milo Kim

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 ?

Signature · 3 lines

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

Milo Kim

2013-09-27 15:51 UTC #5 lore raw

Quoted 16 lines · Alexandre Belloni

Quoted 7 lines · Milo Kim
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.

Quoted 4 lines

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

Alexandre Belloni

author

2013-09-27 16:15 UTC #6 lore raw

Cc the atmel maintainers that may want to chime in.

Quoted 13 lines · Milo Kim

Quoted 8 lines · Alexandre Belloni
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.
Quoted 4 lines
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 ?

Quoted 7 lines

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.

Signature · 3 lines

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

Milo Kim

2013-09-27 17:23 UTC #7 lore raw

Quoted 19 lines · Alexandre Belloni

Cc the atmel maintainers that may want to chime in.
Quoted 16 lines · Milo Kim
Quoted 8 lines · Alexandre Belloni
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.
Quoted 4 lines
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.

Quoted 9 lines

Quoted 4 lines
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

Milo Kim

2013-09-27 17:33 UTC #8 lore raw

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

Quoted 10 lines

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;
}

Quoted 2 lines

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)

Alexandre Belloni

author

2013-09-27 18:34 UTC #9 lore raw

Quoted 10 lines · Milo Kim

Sorry, I meant atmel_pwm_free(), not atmel_pwm_disable().
- copy & paste disaster! :(
Quoted 7 lines
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);

Quoted 10 lines

Quoted 6 lines
     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 !

Signature · 3 lines

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

Alexandre Belloni

author

2013-09-27 19:46 UTC #10 lore raw

Quoted 3 lines · Alexandre Belloni

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,

Signature · 3 lines

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

Milo Kim

2013-09-28 13:29 UTC #11 lore raw

Quoted 5 lines · Alexandre Belloni

Quoted 3 lines · Alexandre Belloni
Ok, I tested that and it seems to be working fine.

Quoted 8 lines

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.

Quoted 2 lines

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.

Quoted 2 lines

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