SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu <[email protected]>
---
drivers/mfd/max77686.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c
index 71faf503844b..8c701f8a9dd5 100644
--- a/drivers/mfd/max77686.c
+++ b/drivers/mfd/max77686.c
@@ -227,7 +227,6 @@ static int max77686_i2c_probe(struct i2c_client *i2c)
return 0;
}
-#ifdef CONFIG_PM_SLEEP
static int max77686_suspend(struct device *dev)
{
struct i2c_client *i2c = to_i2c_client(dev);
@@ -262,7 +261,6 @@ static int max77686_resume(struct device *dev)
return 0;
}
-#endif /* CONFIG_PM_SLEEP */
static SIMPLE_DEV_PM_OPS(max77686_pm, max77686_suspend, max77686_resume);
--
2.28.0
SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu <[email protected]>
---
drivers/mfd/motorola-cpcap.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/mfd/motorola-cpcap.c b/drivers/mfd/motorola-cpcap.c
index 2283d88adcc2..97c07a5641c5 100644
--- a/drivers/mfd/motorola-cpcap.c
+++ b/drivers/mfd/motorola-cpcap.c
@@ -214,7 +214,6 @@ static const struct regmap_config cpcap_regmap_config = {
.val_format_endian = REGMAP_ENDIAN_LITTLE,
};
-#ifdef CONFIG_PM_SLEEP
static int cpcap_suspend(struct device *dev)
{
struct spi_device *spi = to_spi_device(dev);
@@ -232,7 +231,6 @@ static int cpcap_resume(struct device *dev)
return 0;
}
-#endif
static SIMPLE_DEV_PM_OPS(cpcap_pm, cpcap_suspend, cpcap_resume);
--
2.28.0
SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu <[email protected]>
---
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
SET_SYSTEM_SLEEP_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu <[email protected]>
---
drivers/mfd/max77620.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/mfd/max77620.c b/drivers/mfd/max77620.c
index fec2096474ad..17ea0ae6408d 100644
--- a/drivers/mfd/max77620.c
+++ b/drivers/mfd/max77620.c
@@ -574,7 +574,6 @@ static int max77620_probe(struct i2c_client *client,
return 0;
}
-#ifdef CONFIG_PM_SLEEP
static int max77620_set_fps_period(struct max77620_chip *chip,
int fps_id, int time_period)
{
@@ -681,7 +680,6 @@ static int max77620_i2c_resume(struct device *dev)
return 0;
}
-#endif
static const struct i2c_device_id max77620_id[] = {
{"max77620", MAX77620},
--
2.28.0
SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu <[email protected]>
---
drivers/mfd/stmfx.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/mfd/stmfx.c b/drivers/mfd/stmfx.c
index 5e680bfdf5c9..e760cf2be02e 100644
--- a/drivers/mfd/stmfx.c
+++ b/drivers/mfd/stmfx.c
@@ -469,7 +469,6 @@ static int stmfx_remove(struct i2c_client *client)
return stmfx_chip_exit(client);
}
-#ifdef CONFIG_PM_SLEEP
static int stmfx_suspend(struct device *dev)
{
struct stmfx *stmfx = dev_get_drvdata(dev);
@@ -535,7 +534,6 @@ static int stmfx_resume(struct device *dev)
return 0;
}
-#endif
static SIMPLE_DEV_PM_OPS(stmfx_dev_pm_ops, stmfx_suspend, stmfx_resume);
--
2.28.0
SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu <[email protected]>
---
drivers/mfd/max14577.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/mfd/max14577.c b/drivers/mfd/max14577.c
index be185e9d5f16..c619dd8172d5 100644
--- a/drivers/mfd/max14577.c
+++ b/drivers/mfd/max14577.c
@@ -482,7 +482,6 @@ static const struct i2c_device_id max14577_i2c_id[] = {
};
MODULE_DEVICE_TABLE(i2c, max14577_i2c_id);
-#ifdef CONFIG_PM_SLEEP
static int max14577_suspend(struct device *dev)
{
struct i2c_client *i2c = to_i2c_client(dev);
@@ -515,7 +514,6 @@ static int max14577_resume(struct device *dev)
return 0;
}
-#endif /* CONFIG_PM_SLEEP */
static SIMPLE_DEV_PM_OPS(max14577_pm, max14577_suspend, max14577_resume);
--
2.28.0
SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu <[email protected]>
---
drivers/mfd/stpmic1.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/mfd/stpmic1.c b/drivers/mfd/stpmic1.c
index eb3da558c3fb..8be7a6dd9bbb 100644
--- a/drivers/mfd/stpmic1.c
+++ b/drivers/mfd/stpmic1.c
@@ -162,7 +162,6 @@ static int stpmic1_probe(struct i2c_client *i2c,
return devm_of_platform_populate(dev);
}
-#ifdef CONFIG_PM_SLEEP
static int stpmic1_suspend(struct device *dev)
{
struct i2c_client *i2c = to_i2c_client(dev);
@@ -187,7 +186,6 @@ static int stpmic1_resume(struct device *dev)
return 0;
}
-#endif
static SIMPLE_DEV_PM_OPS(stpmic1_pm, stpmic1_suspend, stpmic1_resume);
--
2.28.0
SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu <[email protected]>
---
drivers/mfd/sec-core.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/mfd/sec-core.c b/drivers/mfd/sec-core.c
index 95473ff9bb4b..a5d19798d671 100644
--- a/drivers/mfd/sec-core.c
+++ b/drivers/mfd/sec-core.c
@@ -496,7 +496,6 @@ static void sec_pmic_shutdown(struct i2c_client *i2c)
regmap_update_bits(sec_pmic->regmap_pmic, reg, mask, 0);
}
-#ifdef CONFIG_PM_SLEEP
static int sec_pmic_suspend(struct device *dev)
{
struct i2c_client *i2c = to_i2c_client(dev);
@@ -529,7 +528,6 @@ static int sec_pmic_resume(struct device *dev)
return 0;
}
-#endif /* CONFIG_PM_SLEEP */
static SIMPLE_DEV_PM_OPS(sec_pmic_pm_ops, sec_pmic_suspend, sec_pmic_resume);
--
2.28.0
SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
Signed-off-by: Coiby Xu <[email protected]>
---
drivers/mfd/sprd-sc27xx-spi.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/mfd/sprd-sc27xx-spi.c b/drivers/mfd/sprd-sc27xx-spi.c
index 6b7956604a0f..4db2ec9ef2ff 100644
--- a/drivers/mfd/sprd-sc27xx-spi.c
+++ b/drivers/mfd/sprd-sc27xx-spi.c
@@ -206,7 +206,6 @@ static int sprd_pmic_probe(struct spi_device *spi)
return 0;
}
-#ifdef CONFIG_PM_SLEEP
static int sprd_pmic_suspend(struct device *dev)
{
struct sprd_pmic *ddata = dev_get_drvdata(dev);
@@ -226,7 +225,6 @@ static int sprd_pmic_resume(struct device *dev)
return 0;
}
-#endif
static SIMPLE_DEV_PM_OPS(sprd_pmic_pm_ops, sprd_pmic_suspend, sprd_pmic_resume);
--
2.28.0
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
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
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
On Thu, Oct 29, 2020 at 5:27 PM Lee Jones <[email protected]> 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
On Thu, Oct 29, 2020 at 06:06:39PM +0800, Coiby Xu wrote:
> SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
I don't quite get what did you mean by "took good care". This should
cause warnings of unused structure. Comment applies to other patches as
well.
Also, the title prefix is: "mfd: max77686:"
Best regards,
Krzysztof
>
> Signed-off-by: Coiby Xu <[email protected]>
> ---
> drivers/mfd/max77686.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c
> index 71faf503844b..8c701f8a9dd5 100644
> --- a/drivers/mfd/max77686.c
> +++ b/drivers/mfd/max77686.c
> @@ -227,7 +227,6 @@ static int max77686_i2c_probe(struct i2c_client *i2c)
> return 0;
> }
>
> -#ifdef CONFIG_PM_SLEEP
> static int max77686_suspend(struct device *dev)
> {
> struct i2c_client *i2c = to_i2c_client(dev);
> @@ -262,7 +261,6 @@ static int max77686_resume(struct device *dev)
>
> return 0;
> }
> -#endif /* CONFIG_PM_SLEEP */
>
> static SIMPLE_DEV_PM_OPS(max77686_pm, max77686_suspend, max77686_resume);
>
> --
> 2.28.0
>
Hi Coiby,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on lee-mfd/for-mfd-next]
[also build test ERROR on chanwoo-extcon/extcon-next v5.10-rc1 next-20201029]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Coiby-Xu/mfd-maxim-remove-unnecessary-CONFIG_PM_SLEEP/20201029-180931
base: https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
config: s390-allyesconfig (attached as .config)
compiler: s390-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/af64db6e5065ea1b51445005e0e675f8a49db5ec
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Coiby-Xu/mfd-maxim-remove-unnecessary-CONFIG_PM_SLEEP/20201029-180931
git checkout af64db6e5065ea1b51445005e0e675f8a49db5ec
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=s390
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
drivers/mfd/stmfx.c: In function 'stmfx_suspend':
>> drivers/mfd/stmfx.c:478:16: error: 'struct stmfx' has no member named 'bkp_sysctrl'
478 | &stmfx->bkp_sysctrl, sizeof(stmfx->bkp_sysctrl));
| ^~
drivers/mfd/stmfx.c:478:43: error: 'struct stmfx' has no member named 'bkp_sysctrl'
478 | &stmfx->bkp_sysctrl, sizeof(stmfx->bkp_sysctrl));
| ^~
>> drivers/mfd/stmfx.c:483:16: error: 'struct stmfx' has no member named 'bkp_irqoutpin'
483 | &stmfx->bkp_irqoutpin,
| ^~
drivers/mfd/stmfx.c:484:22: error: 'struct stmfx' has no member named 'bkp_irqoutpin'
484 | sizeof(stmfx->bkp_irqoutpin));
| ^~
drivers/mfd/stmfx.c: In function 'stmfx_resume':
drivers/mfd/stmfx.c:518:17: error: 'struct stmfx' has no member named 'bkp_sysctrl'
518 | &stmfx->bkp_sysctrl, sizeof(stmfx->bkp_sysctrl));
| ^~
drivers/mfd/stmfx.c:518:44: error: 'struct stmfx' has no member named 'bkp_sysctrl'
518 | &stmfx->bkp_sysctrl, sizeof(stmfx->bkp_sysctrl));
| ^~
drivers/mfd/stmfx.c:523:17: error: 'struct stmfx' has no member named 'bkp_irqoutpin'
523 | &stmfx->bkp_irqoutpin,
| ^~
drivers/mfd/stmfx.c:524:23: error: 'struct stmfx' has no member named 'bkp_irqoutpin'
524 | sizeof(stmfx->bkp_irqoutpin));
| ^~
At top level:
drivers/mfd/stmfx.c:496:12: warning: 'stmfx_resume' defined but not used [-Wunused-function]
496 | static int stmfx_resume(struct device *dev)
| ^~~~~~~~~~~~
drivers/mfd/stmfx.c:472:12: warning: 'stmfx_suspend' defined but not used [-Wunused-function]
472 | static int stmfx_suspend(struct device *dev)
| ^~~~~~~~~~~~~
vim +478 drivers/mfd/stmfx.c
06252ade9156657 Amelie Delaunay 2019-05-09 471
06252ade9156657 Amelie Delaunay 2019-05-09 472 static int stmfx_suspend(struct device *dev)
06252ade9156657 Amelie Delaunay 2019-05-09 473 {
06252ade9156657 Amelie Delaunay 2019-05-09 474 struct stmfx *stmfx = dev_get_drvdata(dev);
06252ade9156657 Amelie Delaunay 2019-05-09 475 int ret;
06252ade9156657 Amelie Delaunay 2019-05-09 476
06252ade9156657 Amelie Delaunay 2019-05-09 477 ret = regmap_raw_read(stmfx->map, STMFX_REG_SYS_CTRL,
06252ade9156657 Amelie Delaunay 2019-05-09 @478 &stmfx->bkp_sysctrl, sizeof(stmfx->bkp_sysctrl));
06252ade9156657 Amelie Delaunay 2019-05-09 479 if (ret)
06252ade9156657 Amelie Delaunay 2019-05-09 480 return ret;
06252ade9156657 Amelie Delaunay 2019-05-09 481
06252ade9156657 Amelie Delaunay 2019-05-09 482 ret = regmap_raw_read(stmfx->map, STMFX_REG_IRQ_OUT_PIN,
06252ade9156657 Amelie Delaunay 2019-05-09 @483 &stmfx->bkp_irqoutpin,
06252ade9156657 Amelie Delaunay 2019-05-09 484 sizeof(stmfx->bkp_irqoutpin));
06252ade9156657 Amelie Delaunay 2019-05-09 485 if (ret)
06252ade9156657 Amelie Delaunay 2019-05-09 486 return ret;
06252ade9156657 Amelie Delaunay 2019-05-09 487
97eda5dcc2cde5d Amelie Delaunay 2020-04-22 488 disable_irq(stmfx->irq);
97eda5dcc2cde5d Amelie Delaunay 2020-04-22 489
06252ade9156657 Amelie Delaunay 2019-05-09 490 if (stmfx->vdd)
06252ade9156657 Amelie Delaunay 2019-05-09 491 return regulator_disable(stmfx->vdd);
06252ade9156657 Amelie Delaunay 2019-05-09 492
06252ade9156657 Amelie Delaunay 2019-05-09 493 return 0;
06252ade9156657 Amelie Delaunay 2019-05-09 494 }
06252ade9156657 Amelie Delaunay 2019-05-09 495
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Coiby,
After removing CONFIG_PM_SLEEP, sprd_pmic_suspend/resume() would not
be built into symbol table with clang compiler though, that would
cause clang compiler report warnings of "unused function" if
CONFIG_PM_SLEEP is not set. So I also prefer to add a __maybe_unused
instead as other people suggested in the mail list.
Thanks,
Chunyan
On Thu, 29 Oct 2020 at 18:07, Coiby Xu <[email protected]> wrote:
>
> SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG. Signed-off-by: Coiby Xu <[email protected]> drivers/mfd/sprd-sc27xx-spi.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/mfd/sprd-sc27xx-spi.c b/drivers/mfd/sprd-sc27xx-spi.c index 6b7956604a0f..4db2ec9ef2ff 100644 --- a/drivers/mfd/sprd-sc27xx-spi.c +++ b/drivers/mfd/sprd-sc27xx-spi.c @@ -206,7 +206,6 @@ static int sprd_pmic_probe(struct spi_device *spi) return 0; -#ifdef CONFIG_PM_SLEEP static int sprd_pmic_suspend(struct device *dev) struct sprd_pmic *ddata = dev_get_drvdata(dev); @@ -226,7 +225,6 @@ static int sprd_pmic_resume(struct device *dev) return 0; -#endif static SIMPLE_DEV_PM_OPS(sprd_pmic_pm_ops, sprd_pmic_suspend, sprd_pmic_resume); 2.28.0
On Thu, Oct 29, 2020 at 07:04:44PM +0200, Andy Shevchenko wrote:
>On Thu, Oct 29, 2020 at 5:27 PM Lee Jones <[email protected]> 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
On Fri, Oct 30, 2020 at 4:23 PM Coiby Xu <[email protected]> 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 <[email protected]> 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
Hi Chunyan,
On Fri, Oct 30, 2020 at 12:02:03PM +0800, Chunyan Zhang wrote:
>Hi Coiby,
>
>After removing CONFIG_PM_SLEEP, sprd_pmic_suspend/resume() would not
>be built into symbol table with clang compiler though, that would
>cause clang compiler report warnings of "unused function" if
>CONFIG_PM_SLEEP is not set. So I also prefer to add a __maybe_unused
>instead as other people suggested in the mail list.
>
Thank you for the suggestion! At least Lee prefers to CONFIG_PM_SLEEP
thus to keep the status quo. I'll see he'll change his mind with the
ongoing discussion [1]:)
[1] https://www.mail-archive.com/[email protected]/msg2361371.html
>Thanks,
>Chunyan
>
>
>On Thu, 29 Oct 2020 at 18:07, Coiby Xu <[email protected]> wrote:
>>
>> SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG. Signed-off-by: Coiby Xu <[email protected]> drivers/mfd/sprd-sc27xx-spi.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/mfd/sprd-sc27xx-spi.c b/drivers/mfd/sprd-sc27xx-spi.c index 6b7956604a0f..4db2ec9ef2ff 100644 --- a/drivers/mfd/sprd-sc27xx-spi.c +++ b/drivers/mfd/sprd-sc27xx-spi.c @@ -206,7 +206,6 @@ static int sprd_pmic_probe(struct spi_device *spi) return 0; -#ifdef CONFIG_PM_SLEEP static int sprd_pmic_suspend(struct device *dev) struct sprd_pmic *ddata = dev_get_drvdata(dev); @@ -226,7 +225,6 @@ static int sprd_pmic_resume(struct device *dev) return 0; -#endif static SIMPLE_DEV_PM_OPS(sprd_pmic_pm_ops, sprd_pmic_suspend, sprd_pmic_resume); 2.28.0
--
Best regards,
Coiby
On Thu, Oct 29, 2020 at 07:32:26PM +0100, Krzysztof Kozlowski wrote:
>On Thu, Oct 29, 2020 at 06:06:39PM +0800, Coiby Xu wrote:
>> SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG.
>
>I don't quite get what did you mean by "took good care". This should
>cause warnings of unused structure. Comment applies to other patches as
>well.
>
Sorry I made a mistake.
>Also, the title prefix is: "mfd: max77686:"
>
Thank you for the reminding! I'll improve my script to avoid this issue
when extracting prefix from git log.
>Best regards,
>Krzysztof
>
>
>>
>> Signed-off-by: Coiby Xu <[email protected]>
>> ---
>> drivers/mfd/max77686.c | 2 --
>> 1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c
>> index 71faf503844b..8c701f8a9dd5 100644
>> --- a/drivers/mfd/max77686.c
>> +++ b/drivers/mfd/max77686.c
>> @@ -227,7 +227,6 @@ static int max77686_i2c_probe(struct i2c_client *i2c)
>> return 0;
>> }
>>
>> -#ifdef CONFIG_PM_SLEEP
>> static int max77686_suspend(struct device *dev)
>> {
>> struct i2c_client *i2c = to_i2c_client(dev);
>> @@ -262,7 +261,6 @@ static int max77686_resume(struct device *dev)
>>
>> return 0;
>> }
>> -#endif /* CONFIG_PM_SLEEP */
>>
>> static SIMPLE_DEV_PM_OPS(max77686_pm, max77686_suspend, max77686_resume);
>>
>> --
>> 2.28.0
>>
--
Best regards,
Coiby
On Thu, 29 Oct 2020, Andy Shevchenko wrote:
> On Thu, Oct 29, 2020 at 5:27 PM Lee Jones <[email protected]> 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
On Mon, Nov 2, 2020 at 10:39 AM Lee Jones <[email protected]> wrote:
> On Thu, 29 Oct 2020, Andy Shevchenko wrote:
> > On Thu, Oct 29, 2020 at 5:27 PM Lee Jones <[email protected]> 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
On Mon, 02 Nov 2020, Andy Shevchenko wrote:
> On Mon, Nov 2, 2020 at 10:39 AM Lee Jones <[email protected]> wrote:
> > On Thu, 29 Oct 2020, Andy Shevchenko wrote:
> > > On Thu, Oct 29, 2020 at 5:27 PM Lee Jones <[email protected]> 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