leds/pwm: don't call pwm_disable when setting brightness

This fixes disabling the LED on i.MX28. The PWM hardware delays using the newly set pwm-config until the beginning of a new period. It's very likely that pwm_disable is called before the current period ends. In case the LED was on brightness=max before the LED stays on because in the disabled PWM block the period never ends.

Also only call pwm_enable only once in the probe call back and the matching pwm_disable in .remove(). Moreover the pwm is explicitly initialized to off.

Signed-off-by Uwe Kleine-K?nig <[email protected]>

Uwe Kleine-K?nig <[email protected]> hat am 12. Februar 2015 um 10:44 geschrieben:

This fixes disabling the LED on i.MX28. The PWM hardware delays using the newly set pwm-config until the beginning of a new period. It's very likely that pwm_disable is called before the current period ends. In case the LED was on brightness=max before the LED stays on because in the disabled PWM block the period never ends.

Also only call pwm_enable only once in the probe call back and the matching pwm_disable in .remove(). Moreover the pwm is explicitly initialized to off.

Signed-off-by Uwe Kleine-K?nig <[email protected]>

---
drivers/leds/leds-pwm.c | 10 +++++-----

1 file changed, 5 insertions(+), 5 deletions(-)

drivers/leds/leds-pwm.c +0 -5

index f668500a2157..5dae0d2dc3dc 100644
@@ -44,11 +44,6 @@ static void __led_pwm_set(struct led_pwm_data *led_dat)
 int new_duty = led_dat->duty;
 
 pwm_config(led_dat->pwm, new_duty, led_dat->period);
-
- if (new_duty == 0)
- pwm_disable(led_dat->pwm);
- else
- pwm_enable(led_dat->pwm);
 }
 
 static void led_pwm_work(struct work_struct *work)

After replacing "i" with "priv->num_leds" in this patch we are able to use pwm led with trigger heartbeat.

This is the functional part of this issue, since the warning has been fixed [1]

Hello Stefan,

Quoted 2 lines · Stefan Wahren

After replacing "i" with "priv->num_leds" in this patch we are able to use pwm led with trigger heartbeat.

Right, that's what I did in my tree to to please the build robot :-)

Quoted 1 line

This is the functional part of this issue, since the warning has been fixed [1]

Can I interpret this as an Tested-by: for this patch and an Ack for patch 1?

Best regards
Uwe

Tested-by Stefan Wahren <[email protected]>

Quoted 3 lines

Quoted 1 line

This is the functional part of this issue, since the warning has been fixed [1]

Can I interpret this as an Tested-by: for this patch and an Ack for patch 1?

Sorry, but i don't think i'm the right person for acking patch 1.

on arm/i.MX28 the leds connected to a pwm are still broken and it's more than three years ago that I came up with these patches. I still consider them to do the right thing and they fix a real bug.

Old threads to this topic include:

	http://thread.gmane.org/gmane.linux.kernel/1381289
	http://thread.gmane.org/gmane.linux.ports.arm.kernel/282593/focus=282596
	http://thread.gmane.org/gmane.linux.pwm/1749

Uwe Kleine-K?nig (2):
pwm/doc: Clearify that the pin state after pwm_disable is undefined leds/pwm: Don't disable pwm when setting brightness to 0

 Documentation/pwm.txt   |  5 +++++
 drivers/leds/leds-pwm.c | 10 +++++-----
 2 files changed, 10 insertions(+), 5 deletions(-)

[Cc: += akpm]

Quoted 3 lines · Uwe Kleine-K?nig

on arm/i.MX28 the leds connected to a pwm are still broken and it's more than three years ago that I came up with these patches. I still consider them to do the right thing and they fix a real bug.

I'm really frustrated here. I want to fix a real bug, made several suggestions (with patches) how to fix it and still have to include my local patches in each project that uses leds on i.MX28's pwm output.

Thierry, how can we get this resolved?

Quoted 9 lines · Uwe Kleine-K?nig

Quoted 3 lines · Uwe Kleine-K?nig

on arm/i.MX28 the leds connected to a pwm are still broken and it's more than three years ago that I came up with these patches. I still consider them to do the right thing and they fix a real bug.

I'm really frustrated here. I want to fix a real bug, made several suggestions (with patches) how to fix it and still have to include my local patches in each project that uses leds on i.MX28's pwm output.

Thierry, how can we get this resolved?

We have a couple of other drivers already solving similar issues. Look for example at the pwm-bcm-kona driver. It explicitly sets the duty cycle to 0 on ->disable() and then waits for some time before actually disabling the clock (specifically to avoid a similar than you seem to have on i.MX). See also the notes near the top of the driver source file.

