From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-16.7 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_GIT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5C8CEC433B4 for ; Sat, 1 May 2021 16:09:49 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 2BA47613EF for ; Sat, 1 May 2021 16:09:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230391AbhEAQKi (ORCPT ); Sat, 1 May 2021 12:10:38 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42244 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230195AbhEAQKi (ORCPT ); Sat, 1 May 2021 12:10:38 -0400 Received: from metis.ext.pengutronix.de (metis.ext.pengutronix.de [IPv6:2001:67c:670:201:290:27ff:fe1d:cc33]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 25041C06174A for ; Sat, 1 May 2021 09:09:48 -0700 (PDT) Received: from ptx.hi.pengutronix.de ([2001:67c:670:100:1d::c0]) by metis.ext.pengutronix.de with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1lcsBe-0002jb-49; Sat, 01 May 2021 18:09:46 +0200 Received: from ukl by ptx.hi.pengutronix.de with local (Exim 4.92) (envelope-from ) id 1lcsBd-0007ut-Aw; Sat, 01 May 2021 18:09:45 +0200 From: =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?= To: Thierry Reding , Lee Jones Cc: linux-pwm@vger.kernel.org, kernel@pengutronix.de Subject: [PATCH v2] pwm: Ensure for legacy drivers that pwm->state stays consistent Date: Sat, 1 May 2021 18:09:43 +0200 Message-Id: <20210501160943.108821-1-u.kleine-koenig@pengutronix.de> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20210411160451.1207799-1-u.kleine-koenig@pengutronix.de> References: <20210411160451.1207799-1-u.kleine-koenig@pengutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::c0 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 Precedence: bulk List-ID: X-Mailing-List: linux-pwm@vger.kernel.org Without this change it can happen that if changing the polarity succeeded but changing duty_cycle and period failed pwm->state contains a mixture between the old and the requested state. So remember the initial state before starting to modify the configuration and restore it when one of the required callback fails. Compared to the previous implementation .disable() (if necessary) is called earlier to prevent a glitch. Signed-off-by: Uwe Kleine-König --- Hello, just a small optimisation: At the end of pwm_apply_legacy() state->enabled is known to be true, so simplify if (state->enabled && !pwm->state.enabled) { to if (!pwm->state.enabled) { Best regards Uwe drivers/pwm/core.c | 139 +++++++++++++++++++++++++-------------------- 1 file changed, 78 insertions(+), 61 deletions(-) diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index c4d5c0667137..57105deafb55 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -535,6 +535,71 @@ static void pwm_apply_state_debug(struct pwm_device *pwm, } } +static int pwm_apply_legacy(struct pwm_chip *chip, struct pwm_device *pwm, + const struct pwm_state *state) +{ + int err; + struct pwm_state initial_state = pwm->state; + + if (state->polarity != pwm->state.polarity) { + if (!chip->ops->set_polarity) { + err = -EINVAL; + goto out_err; + } + + /* + * Changing the polarity of a running PWM is only allowed when + * the PWM driver implements ->apply(). + */ + if (pwm->state.enabled) { + chip->ops->disable(chip, pwm); + + /* + * Update pwm->state already here in case + * .set_polarity() or another callback depend on that. + */ + pwm->state.enabled = false; + } + + err = chip->ops->set_polarity(chip, pwm, + state->polarity); + if (err) + goto out_err; + + pwm->state.polarity = state->polarity; + } + + if (!state->enabled) { + if (pwm->state.enabled) + chip->ops->disable(chip, pwm); + return 0; + } + + if (state->period != pwm->state.period || + state->duty_cycle != pwm->state.duty_cycle) { + err = chip->ops->config(pwm->chip, pwm, + state->duty_cycle, + state->period); + if (err) + goto out_err; + + pwm->state.period = state->period; + pwm->state.duty_cycle = state->duty_cycle; + } + + if (!pwm->state.enabled) { + err = chip->ops->enable(chip, pwm); + if (err) + goto out_err; + } + + return 0; + +out_err: + pwm->state = initial_state; + return err; +} + /** * pwm_apply_state() - atomically apply a new state to a PWM device * @pwm: PWM device @@ -544,6 +609,8 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state) { struct pwm_chip *chip; int err; + int (*apply)(struct pwm_chip *chip, struct pwm_device *pwm, + const struct pwm_state *state); if (!pwm || !state || !state->period || state->duty_cycle > state->period) @@ -557,70 +624,20 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state) state->enabled == pwm->state.enabled) return 0; - if (chip->ops->apply) { - err = chip->ops->apply(chip, pwm, state); - if (err) - return err; - - trace_pwm_apply(pwm, state); - - pwm->state = *state; - - /* - * only do this after pwm->state was applied as some - * implementations of .get_state depend on this - */ - pwm_apply_state_debug(pwm, state); - } else { - /* - * FIXME: restore the initial state in case of error. - */ - if (state->polarity != pwm->state.polarity) { - if (!chip->ops->set_polarity) - return -EINVAL; - - /* - * Changing the polarity of a running PWM is - * only allowed when the PWM driver implements - * ->apply(). - */ - if (pwm->state.enabled) { - chip->ops->disable(chip, pwm); - pwm->state.enabled = false; - } + apply = chip->ops->apply ?: pwm_apply_legacy; + err = apply(chip, pwm, state); + if (err) + return err; - err = chip->ops->set_polarity(chip, pwm, - state->polarity); - if (err) - return err; + trace_pwm_apply(pwm, state); - pwm->state.polarity = state->polarity; - } - - if (state->period != pwm->state.period || - state->duty_cycle != pwm->state.duty_cycle) { - err = chip->ops->config(pwm->chip, pwm, - state->duty_cycle, - state->period); - if (err) - return err; + pwm->state = *state; - pwm->state.duty_cycle = state->duty_cycle; - pwm->state.period = state->period; - } - - if (state->enabled != pwm->state.enabled) { - if (state->enabled) { - err = chip->ops->enable(chip, pwm); - if (err) - return err; - } else { - chip->ops->disable(chip, pwm); - } - - pwm->state.enabled = state->enabled; - } - } + /* + * only do this after pwm->state was applied as some + * implementations of .get_state depend on this + */ + pwm_apply_state_debug(pwm, state); return 0; } -- 2.30.2