RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)

Message ID 20180806155129.cjcc7okmwtaujf43@pengutronix.de
State New
Headers show
Series
  • RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)
Related show

Commit Message

Uwe Kleine-König Aug. 6, 2018, 3:51 p.m.
Hello Thierry,

(If you have a déjà vu: Yes, I tried to argue this case already in the
past, it's just that I'm just faced with a new use case that's broken
with the current state.)

Currently the idiom

	pwm_config(mypwm, value, period);
	if (!value)
		pwm_disable(mypwm);
	else
		pwm_enable(mypwm);

(and it's equivalent:

	state.duty_cycle = ...
	state.enable = state.duty_cycle ? true : false;
	pwm_apply_state(...);

) is quite present in the kernel.

In my eyes this is bad for two reasons:

 a) if the caller is supposed to call pwm_disable() after configuring the
    duty cycle to zero, then why doesn't pwm_config() contains this to
    unburden pwm-API users and so reduce open coding this assumption?

 b) it is actually wrong in at least two cases:
    - On i.MX28 pwm_config(mypwm, 0, period) only programs a register
      and only after the current period is elapsed this is clocked into
      the hardware. So it is very likely that when pwm_disable() is
      called the period is still not over, and then the clk is disabled
      and the period never ends resulting in the pwm pin being frozen in
      whatever state it happend to be when the clk was stopped.
    - on i.MX6 if a pwm is inverted pwm_config(mypwm, 0, period)
      correctly ensures the PWM pin to stay at 1. But then pwm_disable()
      puts it to 0. In the current case this implies that the backlight
      is fully enabled instead of off.

Both issues are hard (or at least ugly) to fix. The IMHO most sane
approach is to not let PWM-API-users care about power saving when they
still care about the output level and update the samantic of
pwm_disable() (and struct pwm_state::enabled == 0) to mean: "Stop
toggling, I don't care about the level of the output." in contrast to
the current semantic "Stop toggling. If pwm_config(mypwm, 0, period) was
called before, stop the pin at (maybe inverted) low level (for most pwms
at least).".

This is in my eyes more clear, removes code duplication and power saving
can (and should) be implemented in the pwm backend drivers that have the
domain specific knowledge about the effects of pwm_disable and
eventually unwanted side effects when duty cycle is 0 and just do the
corresponding action if its safe. We could even put something like:

	if (pwm->flags & PWM_AUTO_DISABLE && pwm->period == 0)
		pwm_disable(pwm);

in pwm_config to help backend driver authors of "sane" PWMs.

I grepped in the kernel for callers of pwm_disable and adapted them for
this. I didn't verify all changes are correct, so take with a grain of
salt. The only fishy one is drivers/gpu/drm/i915/intel_panel.c in my
eyes, I'm not sure about drivers/pwm/pwm-lpc18xx-sct.c.

See below for my wip changes.

Looking forward to your comments.

Best regards
Uwe

Comments

Thierry Reding Aug. 9, 2018, 11:30 a.m. | #1
On Mon, Aug 06, 2018 at 05:51:29PM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> (If you have a déjà vu: Yes, I tried to argue this case already in the
> past, it's just that I'm just faced with a new use case that's broken
> with the current state.)
> 
> Currently the idiom
> 
> 	pwm_config(mypwm, value, period);
> 	if (!value)
> 		pwm_disable(mypwm);
> 	else
> 		pwm_enable(mypwm);
> 
> (and it's equivalent:
> 
> 	state.duty_cycle = ...
> 	state.enable = state.duty_cycle ? true : false;
> 	pwm_apply_state(...);
> 
> ) is quite present in the kernel.
> 
> In my eyes this is bad for two reasons:
> 
>  a) if the caller is supposed to call pwm_disable() after configuring the
>     duty cycle to zero, then why doesn't pwm_config() contains this to
>     unburden pwm-API users and so reduce open coding this assumption?

That's because it's not true in all cases. pwm_disable() is not
equivalent to setting the duty cycle to zero. Many users don't care
about the difference, but that doesn't mean it should be assumed to
be always the case.