Another example is pwm-samsung. It also has code specifically aimed at making sure the signal doesn't unexpectedly stay high after calling pwm_disable(). See also the commit message of:

6da89312a39b pwm: samsung: Fix output race on disabling

Both of these examples sound very similar to what you're describing, so I think it'd be best to follow these examples and fix the i.MX driver to behave as expected.

Irrespective of the behaviour of current drivers, the assumptions are already codified in the user drivers and they all assume that calling pwm_disable() means the PWM is off.

Hello Thierry,

Quoted 24 lines · Thierry Reding

Quoted 9 lines · Uwe Kleine-K?nig

Quoted 3 lines · Uwe Kleine-K?nig

on arm/i.MX28 the leds connected to a pwm are still broken and it's more than three years ago that I came up with these patches. I still consider them to do the right thing and they fix a real bug.

I'm really frustrated here. I want to fix a real bug, made several suggestions (with patches) how to fix it and still have to include my local patches in each project that uses leds on i.MX28's pwm output.

Thierry, how can we get this resolved?

We have a couple of other drivers already solving similar issues. Look for example at the pwm-bcm-kona driver. It explicitly sets the duty cycle to 0 on ->disable() and then waits for some time before actually disabling the clock (specifically to avoid a similar than you seem to have on i.MX). See also the notes near the top of the driver source file.

Another example is pwm-samsung. It also has code specifically aimed at making sure the signal doesn't unexpectedly stay high after calling pwm_disable(). See also the commit message of:

6da89312a39b pwm: samsung: Fix output race on disabling

Quoted 3 lines

Both of these examples sound very similar to what you're describing, so I think it'd be best to follow these examples and fix the i.MX driver to behave as expected.

Seeing that more hardware behaves like the mxs pwm makes me still more convinced that the pwm core should be aware of things like double buffering and different behaviour for disabling. Adding code to fulfil the API/user expectation to all three (and maybe more?) drivers seems wrong, don't you think?

There are IMHO two possibilities to define the outcome of pwm_config(duty=0) + pwm_disable():

a) the output must be 0
b) the output is undefined

For a) there are two further cases:

a1) each driver has to assert a) on its own
a2) the pwm framework knows about a few common behaviours and can
handle them.

Currently we are at a1) (which is undocumented in Documentation/pwm.txt btw). IMHO a1) is the worst of the three alternatives!

BTW, what is the (expected but also undocumented) outcome of

	pwm_set_polarity(polarity=PWM_POLARITY_INVERSED);
	pwm_config(duty=0)
	pwm_disable()

Quoted 3 lines

Irrespective of the behaviour of current drivers, the assumptions are already codified in the user drivers and they all assume that calling pwm_disable() means the PWM is off.

Well, if you call pwm_disable for the mxs pwm, it is disabled instantly. Disabling just doesn't imply that the output goes to 0.

The problem at hand is updating the configuration doesn't end the current period. And then stopping the clock lets the output keep the last driven value.

Considering

	$ git grep \\\<pwm_disable | wc -l
	34

going through all callers and fixing them up for changed semantics doesn't seem too hard. Still more as there are several false hits (approx 50%).

Quoted 25 lines · Uwe Kleine-K?nig

Quoted 24 lines · Thierry Reding

Quoted 9 lines · Uwe Kleine-K?nig

Quoted 3 lines · Uwe Kleine-K?nig

on arm/i.MX28 the leds connected to a pwm are still broken and it's more than three years ago that I came up with these patches. I still consider them to do the right thing and they fix a real bug.

I'm really frustrated here. I want to fix a real bug, made several suggestions (with patches) how to fix it and still have to include my local patches in each project that uses leds on i.MX28's pwm output.

Thierry, how can we get this resolved?

We have a couple of other drivers already solving similar issues. Look for example at the pwm-bcm-kona driver. It explicitly sets the duty cycle to 0 on ->disable() and then waits for some time before actually disabling the clock (specifically to avoid a similar than you seem to have on i.MX). See also the notes near the top of the driver source file.

Another example is pwm-samsung. It also has code specifically aimed at making sure the signal doesn't unexpectedly stay high after calling pwm_disable(). See also the commit message of:

6da89312a39b pwm: samsung: Fix output race on disabling

Quoted 8 lines

Quoted 3 lines

Both of these examples sound very similar to what you're describing, so I think it'd be best to follow these examples and fix the i.MX driver to behave as expected.

Seeing that more hardware behaves like the mxs pwm makes me still more convinced that the pwm core should be aware of things like double buffering and different behaviour for disabling. Adding code to fulfil the API/user expectation to all three (and maybe more?) drivers seems wrong, don't you think?

