From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6B87BC433F5 for ; Tue, 19 Apr 2022 17:22:32 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1351435AbiDSRZM (ORCPT ); Tue, 19 Apr 2022 13:25:12 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48208 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1348515AbiDSRZJ (ORCPT ); Tue, 19 Apr 2022 13:25:09 -0400 Received: from aposti.net (aposti.net [89.234.176.197]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 344243C4B3; Tue, 19 Apr 2022 10:22:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=crapouillou.net; s=mail; t=1650388939; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=V5MZ44J/2p8fy6DuvcxG86awKyPycaxt+fsG7bZhs1M=; b=NEBwuEgxhM19apjy6kKt9GrTDsZoBBGDWU2ZgYgPer3n7SvOqsFJ+DUsQ1pIgB4+F/vk9H rOGiYyfKvkzS7g0Rp9uvovjoBpTIvxp+4Xo7qjs+6+DZK09qsExxcwTLRoRDdzu7z97trg EYYwEDCxzmZcauHNO2JYncVeYrydSD8= Date: Tue, 19 Apr 2022 18:22:09 +0100 From: Paul Cercueil Subject: Re: [PATCH] brcmfmac: Remove #ifdef guards for PM related functions To: Arend van Spriel Cc: Arend van Spriel , Franky Lin , Hante Meuleman , Kalle Valo , linux-wireless@vger.kernel.org, brcm80211-dev-list.pdl@broadcom.com, SHA-cyfmac-dev-list@infineon.com, netdev@vger.kernel.org, linux-kernel@vger.kernel.org Message-Id: In-Reply-To: References: <20220415200322.7511-1-paul@crapouillou.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1; format=flowed Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Arend, Le lun., avril 18 2022 at 09:09:46 +0200, Arend van Spriel=20 a =E9crit : > 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. >>=20 >> These macros allow the suspend and resume functions to be=20 >> automatically >> dropped by the compiler when CONFIG_SUSPEND is disabled, without=20 >> having >> to use #ifdef guards. The advantage is then that these functions are=20 >> not >> conditionally compiled. >=20 > The advantage stated here may not be obvious to everyone and that is=20 > because it only scratches the surface. The code is always compiled=20 > independent from the Kconfig options used and because of that the=20 > real advantage is that build regressions are easier to catch. Exactly. I will improve the commit message to make this a bit more=20 explicit. >> Some other functions not directly called by the .suspend/.resume >> callbacks, but still related to PM were also taken outside #ifdef >> guards. >=20 > a few comments on this inline... >=20 >> Signed-off-by: Paul Cercueil >> --- >> .../broadcom/brcm80211/brcmfmac/bcmsdh.c | 44=20 >> +++++++------------ >> .../broadcom/brcm80211/brcmfmac/sdio.c | 5 +-- >> .../broadcom/brcm80211/brcmfmac/sdio.h | 16 ------- >> 3 files changed, 19 insertions(+), 46 deletions(-) >>=20 >> diff --git=20 >> a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c=20 >> 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 >=20 > [...] >=20 >> @@ -873,7 +865,8 @@ int brcmf_sdiod_remove(struct brcmf_sdio_dev=20 >> *sdiodev) >> sdiodev->bus =3D NULL; >> } >> =7F- brcmf_sdiod_freezer_detach(sdiodev); >> + if (IS_ENABLED(CONFIG_PM_SLEEP)) >> + brcmf_sdiod_freezer_detach(sdiodev); >=20 > Please move the if statement inside the function to keep the code=20 > flow in the calling function the same as before. >=20 >> =7F /* Disable Function 2 */ >> sdio_claim_host(sdiodev->func2); >> @@ -949,9 +942,11 @@ int brcmf_sdiod_probe(struct brcmf_sdio_dev=20 >> *sdiodev) >> goto out; >> } >> =7F- ret =3D brcmf_sdiod_freezer_attach(sdiodev); >> - if (ret) >> - goto out; >> + if (IS_ENABLED(CONFIG_PM_SLEEP)) { >> + ret =3D brcmf_sdiod_freezer_attach(sdiodev); >> + if (ret) >> + goto out; >> + } >=20 > Dito. Move the if statement inside the function. Sure. Cheers, -Paul >=20 >> =7F /* try to attach to the target device */ >> sdiodev->bus =3D brcmf_sdio_probe(sdiodev);