>  b) it is actually wrong in at least two cases:
>     - On i.MX28 pwm_config(mypwm, 0, period) only programs a register
>       and only after the current period is elapsed this is clocked into
>       the hardware. So it is very likely that when pwm_disable() is
>       called the period is still not over, and then the clk is disabled
>       and the period never ends resulting in the pwm pin being frozen in
>       whatever state it happend to be when the clk was stopped.

So that's a driver bug then. By definition, after the call to
pwm_config() completes, the hardware should be in the configured state.
If a device doesn't allow the programming to happen immediately, then it
is up to the driver to ensure it waits long enough for the settings to
have been properly applied.

>     - on i.MX6 if a pwm is inverted pwm_config(mypwm, 0, period)
>       correctly ensures the PWM pin to stay at 1. But then pwm_disable()
>       puts it to 0. In the current case this implies that the backlight
>       is fully enabled instead of off.

Again, that's a driver bug. pwm_disable() isn't supposed to change the
pin polarity. It's not supposed to change the pin level.

> Both issues are hard (or at least ugly) to fix. The IMHO most sane
> approach is to not let PWM-API-users care about power saving when they
> still care about the output level and update the samantic of
> pwm_disable() (and struct pwm_state::enabled == 0) to mean: "Stop
> toggling, I don't care about the level of the output." in contrast to
> the current semantic "Stop toggling. If pwm_config(mypwm, 0, period) was
> called before, stop the pin at (maybe inverted) low level (for most pwms
> at least).".

I don't see how you could make that work. In practically all cases a
pwm_disable() would have to be assumed to cause the pin level to become
undefined. That makes it pretty much useless.

> This is in my eyes more clear, removes code duplication and power saving
> can (and should) be implemented in the pwm backend drivers that have the
> domain specific knowledge about the effects of pwm_disable and
> eventually unwanted side effects when duty cycle is 0 and just do the
> corresponding action if its safe. We could even put something like:
> 
> 	if (pwm->flags & PWM_AUTO_DISABLE && pwm->period == 0)
> 		pwm_disable(pwm);
> 
> in pwm_config to help backend driver authors of "sane" PWMs.

I agree that this is simpler and clearer. However it also significantly
reduces the flexibility of the API, and I'm not sure that it is enough.
Again, yes, for many cases this is sufficient, but it doesn't fully
expose the capabilities of hardware.

Thierry
Uwe Kleine-König Aug. 9, 2018, 3:23 p.m. | #2
Hello Thierry,

On Thu, Aug 09, 2018 at 01:30:54PM +0200, Thierry Reding wrote:
> On Mon, Aug 06, 2018 at 05:51:29PM +0200, Uwe Kleine-König wrote:
> > (If you have a déjà vu: Yes, I tried to argue this case already in the
> > past, it's just that I'm just faced with a new use case that's broken
> > with the current state.)
> > 
> > Currently the idiom
> > 
> > 	pwm_config(mypwm, value, period);
> > 	if (!value)
> > 		pwm_disable(mypwm);
> > 	else
> > 		pwm_enable(mypwm);
> > 
> > (and it's equivalent:
> > 
> > 	state.duty_cycle = ...
> > 	state.enable = state.duty_cycle ? true : false;
> > 	pwm_apply_state(...);
> > 
> > ) is quite present in the kernel.
> > 
> > In my eyes this is bad for two reasons:
> > 
> >  a) if the caller is supposed to call pwm_disable() after configuring the
> >     duty cycle to zero, then why doesn't pwm_config() contains this to
> >     unburden pwm-API users and so reduce open coding this assumption?
> 
> That's because it's not true in all cases. pwm_disable() is not
> equivalent to setting the duty cycle to zero. Many users don't care
> about the difference, but that doesn't mean it should be assumed to
> be always the case.

I cannot follow you. You're saying that pwm_disable() is not the same as
duty-cycle := 0. That's different from what I said. I only talked about
pwm_disable() after config(pwm, duty=0, period).

Actually the current semantic of pwm_disable isn't clear to me. Is it:
"Freeze the pin in the current state"? If so, this isn't possible to
implement for some hardware I know. If it's like that, the pin state is
undefined after pwm_disable unless the duty-cycle was 0% or 100%
before, right?

