| Message ID | 20181109094857.27079-1-heiko@sntech.de |
|---|---|
| State | Accepted |
| Commit | e4c8ae3e3d6d473bfbfbb5db234792b9f8ddab3b |
| Headers | show |
| Series | backlight: pwm_bl: re-add driver internal enabled tracking | expand |
Hi Heiko, Many thanks to catch and dig into this, I did more tests but I was not able to reproduce the issue with my nfs environment. Anyway the patch looks good to me and fixes the issue for you, so On 9/11/18 10:48, Heiko Stuebner wrote: > Commit e6bcca0890b9 ("backlight: pwm_bl: Switch to using "atomic" PWM API") > removed the driver internal enabled tracking in favor of simply checking > the pwm state. > > This can lead to issues as all of gpio-, regulator- and pwm-state are used > to determine the initial state and the bootloader or kernel can leave them > in an inconsistent state at boot. > > In my case on rk3399-kevin, the pwm backlight is build as module and the > kernel disables the supply regulator as unused while keeping the pwm running > thus pwm_bl calling pwm_backlight_power_off() during probe and creating an > unmatched regulator-disable call, as it never got enabled from the pwm-bl > before. > > To prevent these consistency issues, reintroduce the driver-internal > tracking of the enabled state. > > Fixes: e6bcca0890b9 ("backlight: pwm_bl: Switch to using "atomic" PWM API") > Signed-off-by: Heiko Stuebner <heiko@sntech.de> > --- > drivers/video/backlight/pwm_bl.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > index 678b27063198..bcd08b41765d 100644 > --- a/drivers/video/backlight/pwm_bl.c > +++ b/drivers/video/backlight/pwm_bl.c > @@ -30,6 +30,7 @@ struct pwm_bl_data { > struct device *dev; > unsigned int lth_brightness; > unsigned int *levels; > + bool enabled; > struct regulator *power_supply; > struct gpio_desc *enable_gpio; > unsigned int scale; > @@ -50,7 +51,7 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb) > int err; > > pwm_get_state(pb->pwm, &state); > - if (state.enabled) > + if (pb->enabled) > return; > > err = regulator_enable(pb->power_supply); > @@ -65,6 +66,8 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb) > > if (pb->enable_gpio) > gpiod_set_value_cansleep(pb->enable_gpio, 1); > + > + pb->enabled = true; > } > > static void pwm_backlight_power_off(struct pwm_bl_data *pb) > @@ -72,7 +75,7 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb) > struct pwm_state state; > > pwm_get_state(pb->pwm, &state); > - if (!state.enabled) > + if (!pb->enabled) > return; > > if (pb->enable_gpio) > @@ -86,6 +89,7 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb) > pwm_apply_state(pb->pwm, &state); > > regulator_disable(pb->power_supply); > + pb->enabled = false; > } > > static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness) > @@ -483,6 +487,7 @@ static int pwm_backlight_probe(struct platform_device *pdev) > pb->check_fb = data->check_fb; > pb->exit = data->exit; > pb->dev = &pdev->dev; > + pb->enabled = false; > pb->post_pwm_on_delay = data->post_pwm_on_delay; > pb->pwm_off_delay = data->pwm_off_delay; > > Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Hello Heiko, On Fri, Nov 09, 2018 at 10:48:57AM +0100, Heiko Stuebner wrote: > Commit e6bcca0890b9 ("backlight: pwm_bl: Switch to using "atomic" PWM API") > removed the driver internal enabled tracking in favor of simply checking > the pwm state. > > This can lead to issues as all of gpio-, regulator- and pwm-state are used > to determine the initial state and the bootloader or kernel can leave them > in an inconsistent state at boot. > > In my case on rk3399-kevin, the pwm backlight is build as module and the > kernel disables the supply regulator as unused while keeping the pwm running > thus pwm_bl calling pwm_backlight_power_off() during probe and creating an > unmatched regulator-disable call, as it never got enabled from the pwm-bl > before. > > To prevent these consistency issues, reintroduce the driver-internal > tracking of the enabled state. > > Fixes: e6bcca0890b9 ("backlight: pwm_bl: Switch to using "atomic" PWM API") > Signed-off-by: Heiko Stuebner <heiko@sntech.de> This looks right, Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Best regards Uwe
On Fri, Nov 09, 2018 at 10:48:57AM +0100, Heiko Stuebner wrote: > Commit e6bcca0890b9 ("backlight: pwm_bl: Switch to using "atomic" PWM API") > removed the driver internal enabled tracking in favor of simply checking > the pwm state. > > This can lead to issues as all of gpio-, regulator- and pwm-state are used > to determine the initial state and the bootloader or kernel can leave them > in an inconsistent state at boot. > > In my case on rk3399-kevin, the pwm backlight is build as module and the > kernel disables the supply regulator as unused while keeping the pwm running > thus pwm_bl calling pwm_backlight_power_off() during probe and creating an > unmatched regulator-disable call, as it never got enabled from the pwm-bl > before. > > To prevent these consistency issues, reintroduce the driver-internal > tracking of the enabled state. > > Fixes: e6bcca0890b9 ("backlight: pwm_bl: Switch to using "atomic" PWM API") > Signed-off-by: Heiko Stuebner <heiko@sntech.de> > --- > drivers/video/backlight/pwm_bl.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) Acked-by: Thierry Reding <thierry.reding@gmail.com>
On Fri, Nov 09, 2018 at 10:48:57AM +0100, Heiko Stuebner wrote: > Commit e6bcca0890b9 ("backlight: pwm_bl: Switch to using "atomic" PWM API") > removed the driver internal enabled tracking in favor of simply checking > the pwm state. > > This can lead to issues as all of gpio-, regulator- and pwm-state are used > to determine the initial state and the bootloader or kernel can leave them > in an inconsistent state at boot. > > In my case on rk3399-kevin, the pwm backlight is build as module and the > kernel disables the supply regulator as unused while keeping the pwm running > thus pwm_bl calling pwm_backlight_power_off() during probe and creating an > unmatched regulator-disable call, as it never got enabled from the pwm-bl > before. > > To prevent these consistency issues, reintroduce the driver-internal > tracking of the enabled state. > > Fixes: e6bcca0890b9 ("backlight: pwm_bl: Switch to using "atomic" PWM API") > Signed-off-by: Heiko Stuebner <heiko@sntech.de> Acked-by: Daniel Thompson <daniel.thompson@linaro.org> > --- > drivers/video/backlight/pwm_bl.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > index 678b27063198..bcd08b41765d 100644 > --- a/drivers/video/backlight/pwm_bl.c > +++ b/drivers/video/backlight/pwm_bl.c > @@ -30,6 +30,7 @@ struct pwm_bl_data { > struct device *dev; > unsigned int lth_brightness; > unsigned int *levels; > + bool enabled; > struct regulator *power_supply; > struct gpio_desc *enable_gpio; > unsigned int scale; > @@ -50,7 +51,7 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb) > int err; > > pwm_get_state(pb->pwm, &state); > - if (state.enabled) > + if (pb->enabled) > return; > > err = regulator_enable(pb->power_supply); > @@ -65,6 +66,8 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb) > > if (pb->enable_gpio) > gpiod_set_value_cansleep(pb->enable_gpio, 1); > + > + pb->enabled = true; > } > > static void pwm_backlight_power_off(struct pwm_bl_data *pb) > @@ -72,7 +75,7 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb) > struct pwm_state state; > > pwm_get_state(pb->pwm, &state); > - if (!state.enabled) > + if (!pb->enabled) > return; > > if (pb->enable_gpio) > @@ -86,6 +89,7 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb) > pwm_apply_state(pb->pwm, &state); > > regulator_disable(pb->power_supply); > + pb->enabled = false; > } > > static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness) > @@ -483,6 +487,7 @@ static int pwm_backlight_probe(struct platform_device *pdev) > pb->check_fb = data->check_fb; > pb->exit = data->exit; > pb->dev = &pdev->dev; > + pb->enabled = false; > pb->post_pwm_on_delay = data->post_pwm_on_delay; > pb->pwm_off_delay = data->pwm_off_delay; > > -- > 2.18.0 >
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index 678b27063198..bcd08b41765d 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -30,6 +30,7 @@ struct pwm_bl_data { struct device *dev; unsigned int lth_brightness; unsigned int *levels; + bool enabled; struct regulator *power_supply; struct gpio_desc *enable_gpio; unsigned int scale; @@ -50,7 +51,7 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb) int err; pwm_get_state(pb->pwm, &state); - if (state.enabled) + if (pb->enabled) return; err = regulator_enable(pb->power_supply); @@ -65,6 +66,8 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb) if (pb->enable_gpio) gpiod_set_value_cansleep(pb->enable_gpio, 1); + + pb->enabled = true; } static void pwm_backlight_power_off(struct pwm_bl_data *pb) @@ -72,7 +75,7 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb) struct pwm_state state; pwm_get_state(pb->pwm, &state); - if (!state.enabled) + if (!pb->enabled) return; if (pb->enable_gpio) @@ -86,6 +89,7 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb) pwm_apply_state(pb->pwm, &state); regulator_disable(pb->power_supply); + pb->enabled = false; } static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness) @@ -483,6 +487,7 @@ static int pwm_backlight_probe(struct platform_device *pdev) pb->check_fb = data->check_fb; pb->exit = data->exit; pb->dev = &pdev->dev; + pb->enabled = false; pb->post_pwm_on_delay = data->post_pwm_on_delay; pb->pwm_off_delay = data->pwm_off_delay;
Commit e6bcca0890b9 ("backlight: pwm_bl: Switch to using "atomic" PWM API") removed the driver internal enabled tracking in favor of simply checking the pwm state. This can lead to issues as all of gpio-, regulator- and pwm-state are used to determine the initial state and the bootloader or kernel can leave them in an inconsistent state at boot. In my case on rk3399-kevin, the pwm backlight is build as module and the kernel disables the supply regulator as unused while keeping the pwm running thus pwm_bl calling pwm_backlight_power_off() during probe and creating an unmatched regulator-disable call, as it never got enabled from the pwm-bl before. To prevent these consistency issues, reintroduce the driver-internal tracking of the enabled state. Fixes: e6bcca0890b9 ("backlight: pwm_bl: Switch to using "atomic" PWM API") Signed-off-by: Heiko Stuebner <heiko@sntech.de> --- drivers/video/backlight/pwm_bl.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)