Received: by 2002:a05:6358:1087:b0:cb:c9d3:cd90 with SMTP id j7csp8179486rwi; Tue, 25 Oct 2022 03:36:45 -0700 (PDT) X-Google-Smtp-Source: AMsMyM6wCxmMq1CccHpi84GjtjlA010Yj97lr9jCUWmR/R0ISRnqhAjB1H1M7D7qkKQwkrgYSLDl X-Received: by 2002:a63:5b5c:0:b0:461:8ba9:457e with SMTP id l28-20020a635b5c000000b004618ba9457emr32627812pgm.218.1666694205444; Tue, 25 Oct 2022 03:36:45 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1666694205; cv=none; d=google.com; s=arc-20160816; b=Nn4bDb42pgEGe9PtYIaNOUdPyyPps5UBB7ib+QI67bWhUws4wOzuNL9Z2S/MjE+fbm fXwufVtTH3mCP13KvLt2ohn8akDp/J66tSCe6J3fb29XnF+RDbqBHboWilltBEt1DLAF dOIlfoxZseyiGqph0GgxOfQFO71sTHcW75lHeNWfpCSvrdaZU+vo9mHkFesVRzp+dlkI y/seswZTXtvigIsdrRDzdWOHu3QeTYfeGTO6vtwP4Flgn+OzKmdcEcY00g671pW122jN d3F/plbH2qKfLIvbaciioR0XETeWhjStqJ851/kSMYoRg9hWJPkWeabX06aZ5QXNK6Ht ukjw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:cc:to:subject:from:date :dkim-signature; bh=5szuNODWsyD37i1tjLgoHPKhtfoUcmFUJEZ/3TrxYgU=; b=U0cHIW2UpnndyS7Pib1keLuOutLLYxTWrcxjyf6qZnxIetEV7AM1G8LWxWEV3X++/z Jt2spakp0Pharpsjj6H/6SXwn8dJfUQrSo2+vVlw/Bo7xtAw1NkewbMrswt3mfJ00Iul X7MdK+zFe961Nc1wjEh67g+sLLIJkveJLs0S73xBToLVEpwJxqoIevvbfii0D9uG9k0j rnV+FYIA4/iB6BENfefQ+16Hsx/8fehN+RwFjs8aUgAm84fT2GdLrm/cYOBZNkrLri+R ZeDEMdlOgZY51QTa+FM+LyoyJP9azIPIjIZg4y92vRGw+eYF3lAfZqcfgBpLlKpNWjcN Q8lg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@crapouillou.net header.s=mail header.b="E/87ejWZ"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=crapouillou.net Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id w13-20020a1709027b8d00b0017f577878edsi1961345pll.368.2022.10.25.03.36.28; Tue, 25 Oct 2022 03:36:45 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@crapouillou.net header.s=mail header.b="E/87ejWZ"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=crapouillou.net Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232386AbiJYKRQ (ORCPT + 99 others); Tue, 25 Oct 2022 06:17:16 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54354 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232384AbiJYKQp (ORCPT ); Tue, 25 Oct 2022 06:16:45 -0400 Received: from aposti.net (aposti.net [89.234.176.197]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id BFFBA10EA18; Tue, 25 Oct 2022 03:10:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=crapouillou.net; s=mail; t=1666692656; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=5szuNODWsyD37i1tjLgoHPKhtfoUcmFUJEZ/3TrxYgU=; b=E/87ejWZxZlkW7IiIX4bDVxP2pmhKIQWnE183GBmF4UXaMvFCFsdb7kzupIZH3R1ZZYFtY BPGIcd9ml7iOf5ZA+TXkgjlRHQ9QalWe1Z4rTvJaGFa6asinRwxsms3ESFpy10OB3kNsKh ozpOzW6JBh7zot1w0pdsUmC1wV08hHc= Date: Tue, 25 Oct 2022 11:10:46 +0100 From: Paul Cercueil Subject: Re: [PATCH 2/5] pwm: jz4740: Fix pin level of disabled TCU2 channels, part 2 To: Uwe =?iso-8859-1?q?Kleine-K=F6nig?= Cc: Thierry Reding , od@opendingux.net, linux-pwm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mips@vger.kernel.org, stable@vger.kernel.org Message-Id: In-Reply-To: <20221025064410.brrx5faa4jtwo67b@pengutronix.de> References: <20221024205213.327001-1-paul@crapouillou.net> <20221024205213.327001-3-paul@crapouillou.net> <20221025064410.brrx5faa4jtwo67b@pengutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1; format=flowed Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_PASS,SPF_PASS, URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Le mar. 25 oct. 2022 =E0 08:44:10 +0200, Uwe Kleine-K=F6nig=20 a =E9crit : > On Mon, Oct 24, 2022 at 09:52:10PM +0100, Paul Cercueil wrote: >> After commit a020f22a4ff5 ("pwm: jz4740: Make PWM start with the=20 >> active part"), >> the trick to set duty > period to properly shut down TCU2 channels=20 >> did >> not work anymore, because of the polarity inversion. >>=20 >> Address this issue by restoring the proper polarity before=20 >> disabling the >> channels. >>=20 >> Fixes: a020f22a4ff5 ("pwm: jz4740: Make PWM start with the active=20 >> part") >> Signed-off-by: Paul Cercueil >> Cc: stable@vger.kernel.org >> --- >> drivers/pwm/pwm-jz4740.c | 62=20 >> ++++++++++++++++++++++++++-------------- >> 1 file changed, 40 insertions(+), 22 deletions(-) >>=20 >> diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c >> index 228eb104bf1e..65462a0052af 100644 >> --- a/drivers/pwm/pwm-jz4740.c >> +++ b/drivers/pwm/pwm-jz4740.c >> @@ -97,6 +97,19 @@ static int jz4740_pwm_enable(struct pwm_chip=20 >> *chip, struct pwm_device *pwm) >> return 0; >> } >>=20 >> +static void jz4740_pwm_set_polarity(struct jz4740_pwm_chip *jz, >> + unsigned int hwpwm, >> + enum pwm_polarity polarity) >> +{ >> + unsigned int value =3D 0; >> + >> + if (polarity =3D=3D PWM_POLARITY_INVERSED) >> + value =3D TCU_TCSR_PWM_INITL_HIGH; >> + >> + regmap_update_bits(jz->map, TCU_REG_TCSRc(hwpwm), >> + TCU_TCSR_PWM_INITL_HIGH, value); >> +} >> + >> static void jz4740_pwm_disable(struct pwm_chip *chip, struct=20 >> pwm_device *pwm) >> { >> struct jz4740_pwm_chip *jz =3D to_jz4740(chip); >> @@ -130,6 +143,7 @@ static int jz4740_pwm_apply(struct pwm_chip=20 >> *chip, struct pwm_device *pwm, >> unsigned long long tmp =3D 0xffffull * NSEC_PER_SEC; >> struct clk *clk =3D pwm_get_chip_data(pwm); >> unsigned long period, duty; >> + enum pwm_polarity polarity; >> long rate; >> int err; >>=20 >> @@ -169,6 +183,9 @@ static int jz4740_pwm_apply(struct pwm_chip=20 >> *chip, struct pwm_device *pwm, >> if (duty >=3D period) >> duty =3D period - 1; >>=20 >> + /* Restore regular polarity before disabling the channel. */ >> + jz4740_pwm_set_polarity(jz4740, pwm->hwpwm, state->polarity); >> + >=20 > Does this introduce a glitch? Maybe. But the PWM is shut down before finishing its period anyway, so=20 there was already a glitch. >> jz4740_pwm_disable(chip, pwm); >>=20 >> err =3D clk_set_rate(clk, rate); >> @@ -190,29 +207,30 @@ static int jz4740_pwm_apply(struct pwm_chip=20 >> *chip, struct pwm_device *pwm, >> regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm), >> TCU_TCSR_PWM_SD, TCU_TCSR_PWM_SD); >>=20 >> - /* >> - * Set polarity. >> - * >> - * The PWM starts in inactive state until the internal timer=20 >> reaches the >> - * duty value, then becomes active until the timer reaches the=20 >> period >> - * value. In theory, we should then use (period - duty) as the=20 >> real duty >> - * value, as a high duty value would otherwise result in the PWM=20 >> pin >> - * being inactive most of the time. >> - * >> - * Here, we don't do that, and instead invert the polarity of the=20 >> PWM >> - * when it is active. This trick makes the PWM start with its=20 >> active >> - * state instead of its inactive state. >> - */ >> - if ((state->polarity =3D=3D PWM_POLARITY_NORMAL) ^ state->enabled) >> - regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm), >> - TCU_TCSR_PWM_INITL_HIGH, 0); >> - else >> - regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm), >> - TCU_TCSR_PWM_INITL_HIGH, >> - TCU_TCSR_PWM_INITL_HIGH); >> - >> - if (state->enabled) >> + if (state->enabled) { >> + /* >> + * Set polarity. >> + * >> + * The PWM starts in inactive state until the internal timer >> + * reaches the duty value, then becomes active until the timer >> + * reaches the period value. In theory, we should then use >> + * (period - duty) as the real duty value, as a high duty value >> + * would otherwise result in the PWM pin being inactive most of >> + * the time. >> + * >> + * Here, we don't do that, and instead invert the polarity of >> + * the PWM when it is active. This trick makes the PWM start >> + * with its active state instead of its inactive state. >> + */ >> + if (state->polarity =3D=3D PWM_POLARITY_NORMAL) >> + polarity =3D PWM_POLARITY_INVERSED; >> + else >> + polarity =3D PWM_POLARITY_NORMAL; >> + >> + jz4740_pwm_set_polarity(jz4740, pwm->hwpwm, polarity); >> + >> jz4740_pwm_enable(chip, pwm); >> + } >=20 > Note that for disabled PWMs there is no official guaranty about the=20 > pin > state. So it would be ok (but admittedly not great) to simplify the > driver and accept that the pinstate is active while the PWM is off. > IMHO this is also better than a glitch. >=20 > If a consumer wants the PWM to be in its inactive state, they should > not disable it. Completely disagree. I absolutely do not want the backlight to go full=20 bright mode when the PWM pin is disabled. And disabling the backlight=20 is a thing (for screen blanking and during mode changes). -Paul