Re: [PATCH v9 4/5] wifi: brcmfmac: Add optional lpo clock enable support

Arend van Spriel [off-list ref] writes:

On 8/10/2024 12:08 PM, jacobe.zang@wesion.com wrote:

quoted

On 2024/8/10 17:44, Sai Krishna Gajula [off-list ref]
wrote:

quoted

quoted

-----Original Message-----
From: Jacobe Zang <redacted>
Sent: Saturday, August 10, 2024 9:22 AM
To: robh@kernel.org; krzk+dt@kernel.org; heiko@sntech.de;
kvalo@kernel.org; davem@davemloft.net; edumazet@google.com;
kuba@kernel.org; pabeni@redhat.com; conor+dt@kernel.org;
arend.vanspriel@broadcom.com
Cc: efectn@protonmail.com; dsimic@manjaro.org; jagan@edgeble.ai;
devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;

linux-

quoted

rockchip@lists.infradead.org; linux-kernel@vger.kernel.org;
arend@broadcom.com; linux-wireless@vger.kernel.org;
netdev@vger.kernel.org; megi@xff.cz; duoming@zju.edu.cn;
bhelgaas@google.com; minipli@grsecurity.net; brcm80211@lists.linux.dev;
brcm80211-dev-list.pdl@broadcom.com; nick@khadas.com; Jacobe Zang
[off-list ref]
Subject:  [PATCH v9 4/5] wifi: brcmfmac: Add optional lpo clock
enable support

WiFi modules often require 32kHz clock to function. Add support

to enable

quoted

the clock to PCIe driver and move "brcm,bcm4329-fmac" check to

the top of

quoted

brcmf_of_probe. Change function prototypes from void to int and add
appropriate errno's for return
WiFi modules often require 32kHz clock to function. Add support

to enable

quoted

the clock to PCIe driver and move "brcm,bcm4329-fmac" check to

the top of

quoted

brcmf_of_probe. Change function prototypes from void to int and add
appropriate errno's for return values that will be send to bus

when error

quoted

occurred.

Co-developed-by: Ondrej Jirman <megi@xff.cz>
Signed-off-by: Ondrej Jirman <megi@xff.cz>
Co-developed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
Signed-off-by: Jacobe Zang <redacted>
---
  .../broadcom/brcm80211/brcmfmac/bcmsdh.c      |  4 +-
  .../broadcom/brcm80211/brcmfmac/common.c      |  3 +-
  .../wireless/broadcom/brcm80211/brcmfmac/of.c | 53

+++++++++++--------

quoted

.../wireless/broadcom/brcm80211/brcmfmac/of.h |  9 ++--
  .../broadcom/brcm80211/brcmfmac/pcie.c        |  3 ++
  .../broadcom/brcm80211/brcmfmac/sdio.c        | 24 ++++++---
  .../broadcom/brcm80211/brcmfmac/usb.c         |  3 ++
  7 files changed, 63 insertions(+), 36 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c

b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
index 13391c2d82aae..b2ede4e579c5c 100644

--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c

@@ -947,8 +947,8 @@ int brcmf_sdiod_probe(struct brcmf_sdio_dev

*sdiodev)

      /* try to attach to the target device */
      sdiodev->bus = brcmf_sdio_probe(sdiodev);
-    if (!sdiodev->bus) {
-        ret = -ENODEV;
+    if (IS_ERR(sdiodev->bus)) {
+        ret = PTR_ERR(sdiodev->bus);
          goto out;
      }
      brcmf_sdiod_host_fixup(sdiodev->func2->card->host);

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c

b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
index b24faae35873d..58d50918dd177 100644

--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c

@@ -561,7 +561,8 @@ struct brcmf_mp_device

*brcmf_get_module_param(struct device *dev,
      if (!found) {
          /* No platform data for this device, try OF and DMI data */
          brcmf_dmi_probe(settings, chip, chiprev);
-        brcmf_of_probe(dev, bus_type, settings);
+        if (brcmf_of_probe(dev, bus_type, settings) == -
EPROBE_DEFER)
+            return ERR_PTR(-EPROBE_DEFER);
          brcmf_acpi_probe(dev, bus_type, settings);
      }
      return settings;

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c

b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
index e406e11481a62..f19dc7355e0e8 100644

--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c

@@ -6,6 +6,7 @@

  #include <linux/of.h>
  #include <linux/of_irq.h>
  #include <linux/of_net.h>
+#include <linux/clk.h>

  #include <defs.h>
  #include "debug.h"

@@ -65,17 +66,21 @@ static int brcmf_of_get_country_codes(struct device

*dev,
      return 0;
  }

-void brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type,
-            struct brcmf_mp_device *settings)
+int brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type,
+           struct brcmf_mp_device *settings)
  {
      struct brcmfmac_sdio_pd *sdio = &settings->bus.sdio;
      struct device_node *root, *np = dev->of_node;
+    struct clk *clk;
      const char *prop;

Small nit, please check if reverse x-mas tree order need to be
follow here.

quoted

      int irq;
      int err;
      u32 irqf;

It can be seen from this line that there should be no need to follow
the reverse x-mas tree order. Because it is a struct variable, so
place with other struct ones.

As driver maintainer I do not care about such neatness, but maybe
Kalle has another preference. The code above looks fine to me.

We haven't been strongly requiring reverse x-mas tree in wireless code,
it has been just a preference. Also for me the code above looks fine.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches