On Thu, Oct 17, 2019 at 8:05 AM Thierry Reding thierry.reding@gmail.com wrote:
On Thu, Oct 17, 2019 at 07:40:47AM -0500, Adam Ford wrote:
On Thu, Oct 17, 2019 at 7:34 AM Adam Ford aford173@gmail.com wrote:
On Thu, Oct 17, 2019 at 7:19 AM Uwe Kleine-König u.kleine-koenig@pengutronix.de wrote:
On Thu, Oct 17, 2019 at 12:47:27PM +0100, Daniel Thompson wrote:
On Thu, Oct 17, 2019 at 10:10:59AM +0200, Uwe Kleine-König wrote:
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.
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.
I may have spoken too soon. The patch fixes the display in that it comes on when it previously did not, but changes to brightness do not appear to do anything anymore. I don't have an oscilloscope where I am now, so I cannot verify whether or not the PWM duty cycle changes.
To my eye, the following do not appear to change the brightness of the screen: echo 7 > /sys/devices/soc0/backlight-lvds/backlight/backlight-lvds/brightness echo 2 > /sys/devices/soc0/backlight-lvds/backlight/backlight-lvds/brightness echo 5 > /sys/devices/soc0/backlight-lvds/backlight/backlight-lvds/brightness
Hi Adam,
can you try the i.MX PWM patch that I posted earlier instead? I really think we need to fix this in the PWM drivers because they are broken. pwm-backlight is not. -rc3 is really not a time to work around breakage in consumers.
I did try your patch, but it did not appear to make any difference on my i.MX6Q.
If my patch doesn't help, can you try reverting the offending patch? If we can't come up with a good fix for the drivers, reverting is the next best option.
I am able to get an image after reverting the offending patch. I did not try playing with brightness levels after reverting.
Thierry