| Message ID | 20220415200322.7511-1-paul@crapouillou.net |
|---|---|
| State | New |
| Headers | show |
| Series | brcmfmac: Remove #ifdef guards for PM related functions | expand |
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. > 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. > > /* try to attach to the target device */ > sdiodev->bus = brcmf_sdio_probe(sdiodev);
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);
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 @@ -784,7 +784,6 @@ void brcmf_sdiod_sgtable_alloc(struct brcmf_sdio_dev *sdiodev) sdiodev->txglomsz = sdiodev->settings->bus.sdio.txglomsz; } -#ifdef CONFIG_PM_SLEEP static int brcmf_sdiod_freezer_attach(struct brcmf_sdio_dev *sdiodev) { sdiodev->freezer = kzalloc(sizeof(*sdiodev->freezer), GFP_KERNEL); @@ -833,7 +832,8 @@ static void brcmf_sdiod_freezer_off(struct brcmf_sdio_dev *sdiodev) bool brcmf_sdiod_freezing(struct brcmf_sdio_dev *sdiodev) { - return atomic_read(&sdiodev->freezer->freezing); + return IS_ENABLED(CONFIG_PM_SLEEP) && + atomic_read(&sdiodev->freezer->freezing); } void brcmf_sdiod_try_freeze(struct brcmf_sdio_dev *sdiodev) @@ -847,24 +847,16 @@ void brcmf_sdiod_try_freeze(struct brcmf_sdio_dev *sdiodev) void brcmf_sdiod_freezer_count(struct brcmf_sdio_dev *sdiodev) { - atomic_inc(&sdiodev->freezer->thread_count); + if (IS_ENABLED(CONFIG_PM_SLEEP)) + atomic_inc(&sdiodev->freezer->thread_count); } void brcmf_sdiod_freezer_uncount(struct brcmf_sdio_dev *sdiodev) { - atomic_dec(&sdiodev->freezer->thread_count); -} -#else -static int brcmf_sdiod_freezer_attach(struct brcmf_sdio_dev *sdiodev) -{ - return 0; + if (IS_ENABLED(CONFIG_PM_SLEEP)) + atomic_dec(&sdiodev->freezer->thread_count); } -static void brcmf_sdiod_freezer_detach(struct brcmf_sdio_dev *sdiodev) -{ -} -#endif /* CONFIG_PM_SLEEP */ - int brcmf_sdiod_remove(struct brcmf_sdio_dev *sdiodev) { sdiodev->state = BRCMF_SDIOD_DOWN; @@ -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); /* 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; + } /* try to attach to the target device */ sdiodev->bus = brcmf_sdio_probe(sdiodev); @@ -1124,7 +1119,6 @@ void brcmf_sdio_wowl_config(struct device *dev, bool enabled) sdiodev->wowl_enabled = enabled; } -#ifdef CONFIG_PM_SLEEP static int brcmf_ops_sdio_suspend(struct device *dev) { struct sdio_func *func; @@ -1199,11 +1193,9 @@ static int brcmf_ops_sdio_resume(struct device *dev) return ret; } -static const struct dev_pm_ops brcmf_sdio_pm_ops = { - .suspend = brcmf_ops_sdio_suspend, - .resume = brcmf_ops_sdio_resume, -}; -#endif /* CONFIG_PM_SLEEP */ +static DEFINE_SIMPLE_DEV_PM_OPS(brcmf_sdio_pm_ops, + brcmf_ops_sdio_suspend, + brcmf_ops_sdio_resume); static struct sdio_driver brcmf_sdmmc_driver = { .probe = brcmf_ops_sdio_probe, @@ -1212,9 +1204,7 @@ static struct sdio_driver brcmf_sdmmc_driver = { .id_table = brcmf_sdmmc_ids, .drv = { .owner = THIS_MODULE, -#ifdef CONFIG_PM_SLEEP - .pm = &brcmf_sdio_pm_ops, -#endif /* CONFIG_PM_SLEEP */ + .pm = pm_sleep_ptr(&brcmf_sdio_pm_ops), .coredump = brcmf_dev_coredump, }, }; diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c index 55285cad527f..d34797a214ba 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c @@ -4020,15 +4020,14 @@ brcmf_sdio_probe_attach(struct brcmf_sdio *bus) */ brcmf_sdiod_sgtable_alloc(sdiodev); -#ifdef CONFIG_PM_SLEEP /* wowl can be supported when KEEP_POWER is true and (WAKE_SDIO_IRQ * is true or when platform data OOB irq is true). */ - if ((sdio_get_host_pm_caps(sdiodev->func1) & MMC_PM_KEEP_POWER) && + if (IS_ENABLED(CONFIG_PM_SLEEP) && + (sdio_get_host_pm_caps(sdiodev->func1) & MMC_PM_KEEP_POWER) && ((sdio_get_host_pm_caps(sdiodev->func1) & MMC_PM_WAKE_SDIO_IRQ) || (sdiodev->settings->bus.sdio.oob_irq_supported))) sdiodev->bus_if->wowl_supported = true; -#endif if (brcmf_sdio_kso_init(bus)) { brcmf_err("error enabling KSO\n"); diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h index 15d2c02fa3ec..47351ff458ca 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h @@ -346,26 +346,10 @@ int brcmf_sdiod_abort(struct brcmf_sdio_dev *sdiodev, struct sdio_func *func); void brcmf_sdiod_sgtable_alloc(struct brcmf_sdio_dev *sdiodev); void brcmf_sdiod_change_state(struct brcmf_sdio_dev *sdiodev, enum brcmf_sdiod_state state); -#ifdef CONFIG_PM_SLEEP bool brcmf_sdiod_freezing(struct brcmf_sdio_dev *sdiodev); void brcmf_sdiod_try_freeze(struct brcmf_sdio_dev *sdiodev); void brcmf_sdiod_freezer_count(struct brcmf_sdio_dev *sdiodev); void brcmf_sdiod_freezer_uncount(struct brcmf_sdio_dev *sdiodev); -#else -static inline bool brcmf_sdiod_freezing(struct brcmf_sdio_dev *sdiodev) -{ - return false; -} -static inline void brcmf_sdiod_try_freeze(struct brcmf_sdio_dev *sdiodev) -{ -} -static inline void brcmf_sdiod_freezer_count(struct brcmf_sdio_dev *sdiodev) -{ -} -static inline void brcmf_sdiod_freezer_uncount(struct brcmf_sdio_dev *sdiodev) -{ -} -#endif /* CONFIG_PM_SLEEP */ int brcmf_sdiod_probe(struct brcmf_sdio_dev *sdiodev); int brcmf_sdiod_remove(struct brcmf_sdio_dev *sdiodev);
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. Some other functions not directly called by the .suspend/.resume callbacks, but still related to PM were also taken outside #ifdef guards. 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(-)