On 07/08/2024 2:17 pm, Arend van Spriel wrote:

On 8/7/2024 1:10 AM, Alexey Charkov wrote:

quoted

Hi Jacobe,

On 05/08/2024 10:34 am, Jacobe Zang wrote:

quoted

WiFi modules often require 32kHz clock to function. Add support to
enable the clock to PCIe driver and move "brcm,bcm4329-fmac" check
to the top of 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 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      |  6 +++-
  .../wireless/broadcom/brcm80211/brcmfmac/of.c | 28 +++++++++++++------
  .../wireless/broadcom/brcm80211/brcmfmac/of.h |  9 +++---
  .../broadcom/brcm80211/brcmfmac/pcie.c        |  3 ++
  .../broadcom/brcm80211/brcmfmac/sdio.c        | 18 ++++++++----
  .../broadcom/brcm80211/brcmfmac/usb.c         |  3 ++
  7 files changed, 52 insertions(+), 19 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/

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

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

@@ -951,6 +951,10 @@ int brcmf_sdiod_probe(struct brcmf_sdio_dev

*sdiodev)
          ret = -ENODEV;
          goto out;
      }
+    if (IS_ERR(sdiodev->bus)) {
+        ret = PTR_ERR(sdiodev->bus);
+        goto out;
+    }

Maybe return -ENODEV error pointer instead of NULL from
brcmf_sdio_probe as the default for the fail path? Then you can
condense these two checks into one

Sound reasonable.

quoted

quoted

      brcmf_sdiod_host_fixup(sdiodev->func2->card->host);
  out:
      if (ret)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/

common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
index b24faae35873d..6c5d26f9b7661 100644

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

@@ -561,8 +561,12 @@ 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);
          brcmf_acpi_probe(dev, bus_type, settings);
+        i = brcmf_of_probe(dev, bus_type, settings);
+        if (i < 0) {
+            kfree(settings);
+            settings = ERR_PTR(i);
+        }

This looks wrong. First, you're calling brcmf_of_probe twice. Second,
if either DMI or ACPI probe successfully but OF doesn't, then you
return an error code instead of success, and also overwrite settings
with an error pointer thus rendering both brcmf_dmi_probe and
brcmf_acpi_probe useless

Twice? it is removed and added few lines below.

Indeed, time to change glasses :) Didn't see the minus sign

It does change the order
so that may not be best thing to do here. We actually only want to
handle the scenario where the clock resources are not yet available, ie.
when -EPROBE_DEFER is returned because that error value is taken into
account by the bus driver and tries to bind the driver again later.

Maybe we then do something like the following, which would retain the
old behavior but pass -EPROBE_DEFER on to the bus:

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

b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
index b24faae35873d..6c5d26f9b7661 100644

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

@@ -561,8 +561,12 @@ 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;
 }

quoted

quoted

      }
      return settings;
  }

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

drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
index e406e11481a62..5f61363fb5d0e 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;
      int irq;
      int err;
      u32 irqf;
      u32 val;
+    if (!np || !of_device_is_compatible(np, "brcm,bcm4329-fmac"))
+        return 0;
+

return 0 implies this function has completed successfully, while in
this case it's obviously returned early due to not finding the correct
device in DT. -ENODEV perhaps?

This was a void function so returning 0 retains the behavior as before,
which is important to keep in mind here.

This function will be called if the platform has CONFIG_OF enabled.
However, that does not mean that on every platform there is a node
defined for the struct device being probed. That is fine if it does not
require any DT properties to be functional. Hence we bail out here
without an error.

Fair enough, thanks for the explanation!

Best regards,
Alexey