> >  b) it is actually wrong in at least two cases:
> >     - On i.MX28 pwm_config(mypwm, 0, period) only programs a register
> >       and only after the current period is elapsed this is clocked into
> >       the hardware. So it is very likely that when pwm_disable() is
> >       called the period is still not over, and then the clk is disabled
> >       and the period never ends resulting in the pwm pin being frozen in
> >       whatever state it happend to be when the clk was stopped.
> 
> So that's a driver bug then. By definition, after the call to
> pwm_config() completes, the hardware should be in the configured state.
> If a device doesn't allow the programming to happen immediately, then it
> is up to the driver to ensure it waits long enough for the settings to
> have been properly applied.

Unfortunately there is no way (that I know of at least) to notice when a
period is over, so only an unconditional delay/sleep is the way out
here.
 
> >     - on i.MX6 if a pwm is inverted pwm_config(mypwm, 0, period)
> >       correctly ensures the PWM pin to stay at 1. But then pwm_disable()
> >       puts it to 0. In the current case this implies that the backlight
> >       is fully enabled instead of off.
> 
> Again, that's a driver bug. pwm_disable() isn't supposed to change the
> pin polarity. It's not supposed to change the pin level.

So the disable callback must look as follows:

	def imx_pwm_disable(pwm):
	    state = pwm_get_state(...)
	    if state.polarity == PWM_POLARITY_INVERSED:
	        return

	    hw_access_to_disable_pwm()

(Alternatively the pin could be configured to gpio, the right level set
and the hw disable unconditionally.)

And actually because for an inverted polarity after setting the
duty_cycle to 100% it is possible to already disable the hardware in the
config callback. And for the users that config for duty=0 but don't call
pwm_disable() the symmetric case could be handled, too, so the config
callback optimally looks like:

	def imx_pwm_config(chip, duty, period):
	    state = pwm_get_state(...)
	    if (duty == 0 and not state.polarity == PWM_POLARITY_INVERSED) ||
	       (duty == period and state.polarity == PWM_POLARITY_INVERSED):
	        hw_access_to_disable_pwm()
	    else:
	        hw_access_to_config(duty, period)

The obvious downsides are:

  a) if pwm_disable() was called because the user doesn't need the pwm to
     toggle any more, it stays enabled.
  b) pwm_disable() after pwm_config() that allows to disable the
     hardware, is a noop

So it would be much saner from the backend implementations POV if the
PWM-API user doesn't call pwm_disable() if he cares about the output
level of the pin. And I cannot imagine any pwm implementation where this
requirement would make the implementation worse.

Unless I miss something currently there is no user-visible effect of
calling pwm_disable after duty was set to 0. So as an API-user I cannot
differentiate between

 - set the output to low; and
 - don't care about the pwm pin state, turn off and save power

All downsides could be fixed if the only way to set the output to low
would be

	pwm_config(pwm, 0, period);

and pwm_disable() would imply to turn off with no guarantees about the pin
state.

This would simplify some drivers but not complicate any.

This would make the API more expressive because it allows to
differentiate between "output low" and "power off".

The only downside I see is that all users must be reviewed and
eventually fixed for the slightly different semantic. Parts of that was
already done in my patch in the previous mail.

> > Both issues are hard (or at least ugly) to fix. The IMHO most sane
> > approach is to not let PWM-API-users care about power saving when they
> > still care about the output level and update the samantic of
> > pwm_disable() (and struct pwm_state::enabled == 0) to mean: "Stop
> > toggling, I don't care about the level of the output." in contrast to
> > the current semantic "Stop toggling. If pwm_config(mypwm, 0, period) was
> > called before, stop the pin at (maybe inverted) low level (for most pwms
> > at least).".
> 
> I don't see how you could make that work. In practically all cases a
> pwm_disable() would have to be assumed to cause the pin level to become
> undefined. That makes it pretty much useless.

Right, that's what I want. pwm_disable() should make no guarantees about
the pin level. So only call it if you don't care about the pin state. If
you care, only do

	pwm_config(pwm, duty=0, period);

and let the backend driver save power if it possible already in the
corresponding callback.

That is actually easier and more expressive! Currently the semantic of
pwm_disable() is ambiguous. The two possible meanings are:

   - save power and keep the pin state
   - save power