Erm... no. The whole point of a generic API is to abstract these kinds of differences between various drivers. The goal is that users of the API don't have to worry about the differences anymore and can expect a certain behaviour.

Quoted 12 lines

There are IMHO two possibilities to define the outcome of pwm_config(duty=0) + pwm_disable():

a) the output must be 0
b) the output is undefined

For a) there are two further cases:

a1) each driver has to assert a) on its own
a2) the pwm framework knows about a few common behaviours and can
handle them.

Quoted 2 lines

Currently we are at a1) (which is undocumented in Documentation/pwm.txt btw). IMHO a1) is the worst of the three alternatives!

Why do you think it is the worst? If we define the behaviour as b) what does that gain us? Why would users want to call these functions if their

behaviour is undefined? What that will result in is that code happens to work with some PWM drivers but not with others. Then drivers will go and implement workarounds that work only for specific drivers and so on.

a2) isn't much better. It hides the specifics from the users, but at the

same time it complicates the core because you're trying to generically describe something that's inherently not generic. Device-specific code

belongs in drivers, not in the framework.

Quoted 5 lines

Quoted 3 lines

Irrespective of the behaviour of current drivers, the assumptions are already codified in the user drivers and they all assume that calling pwm_disable() means the PWM is off.

Well, if you call pwm_disable for the mxs pwm, it is disabled instantly. Disabling just doesn't imply that the output goes to 0.

Right, and that's precisely what other drivers have extra code for to handle. If you look at the pwm-bcm-kona driver it has an extra delay of 400 ns to make sure that the waveform is really updated.

Quoted 3 lines

The problem at hand is updating the configuration doesn't end the current period. And then stopping the clock lets the output keep the last driven value.

So why can't you just block in ->disable() until you know the output is actually what's been requested? If you can't read that out from the hardware, then simply wait for a whole period before stopping the clock.

Quoted 5 lines

Considering

	$ git grep \\\<pwm_disable | wc -l
	34

Quoted 3 lines

going through all callers and fixing them up for changed semantics doesn't seem too hard. Still more as there are several false hits (approx 50%).

Keep in mind that you'd need to retest all combinations of these users with all PWM drivers to make sure you're not inadvertently breaking existing setups.

[added the people being involved with the other two drivers in question, maybe you want to comment?]

Hello Thierry,

Quoted 35 lines · Thierry Reding

Quoted 25 lines · Uwe Kleine-K?nig

Quoted 24 lines · Thierry Reding

Quoted 9 lines · Uwe Kleine-K?nig

Quoted 3 lines · Uwe Kleine-K?nig

on arm/i.MX28 the leds connected to a pwm are still broken and it's more than three years ago that I came up with these patches. I still consider them to do the right thing and they fix a real bug.

I'm really frustrated here. I want to fix a real bug, made several suggestions (with patches) how to fix it and still have to include my local patches in each project that uses leds on i.MX28's pwm output.

Thierry, how can we get this resolved?

We have a couple of other drivers already solving similar issues. Look for example at the pwm-bcm-kona driver. It explicitly sets the duty cycle to 0 on ->disable() and then waits for some time before actually disabling the clock (specifically to avoid a similar than you seem to have on i.MX). See also the notes near the top of the driver source file.

Another example is pwm-samsung. It also has code specifically aimed at making sure the signal doesn't unexpectedly stay high after calling pwm_disable(). See also the commit message of:

6da89312a39b pwm: samsung: Fix output race on disabling

Quoted 8 lines

Quoted 3 lines

Both of these examples sound very similar to what you're describing, so I think it'd be best to follow these examples and fix the i.MX driver to behave as expected.

Seeing that more hardware behaves like the mxs pwm makes me still more convinced that the pwm core should be aware of things like double buffering and different behaviour for disabling. Adding code to fulfil the API/user expectation to all three (and maybe more?) drivers seems wrong, don't you think?

Quoted 4 lines

Erm... no. The whole point of a generic API is to abstract these kinds of differences between various drivers. The goal is that users of the API don't have to worry about the differences anymore and can expect a certain behaviour.

I agree to a certain degree here. Yes, an API is there to abstract between different drivers. But if the content of an API that was once been considered "generic" turns out to be not so generic after all, the right thing to do is to change it. If it's possible to keep the behaviour for users that's great, if not this shouldn't stop you.

Quoted 15 lines

Quoted 14 lines

There are IMHO two possibilities to define the outcome of pwm_config(duty=0) + pwm_disable():

a) the output must be 0
b) the output is undefined

For a) there are two further cases:

a1) each driver has to assert a) on its own
a2) the pwm framework knows about a few common behaviours and can
handle them.

Currently we are at a1) (which is undocumented in Documentation/pwm.txt btw). IMHO a1) is the worst of the three alternatives!

