Skip to content

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

Uwe Kleine-König linux-arm-kernel linux-leds linux-pwm 1 version New

Get series with b4
b4 am [email protected]

Versions newest first

  1. v1 Latest0 of 2 patches 0 repliesNew

Patches in v1 No applies recorded yet

None of the 2 patches the cover letter announces are in this archive.

Reviews trailers per patch

No Reviewed-by, Acked-by or Tested-by trailers for v1 yet.

Read the v1 thread

Cover letter [PATCH 0/2] leds/pwm: don't call pwm_disable when setting brightness

Hello Thierry,

Quoted 22 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:

Quoted 5 lines

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.

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

Best regards
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Read the cover letter in its thread