* Re: [PATCH] mmc: sdhci-esdhc-imx: avoid unused function warnings
2016-07-26 12:32 [PATCH] mmc: sdhci-esdhc-imx: avoid unused function warnings Arnd Bergmann
@ 2016-07-26 13:41 ` Dong Aisheng
2016-07-26 14:28 ` Arnd Bergmann
2016-07-26 20:18 ` Ulf Hansson
2016-07-27 7:10 ` Arnd Bergmann
2 siblings, 1 reply; 8+ messages in thread
From: Dong Aisheng @ 2016-07-26 13:41 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Adrian Hunter, Ulf Hansson, Dong Aisheng, Haibo Chen,
linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org
Hi Arnd,
On Tue, Jul 26, 2016 at 8:32 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> The driver has just gained a slightly incorrect pm-sleep implementation that causes
> warnings when CONFIG_PM is set but CONFIG_PM_SLEEP is not:
>
> drivers/mmc/host/sdhci-esdhc-imx.c:1302:12: error: 'sdhci_esdhc_resume' defined but not used [-Werror=unused-function]
> static int sdhci_esdhc_resume(struct device *dev)
> ^~~~~~~~~~~~~~~~~~
> drivers/mmc/host/sdhci-esdhc-imx.c:1297:12: error: 'sdhci_esdhc_suspend' defined but not used [-Werror=unused-function]
> static int sdhci_esdhc_suspend(struct device *dev)
>
> This replaces the incorrect #ifdef with a __maybe_unused annotation that does
> the right thing in all configurations and is more readable.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: b70d0b3b5b29 ("mmc: sdhci-esdhc-imx: add esdhc specific suspend resume callback")
Thanks for the fix.
Looks like the default kernel Makefile does not enable
-Werror=unused-function wanrings,
so did not expose it before.
Acked-by: Dong Aisheng <aisheng.dong@nxp.com>
Regards
Dong Aisheng
> ---
> drivers/mmc/host/sdhci-esdhc-imx.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 2bb326bbc34a..593e34053c4b 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -1293,13 +1293,12 @@ static int sdhci_esdhc_imx_remove(struct platform_device *pdev)
> return 0;
> }
>
> -#ifdef CONFIG_PM
> -static int sdhci_esdhc_suspend(struct device *dev)
> +static int __maybe_unused sdhci_esdhc_suspend(struct device *dev)
> {
> return sdhci_pltfm_suspend(dev);
> }
>
> -static int sdhci_esdhc_resume(struct device *dev)
> +static int __maybe_unused sdhci_esdhc_resume(struct device *dev)
> {
> struct sdhci_host *host = dev_get_drvdata(dev);
>
> @@ -1309,7 +1308,7 @@ static int sdhci_esdhc_resume(struct device *dev)
> return sdhci_pltfm_resume(dev);
> }
>
> -static int sdhci_esdhc_runtime_suspend(struct device *dev)
> +static int __maybe_unused sdhci_esdhc_runtime_suspend(struct device *dev)
> {
> struct sdhci_host *host = dev_get_drvdata(dev);
> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> @@ -1327,7 +1326,7 @@ static int sdhci_esdhc_runtime_suspend(struct device *dev)
> return ret;
> }
>
> -static int sdhci_esdhc_runtime_resume(struct device *dev)
> +static int __maybe_unused sdhci_esdhc_runtime_resume(struct device *dev)
> {
> struct sdhci_host *host = dev_get_drvdata(dev);
> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> @@ -1341,7 +1340,6 @@ static int sdhci_esdhc_runtime_resume(struct device *dev)
>
> return sdhci_runtime_resume_host(host);
> }
> -#endif
>
> static const struct dev_pm_ops sdhci_esdhc_pmops = {
> SET_SYSTEM_SLEEP_PM_OPS(sdhci_esdhc_suspend, sdhci_esdhc_resume)
> --
> 2.9.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] mmc: sdhci-esdhc-imx: avoid unused function warnings
2016-07-26 13:41 ` Dong Aisheng
@ 2016-07-26 14:28 ` Arnd Bergmann
0 siblings, 0 replies; 8+ messages in thread
From: Arnd Bergmann @ 2016-07-26 14:28 UTC (permalink / raw)
To: Dong Aisheng
Cc: Adrian Hunter, Ulf Hansson, Dong Aisheng, Haibo Chen,
linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org
On Tuesday, July 26, 2016 9:41:50 PM CEST Dong Aisheng wrote:
>
> On Tue, Jul 26, 2016 at 8:32 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > The driver has just gained a slightly incorrect pm-sleep implementation that causes
> > warnings when CONFIG_PM is set but CONFIG_PM_SLEEP is not:
> >
> > drivers/mmc/host/sdhci-esdhc-imx.c:1302:12: error: 'sdhci_esdhc_resume' defined but not used [-Werror=unused-function]
> > static int sdhci_esdhc_resume(struct device *dev)
> > ^~~~~~~~~~~~~~~~~~
> > drivers/mmc/host/sdhci-esdhc-imx.c:1297:12: error: 'sdhci_esdhc_suspend' defined but not used [-Werror=unused-function]
> > static int sdhci_esdhc_suspend(struct device *dev)
> >
> > This replaces the incorrect #ifdef with a __maybe_unused annotation that does
> > the right thing in all configurations and is more readable.
> >
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > Fixes: b70d0b3b5b29 ("mmc: sdhci-esdhc-imx: add esdhc specific suspend resume callback")
>
> Thanks for the fix.
> Looks like the default kernel Makefile does not enable
> -Werror=unused-function wanrings,
> so did not expose it before.
They are enabled by default as warnings, not errors. My test build setup
uses -Werror to turn all warnings into errors so I can catch them
more easily as failed builds.
> Acked-by: Dong Aisheng <aisheng.dong@nxp.com>
>
Thanks,
Arnd
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mmc: sdhci-esdhc-imx: avoid unused function warnings
2016-07-26 12:32 [PATCH] mmc: sdhci-esdhc-imx: avoid unused function warnings Arnd Bergmann
2016-07-26 13:41 ` Dong Aisheng
@ 2016-07-26 20:18 ` Ulf Hansson
2016-07-26 20:56 ` Arnd Bergmann
2016-07-27 7:10 ` Arnd Bergmann
2 siblings, 1 reply; 8+ messages in thread
From: Ulf Hansson @ 2016-07-26 20:18 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Adrian Hunter, Dong Aisheng, Haibo Chen, linux-mmc,
linux-kernel@vger.kernel.org
On 26 July 2016 at 14:32, Arnd Bergmann <arnd@arndb.de> wrote:
> The driver has just gained a slightly incorrect pm-sleep implementation that causes
> warnings when CONFIG_PM is set but CONFIG_PM_SLEEP is not:
>
> drivers/mmc/host/sdhci-esdhc-imx.c:1302:12: error: 'sdhci_esdhc_resume' defined but not used [-Werror=unused-function]
> static int sdhci_esdhc_resume(struct device *dev)
> ^~~~~~~~~~~~~~~~~~
> drivers/mmc/host/sdhci-esdhc-imx.c:1297:12: error: 'sdhci_esdhc_suspend' defined but not used [-Werror=unused-function]
> static int sdhci_esdhc_suspend(struct device *dev)
>
> This replaces the incorrect #ifdef with a __maybe_unused annotation that does
> the right thing in all configurations and is more readable.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: b70d0b3b5b29 ("mmc: sdhci-esdhc-imx: add esdhc specific suspend resume callback")
> ---
> drivers/mmc/host/sdhci-esdhc-imx.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 2bb326bbc34a..593e34053c4b 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -1293,13 +1293,12 @@ static int sdhci_esdhc_imx_remove(struct platform_device *pdev)
> return 0;
> }
>
> -#ifdef CONFIG_PM
> -static int sdhci_esdhc_suspend(struct device *dev)
> +static int __maybe_unused sdhci_esdhc_suspend(struct device *dev)
Instead of using __maybe_unused, I prefer to change above "#ifdef
CONFIG_PMf" to "#ifdef CONFIG_PM_SLEEP".
I do realize that the runtime PM callbacks still requires #ifdef
CONFIG_PM, so yes that's requires an extra "ifdef".
Sure, it's more a matter of taste (and micro optimizations).
> {
> return sdhci_pltfm_suspend(dev);
> }
>
> -static int sdhci_esdhc_resume(struct device *dev)
> +static int __maybe_unused sdhci_esdhc_resume(struct device *dev)
> {
> struct sdhci_host *host = dev_get_drvdata(dev);
>
> @@ -1309,7 +1308,7 @@ static int sdhci_esdhc_resume(struct device *dev)
> return sdhci_pltfm_resume(dev);
> }
>
> -static int sdhci_esdhc_runtime_suspend(struct device *dev)
> +static int __maybe_unused sdhci_esdhc_runtime_suspend(struct device *dev)
> {
> struct sdhci_host *host = dev_get_drvdata(dev);
> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> @@ -1327,7 +1326,7 @@ static int sdhci_esdhc_runtime_suspend(struct device *dev)
> return ret;
> }
>
> -static int sdhci_esdhc_runtime_resume(struct device *dev)
> +static int __maybe_unused sdhci_esdhc_runtime_resume(struct device *dev)
> {
> struct sdhci_host *host = dev_get_drvdata(dev);
> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> @@ -1341,7 +1340,6 @@ static int sdhci_esdhc_runtime_resume(struct device *dev)
>
> return sdhci_runtime_resume_host(host);
> }
> -#endif
>
> static const struct dev_pm_ops sdhci_esdhc_pmops = {
> SET_SYSTEM_SLEEP_PM_OPS(sdhci_esdhc_suspend, sdhci_esdhc_resume)
> --
> 2.9.0
>
Kind regards
Uffe
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] mmc: sdhci-esdhc-imx: avoid unused function warnings
2016-07-26 20:18 ` Ulf Hansson
@ 2016-07-26 20:56 ` Arnd Bergmann
2016-07-26 22:48 ` Ulf Hansson
0 siblings, 1 reply; 8+ messages in thread
From: Arnd Bergmann @ 2016-07-26 20:56 UTC (permalink / raw)
To: Ulf Hansson
Cc: Adrian Hunter, Dong Aisheng, Haibo Chen, linux-mmc,
linux-kernel@vger.kernel.org
On Tuesday, July 26, 2016 10:18:53 PM CEST Ulf Hansson wrote:
> > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> > index 2bb326bbc34a..593e34053c4b 100644
> > --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> > @@ -1293,13 +1293,12 @@ static int sdhci_esdhc_imx_remove(struct platform_device *pdev)
> > return 0;
> > }
> >
> > -#ifdef CONFIG_PM
> > -static int sdhci_esdhc_suspend(struct device *dev)
> > +static int __maybe_unused sdhci_esdhc_suspend(struct device *dev)
>
> Instead of using __maybe_unused, I prefer to change above "#ifdef
> CONFIG_PMf" to "#ifdef CONFIG_PM_SLEEP".
> I do realize that the runtime PM callbacks still requires #ifdef
> CONFIG_PM, so yes that's requires an extra "ifdef".
>
> Sure, it's more a matter of taste (and micro optimizations).
I was hoping that we could eventually do a mass-conversion to
__maybe_unused, as everybody seems to get the #ifdef wrong.
Any specific reason for your preference?
Arnd
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mmc: sdhci-esdhc-imx: avoid unused function warnings
2016-07-26 20:56 ` Arnd Bergmann
@ 2016-07-26 22:48 ` Ulf Hansson
2016-07-27 7:30 ` Arnd Bergmann
0 siblings, 1 reply; 8+ messages in thread
From: Ulf Hansson @ 2016-07-26 22:48 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Adrian Hunter, Dong Aisheng, Haibo Chen, linux-mmc,
linux-kernel@vger.kernel.org
On 26 July 2016 at 22:56, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday, July 26, 2016 10:18:53 PM CEST Ulf Hansson wrote:
>> > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
>> > index 2bb326bbc34a..593e34053c4b 100644
>> > --- a/drivers/mmc/host/sdhci-esdhc-imx.c
>> > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
>> > @@ -1293,13 +1293,12 @@ static int sdhci_esdhc_imx_remove(struct platform_device *pdev)
>> > return 0;
>> > }
>> >
>> > -#ifdef CONFIG_PM
>> > -static int sdhci_esdhc_suspend(struct device *dev)
>> > +static int __maybe_unused sdhci_esdhc_suspend(struct device *dev)
>>
>> Instead of using __maybe_unused, I prefer to change above "#ifdef
>> CONFIG_PMf" to "#ifdef CONFIG_PM_SLEEP".
>> I do realize that the runtime PM callbacks still requires #ifdef
>> CONFIG_PM, so yes that's requires an extra "ifdef".
>>
>> Sure, it's more a matter of taste (and micro optimizations).
>
> I was hoping that we could eventually do a mass-conversion to
> __maybe_unused, as everybody seems to get the #ifdef wrong.
>
> Any specific reason for your preference?
Only that this is how I get used to do it - and that it becomes a bit
more clear what is needed to support the various PM configurations.
If you still insist on the "maybe_unused" option, that's okay as well.
Kind regards
Uffe
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mmc: sdhci-esdhc-imx: avoid unused function warnings
2016-07-26 22:48 ` Ulf Hansson
@ 2016-07-27 7:30 ` Arnd Bergmann
0 siblings, 0 replies; 8+ messages in thread
From: Arnd Bergmann @ 2016-07-27 7:30 UTC (permalink / raw)
To: Ulf Hansson
Cc: Adrian Hunter, Dong Aisheng, Haibo Chen, linux-mmc,
linux-kernel@vger.kernel.org
On Wednesday, July 27, 2016 12:48:23 AM CEST Ulf Hansson wrote:
> On 26 July 2016 at 22:56, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Tuesday, July 26, 2016 10:18:53 PM CEST Ulf Hansson wrote:
> >> > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> >> > index 2bb326bbc34a..593e34053c4b 100644
> >> > --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> >> > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> >> > @@ -1293,13 +1293,12 @@ static int sdhci_esdhc_imx_remove(struct platform_device *pdev)
> >> > return 0;
> >> > }
> >> >
> >> > -#ifdef CONFIG_PM
> >> > -static int sdhci_esdhc_suspend(struct device *dev)
> >> > +static int __maybe_unused sdhci_esdhc_suspend(struct device *dev)
> >>
> >> Instead of using __maybe_unused, I prefer to change above "#ifdef
> >> CONFIG_PMf" to "#ifdef CONFIG_PM_SLEEP".
> >> I do realize that the runtime PM callbacks still requires #ifdef
> >> CONFIG_PM, so yes that's requires an extra "ifdef".
> >>
> >> Sure, it's more a matter of taste (and micro optimizations).
> >
> > I was hoping that we could eventually do a mass-conversion to
> > __maybe_unused, as everybody seems to get the #ifdef wrong.
> >
> > Any specific reason for your preference?
>
> Only that this is how I get used to do it - and that it becomes a bit
> more clear what is needed to support the various PM configurations.
>
> If you still insist on the "maybe_unused" option, that's okay as well.
I only have a mild preference for __maybe_unused because it's harder
to get wrong. I have sent dozens of bugfixes for drivers that have
incorrect #ifdef, and I'd prefer not having to send that many in the
future, so I try to convert them to __maybe_unused whenever a problem
shows up to slowly change the coding style in the kernel.
Build time coverage is another point here, but that is also what caused
problems in my patch since the declarations were hidden (I assume not
intentionally).
Arnd
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mmc: sdhci-esdhc-imx: avoid unused function warnings
2016-07-26 12:32 [PATCH] mmc: sdhci-esdhc-imx: avoid unused function warnings Arnd Bergmann
2016-07-26 13:41 ` Dong Aisheng
2016-07-26 20:18 ` Ulf Hansson
@ 2016-07-27 7:10 ` Arnd Bergmann
2 siblings, 0 replies; 8+ messages in thread
From: Arnd Bergmann @ 2016-07-27 7:10 UTC (permalink / raw)
To: Adrian Hunter
Cc: Ulf Hansson, Dong Aisheng, Haibo Chen, linux-mmc, linux-kernel
On Tuesday, July 26, 2016 2:32:34 PM CEST Arnd Bergmann wrote:
> The driver has just gained a slightly incorrect pm-sleep implementation that causes
> warnings when CONFIG_PM is set but CONFIG_PM_SLEEP is not:
>
> drivers/mmc/host/sdhci-esdhc-imx.c:1302:12: error: 'sdhci_esdhc_resume' defined but not used [-Werror=unused-function]
> static int sdhci_esdhc_resume(struct device *dev)
> ^~~~~~~~~~~~~~~~~~
> drivers/mmc/host/sdhci-esdhc-imx.c:1297:12: error: 'sdhci_esdhc_suspend' defined but not used [-Werror=unused-function]
> static int sdhci_esdhc_suspend(struct device *dev)
>
> This replaces the incorrect #ifdef with a __maybe_unused annotation that does
> the right thing in all configurations and is more readable.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: b70d0b3b5b29 ("mmc: sdhci-esdhc-imx: add esdhc specific suspend resume callback")
It turns out that the version of the fix above requires another change
below. Fixing up the #ifdef as Ulf suggested would not require that,
but on the other hand, we generally don't hide declarations in header
files unless there is an alternative in the #else path, so we
probably want to do that anyway.
We could either fold this patch into the one I sent already, or
have this one applied separately and use the version that Ulf
preferred for the warning fix. I can send whichever we decide to
do with a proper changelog once we have consensus.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
diff --git a/drivers/mmc/host/sdhci-pltfm.h b/drivers/mmc/host/sdhci-pltfm.h
index d38053bf9e4d..173ef914281d 100644
--- a/drivers/mmc/host/sdhci-pltfm.h
+++ b/drivers/mmc/host/sdhci-pltfm.h
@@ -109,9 +109,9 @@ static inline void *sdhci_pltfm_priv(struct sdhci_pltfm_host *host)
return (void *)host->private;
}
-#ifdef CONFIG_PM
extern int sdhci_pltfm_suspend(struct device *dev);
extern int sdhci_pltfm_resume(struct device *dev);
+#ifdef CONFIG_PM
extern const struct dev_pm_ops sdhci_pltfm_pmops;
#define SDHCI_PLTFM_PMOPS (&sdhci_pltfm_pmops)
#else
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 0411c9f36461..9a77cb009fc8 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -686,12 +686,10 @@ void sdhci_set_bus_width(struct sdhci_host *host, int width);
void sdhci_reset(struct sdhci_host *host, u8 mask);
void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing);
-#ifdef CONFIG_PM
extern int sdhci_suspend_host(struct sdhci_host *host);
extern int sdhci_resume_host(struct sdhci_host *host);
extern void sdhci_enable_irq_wakeups(struct sdhci_host *host);
extern int sdhci_runtime_suspend_host(struct sdhci_host *host);
extern int sdhci_runtime_resume_host(struct sdhci_host *host);
-#endif
#endif /* __SDHCI_HW_H */
^ permalink raw reply related [flat|nested] 8+ messages in thread