| Message ID | 20150713010406.GA25443@verge.net.au (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 151dd346a2dadaa151d5110553e3fb08774c888e |
| Delegated to: | Simon Horman |
| Headers | show |
On Mon, Jul 13, 2015 at 10:04:06AM +0900, Simon Horman wrote: > Hi Wolfram, > > On Fri, Jul 10, 2015 at 10:48:16PM +0200, Wolfram Sang wrote: > > From: Wolfram Sang <wsa+renesas@sang-engineering.com> > > > > With shmobile_defconfig but SMP=n && SUSPEND=n, I get: > > > > arch/arm/mach-shmobile/platsmp-apmu.c:49:12: warning: 'apmu_power_off' defined but not used [-Wunused-function] > > arch/arm/mach-shmobile/platsmp-apmu.c:70:12: warning: 'apmu_wrap' defined but not used [-Wunused-function] > > > > Annotate those functions like the functions around it. > > thanks for noticing this. > > I'm not familiar with when it is appropriate to use __maybe_unused but does > anything speak against using #if as per the conditional compilation of the > callers in platsmp-apmu.c of the above functions? I did my approach for consistency reasons. It looked to me that the *_on/off functions wanted to be grouped at the beginning. Your approach works but looks fragile to me as soon as the ifdeffery of the callers change. So, we could move the code into the callers ifdef block, but we should do this consistently, even for the functions currently marked __maybe_unused. That would scatter them a little, though. apmu_wrap needs its own ifdeffery in any case. Do you have a preference? > > diff --git a/arch/arm/mach-shmobile/platsmp-apmu.c b/arch/arm/mach-shmobile/platsmp-apmu.c > index b0790fc32282..54cf153b570f 100644 > --- a/arch/arm/mach-shmobile/platsmp-apmu.c > +++ b/arch/arm/mach-shmobile/platsmp-apmu.c > @@ -46,12 +46,14 @@ static int __maybe_unused apmu_power_on(void __iomem *p, int bit) > return 0; > } > > +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_SUSPEND) > static int apmu_power_off(void __iomem *p, int bit) > { > /* request Core Standby for next WFI */ > writel_relaxed(3, p + CPUNCR_OFFS(bit)); > return 0; > } > +#endif > > static int __maybe_unused apmu_power_off_poll(void __iomem *p, int bit) > { > @@ -67,12 +69,14 @@ static int __maybe_unused apmu_power_off_poll(void __iomem *p, int bit) > return 0; > } > > +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_SUSPEND) || defined(CONFIG_SMP) > static int apmu_wrap(int cpu, int (*fn)(void __iomem *p, int cpu)) > { > void __iomem *p = apmu_cpus[cpu].iomem; > > return p ? fn(p, apmu_cpus[cpu].bit) : -EINVAL; > } > +#endif > > static void apmu_init_cpu(struct resource *res, int cpu, int bit) > {
On Mon, Jul 13, 2015 at 10:58:07AM +0200, Wolfram Sang wrote: > On Mon, Jul 13, 2015 at 10:04:06AM +0900, Simon Horman wrote: > > Hi Wolfram, > > > > On Fri, Jul 10, 2015 at 10:48:16PM +0200, Wolfram Sang wrote: > > > From: Wolfram Sang <wsa+renesas@sang-engineering.com> > > > > > > With shmobile_defconfig but SMP=n && SUSPEND=n, I get: > > > > > > arch/arm/mach-shmobile/platsmp-apmu.c:49:12: warning: 'apmu_power_off' defined but not used [-Wunused-function] > > > arch/arm/mach-shmobile/platsmp-apmu.c:70:12: warning: 'apmu_wrap' defined but not used [-Wunused-function] > > > > > > Annotate those functions like the functions around it. > > > > thanks for noticing this. > > > > I'm not familiar with when it is appropriate to use __maybe_unused but does > > anything speak against using #if as per the conditional compilation of the > > callers in platsmp-apmu.c of the above functions? > > I did my approach for consistency reasons. It looked to me that the > *_on/off functions wanted to be grouped at the beginning. > > Your approach works but looks fragile to me as soon as the ifdeffery of > the callers change. So, we could move the code into the callers ifdef > block, but we should do this consistently, even for the functions > currently marked __maybe_unused. That would scatter them a little, > though. apmu_wrap needs its own ifdeffery in any case. > > Do you have a preference? Less fragile sounds good. I have queued up your patch. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/mach-shmobile/platsmp-apmu.c b/arch/arm/mach-shmobile/platsmp-apmu.c index b0790fc32282..54cf153b570f 100644 --- a/arch/arm/mach-shmobile/platsmp-apmu.c +++ b/arch/arm/mach-shmobile/platsmp-apmu.c @@ -46,12 +46,14 @@ static int __maybe_unused apmu_power_on(void __iomem *p, int bit) return 0; } +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_SUSPEND) static int apmu_power_off(void __iomem *p, int bit) { /* request Core Standby for next WFI */ writel_relaxed(3, p + CPUNCR_OFFS(bit)); return 0; } +#endif static int __maybe_unused apmu_power_off_poll(void __iomem *p, int bit) { @@ -67,12 +69,14 @@ static int __maybe_unused apmu_power_off_poll(void __iomem *p, int bit) return 0; } +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_SUSPEND) || defined(CONFIG_SMP) static int apmu_wrap(int cpu, int (*fn)(void __iomem *p, int cpu)) { void __iomem *p = apmu_cpus[cpu].iomem; return p ? fn(p, apmu_cpus[cpu].bit) : -EINVAL; } +#endif static void apmu_init_cpu(struct resource *res, int cpu, int bit) {