From patchwork Mon Aug 6 15:51:29 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Uwe_Kleine-K=C3=B6nig?= X-Patchwork-Id: 953974 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=none (mailfrom) smtp.mailfrom=vger.kernel.org (client-ip=209.132.180.67; helo=vger.kernel.org; envelope-from=linux-pwm-owner@vger.kernel.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=pengutronix.de Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 41khw53Ssqz9s3q for ; Tue, 7 Aug 2018 01:51:33 +1000 (AEST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1732743AbeHFSBO (ORCPT ); Mon, 6 Aug 2018 14:01:14 -0400 Received: from metis.ext.pengutronix.de ([85.220.165.71]:57199 "EHLO metis.ext.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728626AbeHFSBO (ORCPT ); Mon, 6 Aug 2018 14:01:14 -0400 Received: from pty.hi.pengutronix.de ([2001:67c:670:100:1d::c5]) by metis.ext.pengutronix.de with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.89) (envelope-from ) id 1fmhn8-0004ij-GX; Mon, 06 Aug 2018 17:51:30 +0200 Received: from ukl by pty.hi.pengutronix.de with local (Exim 4.89) (envelope-from ) id 1fmhn7-0001P7-Rm; Mon, 06 Aug 2018 17:51:29 +0200 Date: Mon, 6 Aug 2018 17:51:29 +0200 From: Uwe =?iso-8859-1?q?Kleine-K=F6nig?= To: Thierry Reding Cc: linux-pwm@vger.kernel.org, Gavin Schenk , kernel@pengutronix.de Subject: RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period) Message-ID: <20180806155129.cjcc7okmwtaujf43@pengutronix.de> MIME-Version: 1.0 Content-Disposition: inline User-Agent: NeoMutt/20170113 (1.7.2) X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::c5 X-SA-Exim-Mail-From: ukl@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-pwm@vger.kernel.org Sender: linux-pwm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pwm@vger.kernel.org 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 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,