Re: [PATCH] brcmfmac: Remove #ifdef guards for PM related functions

[Arend van Spriel <arend.vanspriel@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 2365 bytes --]

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);

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]