On 07/08/2024 2:17 pm, Arend van Spriel wrote:
> On 8/7/2024 1:10 AM, Alexey Charkov wrote:
>> Hi Jacobe,
>>
>> On 05/08/2024 10:34 am, Jacobe Zang wrote:
>>> 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@xxxxxx>
>>> Signed-off-by: Ondrej Jirman <megi@xxxxxx>
>>> Co-developed-by: Arend van Spriel <arend.vanspriel@xxxxxxxxxxxx>
>>> Signed-off-by: Arend van Spriel <arend.vanspriel@xxxxxxxxxxxx>
>>> Signed-off-by: Jacobe Zang <jacobe.zang@xxxxxxxxxx>
>>> ---
>>> .../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.
>
>>> 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;
}
>>> }
>>> 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
