Messages in this thread | | | Date | Tue, 19 Apr 2022 18:22:09 +0100 | From | Paul Cercueil <> | Subject | Re: [PATCH] brcmfmac: Remove #ifdef guards for PM related functions |
| |
Hi Arend,
Le lun., avril 18 2022 at 09:09:46 +0200, Arend van Spriel <arend.vanspriel@broadcom.com> a écrit : > On 4/15/2022 10:03 PM, Paul Cercueil wrote: >> Use the new DEFINE_SIMPLE_DEV_PM_OPS() and pm_sleep_ptr() macros to >> handle the .suspend/.resume callbacks. >> >> These macros allow the suspend and resume functions to be >> automatically >> dropped by the compiler when CONFIG_SUSPEND is disabled, without >> having >> to use #ifdef guards. The advantage is then that these functions are >> not >> conditionally compiled. > > The advantage stated here may not be obvious to everyone and that is > because it only scratches the surface. The code is always compiled > independent from the Kconfig options used and because of that the > real advantage is that build regressions are easier to catch.
Exactly. I will improve the commit message to make this a bit more explicit.
>> Some other functions not directly called by the .suspend/.resume >> callbacks, but still related to PM were also taken outside #ifdef >> guards. > > a few comments on this inline... > >> Signed-off-by: Paul Cercueil <paul@crapouillou.net> >> --- >> .../broadcom/brcm80211/brcmfmac/bcmsdh.c | 44 >> +++++++------------ >> .../broadcom/brcm80211/brcmfmac/sdio.c | 5 +-- >> .../broadcom/brcm80211/brcmfmac/sdio.h | 16 ------- >> 3 files changed, 19 insertions(+), 46 deletions(-) >> >> diff --git >> a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c >> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c >> index ac02244a6fdf..a8cf5a570101 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > > [...] > >> @@ -873,7 +865,8 @@ int brcmf_sdiod_remove(struct brcmf_sdio_dev >> *sdiodev) >> sdiodev->bus = NULL; >> } >> - brcmf_sdiod_freezer_detach(sdiodev); >> + if (IS_ENABLED(CONFIG_PM_SLEEP)) >> + brcmf_sdiod_freezer_detach(sdiodev); > > Please move the if statement inside the function to keep the code > flow in the calling function the same as before. > >> /* Disable Function 2 */ >> sdio_claim_host(sdiodev->func2); >> @@ -949,9 +942,11 @@ int brcmf_sdiod_probe(struct brcmf_sdio_dev >> *sdiodev) >> goto out; >> } >> - ret = brcmf_sdiod_freezer_attach(sdiodev); >> - if (ret) >> - goto out; >> + if (IS_ENABLED(CONFIG_PM_SLEEP)) { >> + ret = brcmf_sdiod_freezer_attach(sdiodev); >> + if (ret) >> + goto out; >> + } > > Dito. Move the if statement inside the function.
Sure.
Cheers, -Paul
> >> /* try to attach to the target device */ >> sdiodev->bus = brcmf_sdio_probe(sdiodev);
| |