and the backend driver cannot know which one the caller actually wants.
So the only way is to assume the stronger one and keep the pin state
which results in more power usage for some hardware.

> > This is in my eyes more clear, removes code duplication and power saving
> > can (and should) be implemented in the pwm backend drivers that have the
> > domain specific knowledge about the effects of pwm_disable and
> > eventually unwanted side effects when duty cycle is 0 and just do the
> > corresponding action if its safe. We could even put something like:
> > 
> > 	if (pwm->flags & PWM_AUTO_DISABLE && pwm->period == 0)
> > 		pwm_disable(pwm);
> > 
> > in pwm_config to help backend driver authors of "sane" PWMs.
> 
> I agree that this is simpler and clearer. However it also significantly
> reduces the flexibility of the API, and I'm not sure that it is enough.
> Again, yes, for many cases this is sufficient, but it doesn't fully
> expose the capabilities of hardware.

Can you show me a use case that isn't possible to express with the
suggested change in semantic?

Best regards
Uwe
Uwe Kleine-König Aug. 20, 2018, 9:52 a.m. | #3
Hello Thierry,

On Thu, Aug 09, 2018 at 05:23:30PM +0200, Uwe Kleine-König wrote:
> On Thu, Aug 09, 2018 at 01:30:54PM +0200, Thierry Reding wrote:
> > I don't see how you could make that work. In practically all cases a
> > pwm_disable() would have to be assumed to cause the pin level to become
> > undefined. That makes it pretty much useless.
> 
> Right, that's what I want. pwm_disable() should make no guarantees about
> the pin level. [...]
>
> > I agree that this is simpler and clearer. However it also significantly
> > reduces the flexibility of the API, and I'm not sure that it is enough.
> > Again, yes, for many cases this is sufficient, but it doesn't fully
> > expose the capabilities of hardware.
> 
> Can you show me a use case that isn't possible to express with the
> suggested change in semantic?

To get forward here the only missing points are:

 a) Your claim that this simplification looses expressive power.
 b) Actually convert the users (assuming a) can be resolved)

I cannot find a usecase that cannot be handled with the suggested change
in semantic. So can I lure you in explaining what setup you have in
mind?

Best regards
Uwe

Patch

diff --git a/arch/arm/mach-s3c24xx/mach-rx1950.c b/arch/arm/mach-s3c24xx/mach-rx1950.c
index 7f5a18fa305b..55db3b63add8 100644
--- a/arch/arm/mach-s3c24xx/mach-rx1950.c
+++ b/arch/arm/mach-s3c24xx/mach-rx1950.c
@@ -429,7 +429,6 @@  static void rx1950_lcd_power(int enable)
 		/* GPB1->OUTPUT, GPB1->0 */
 		gpio_direction_output(S3C2410_GPB(1), 0);
 		pwm_config(lcd_pwm, 0, LCD_PWM_PERIOD);
-		pwm_disable(lcd_pwm);
 
 		/* GPC0->0, GPC10->0 */
 		gpio_direction_output(S3C2410_GPC(0), 0);
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index b443278e569c..a2b0e5f74ac8 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -784,8 +784,6 @@  static void pwm_disable_backlight(const struct drm_connector_state *old_conn_sta
 
 	/* Disable the backlight */
 	pwm_config(panel->backlight.pwm, 0, CRC_PMIC_PWM_PERIOD_NS);
-	usleep_range(2000, 3000);
-	pwm_disable(panel->backlight.pwm);
 }
 
 void intel_panel_disable_backlight(const struct drm_connector_state *old_conn_state)
diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index 7838af58f92d..0f1b597bb252 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -51,7 +51,7 @@  static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
 	pwm_init_state(ctx->pwm, &state);
 	period = ctx->pwm->args.period;
 	state.duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
-	state.enabled = pwm ? true : false;
+	state.enabled = true;
 
 	ret = pwm_apply_state(ctx->pwm, &state);
 	if (!ret)
@@ -244,8 +244,7 @@  static int pwm_fan_probe(struct platform_device *pdev)
 						       ctx, pwm_fan_groups);
 	if (IS_ERR(hwmon)) {
 		dev_err(&pdev->dev, "Failed to register hwmon device\n");
-		ret = PTR_ERR(hwmon);
-		goto err_pwm_disable;
+		return PTR_ERR(hwmon);
 	}
 
 	ret = pwm_fan_of_get_cooling_data(&pdev->dev, ctx);
