Message ID | 20230717172821.62827-5-andriy.shevchenko@linux.intel.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:c923:0:b0:3e4:2afc:c1 with SMTP id j3csp1250583vqt; Mon, 17 Jul 2023 10:35:09 -0700 (PDT) X-Google-Smtp-Source: APBJJlFX2Tmtkzj3KBqXkwPoJDoMk17Dhpoj1OlxvEgs0i5UOhpwQbtXkENXICIWzwEPwrJ15Tet X-Received: by 2002:a17:906:5e:b0:994:554b:7f27 with SMTP id 30-20020a170906005e00b00994554b7f27mr8612281ejg.2.1689615309323; Mon, 17 Jul 2023 10:35:09 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1689615309; cv=none; d=google.com; s=arc-20160816; b=Tcwu+4GnppTAGrma3vzhpLOTXiC9WO7wE+86rdHvkcdEUSnUalKYbhiLVJ+Q4NxmNc G48uG+liI+JJoxTn4BpdSiSW44jU8y1DNJGgjovdJK6BHTDKf/rL8Ec4T46uecqVmzGd +JRiph9fnjXgRl8irgPGBjya487hhTuL+UNP7ufGhxd4eUM6kJrASIsvGKYT8Nd90A/+ cwjQ9hRbAIkA73HRrpqUI3JmRRLO8ll/osJST4+out1IyCyPUV8C+8CuXqLVYNgTPRV2 vHEr8w/qs3Hj5C0O5b2BonXVzjUWVic4K+wiyIfvagR0JZkCKHURctSJ8qaUzqxTfj8J Yi6Q== 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; bh=cN+3R5jCreRGIRAs1BjfuyKIyg4Fr0N+HoZMvvavHBs=; fh=BB0GY8bRtFrF5EQ7S2W7nc4ojj3WvWGP74FS8Jyyj9E=; b=mn0gzqblzS5V74q6tsJX99EaZrFUTjuh9Q+48hFMCU28IXQKIRpM+0ZbfRDvS04YHj XNF82lDjX4h4vbARsJNHPKB0uxmRuw6uRolqAgYtVbgXSx0P2RHRX41jUIGC9cIErr0N 7ed1isnN/C/r6itIY17l0iX9E/rBmj2iRmBjMkX/SyCufCn6HXs0ugeyx5qnp/WhyZPt 38ODV9JBtn0cTmUnw5tkCnYqYZp6fLcKw90ZV1i05nOxKTCzZ9lI5Zf8jguUtgY8pZyC t/vWnBldvS22kWrjwNRPaArfDIrLQ1SKnBuw/ONty46p6gBvDxtUhWlXWLX8U1dkNfe1 ekDw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=hJrFayGS; 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=intel.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id u12-20020a1709064acc00b00992e26642ddsi13319083ejt.251.2023.07.17.10.34.43; Mon, 17 Jul 2023 10:35:09 -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=@intel.com header.s=Intel header.b=hJrFayGS; 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=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231463AbjGQRa1 (ORCPT <rfc822;daweilics@gmail.com> + 99 others); Mon, 17 Jul 2023 13:30:27 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46178 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231535AbjGQRaK (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 17 Jul 2023 13:30:10 -0400 Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 75E2519B1; Mon, 17 Jul 2023 10:29:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1689614987; x=1721150987; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=WzIdieUjIIIcjkcN9h2WZt4G3gLcBLgRvdk3paxMv94=; b=hJrFayGSOoqabwbJUBYQrIG77bKHR6P/OfRciOA9rMcFRsWXM1wbqOqN RQbS1ZqGHRs4NH1X9lTJTsNSjSIUQZWf/p3paHZcDcS1X2vLhv1tkuZeh g30k6q8cm8wB6n5K4XHXyhHzOCOFYzUplPVmUm3DlTeFE+97YNZRrCbNy utjJmj4D4sGEP9hPRB7UJ8LJfNm/iNhUjbNYBA0UlDg2UmTHfOycdudVe YlOJO4OFaHQPXeVKqJBbeJqMh0gOrIXb5AHYMzJ4tHWtyasZDn8v3GnJ5 7svAGBzuLsT3hBuUP1ditS/0P5POsuBAz10DZY7FGK98H/+9eJyA6kSgw g==; X-IronPort-AV: E=McAfee;i="6600,9927,10774"; a="350854514" X-IronPort-AV: E=Sophos;i="6.01,211,1684825200"; d="scan'208";a="350854514" Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 Jul 2023 10:28:40 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10774"; a="813426656" X-IronPort-AV: E=Sophos;i="6.01,211,1684825200"; d="scan'208";a="813426656" Received: from black.fi.intel.com ([10.237.72.28]) by FMSMGA003.fm.intel.com with ESMTP; 17 Jul 2023 10:28:33 -0700 Received: by black.fi.intel.com (Postfix, from userid 1003) id 0D2A65FC; Mon, 17 Jul 2023 20:28:40 +0300 (EEST) From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Mika Westerberg <mika.westerberg@linux.intel.com>, Linus Walleij <linus.walleij@linaro.org>, Balsam CHIHI <bchihi@baylibre.com>, Claudiu Beznea <claudiu.beznea@microchip.com>, Geert Uytterhoeven <geert+renesas@glider.be>, Wolfram Sang <wsa+renesas@sang-engineering.com>, Thierry Reding <thierry.reding@gmail.com>, Paul Cercueil <paul@crapouillou.net>, linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-renesas-soc@vger.kernel.org, linux-tegra@vger.kernel.org, linux-pm@vger.kernel.org Cc: Andy Shevchenko <andy@kernel.org>, Sean Wang <sean.wang@kernel.org>, Matthias Brugger <matthias.bgg@gmail.com>, AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>, Andrew Lunn <andrew@lunn.ch>, Gregory Clement <gregory.clement@bootlin.com>, Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>, Ludovic Desroches <ludovic.desroches@microchip.com>, Nicolas Ferre <nicolas.ferre@microchip.com>, Alexandre Belloni <alexandre.belloni@bootlin.com>, Jonathan Hunter <jonathanh@nvidia.com>, "Rafael J. Wysocki" <rafael@kernel.org>, Len Brown <len.brown@intel.com>, Pavel Machek <pavel@ucw.cz> Subject: [PATCH v2 04/10] pinctrl: intel: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper Date: Mon, 17 Jul 2023 20:28:15 +0300 Message-Id: <20230717172821.62827-5-andriy.shevchenko@linux.intel.com> X-Mailer: git-send-email 2.40.0.1.gaa8946217a0b In-Reply-To: <20230717172821.62827-1-andriy.shevchenko@linux.intel.com> References: <20230717172821.62827-1-andriy.shevchenko@linux.intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_EF,SPF_HELO_NONE,SPF_NONE, T_SCC_BODY_TEXT_LINE 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: 1771690062395014120 X-GMAIL-MSGID: 1771690062395014120 |
Series |
pinctrl: Provide NOIRQ PM helper and use it
|
|
Commit Message
Andy Shevchenko
July 17, 2023, 5:28 p.m. UTC
Since pm.h provides a helper for system no-IRQ PM callbacks,
switch the driver to use it instead of open coded variant.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/pinctrl/intel/pinctrl-intel.c | 5 +----
drivers/pinctrl/intel/pinctrl-intel.h | 9 ++-------
2 files changed, 3 insertions(+), 11 deletions(-)
Comments
On Mon, Jul 17, 2023 at 10:02 PM Paul Cercueil <paul@crapouillou.net> wrote: > Le lundi 17 juillet 2023 à 20:28 +0300, Andy Shevchenko a écrit : ... > Unrelated change. OK. ... > So the correct way to update this driver would be to have a > conditionally-exported dev_pm_ops structure: > > EXPORT_GPL_DEV_PM_OPS(intel_pinctrl_pm_ops) = { > NOIRQ_SYSTEM_SLEEP_PM_OPS(intel_pinctrl_suspend_noirq, > intel_pinctrl_resume_noirq), > }; This looks ugly. I didn't know that EXPORT*PM_OPS designed that way, but it seems pm.h in such case needs EXPORT for NOIRQ case as well. > Then your two callbacks can be "static" and without #ifdef guards. > > The resulting "intel_pinctrl_pm_ops" can be marked as "extern" in the > pinctrl-intel.h without any guards, as long as it is only referenced > with the pm_ptr() macro. I'm not sure I got this. Currently drivers do not have any guards. Moreover, the correct one for noirq is pm_sleep_ptr(), isn't it?
Le lundi 17 juillet 2023 à 22:33 +0300, Andy Shevchenko a écrit : > On Mon, Jul 17, 2023 at 10:02 PM Paul Cercueil <paul@crapouillou.net> > wrote: > > Le lundi 17 juillet 2023 à 20:28 +0300, Andy Shevchenko a écrit : > > ... > > > Unrelated change. > > OK. > > ... > > > So the correct way to update this driver would be to have a > > conditionally-exported dev_pm_ops structure: > > > > EXPORT_GPL_DEV_PM_OPS(intel_pinctrl_pm_ops) = { > > NOIRQ_SYSTEM_SLEEP_PM_OPS(intel_pinctrl_suspend_noirq, > > intel_pinctrl_resume_noirq), > > }; > > This looks ugly. I didn't know that EXPORT*PM_OPS designed that way, > but it seems pm.h in such case needs EXPORT for NOIRQ case as well. It's designed so that when CONFIG_PM is disabled, the dev_pm_ops is garbage-collected along with all its callbacks. I know it looks ugly, but we already have 4 variants (regular, namespace, GPL, namespace + GPL), if we start to add macros for specific use-cases then it will become bloated really quick. And the "bloat" I'm trying to avoid here is the extreme expansion of the API which makes it hard for people not familiar to the code to understand what should be used and how. > > Then your two callbacks can be "static" and without #ifdef guards. > > > > The resulting "intel_pinctrl_pm_ops" can be marked as "extern" in > > the > > pinctrl-intel.h without any guards, as long as it is only > > referenced > > with the pm_ptr() macro. > > I'm not sure I got this. Currently drivers do not have any guards. > Moreover, the correct one for noirq is pm_sleep_ptr(), isn't it? > The EXPORT_*_DEV_PM_OPS() macros do export the "dev_pm_ops" conditionally depending on CONFIG_PM. We could add variants that export it conditionally depending on CONFIG_PM_SLEEP, but we're back at the problem of adding bloat. You could use pm_sleep_ptr() indeed, with the existing macros, with the drawback that in the case where CONFIG_PM && !CONFIG_PM_SLEEP, the dev_pm_ops + callbacks are compiled in but never referenced. Cheers, -Paul
On Mon, Jul 17, 2023 at 10:56 PM Paul Cercueil <paul@crapouillou.net> wrote: > Le lundi 17 juillet 2023 à 22:33 +0300, Andy Shevchenko a écrit : > > On Mon, Jul 17, 2023 at 10:02 PM Paul Cercueil <paul@crapouillou.net> > > wrote: > > > Le lundi 17 juillet 2023 à 20:28 +0300, Andy Shevchenko a écrit : ... > > > So the correct way to update this driver would be to have a > > > conditionally-exported dev_pm_ops structure: > > > > > > EXPORT_GPL_DEV_PM_OPS(intel_pinctrl_pm_ops) = { > > > NOIRQ_SYSTEM_SLEEP_PM_OPS(intel_pinctrl_suspend_noirq, > > > intel_pinctrl_resume_noirq), > > > }; > > > > This looks ugly. I didn't know that EXPORT*PM_OPS designed that way, > > but it seems pm.h in such case needs EXPORT for NOIRQ case as well. > > It's designed so that when CONFIG_PM is disabled, the dev_pm_ops is > garbage-collected along with all its callbacks. > > I know it looks ugly, but we already have 4 variants (regular, > namespace, GPL, namespace + GPL), if we start to add macros for > specific use-cases then it will become bloated really quick. Maybe macros can be replaced / changed to make it scale? > And the "bloat" I'm trying to avoid here is the extreme expansion of > the API which makes it hard for people not familiar to the code to > understand what should be used and how. So far, based on the rest of the messages in the thread the EXPORT*PM_OPS() have the following issues: 1) do not scale (for variants with different scope we need new set of macros); 2) do not cover cases with pm_sleep_ptr(); 3) export symbols in case when it's not needed. Am I right? > > > Then your two callbacks can be "static" and without #ifdef guards. > > > > > > The resulting "intel_pinctrl_pm_ops" can be marked as "extern" in > > > the > > > pinctrl-intel.h without any guards, as long as it is only > > > referenced > > > with the pm_ptr() macro. > > > > I'm not sure I got this. Currently drivers do not have any guards. > > Moreover, the correct one for noirq is pm_sleep_ptr(), isn't it? > > The EXPORT_*_DEV_PM_OPS() macros do export the "dev_pm_ops" > conditionally depending on CONFIG_PM. We could add variants that export > it conditionally depending on CONFIG_PM_SLEEP, but we're back at the > problem of adding bloat. Exactly. > You could use pm_sleep_ptr() indeed, with the existing macros, with the > drawback that in the case where CONFIG_PM && !CONFIG_PM_SLEEP, the > dev_pm_ops + callbacks are compiled in but never referenced. And exactly. I don't think they are ready to use (in the current form). But let's see what we may do better here... -- With Best Regards, Andy Shevchenko
On Tue, Jul 18, 2023 at 11:04:51AM +0100, Jonathan Cameron wrote: > On Mon, 17 Jul 2023 20:28:15 +0300 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: ... > > EXPORT_SYMBOL_GPL(intel_pinctrl_resume_noirq); > > Can you check if this is successfully removed? I think it won't be. > Not immediately obvious how to tidy that up given these are used > in a macro called from lots of drivers. That's what Paul noticed I think with his proposal to export only the ops variable and make these to be static. > Maybe just leaving the ifdef is best we can do here. See above.
Hi Andy, Le mardi 18 juillet 2023 à 15:57 +0300, Andy Shevchenko a écrit : > On Mon, Jul 17, 2023 at 10:56 PM Paul Cercueil <paul@crapouillou.net> > wrote: > > Le lundi 17 juillet 2023 à 22:33 +0300, Andy Shevchenko a écrit : > > > On Mon, Jul 17, 2023 at 10:02 PM Paul Cercueil > > > <paul@crapouillou.net> > > > wrote: > > > > Le lundi 17 juillet 2023 à 20:28 +0300, Andy Shevchenko a écrit > > > > : > > ... > > > > > So the correct way to update this driver would be to have a > > > > conditionally-exported dev_pm_ops structure: > > > > > > > > EXPORT_GPL_DEV_PM_OPS(intel_pinctrl_pm_ops) = { > > > > NOIRQ_SYSTEM_SLEEP_PM_OPS(intel_pinctrl_suspend_noirq, > > > > intel_pinctrl_resume_noirq), > > > > }; > > > > > > This looks ugly. I didn't know that EXPORT*PM_OPS designed that > > > way, > > > but it seems pm.h in such case needs EXPORT for NOIRQ case as > > > well. > > > > It's designed so that when CONFIG_PM is disabled, the dev_pm_ops is > > garbage-collected along with all its callbacks. > > > > I know it looks ugly, but we already have 4 variants (regular, > > namespace, GPL, namespace + GPL), if we start to add macros for > > specific use-cases then it will become bloated really quick. > > Maybe macros can be replaced / changed to make it scale? If you have any ideas, then yes absolutely. > > > And the "bloat" I'm trying to avoid here is the extreme expansion > > of > > the API which makes it hard for people not familiar to the code to > > understand what should be used and how. > > So far, based on the rest of the messages in the thread the > EXPORT*PM_OPS() have the following issues: > 1) do not scale (for variants with different scope we need new set of > macros); > 2) do not cover cases with pm_sleep_ptr(); > 3) export symbols in case when it's not needed. > > Am I right? I think that's right yes. > > > > > Then your two callbacks can be "static" and without #ifdef > > > > guards. > > > > > > > > The resulting "intel_pinctrl_pm_ops" can be marked as "extern" > > > > in > > > > the > > > > pinctrl-intel.h without any guards, as long as it is only > > > > referenced > > > > with the pm_ptr() macro. > > > > > > I'm not sure I got this. Currently drivers do not have any > > > guards. > > > Moreover, the correct one for noirq is pm_sleep_ptr(), isn't it? > > > > The EXPORT_*_DEV_PM_OPS() macros do export the "dev_pm_ops" > > conditionally depending on CONFIG_PM. We could add variants that > > export > > it conditionally depending on CONFIG_PM_SLEEP, but we're back at > > the > > problem of adding bloat. > > Exactly. > > > You could use pm_sleep_ptr() indeed, with the existing macros, with > > the > > drawback that in the case where CONFIG_PM && !CONFIG_PM_SLEEP, the > > dev_pm_ops + callbacks are compiled in but never referenced. > > And exactly. > > I don't think they are ready to use (in the current form). But let's > see what we may do better here... They were OK when Jonathan and myself were updating the IIO drivers - but now they definitely show their limitations. Cheers, -Paul
diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c index 64c3e62b4348..40e45c4889ee 100644 --- a/drivers/pinctrl/intel/pinctrl-intel.c +++ b/drivers/pinctrl/intel/pinctrl-intel.c @@ -929,7 +929,7 @@ static int intel_gpio_to_pin(struct intel_pinctrl *pctrl, unsigned int offset, * * Return: a GPIO offset, or negative error code if translation can't be done. */ -static __maybe_unused int intel_pin_to_gpio(struct intel_pinctrl *pctrl, int pin) +static int intel_pin_to_gpio(struct intel_pinctrl *pctrl, int pin) { const struct intel_community *community; const struct intel_padgroup *padgrp; @@ -1488,7 +1488,6 @@ static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl) if (!communities) return -ENOMEM; - for (i = 0; i < pctrl->ncommunities; i++) { struct intel_community *community = &pctrl->communities[i]; u32 *intmask, *hostown; @@ -1712,7 +1711,6 @@ const struct intel_pinctrl_soc_data *intel_pinctrl_get_soc_data(struct platform_ } EXPORT_SYMBOL_GPL(intel_pinctrl_get_soc_data); -#ifdef CONFIG_PM_SLEEP static bool __intel_gpio_is_direct_irq(u32 value) { return (value & PADCFG0_GPIROUTIOXAPIC) && (value & PADCFG0_GPIOTXDIS) && @@ -1913,7 +1911,6 @@ int intel_pinctrl_resume_noirq(struct device *dev) return 0; } EXPORT_SYMBOL_GPL(intel_pinctrl_resume_noirq); -#endif MODULE_AUTHOR("Mathias Nyman <mathias.nyman@linux.intel.com>"); MODULE_AUTHOR("Mika Westerberg <mika.westerberg@linux.intel.com>"); diff --git a/drivers/pinctrl/intel/pinctrl-intel.h b/drivers/pinctrl/intel/pinctrl-intel.h index 1faf2ada480a..65b1699a2ed5 100644 --- a/drivers/pinctrl/intel/pinctrl-intel.h +++ b/drivers/pinctrl/intel/pinctrl-intel.h @@ -255,15 +255,10 @@ struct intel_pinctrl { int intel_pinctrl_probe_by_hid(struct platform_device *pdev); int intel_pinctrl_probe_by_uid(struct platform_device *pdev); -#ifdef CONFIG_PM_SLEEP int intel_pinctrl_suspend_noirq(struct device *dev); int intel_pinctrl_resume_noirq(struct device *dev); -#endif -#define INTEL_PINCTRL_PM_OPS(_name) \ -const struct dev_pm_ops _name = { \ - SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(intel_pinctrl_suspend_noirq, \ - intel_pinctrl_resume_noirq) \ -} +#define INTEL_PINCTRL_PM_OPS(_name) \ + DEFINE_NOIRQ_DEV_PM_OPS(_name, intel_pinctrl_suspend_noirq, intel_pinctrl_resume_noirq) #endif /* PINCTRL_INTEL_H */