Re: [PATCH 3/9] mfd: intel_soc_pmic: remove unnecessary CONFIG_PM_SLEEP
On Mon, 02 Nov 2020, Andy Shevchenko wrote: > On Mon, Nov 2, 2020 at 10:39 AM Lee Jones wrote: > > On Thu, 29 Oct 2020, Andy Shevchenko wrote: > > > On Thu, Oct 29, 2020 at 5:27 PM Lee Jones wrote: > > > > On Thu, 29 Oct 2020, Coiby Xu wrote: > > > > > On Thu, Oct 29, 2020 at 01:00:29PM +0200, Andy Shevchenko wrote: > > > > > > On Thu, Oct 29, 2020 at 06:06:41PM +0800, Coiby Xu wrote: > > > > > > > SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG. > > > > > > > > > > > > Have you compiled this with > > > > > > % make W=1 ... > > > > > > ? > > > > > > > > > > > > > > > > Sorry my bad. I thought I had run "make modules" with CONFIG_PM_SLEEP > > > > > disabled. I'll run "make W=1 M=..." for each driver after adding > > > > > __maybe_unused in v2. > > > > > > > > No, thank you. Just keep it as it is. > > > > > > > > The current code is space saving. > > > > > > Perhaps you need to go thru __maybe_unused handling. > > > There are pros and cons of each approach, but not above. > > > > Do you know that all compilers drop the section? > > At least all that Linux kernel can be officially built with. Fair enough. -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH 3/9] mfd: intel_soc_pmic: remove unnecessary CONFIG_PM_SLEEP
On Mon, Nov 2, 2020 at 10:39 AM Lee Jones wrote: > On Thu, 29 Oct 2020, Andy Shevchenko wrote: > > On Thu, Oct 29, 2020 at 5:27 PM Lee Jones wrote: > > > On Thu, 29 Oct 2020, Coiby Xu wrote: > > > > On Thu, Oct 29, 2020 at 01:00:29PM +0200, Andy Shevchenko wrote: > > > > > On Thu, Oct 29, 2020 at 06:06:41PM +0800, Coiby Xu wrote: > > > > > > SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG. > > > > > > > > > > Have you compiled this with > > > > > % make W=1 ... > > > > > ? > > > > > > > > > > > > > Sorry my bad. I thought I had run "make modules" with CONFIG_PM_SLEEP > > > > disabled. I'll run "make W=1 M=..." for each driver after adding > > > > __maybe_unused in v2. > > > > > > No, thank you. Just keep it as it is. > > > > > > The current code is space saving. > > > > Perhaps you need to go thru __maybe_unused handling. > > There are pros and cons of each approach, but not above. > > Do you know that all compilers drop the section? At least all that Linux kernel can be officially built with. -- With Best Regards, Andy Shevchenko
Re: [PATCH 3/9] mfd: intel_soc_pmic: remove unnecessary CONFIG_PM_SLEEP
On Thu, 29 Oct 2020, Andy Shevchenko wrote: > On Thu, Oct 29, 2020 at 5:27 PM Lee Jones wrote: > > On Thu, 29 Oct 2020, Coiby Xu wrote: > > > On Thu, Oct 29, 2020 at 01:00:29PM +0200, Andy Shevchenko wrote: > > > > On Thu, Oct 29, 2020 at 06:06:41PM +0800, Coiby Xu wrote: > > > > > SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG. > > > > > > > > Have you compiled this with > > > > % make W=1 ... > > > > ? > > > > > > > > > > Sorry my bad. I thought I had run "make modules" with CONFIG_PM_SLEEP > > > disabled. I'll run "make W=1 M=..." for each driver after adding > > > __maybe_unused in v2. > > > > No, thank you. Just keep it as it is. > > > > The current code is space saving. > > Perhaps you need to go thru __maybe_unused handling. > There are pros and cons of each approach, but not above. Do you know that all compilers drop the section? -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH 3/9] mfd: intel_soc_pmic: remove unnecessary CONFIG_PM_SLEEP
On Fri, Oct 30, 2020 at 4:23 PM Coiby Xu wrote: > On Thu, Oct 29, 2020 at 07:04:44PM +0200, Andy Shevchenko wrote: > >On Thu, Oct 29, 2020 at 5:27 PM Lee Jones wrote: ... > >There are pros and cons of each approach, but not above. > > > Can you elaborate on the pros and cons of each approach? There's > convincing reason to prefer __maybe_unused over CONFIG_PM_SLEEP > according to Arnd Bergmann [1], First what comes to my mind. Perhaps more, but somebody else may extend / correct below. ifdeffery (pros): - compiler doesn't need even to look at that code ifdeffery (cons): - if depends on configuration and thus harder to test coverage __maybe_unused (pros): - removes ugly ifdeffery in the code, increases readability __maybe_unused (cons): - it's a burden for compiler (increasing compilation time) and to linker (to drop the section) -- With Best Regards, Andy Shevchenko
Re: [PATCH 3/9] mfd: intel_soc_pmic: remove unnecessary CONFIG_PM_SLEEP
On Thu, Oct 29, 2020 at 07:04:44PM +0200, Andy Shevchenko wrote: On Thu, Oct 29, 2020 at 5:27 PM Lee Jones wrote: On Thu, 29 Oct 2020, Coiby Xu wrote: > On Thu, Oct 29, 2020 at 01:00:29PM +0200, Andy Shevchenko wrote: > > On Thu, Oct 29, 2020 at 06:06:41PM +0800, Coiby Xu wrote: > > > SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG. > > > > Have you compiled this with > > % make W=1 ... > > ? > > > > Sorry my bad. I thought I had run "make modules" with CONFIG_PM_SLEEP > disabled. I'll run "make W=1 M=..." for each driver after adding > __maybe_unused in v2. No, thank you. Just keep it as it is. The current code is space saving. Perhaps you need to go thru __maybe_unused handling. There are pros and cons of each approach, but not above. Can you elaborate on the pros and cons of each approach? There's convincing reason to prefer __maybe_unused over CONFIG_PM_SLEEP according to Arnd Bergmann [1], > By and large, drivers handle this by using a CONFIG_PM_SLEEP ifdef. > > Unless you can make an extremely convincing argument why not to do > so here, I'd like you to handle it that way instead. [adding linux-pm to Cc] The main reason is that everyone gets the #ifdef wrong, I run into half a dozen new build regressions with linux-next every week on average, the typical problems being: - testing CONFIG_PM_SLEEP instead of CONFIG_PM, leading to an unused function warning - testing CONFIG_PM instead of CONFIG_PM_SLEEP, leading to a build failure - calling a function outside of the #ifdef only from inside an otherwise correct #ifdef, again leading to an unused function warning - causing a warning inside of the #ifdef but only testing if that is disabled, leading to a problem if the macro is set (this is rare these days for CONFIG_PM as that is normally enabled) Using __maybe_unused avoids all of the above. You option is valuable to me because I'm making a tree-wide change. Currently there are 929 drivers having device PM callbacks, $ grep -rI "\.pm = &" --include=*.c ./|wc -l 929 I put all files having device PM callbacks into four categories based on weather a file has CONFIG_PM_SLEEP or PM macro like SET_SYSTEM_SLEEP_PM_OPS, here are the statistics, 1. have both CONFIG_PM_SLEEP and PM_OPS macro: 213 2. have CONFIG_PM_SLEEP but no PM_OPS macro: 19 3. have PM macro but not CONFIG_PM_SLEEP: 347 4. no PM macro or CONFIG_PM_SLEEP: 302 [1] https://lore.kernel.org/patchwork/comment/919944/ -- With Best Regards, Andy Shevchenko -- Best regards, Coiby
Re: [PATCH 3/9] mfd: intel_soc_pmic: remove unnecessary CONFIG_PM_SLEEP
On Thu, Oct 29, 2020 at 5:27 PM Lee Jones wrote: > On Thu, 29 Oct 2020, Coiby Xu wrote: > > On Thu, Oct 29, 2020 at 01:00:29PM +0200, Andy Shevchenko wrote: > > > On Thu, Oct 29, 2020 at 06:06:41PM +0800, Coiby Xu wrote: > > > > SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG. > > > > > > Have you compiled this with > > > % make W=1 ... > > > ? > > > > > > > Sorry my bad. I thought I had run "make modules" with CONFIG_PM_SLEEP > > disabled. I'll run "make W=1 M=..." for each driver after adding > > __maybe_unused in v2. > > No, thank you. Just keep it as it is. > > The current code is space saving. Perhaps you need to go thru __maybe_unused handling. There are pros and cons of each approach, but not above. -- With Best Regards, Andy Shevchenko
Re: [PATCH 3/9] mfd: intel_soc_pmic: remove unnecessary CONFIG_PM_SLEEP
On Thu, 29 Oct 2020, Coiby Xu wrote: > On Thu, Oct 29, 2020 at 01:00:29PM +0200, Andy Shevchenko wrote: > > On Thu, Oct 29, 2020 at 06:06:41PM +0800, Coiby Xu wrote: > > > SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG. > > > > Have you compiled this with > > % make W=1 ... > > ? > > > > Sorry my bad. I thought I had run "make modules" with CONFIG_PM_SLEEP > disabled. I'll run "make W=1 M=..." for each driver after adding > __maybe_unused in v2. No, thank you. Just keep it as it is. The current code is space saving. -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH 3/9] mfd: intel_soc_pmic: remove unnecessary CONFIG_PM_SLEEP
On Thu, Oct 29, 2020 at 01:00:29PM +0200, Andy Shevchenko wrote: On Thu, Oct 29, 2020 at 06:06:41PM +0800, Coiby Xu wrote: SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG. Have you compiled this with % make W=1 ... ? Sorry my bad. I thought I had run "make modules" with CONFIG_PM_SLEEP disabled. I'll run "make W=1 M=..." for each driver after adding __maybe_unused in v2. -- With Best Regards, Andy Shevchenko -- Best regards, Coiby
Re: [PATCH 3/9] mfd: intel_soc_pmic: remove unnecessary CONFIG_PM_SLEEP
On Thu, Oct 29, 2020 at 06:06:41PM +0800, Coiby Xu wrote: > SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG. Have you compiled this with % make W=1 ... ? -- With Best Regards, Andy Shevchenko
[PATCH 3/9] mfd: intel_soc_pmic: remove unnecessary CONFIG_PM_SLEEP
SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu
---
drivers/mfd/intel_soc_pmic_core.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/mfd/intel_soc_pmic_core.c
b/drivers/mfd/intel_soc_pmic_core.c
index ddd64f9e3341..c980af9ae1c0 100644
--- a/drivers/mfd/intel_soc_pmic_core.c
+++ b/drivers/mfd/intel_soc_pmic_core.c
@@ -122,7 +122,6 @@ static void intel_soc_pmic_shutdown(struct i2c_client *i2c)
return;
}
-#if defined(CONFIG_PM_SLEEP)
static int intel_soc_pmic_suspend(struct device *dev)
{
struct intel_soc_pmic *pmic = dev_get_drvdata(dev);
@@ -140,7 +139,6 @@ static int intel_soc_pmic_resume(struct device *dev)
return 0;
}
-#endif
static SIMPLE_DEV_PM_OPS(intel_soc_pmic_pm_ops, intel_soc_pmic_suspend,
intel_soc_pmic_resume);
--
2.28.0