@@ -260,20 +259,13 @@  static int pwm_fan_probe(struct platform_device *pdev)
 		if (IS_ERR(cdev)) {
 			dev_err(&pdev->dev,
 				"Failed to register pwm-fan as cooling device");
-			ret = PTR_ERR(cdev);
-			goto err_pwm_disable;
+			return PTR_ERR(cdev);
 		}
 		ctx->cdev = cdev;
 		thermal_cdev_update(cdev);
 	}
 
 	return 0;
-
-err_pwm_disable:
-	state.enabled = false;
-	pwm_apply_state(ctx->pwm, &state);
-
-	return ret;
 }
 
 static int pwm_fan_remove(struct platform_device *pdev)
diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index df80c89ebe7f..755e13721a16 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -40,11 +40,6 @@  static void __led_pwm_set(struct led_pwm_data *led_dat)
 	int new_duty = led_dat->duty;
 
 	pwm_config(led_dat->pwm, new_duty, led_dat->period);
-
-	if (new_duty == 0)
-		pwm_disable(led_dat->pwm);
-	else
-		pwm_enable(led_dat->pwm);
 }
 
 static int led_pwm_set(struct led_classdev *led_cdev,
@@ -105,6 +100,8 @@  static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
 		return ret;
 	}
 
+	pwm_enable(led_data->pwm);
+
 	led_data->cdev.brightness_set_blocking = led_pwm_set;
 
 	/*

(This should better make use of pwm_state instead.)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 1581f6ab1b1f..38f17c8b6364 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -864,6 +864,8 @@  void pwm_put(struct pwm_device *pwm)
 	if (!pwm)
 		return;
 
+	pwm_disable(pwm);
+
 	mutex_lock(&pwm_lock);
 
 	if (!test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
diff --git a/drivers/pwm/pwm-lpc18xx-sct.c b/drivers/pwm/pwm-lpc18xx-sct.c
index d7f5f7de030d..61125052ac3d 100644
--- a/drivers/pwm/pwm-lpc18xx-sct.c
+++ b/drivers/pwm/pwm-lpc18xx-sct.c
@@ -306,8 +306,6 @@  static void lpc18xx_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 	struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
 	struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
 
-	pwm_disable(pwm);
-	pwm_set_duty_cycle(pwm, 0);
 	clear_bit(lpc18xx_data->duty_event, &lpc18xx_pwm->event_map);
 }
 
diff --git a/drivers/video/backlight/lm3630a_bl.c b/drivers/video/backlight/lm3630a_bl.c
index 2030a6b77a09..293abb823fde 100644
--- a/drivers/video/backlight/lm3630a_bl.c
+++ b/drivers/video/backlight/lm3630a_bl.c
@@ -165,12 +165,15 @@  static void lm3630a_pwm_ctrl(struct lm3630a_chip *pchip, int br, int br_max)
 {
 	unsigned int period = pchip->pdata->pwm_period;
 	unsigned int duty = br * period / br_max;
+	struct pwm_state state;
 
-	pwm_config(pchip->pwmd, duty, period);
-	if (duty)
-		pwm_enable(pchip->pwmd);
-	else
-		pwm_disable(pchip->pwmd);
+	pwm_get_state(pchip->pwmd);
+
+	state.duty_cycle = duty;
+	state.period = period;
+	state.enabled = true;
+
+	pwm_apply_state(pchip->pwmd, &state);
 }
 
 /* update and get brightness */
diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c
index 73612485ed07..ba4e71b99c45 100644
--- a/drivers/video/backlight/lp855x_bl.c
+++ b/drivers/video/backlight/lp855x_bl.c
@@ -240,6 +240,7 @@  static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
 	unsigned int period = lp->pdata->period_ns;
 	unsigned int duty = br * period / max_br;
 	struct pwm_device *pwm;
+	struct pwm_state state;
 
 	/* request pwm device with the consumer name */
 	if (!lp->pwm) {
@@ -248,19 +249,15 @@  static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
 			return;
 
 		lp->pwm = pwm;
-
-		/*
-		 * FIXME: pwm_apply_args() should be removed when switching to
-		 * the atomic PWM API.
-		 */
-		pwm_apply_args(pwm);
 	}
 
-	pwm_config(lp->pwm, duty, period);
-	if (duty)
-		pwm_enable(lp->pwm);
-	else
-		pwm_disable(lp->pwm);
+	pwm_get_state(lp->pwm, &state);
+
+	state.duty = duty;
+	state.period = period;
+	state.enabled = true;
+
+	pwm_apply_state(lp->pwm, &state);
 }
 
 static int lp855x_bl_update_status(struct backlight_device *bl)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 44ac5bde4e9d..e60831f3e29a 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -46,18 +46,42 @@  struct pwm_bl_data {
 	void			(*exit)(struct device *);
 };
 
+static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness)
+{
+	unsigned int lth = pb->lth_brightness;
+	u64 duty_cycle;
+
+	if (pb->levels)
+		duty_cycle = pb->levels[brightness];
+	else
+		duty_cycle = brightness;
+
+	duty_cycle *= pb->period - lth;
+	do_div(duty_cycle, pb->scale);
+
+	return duty_cycle + lth;
+}
+
 static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
 {
 	int err;
+	int duty_cycle;
+	struct pwm_state state;
 
 	if (pb->enabled)
 		return;
 
+	duty_cycle = compute_duty_cycle(pb, brightness);
+
 	err = regulator_enable(pb->power_supply);
 	if (err < 0)
 		dev_err(pb->dev, "failed to enable power supply\n");
 
-	pwm_enable(pb->pwm);
+	pwm_get_state(pb->pwm, &state);
+	state.duty_cycle = duty_cycle;
+	state.period = pb->period;
+	state.enabled = true;
+	pwm_apply_state(pb->pwm, &state);
 
 	if (pb->post_pwm_on_delay)
 		msleep(pb->post_pwm_on_delay);
@@ -80,33 +104,15 @@  static void pwm_backlight_power_off(struct pwm_bl_data *pb)
 		msleep(pb->pwm_off_delay);
 
 	pwm_config(pb->pwm, 0, pb->period);
-	pwm_disable(pb->pwm);
 
 	regulator_disable(pb->power_supply);
 	pb->enabled = false;
 }
 
-static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness)
-{
-	unsigned int lth = pb->lth_brightness;
-	u64 duty_cycle;
-
-	if (pb->levels)
-		duty_cycle = pb->levels[brightness];
-	else
-		duty_cycle = brightness;
-
-	duty_cycle *= pb->period - lth;
-	do_div(duty_cycle, pb->scale);
-
-	return duty_cycle + lth;
-}
-
 static int pwm_backlight_update_status(struct backlight_device *bl)
 {
 	struct pwm_bl_data *pb = bl_get_data(bl);
 	int brightness = bl->props.brightness;
-	int duty_cycle;
 
 	if (bl->props.power != FB_BLANK_UNBLANK ||
 	    bl->props.fb_blank != FB_BLANK_UNBLANK ||
@@ -116,11 +122,9 @@  static int pwm_backlight_update_status(struct backlight_device *bl)
 	if (pb->notify)
 		brightness = pb->notify(pb->dev, brightness);
 
-	if (brightness > 0) {
-		duty_cycle = compute_duty_cycle(pb, brightness);
-		pwm_config(pb->pwm, duty_cycle, pb->period);
+	if (brightness > 0)
 		pwm_backlight_power_on(pb, brightness);
-	} else
+	else
 		pwm_backlight_power_off(pb);
 
 	if (pb->notify_after)
@@ -353,12 +357,6 @@  static int pwm_backlight_probe(struct platform_device *pdev)
 
 	dev_dbg(&pdev->dev, "got pwm for backlight\n");
 
-	/*
-	 * FIXME: pwm_apply_args() should be removed when switching to
-	 * the atomic PWM API.
-	 */
-	pwm_apply_args(pb->pwm);
-
 	/*
 	 * The DT case will set the pwm_period_ns field to 0 and store the
 	 * period, parsed from the DT, in the PWM device. For the non-DT case,