Message ID | 20230714190320.308320-2-gnstark@sberdevices.ru |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:a6b2:0:b0:3e4:2afc:c1 with SMTP id c18csp2725353vqm; Fri, 14 Jul 2023 13:05:00 -0700 (PDT) X-Google-Smtp-Source: APBJJlFZlCBfh8GdX5LWtHgrMV/zjBuLcP/GE5pYLyIl+MsG17yVxR4pt6OhgB+O3IY5eYYkRw/Y X-Received: by 2002:a17:903:41cb:b0:1ac:aba5:7885 with SMTP id u11-20020a17090341cb00b001acaba57885mr6205751ple.47.1689365100164; Fri, 14 Jul 2023 13:05:00 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1689365100; cv=none; d=google.com; s=arc-20160816; b=f0AJLUGJiWnhiAj+FQJNPkKrS/1G/lBezb1j4Mpl/k6WWnDhM7hQlCf4QexRYR+YJ0 O8W83QcwLtWkYFtAphBb8pFrgvJjHohbGlNWVwqiqASOHxP+9Fkq9doYeaHaOCGNWqkt SMucoHMiQyxc1eMoczYwAEhGZxpmE3aN7FHyNBr/vAgdEVeYntDn/A8aC06G2asyHGjH /Www/Rtpu2Ej/otzUKmBFptzso2/k4lE+IG/kNehIGVo/8cyqiMvDzrIKpeG8gzYBkLC L7KnpCOjjmzOdLLid3UBxaeAMx415O2gSwv8dcG7wk3Col/FLzAKTNeYKa+tvpSz8n0b 6jKg== 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:date:subject:cc:to:from :dkim-signature:dkim-filter; bh=Fb0dBL6rswv5znabzmWo6BH+sWIBGMpwWD/C6JhcIu4=; fh=VM+ke2h5JKUfvXdNdLjgF4bw6fPLt1uDttKmal/DDbQ=; b=kOxrePWt7dsg5IQZJriNzERe3zn3ePnB+9GX4L72jy0RLEHIWeSBdtWju7nSN2hyIJ fyCkshN2e/PdXths98hAXWVHbiL06VC2vQH4K2s6WJbV6LPzAiGuOuc6FViliynxtR+O yNNIv7uswWBa9MMwxR2qhoAy/IQB53WhBgeL7t/3R6aCTDUwQYVRHxXRv1wKxICEpH32 lCZy3fp7xniRWdi7c0DlhCG0E2vSEk0Lix/MuJQL+lXRGyATIPdx8cBkzV1GcECBfKuD buaGF/MldlIptgmL8v/R+QvKSxVRkO9/cRSBndaI76d/vclY/kDSSpfB0KROifNPtfqQ 8Peg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@sberdevices.ru header.s=mail header.b=Uldy4UkG; 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=QUARANTINE sp=QUARANTINE dis=NONE) header.from=sberdevices.ru Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id lo6-20020a170903434600b001ae5fe35b6esi7177541plb.261.2023.07.14.13.04.47; Fri, 14 Jul 2023 13:04:59 -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=@sberdevices.ru header.s=mail header.b=Uldy4UkG; 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=QUARANTINE sp=QUARANTINE dis=NONE) header.from=sberdevices.ru Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235568AbjGNTDj (ORCPT <rfc822;hadasmailinglist@gmail.com> + 99 others); Fri, 14 Jul 2023 15:03:39 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:59540 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235666AbjGNTDi (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 14 Jul 2023 15:03:38 -0400 Received: from mx1.sberdevices.ru (mx1.sberdevices.ru [37.18.73.165]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 73C0D358C; Fri, 14 Jul 2023 12:03:35 -0700 (PDT) Received: from p-infra-ksmg-sc-msk01 (localhost [127.0.0.1]) by mx1.sberdevices.ru (Postfix) with ESMTP id DAB40100006; Fri, 14 Jul 2023 22:03:32 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.sberdevices.ru DAB40100006 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sberdevices.ru; s=mail; t=1689361412; bh=Fb0dBL6rswv5znabzmWo6BH+sWIBGMpwWD/C6JhcIu4=; h=From:To:Subject:Date:Message-ID:MIME-Version:Content-Type:From; b=Uldy4UkG0hFpwUJ+wHAxNeY38ov1cYZUx+gIPVLrldD+FnP/kQ4c5bcPPjI7yCRi8 /t6bv60dz4CN9hUl/6RqTQ6fujEhxfrIUG1JCfMQDFcooMk9XvsekvlSZi7k0bj/y0 24gLctBZrwL67F7uCxNLVXsAZz1lJGzg/ujCpo9lJAn3ZEC7OYr23Jw7XZG0YHjJzU 87nXW4Y8X7+xSP5z6R0s1Ma5ikVOdVzJ/bsqY0r5wg8mbaFwSxRXYRM5nZsny/nW11 HnjVK2z+xXWon3EfIRnFn73lVSNd9LW/Y4gQkSW2kQ8/dLShsA18/hYCfc/hKAuVhR ryicvlcjGuBfQ== Received: from p-i-exch-sc-m02.sberdevices.ru (p-i-exch-sc-m02.sberdevices.ru [172.16.192.103]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.sberdevices.ru (Postfix) with ESMTPS; Fri, 14 Jul 2023 22:03:32 +0300 (MSK) Received: from localhost.localdomain (100.64.160.123) by p-i-exch-sc-m02.sberdevices.ru (172.16.192.103) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1118.30; Fri, 14 Jul 2023 22:03:32 +0300 From: George Stark <gnstark@sberdevices.ru> To: <jic23@kernel.org>, <lars@metafoo.de>, <neil.armstrong@linaro.org>, <khilman@baylibre.com>, <jbrunet@baylibre.com>, <martin.blumenstingl@googlemail.com>, <andriy.shevchenko@linux.intel.com>, <nuno.sa@analog.com>, <gnstark@sberdevices.ru> CC: <linux-iio@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>, <linux-kernel@vger.kernel.org>, <linux-amlogic@lists.infradead.org>, <kernel@sberdevices.ru> Subject: [PATCH v1 1/1] iio: adc: meson: fix core clock enable/disable moment Date: Fri, 14 Jul 2023 22:03:15 +0300 Message-ID: <20230714190320.308320-2-gnstark@sberdevices.ru> X-Mailer: git-send-email 2.41.0 In-Reply-To: <20230714190320.308320-1-gnstark@sberdevices.ru> References: <20230714190320.308320-1-gnstark@sberdevices.ru> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-Originating-IP: [100.64.160.123] X-ClientProxiedBy: p-i-exch-sc-m01.sberdevices.ru (172.16.192.107) To p-i-exch-sc-m02.sberdevices.ru (172.16.192.103) X-KSMG-Rule-ID: 10 X-KSMG-Message-Action: clean X-KSMG-AntiSpam-Lua-Profiles: 178634 [Jul 14 2023] X-KSMG-AntiSpam-Version: 5.9.59.0 X-KSMG-AntiSpam-Envelope-From: GNStark@sberdevices.ru X-KSMG-AntiSpam-Rate: 0 X-KSMG-AntiSpam-Status: not_detected X-KSMG-AntiSpam-Method: none X-KSMG-AntiSpam-Auth: dkim=none X-KSMG-AntiSpam-Info: LuaCore: 523 523 523027ce26ed1d9067f7a52a4756a876e54db27c, {Tracking_from_domain_doesnt_match_to}, p-i-exch-sc-m02.sberdevices.ru:5.0.1,7.1.1;sberdevices.ru:5.0.1,7.1.1;127.0.0.199:7.1.2;100.64.160.123:7.1.2;d41d8cd98f00b204e9800998ecf8427e.com:7.1.1, FromAlignment: s, {Tracking_white_helo}, ApMailHostAddress: 100.64.160.123 X-MS-Exchange-Organization-SCL: -1 X-KSMG-AntiSpam-Interceptor-Info: scan successful X-KSMG-AntiPhishing: Clean X-KSMG-LinksScanning: Clean X-KSMG-AntiVirus: Kaspersky Secure Mail Gateway, version 2.0.1.6960, bases: 2023/07/14 14:48:00 #21614190 X-KSMG-AntiVirus-Status: Clean, skipped X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_BLOCKED, SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE,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: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1771427166146310373 X-GMAIL-MSGID: 1771427699655504829 |
Series |
iio: adc: meson: fix core clock enable/disable moment
|
|
Commit Message
George Stark
July 14, 2023, 7:03 p.m. UTC
Enable core clock at probe stage and disable it at remove stage.
Core clock is responsible for turning on/off the entire SoC module so
it should be on before the first module register is touched and be off
at very last moment.
Signed-off-by: George Stark <gnstark@sberdevices.ru>
---
drivers/iio/adc/meson_saradc.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
Comments
On Fri, 14 Jul 2023 22:03:15 +0300 George Stark <gnstark@sberdevices.ru> wrote: > Enable core clock at probe stage and disable it at remove stage. > Core clock is responsible for turning on/off the entire SoC module so > it should be on before the first module register is touched and be off > at very last moment. > > Signed-off-by: George Stark <gnstark@sberdevices.ru> This sounds to me like this should perhaps have a fixes tag? Given where it is in the new sequence you can also use devm_clk_get_enabled() though that makes a potential backport of the fix trickier... I'd go with it anyway as it will make this change quite a bit simpler. More comments inline Jonathan > --- > drivers/iio/adc/meson_saradc.c | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) > > diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c > index fe066c9fab83..5a5bb5cc1320 100644 > --- a/drivers/iio/adc/meson_saradc.c > +++ b/drivers/iio/adc/meson_saradc.c > @@ -1055,12 +1055,6 @@ static int meson_sar_adc_hw_enable(struct iio_dev *indio_dev) > goto err_vref; > } > > - ret = clk_prepare_enable(priv->core_clk); > - if (ret) { > - dev_err(dev, "failed to enable core clk\n"); > - goto err_core_clk; > - } > - > regval = FIELD_PREP(MESON_SAR_ADC_REG0_FIFO_CNT_IRQ_MASK, 1); > regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG0, > MESON_SAR_ADC_REG0_FIFO_CNT_IRQ_MASK, regval); > @@ -1087,8 +1081,6 @@ static int meson_sar_adc_hw_enable(struct iio_dev *indio_dev) > regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG3, > MESON_SAR_ADC_REG3_ADC_EN, 0); > meson_sar_adc_set_bandgap(indio_dev, false); > - clk_disable_unprepare(priv->core_clk); > -err_core_clk: > regulator_disable(priv->vref); > err_vref: > meson_sar_adc_unlock(indio_dev); > @@ -1116,8 +1108,6 @@ static void meson_sar_adc_hw_disable(struct iio_dev *indio_dev) > > meson_sar_adc_set_bandgap(indio_dev, false); > > - clk_disable_unprepare(priv->core_clk); > - > regulator_disable(priv->vref); > > if (!ret) > @@ -1420,6 +1410,12 @@ static int meson_sar_adc_probe(struct platform_device *pdev) > ARRAY_SIZE(meson_sar_adc_iio_channels); > } > > + ret = clk_prepare_enable(priv->core_clk); > + if (ret) { > + dev_err(dev, "failed to enable core clk\n"); > + goto err; If clk_prepare_enable() failed, then you shoudl not call clk_disable_unprepare() > + } > + > ret = meson_sar_adc_init(indio_dev); > if (ret) > goto err; > @@ -1445,17 +1441,21 @@ static int meson_sar_adc_probe(struct platform_device *pdev) > err_hw: > meson_sar_adc_hw_disable(indio_dev); > err: > + clk_disable_unprepare(priv->core_clk); Nothing to do with your patch, but this driver previously did some odd mixing of direct returns and going to an empty label which definitely doesn't help make this clear to follow. > return ret; > } > > static int meson_sar_adc_remove(struct platform_device *pdev) > { > struct iio_dev *indio_dev = platform_get_drvdata(pdev); > + struct meson_sar_adc_priv *priv = iio_priv(indio_dev); > > iio_device_unregister(indio_dev); > > meson_sar_adc_hw_disable(indio_dev); > > + clk_disable_unprepare(priv->core_clk); > + > return 0; > } >
Hello Jonathan Thanks for the feedback. On 7/15/23 20:21, Jonathan Cameron wrote: > On Fri, 14 Jul 2023 22:03:15 +0300 > George Stark <gnstark@sberdevices.ru> wrote: > >> Enable core clock at probe stage and disable it at remove stage. >> Core clock is responsible for turning on/off the entire SoC module so >> it should be on before the first module register is touched and be off >> at very last moment. >> >> Signed-off-by: George Stark <gnstark@sberdevices.ru> > > This sounds to me like this should perhaps have a fixes tag? ack > > Given where it is in the new sequence you can also use > devm_clk_get_enabled() though that makes a potential backport of > the fix trickier... > > I'd go with it anyway as it will make this change quite a bit simpler. ack > > More comments inline > > Jonathan > >> --- >> drivers/iio/adc/meson_saradc.c | 20 ++++++++++---------- >> 1 file changed, 10 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c >> index fe066c9fab83..5a5bb5cc1320 100644 >> --- a/drivers/iio/adc/meson_saradc.c >> +++ b/drivers/iio/adc/meson_saradc.c >> @@ -1055,12 +1055,6 @@ static int meson_sar_adc_hw_enable(struct iio_dev *indio_dev) >> goto err_vref; >> } >> >> - ret = clk_prepare_enable(priv->core_clk); >> - if (ret) { >> - dev_err(dev, "failed to enable core clk\n"); >> - goto err_core_clk; >> - } >> - >> regval = FIELD_PREP(MESON_SAR_ADC_REG0_FIFO_CNT_IRQ_MASK, 1); >> regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG0, >> MESON_SAR_ADC_REG0_FIFO_CNT_IRQ_MASK, regval); >> @@ -1087,8 +1081,6 @@ static int meson_sar_adc_hw_enable(struct iio_dev *indio_dev) >> regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG3, >> MESON_SAR_ADC_REG3_ADC_EN, 0); >> meson_sar_adc_set_bandgap(indio_dev, false); >> - clk_disable_unprepare(priv->core_clk); >> -err_core_clk: >> regulator_disable(priv->vref); >> err_vref: >> meson_sar_adc_unlock(indio_dev); >> @@ -1116,8 +1108,6 @@ static void meson_sar_adc_hw_disable(struct iio_dev *indio_dev) >> >> meson_sar_adc_set_bandgap(indio_dev, false); >> >> - clk_disable_unprepare(priv->core_clk); >> - >> regulator_disable(priv->vref); >> >> if (!ret) >> @@ -1420,6 +1410,12 @@ static int meson_sar_adc_probe(struct platform_device *pdev) >> ARRAY_SIZE(meson_sar_adc_iio_channels); >> } >> >> + ret = clk_prepare_enable(priv->core_clk); >> + if (ret) { >> + dev_err(dev, "failed to enable core clk\n"); >> + goto err; > > If clk_prepare_enable() failed, then you shoudl not call clk_disable_unprepare() yes, but with devm_clk_get_enabled we dont't need this line anymore > >> + } >> + >> ret = meson_sar_adc_init(indio_dev); >> if (ret) >> goto err; >> @@ -1445,17 +1441,21 @@ static int meson_sar_adc_probe(struct platform_device *pdev) >> err_hw: >> meson_sar_adc_hw_disable(indio_dev); >> err: >> + clk_disable_unprepare(priv->core_clk); > > Nothing to do with your patch, but this driver previously did some odd mixing > of direct returns and going to an empty label which definitely doesn't help > make this clear to follow. I made additional patch adding log messages to all errors those can break probe stage > >> return ret; >> } >> >> static int meson_sar_adc_remove(struct platform_device *pdev) >> { >> struct iio_dev *indio_dev = platform_get_drvdata(pdev); >> + struct meson_sar_adc_priv *priv = iio_priv(indio_dev); >> >> iio_device_unregister(indio_dev); >> >> meson_sar_adc_hw_disable(indio_dev); >> >> + clk_disable_unprepare(priv->core_clk); >> + >> return 0; >> } >> >
diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c index fe066c9fab83..5a5bb5cc1320 100644 --- a/drivers/iio/adc/meson_saradc.c +++ b/drivers/iio/adc/meson_saradc.c @@ -1055,12 +1055,6 @@ static int meson_sar_adc_hw_enable(struct iio_dev *indio_dev) goto err_vref; } - ret = clk_prepare_enable(priv->core_clk); - if (ret) { - dev_err(dev, "failed to enable core clk\n"); - goto err_core_clk; - } - regval = FIELD_PREP(MESON_SAR_ADC_REG0_FIFO_CNT_IRQ_MASK, 1); regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG0, MESON_SAR_ADC_REG0_FIFO_CNT_IRQ_MASK, regval); @@ -1087,8 +1081,6 @@ static int meson_sar_adc_hw_enable(struct iio_dev *indio_dev) regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG3, MESON_SAR_ADC_REG3_ADC_EN, 0); meson_sar_adc_set_bandgap(indio_dev, false); - clk_disable_unprepare(priv->core_clk); -err_core_clk: regulator_disable(priv->vref); err_vref: meson_sar_adc_unlock(indio_dev); @@ -1116,8 +1108,6 @@ static void meson_sar_adc_hw_disable(struct iio_dev *indio_dev) meson_sar_adc_set_bandgap(indio_dev, false); - clk_disable_unprepare(priv->core_clk); - regulator_disable(priv->vref); if (!ret) @@ -1420,6 +1410,12 @@ static int meson_sar_adc_probe(struct platform_device *pdev) ARRAY_SIZE(meson_sar_adc_iio_channels); } + ret = clk_prepare_enable(priv->core_clk); + if (ret) { + dev_err(dev, "failed to enable core clk\n"); + goto err; + } + ret = meson_sar_adc_init(indio_dev); if (ret) goto err; @@ -1445,17 +1441,21 @@ static int meson_sar_adc_probe(struct platform_device *pdev) err_hw: meson_sar_adc_hw_disable(indio_dev); err: + clk_disable_unprepare(priv->core_clk); return ret; } static int meson_sar_adc_remove(struct platform_device *pdev) { struct iio_dev *indio_dev = platform_get_drvdata(pdev); + struct meson_sar_adc_priv *priv = iio_priv(indio_dev); iio_device_unregister(indio_dev); meson_sar_adc_hw_disable(indio_dev); + clk_disable_unprepare(priv->core_clk); + return 0; }