Quoted 2 lines

Why do you think it is the worst? If we define the behaviour as b) what does that gain us? Why would users want to call these functions if their

The gain is that the three (or more?) pwm drivers don't need to special case an artificial requirement of the framework. OK, users have to adapt, but it's as simple as substituting all questionable calls to pwm_disable by a call to pwm_config(duty=0).

Quoted 3 lines

behaviour is undefined? What that will result in is that code happens to work with some PWM drivers but not with others. Then drivers will go and implement workarounds that work only for specific drivers and so on.

In this case the only "workaround" is to drop the pwm_disable. This is a correct fix however with my suggested change.

Quoted 1 line

a2) isn't much better. It hides the specifics from the users, but at the

a1) hides the specifics, don't it?

a2) explicitly states that there are specifics and users should be aware.

a1) *ignores* specifics at the cost of additional effort for drivers where a1) is wrong in hardware!

Quoted 2 lines

same time it complicates the core because you're trying to generically describe something that's inherently not generic. Device-specific code

You claim that "pwms yield a constant 0 on disable".

I say: "It depends on the pwm what happens if you disable it".

Your claim is wrong for at least three pwms. Mine holds for all. So which is more generic?

Also note that with a) the API has two properties that are bad for an API:

- there are two ways to request the output to be constant low:
1) pwm_config(duty=0)
2) pwm_config(duty=0) + pwm_disable()

- pwm_disable fills two different purposes:
1) The user has to (or can) use it as part of requesting a 0

2) The user tells to not need the pwm any more

Both are fixed by my suggestion.

Quoted 1 line

belongs in drivers, not in the framework.

Device-specific code belongs into the framework if it is common enough to allow simplification of several drivers. IMHO an API has two sides, in the case of the pwm framework it's:

- the drivers that use a pwm (e.g. drivers/leds/leds-pwm.c)
- the drivers that know how to operate a pwm (e.g. drivers/pwm/pwm-mxs.c)

If the API pleases both sides that's better than only being nice to one side.

Quoted 6 lines

Quoted 5 lines

Quoted 3 lines

Irrespective of the behaviour of current drivers, the assumptions are already codified in the user drivers and they all assume that calling pwm_disable() means the PWM is off.

Well, if you call pwm_disable for the mxs pwm, it is disabled instantly. Disabling just doesn't imply that the output goes to 0.

Quoted 3 lines

Right, and that's precisely what other drivers have extra code for to handle. If you look at the pwm-bcm-kona driver it has an extra delay of 400 ns to make sure that the waveform is really updated.

I claim it was already wrong to add this extra code to pwm-bcm-kona. Also note that in some usecases this delay is not needed (i.e. depending on where the delay is added, the user might not care if pwm_config is in effect when pwm_config returns or pwm_disable doesn't need to result in a 0 because it was just called in the error path of another driver).

Quoted 7 lines

Quoted 3 lines

The problem at hand is updating the configuration doesn't end the current period. And then stopping the clock lets the output keep the last driven value.

So why can't you just block in ->disable() until you know the output is actually what's been requested? If you can't read that out from the hardware, then simply wait for a whole period before stopping the clock.

I can, but that's not the point. It complicates the driver while the availability of a few helper constructs in the framework could do this for three drivers.

Quoted 5 lines

Quoted 5 lines

Considering

	$ git grep \\\<pwm_disable | wc -l
	34

Quoted 7 lines

Quoted 3 lines

going through all callers and fixing them up for changed semantics doesn't seem too hard. Still more as there are several false hits (approx 50%).

Keep in mind that you'd need to retest all combinations of these users with all PWM drivers to make sure you're not inadvertently breaking existing setups.

That's what we have release candidates for. If you really only want to take changes that with a probability of 0 break any existing setups, we can substitute you by a procmail rule that naks all patches. This is about best effort. Yes, API changes are annoying, but in the long run they are good.
Having said that, I don't consider my suggested change as "dangerous". Changing the documented behaviour doesn't affect any setup. Then fixing each bug that pops up is good enough.

Quoted 6 lines · Uwe Kleine-K?nig

Quoted 2 lines

Why do you think it is the worst? If we define the behaviour as b) what does that gain us? Why would users want to call these functions if their

The gain is that the three (or more?) pwm drivers don't need to special case an artificial requirement of the framework. OK, users have to adapt, but it's as simple as substituting all questionable calls to pwm_disable by a call to pwm_config(duty=0).

There is no gain. If users suddenly have to care about hardware specifics I call that a loss. Equivalently if the PWM core framework needs to know about these specifics that's also a loss because it puts complexity in the core where device-specific properties clearly shouldn't be handled.