| Message ID | 20201211190335.16501-1-rdunlap@infradead.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [-next] platform: surface: fix non-PM_SLEEP build warnings | expand |
On 12/11/20 8:03 PM, Randy Dunlap wrote: > Fix build warnings when CONFIG_PM_SLEEP is not enabled and these > functions are not used: > > ../drivers/platform/surface/surface_gpe.c:189:12: warning: ‘surface_gpe_resume’ defined but not used [-Wunused-function] > static int surface_gpe_resume(struct device *dev) > ^~~~~~~~~~~~~~~~~~ > ../drivers/platform/surface/surface_gpe.c:184:12: warning: ‘surface_gpe_suspend’ defined but not used [-Wunused-function] > static int surface_gpe_suspend(struct device *dev) > ^~~~~~~~~~~~~~~~~~~ > > Fixes: 274335f1c557 ("platform/surface: Add Driver to set up lid GPEs on MS Surface device") > Signed-off-by: Randy Dunlap <rdunlap@infradead.org> > Cc: Maximilian Luz <luzmaximilian@gmail.com> > Cc: Hans de Goede <hdegoede@redhat.com> > Cc: platform-driver-x86@vger.kernel.org > --- > drivers/platform/surface/surface_gpe.c | 2 ++ > 1 file changed, 2 insertions(+) > > --- linux-next-20201210.orig/drivers/platform/surface/surface_gpe.c > +++ linux-next-20201210/drivers/platform/surface/surface_gpe.c > @@ -181,6 +181,7 @@ static int surface_lid_enable_wakeup(str > return 0; > } > > +#ifdef CONFIG_PM_SLEEP > static int surface_gpe_suspend(struct device *dev) > { > return surface_lid_enable_wakeup(dev, true); > @@ -190,6 +191,7 @@ static int surface_gpe_resume(struct dev > { > return surface_lid_enable_wakeup(dev, false); > } > +#endif > > static SIMPLE_DEV_PM_OPS(surface_gpe_pm, surface_gpe_suspend, surface_gpe_resume); > > Right, thanks. I assume this covers all instances of this warning in platform/surface? Otherwise, a "platform: surface: gpe: ..." subject would make more sense. As for the rest: Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com> Regards, Max
On 12/11/20 12:23 PM, Maximilian Luz wrote: > On 12/11/20 8:03 PM, Randy Dunlap wrote: >> Fix build warnings when CONFIG_PM_SLEEP is not enabled and these >> functions are not used: >> >> ../drivers/platform/surface/surface_gpe.c:189:12: warning: ‘surface_gpe_resume’ defined but not used [-Wunused-function] >> static int surface_gpe_resume(struct device *dev) >> ^~~~~~~~~~~~~~~~~~ >> ../drivers/platform/surface/surface_gpe.c:184:12: warning: ‘surface_gpe_suspend’ defined but not used [-Wunused-function] >> static int surface_gpe_suspend(struct device *dev) >> ^~~~~~~~~~~~~~~~~~~ >> >> Fixes: 274335f1c557 ("platform/surface: Add Driver to set up lid GPEs on MS Surface device") >> Signed-off-by: Randy Dunlap <rdunlap@infradead.org> >> Cc: Maximilian Luz <luzmaximilian@gmail.com> >> Cc: Hans de Goede <hdegoede@redhat.com> >> Cc: platform-driver-x86@vger.kernel.org >> --- >> drivers/platform/surface/surface_gpe.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> --- linux-next-20201210.orig/drivers/platform/surface/surface_gpe.c >> +++ linux-next-20201210/drivers/platform/surface/surface_gpe.c >> @@ -181,6 +181,7 @@ static int surface_lid_enable_wakeup(str >> return 0; >> } >> +#ifdef CONFIG_PM_SLEEP >> static int surface_gpe_suspend(struct device *dev) >> { >> return surface_lid_enable_wakeup(dev, true); >> @@ -190,6 +191,7 @@ static int surface_gpe_resume(struct dev >> { >> return surface_lid_enable_wakeup(dev, false); >> } >> +#endif >> static SIMPLE_DEV_PM_OPS(surface_gpe_pm, surface_gpe_suspend, surface_gpe_resume); >> > > Right, thanks. > > I assume this covers all instances of this warning in platform/surface? > Otherwise, a "platform: surface: gpe: ..." subject would make more sense. It should cover all of surface/. It was an allmodconfig and then I disabled CONFIG_PM and CONFIG_PM_SLEEP etc. As for prefixes, how many levels do we want to use? (that's mostly rhetorical, although I would read answers :) > As for the rest: > > Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com> thanks.
On 12/11/20 9:41 PM, Randy Dunlap wrote: > On 12/11/20 12:23 PM, Maximilian Luz wrote: >> On 12/11/20 8:03 PM, Randy Dunlap wrote: >>> Fix build warnings when CONFIG_PM_SLEEP is not enabled and these >>> functions are not used: >>> >>> ../drivers/platform/surface/surface_gpe.c:189:12: warning: ‘surface_gpe_resume’ defined but not used [-Wunused-function] >>> static int surface_gpe_resume(struct device *dev) >>> ^~~~~~~~~~~~~~~~~~ >>> ../drivers/platform/surface/surface_gpe.c:184:12: warning: ‘surface_gpe_suspend’ defined but not used [-Wunused-function] >>> static int surface_gpe_suspend(struct device *dev) >>> ^~~~~~~~~~~~~~~~~~~ >>> >>> Fixes: 274335f1c557 ("platform/surface: Add Driver to set up lid GPEs on MS Surface device") >>> Signed-off-by: Randy Dunlap <rdunlap@infradead.org> >>> Cc: Maximilian Luz <luzmaximilian@gmail.com> >>> Cc: Hans de Goede <hdegoede@redhat.com> >>> Cc: platform-driver-x86@vger.kernel.org >>> --- >>> drivers/platform/surface/surface_gpe.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> --- linux-next-20201210.orig/drivers/platform/surface/surface_gpe.c >>> +++ linux-next-20201210/drivers/platform/surface/surface_gpe.c >>> @@ -181,6 +181,7 @@ static int surface_lid_enable_wakeup(str >>> return 0; >>> } >>> +#ifdef CONFIG_PM_SLEEP >>> static int surface_gpe_suspend(struct device *dev) >>> { >>> return surface_lid_enable_wakeup(dev, true); >>> @@ -190,6 +191,7 @@ static int surface_gpe_resume(struct dev >>> { >>> return surface_lid_enable_wakeup(dev, false); >>> } >>> +#endif >>> static SIMPLE_DEV_PM_OPS(surface_gpe_pm, surface_gpe_suspend, surface_gpe_resume); >>> >> >> Right, thanks. >> >> I assume this covers all instances of this warning in platform/surface? >> Otherwise, a "platform: surface: gpe: ..." subject would make more sense. > > It should cover all of surface/. It was an allmodconfig and then I disabled > CONFIG_PM and CONFIG_PM_SLEEP etc. Perfect, thanks! > As for prefixes, how many levels do we want to use? > (that's mostly rhetorical, although I would read answers :) Looking at platform/x86 and past commit messages, I'd prefer something like platform/surface: <component>: <message> This would be similar to the platform/x86 style. So two or three, depending on how you count "platform/surface". I agree that this probably tends to get a bit long, so I propose we drop the surface_ prefix on the component part to help with that. So, for example, "surface_gpe" will become "gpe". > >> As for the rest: >> >> Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com> > > thanks. > Regards, Max
On Fri, Dec 11, 2020 at 9:20 PM Randy Dunlap <rdunlap@infradead.org> wrote: > > Fix build warnings when CONFIG_PM_SLEEP is not enabled and these > functions are not used: > > ../drivers/platform/surface/surface_gpe.c:189:12: warning: ‘surface_gpe_resume’ defined but not used [-Wunused-function] > static int surface_gpe_resume(struct device *dev) > ^~~~~~~~~~~~~~~~~~ > ../drivers/platform/surface/surface_gpe.c:184:12: warning: ‘surface_gpe_suspend’ defined but not used [-Wunused-function] > static int surface_gpe_suspend(struct device *dev) > ^~~~~~~~~~~~~~~~~~~ ... > +#ifdef CONFIG_PM_SLEEP > static int surface_gpe_suspend(struct device *dev) Perhaps __maybe_unused ?
On 12/12/20 5:24 AM, Andy Shevchenko wrote: > On Fri, Dec 11, 2020 at 9:20 PM Randy Dunlap <rdunlap@infradead.org> wrote: >> >> Fix build warnings when CONFIG_PM_SLEEP is not enabled and these >> functions are not used: >> >> ../drivers/platform/surface/surface_gpe.c:189:12: warning: ‘surface_gpe_resume’ defined but not used [-Wunused-function] >> static int surface_gpe_resume(struct device *dev) >> ^~~~~~~~~~~~~~~~~~ >> ../drivers/platform/surface/surface_gpe.c:184:12: warning: ‘surface_gpe_suspend’ defined but not used [-Wunused-function] >> static int surface_gpe_suspend(struct device *dev) >> ^~~~~~~~~~~~~~~~~~~ > > ... > >> +#ifdef CONFIG_PM_SLEEP >> static int surface_gpe_suspend(struct device *dev) > > Perhaps __maybe_unused ? > Yes, I am aware of that option. I don't know why it would be preferred though.
On Sat, Dec 12, 2020 at 7:05 PM Randy Dunlap <rdunlap@infradead.org> wrote: > On 12/12/20 5:24 AM, Andy Shevchenko wrote: > > On Fri, Dec 11, 2020 at 9:20 PM Randy Dunlap <rdunlap@infradead.org> wrote: ... > >> +#ifdef CONFIG_PM_SLEEP > >> static int surface_gpe_suspend(struct device *dev) > > > > Perhaps __maybe_unused ? > > Yes, I am aware of that option. > I don't know why it would be preferred though. Here [1] is the rationale behind annotation vs. ifdeffery. [1]: https://lore.kernel.org/patchwork/patch/732981/
On 12/12/20 11:07 AM, Andy Shevchenko wrote: > On Sat, Dec 12, 2020 at 7:05 PM Randy Dunlap <rdunlap@infradead.org> wrote: >> On 12/12/20 5:24 AM, Andy Shevchenko wrote: >>> On Fri, Dec 11, 2020 at 9:20 PM Randy Dunlap <rdunlap@infradead.org> wrote: > > ... > >>>> +#ifdef CONFIG_PM_SLEEP >>>> static int surface_gpe_suspend(struct device *dev) >>> >>> Perhaps __maybe_unused ? >> >> Yes, I am aware of that option. >> I don't know why it would be preferred though. > > Here [1] is the rationale behind annotation vs. ifdeffery. > > [1]: https://lore.kernel.org/patchwork/patch/732981/ > Thanks for the link. I'll send a v2. Could we add that to the Linux BKP (Best Known Practices) document?
On Mon, Dec 14, 2020 at 2:53 AM Randy Dunlap <rdunlap@infradead.org> wrote: > On 12/12/20 11:07 AM, Andy Shevchenko wrote: > > On Sat, Dec 12, 2020 at 7:05 PM Randy Dunlap <rdunlap@infradead.org> wrote: ... > > Here [1] is the rationale behind annotation vs. ifdeffery. > > > > [1]: https://lore.kernel.org/patchwork/patch/732981/ > > > Thanks for the link. I'll send a v2. > > Could we add that to the Linux BKP (Best Known Practices) > document? Perhaps. The author of that is Arnd, maybe he has something to add.
On 12/14/20 3:19 AM, Andy Shevchenko wrote: > On Mon, Dec 14, 2020 at 2:53 AM Randy Dunlap <rdunlap@infradead.org> wrote: >> On 12/12/20 11:07 AM, Andy Shevchenko wrote: >>> On Sat, Dec 12, 2020 at 7:05 PM Randy Dunlap <rdunlap@infradead.org> wrote: > > ... > >>> Here [1] is the rationale behind annotation vs. ifdeffery. >>> >>> [1]: https://lore.kernel.org/patchwork/patch/732981/ >>> >> Thanks for the link. I'll send a v2. >> >> Could we add that to the Linux BKP (Best Known Practices) >> document? > > Perhaps. The author of that is Arnd, maybe he has something to add. > Where is it located? My search foo could not find it. thanks.
On Tue, Dec 15, 2020 at 1:49 AM Randy Dunlap <rdunlap@infradead.org> wrote: > On 12/14/20 3:19 AM, Andy Shevchenko wrote: > > On Mon, Dec 14, 2020 at 2:53 AM Randy Dunlap <rdunlap@infradead.org> wrote: > >> On 12/12/20 11:07 AM, Andy Shevchenko wrote: > >>> On Sat, Dec 12, 2020 at 7:05 PM Randy Dunlap <rdunlap@infradead.org> wrote: > > > > ... > > > >>> Here [1] is the rationale behind annotation vs. ifdeffery. > >>> > >>> [1]: https://lore.kernel.org/patchwork/patch/732981/ > >>> > >> Thanks for the link. I'll send a v2. > >> > >> Could we add that to the Linux BKP (Best Known Practices) > >> document? > > > > Perhaps. The author of that is Arnd, maybe he has something to add. > > > > Where is it located? My search foo could not find it. Closest what I know is [2]. [2]: https://kernelnewbies.org/FAQ/CodingStyle -- With Best Regards, Andy Shevchenko
--- linux-next-20201210.orig/drivers/platform/surface/surface_gpe.c +++ linux-next-20201210/drivers/platform/surface/surface_gpe.c @@ -181,6 +181,7 @@ static int surface_lid_enable_wakeup(str return 0; } +#ifdef CONFIG_PM_SLEEP static int surface_gpe_suspend(struct device *dev) { return surface_lid_enable_wakeup(dev, true); @@ -190,6 +191,7 @@ static int surface_gpe_resume(struct dev { return surface_lid_enable_wakeup(dev, false); } +#endif static SIMPLE_DEV_PM_OPS(surface_gpe_pm, surface_gpe_suspend, surface_gpe_resume);
Fix build warnings when CONFIG_PM_SLEEP is not enabled and these functions are not used: ../drivers/platform/surface/surface_gpe.c:189:12: warning: ‘surface_gpe_resume’ defined but not used [-Wunused-function] static int surface_gpe_resume(struct device *dev) ^~~~~~~~~~~~~~~~~~ ../drivers/platform/surface/surface_gpe.c:184:12: warning: ‘surface_gpe_suspend’ defined but not used [-Wunused-function] static int surface_gpe_suspend(struct device *dev) ^~~~~~~~~~~~~~~~~~~ Fixes: 274335f1c557 ("platform/surface: Add Driver to set up lid GPEs on MS Surface device") Signed-off-by: Randy Dunlap <rdunlap@infradead.org> Cc: Maximilian Luz <luzmaximilian@gmail.com> Cc: Hans de Goede <hdegoede@redhat.com> Cc: platform-driver-x86@vger.kernel.org --- drivers/platform/surface/surface_gpe.c | 2 ++ 1 file changed, 2 insertions(+)