On Thu, Oct 17, 2019 at 7:19 AM Uwe Kleine-König
[off-list ref] wrote:

On Thu, Oct 17, 2019 at 12:47:27PM +0100, Daniel Thompson wrote:

quoted

On Thu, Oct 17, 2019 at 10:10:59AM +0200, Uwe Kleine-König wrote:

quoted

A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let
pwm_get_state() return the last implemented state")) changed the
semantic of pwm_get_state() and disclosed an (as it seems) common
problem in lowlevel PWM drivers. By not relying on the period and duty
cycle being retrievable from a disabled PWM this type of problem is
worked around.

Apart from this issue only calling the pwm_get_state/pwm_apply_state
combo once is also more effective.

I'm only interested in the second paragraph here.

There seems to be a reasonable consensus that the i.MX27 and cros-ec
PWM drivers should be fixed for the benefit of other PWM clients.
So we make this change because it makes the pwm-bl better... not to
work around bugs ;-).

That's fine, still I think it's fair to explain the motivation of
creating this patch.

quoted

quoted

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 746eebc411df..ddebd62b3978 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c

@@ -67,40 +62,27 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb)

 static void pwm_backlight_power_off(struct pwm_bl_data *pb)
 {
-   struct pwm_state state;
-
-   pwm_get_state(pb->pwm, &state);
-   if (!pb->enabled)
-           return;
-

Why remove the pb->enabled check? I thought that was there to ensure we
don't mess up the regular reference counts.

I havn't looked yet, but I guess I have to respin. Expect a v2 later
today.

I would agree that a high-level fix is better than a series of low
level driver fixes.  For what its worth, your V1 patch worked fine on
my i.MX6Q.  I can test the V2 patch when its ready.

adam

Best regards
Uwe

--
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |