Messages in this thread | | | Date | Fri, 20 Oct 2023 14:11:48 +0200 | From | Uwe Kleine-König <> | Subject | Re: [PATCH] backlight: pwm_bl: Avoid backlight flicker applying initial PWM state |
| |
Hello Daniel,
On Fri, Oct 20, 2023 at 12:27:27PM +0100, Daniel Thompson wrote: > On Wed, Oct 18, 2023 at 11:07:41PM +0200, Uwe Kleine-König wrote: > > Hello Philipp, > > > > On Thu, Jun 08, 2023 at 04:11:14PM +0200, Philipp Zabel wrote: > > > The initial PWM state returned by pwm_init_state() has a duty cycle > > > of 0 ns. > > > > This is only true for drivers without a .get_state() callback, isn't it? > > pwm_init_state() explicitly zeros the duty-cycle in order to avoid > problems when the default args have a different period to the currently > applied config: > https://elixir.bootlin.com/linux/latest/source/include/linux/pwm.h#L174
Ah right, pwm_init_state() is strange in a different way than I remembered :-) pwm_get_state() is only called to get .enabled set appropriately.
Looking at the callers:
- drivers/gpu/drm/solomon/ssd130x.c It does: pwm_init_state(ssd130x->pwm, &pwmstate); pwm_set_relative_duty_cycle(&pwmstate, 50, 100); pwm_apply_state(ssd130x->pwm, &pwmstate); pwm_enable(ssd130x->pwm);
A probably better result can be reached quicker using: pwm_init_state(ssd130x->pwm, &pwmstate); pwm_set_relative_duty_cycle(&pwmstate, 50, 100); pwmstate.enabled = true; pwm_apply_state(ssd130x->pwm, &pwmstate); - drivers/hwmon/pwm-fan.c __set_pwm should probably explicitly set .enabled. All other calls to pwm_apply_state set .enabled explicitly.
- drivers/input/misc/da7280.c explicitly sets .enabled after calling pwm_init_state()
- drivers/input/misc/pwm-beeper.c explicitly sets .enabled after calling pwm_init_state()
- drivers/input/misc/pwm-vibra.c explicitly sets .enabled after calling pwm_init_state()
- drivers/leds/leds-pwm.c explictily sets .enabled before calling pwm_apply_state()
- drivers/leds/rgb/leds-pwm-multicolor.c explictily sets .enabled before calling pwm_apply_state()
- drivers/media/rc/ir-rx51.c explictily sets .enabled before calling pwm_apply_state()
- drivers/media/rc/pwm-ir-tx.c explictily sets .enabled before calling pwm_apply_state()
- drivers/regulator/pwm-regulator.c never sets .enabled, probably a bug
- drivers/video/backlight/lm3630a_bl.c explictily sets .enabled before calling pwm_apply_state()
- drivers/video/backlight/lp855x_bl.c explictily sets .enabled before calling pwm_apply_state()
- drivers/video/backlight/pwm_bl.c This is the one we currently discuss. I think even with the patch applied it uses the .enabled value returned by pwm_init_state() but it shouldn't.
- drivers/video/fbdev/ssd1307fb.c Similar to drivers/gpu/drm/solomon/ssd130x.c. Probably the one was copied to the other given that it seems to handle the same hardware.
So all consumers using pwm_init_state() either don't use the .enabled value returned by pwm_init_state() or at least shouldn't do that.
Best regards Uwe
-- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [unhandled content-type:application/pgp-signature] | |