| Message ID | 20240805073425.3492078-5-jacobe.zang@wesion.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | Add AP6275P wireless support | expand |
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@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 <jacobe.zang@wesion.com> > --- > .../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 > 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 > } > 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? > /* Apple ARM64 platforms have their own idea of board type, passed in > * via the device tree. They also have an antenna SKU parameter > */ > @@ -105,7 +110,7 @@ void brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type, > board_type = devm_kstrdup(dev, tmp, GFP_KERNEL); > if (!board_type) { > of_node_put(root); > - return; > + return 0; -ENOMEM? > } > strreplace(board_type, '/', '-'); > settings->board_type = board_type; > @@ -113,8 +118,13 @@ void brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type, > of_node_put(root); > } > > - if (!np || !of_device_is_compatible(np, "brcm,bcm4329-fmac")) > - return; > + clk = devm_clk_get_optional_enabled(dev, "lpo"); > + if (!IS_ERR_OR_NULL(clk)) { > + brcmf_dbg(INFO, "enabling 32kHz clock\n"); > + clk_set_rate(clk, 32768); > + } else if (PTR_ERR_OR_ZERO(clk) == -EPROBE_DEFER) { PTR_ERR should be enough, no? Or better yet move this to the bottom of the function as was discussed on your previous submission, then you can return PTR_ERR_OR_ZERO(clk) right away, which would be a bit neater. > + return -EPROBE_DEFER; > + } > > err = brcmf_of_get_country_codes(dev, settings); > if (err) > @@ -123,23 +133,25 @@ void brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type, > of_get_mac_address(np, settings->mac); > > if (bus_type != BRCMF_BUSTYPE_SDIO) > - return; > + return 0; > > if (of_property_read_u32(np, "brcm,drive-strength", &val) == 0) > sdio->drive_strength = val; > > /* make sure there are interrupts defined in the node */ > if (!of_property_present(np, "interrupts")) > - return; > + return 0; -ENOENT? > > irq = irq_of_parse_and_map(np, 0); > if (!irq) { > brcmf_err("interrupt could not be mapped\n"); > - return; > + return 0; -ENXIO? > } > irqf = irqd_get_trigger_type(irq_get_irq_data(irq)); > > sdio->oob_irq_supported = true; > sdio->oob_irq_nr = irq; > sdio->oob_irq_flags = irqf; > + > + return 0; > } > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.h > index 10bf52253337e..ae124c73fc3b7 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.h > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.h > @@ -3,11 +3,12 @@ > * Copyright (c) 2014 Broadcom Corporation > */ > #ifdef CONFIG_OF > -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); > #else > -static void brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type, > - struct brcmf_mp_device *settings) > +static int brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type, > + struct brcmf_mp_device *settings) > { > + return 0; > } > #endif /* CONFIG_OF */ > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > index 06698a714b523..c34405a6d38b8 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > @@ -2457,6 +2457,9 @@ brcmf_pcie_probe(struct pci_dev *pdev, const struct pci_device_id *id) > ret = -ENOMEM; > goto fail; > } > + ret = PTR_ERR_OR_ZERO(devinfo->settings); > + if (ret < 0) > + goto fail; > > bus = kzalloc(sizeof(*bus), GFP_KERNEL); > if (!bus) { > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c > index 6b38d9de71af6..7d79e2db201b5 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c > @@ -3943,7 +3943,7 @@ static const struct brcmf_buscore_ops brcmf_sdio_buscore_ops = { > .write32 = brcmf_sdio_buscore_write32, > }; > > -static bool > +static int > brcmf_sdio_probe_attach(struct brcmf_sdio *bus) > { > struct brcmf_sdio_dev *sdiodev; > @@ -3953,6 +3953,7 @@ brcmf_sdio_probe_attach(struct brcmf_sdio *bus) > u32 reg_val; > u32 drivestrength; > u32 enum_base; > + int ret = -EBADE; > > sdiodev = bus->sdiodev; > sdio_claim_host(sdiodev->func1); > @@ -4001,8 +4002,9 @@ brcmf_sdio_probe_attach(struct brcmf_sdio *bus) > BRCMF_BUSTYPE_SDIO, > bus->ci->chip, > bus->ci->chiprev); > - if (!sdiodev->settings) { > + if (IS_ERR_OR_NULL(sdiodev->settings)) { > brcmf_err("Failed to get device parameters\n"); > + ret = PTR_ERR_OR_ZERO(sdiodev->settings); > goto fail; > } > /* platform specific configuration: > @@ -4071,7 +4073,7 @@ brcmf_sdio_probe_attach(struct brcmf_sdio *bus) > /* allocate header buffer */ > bus->hdrbuf = kzalloc(MAX_HDR_READ + bus->head_align, GFP_KERNEL); > if (!bus->hdrbuf) > - return false; > + return -ENOMEM; > /* Locate an appropriately-aligned portion of hdrbuf */ > bus->rxhdr = (u8 *) roundup((unsigned long)&bus->hdrbuf[0], > bus->head_align); > @@ -4082,11 +4084,11 @@ brcmf_sdio_probe_attach(struct brcmf_sdio *bus) > if (bus->poll) > bus->pollrate = 1; > > - return true; > + return 0; > > fail: > sdio_release_host(sdiodev->func1); > - return false; > + return ret; > } > > static int > @@ -4446,6 +4448,7 @@ struct brcmf_sdio *brcmf_sdio_probe(struct brcmf_sdio_dev *sdiodev) > struct brcmf_sdio *bus; > struct workqueue_struct *wq; > struct brcmf_fw_request *fwreq; > + int probe_attach_result; > > brcmf_dbg(TRACE, "Enter\n"); > > @@ -4474,7 +4477,8 @@ struct brcmf_sdio *brcmf_sdio_probe(struct brcmf_sdio_dev *sdiodev) > bus->brcmf_wq = wq; > > /* attempt to attach to the dongle */ > - if (!(brcmf_sdio_probe_attach(bus))) { > + probe_attach_result = brcmf_sdio_probe_attach(bus); > + if (probe_attach_result < 0) { > brcmf_err("brcmf_sdio_probe_attach failed\n"); > goto fail; > } > @@ -4546,6 +4550,8 @@ struct brcmf_sdio *brcmf_sdio_probe(struct brcmf_sdio_dev *sdiodev) > > fail: > brcmf_sdio_remove(bus); > + if (probe_attach_result < 0) > + return ERR_PTR(probe_attach_result); > return NULL; See my comment on the bcmsdh.c part above - perhaps initialize probe_attach_result to -ENODEV by default and just return ERR_PTR(probe_attach_result) here instead Best regards, Alexey
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@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 <jacobe.zang@wesion.com> >> --- >> .../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. 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. >> } >> 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. >> /* Apple ARM64 platforms have their own idea of board type, >> passed in >> * via the device tree. They also have an antenna SKU parameter >> */ >> @@ -105,7 +110,7 @@ void brcmf_of_probe(struct device *dev, enum >> brcmf_bus_type bus_type, >> board_type = devm_kstrdup(dev, tmp, GFP_KERNEL); >> if (!board_type) { >> of_node_put(root); >> - return; >> + return 0; > > -ENOMEM? Retain behavior. >> } >> strreplace(board_type, '/', '-'); >> settings->board_type = board_type; >> @@ -113,8 +118,13 @@ void brcmf_of_probe(struct device *dev, enum >> brcmf_bus_type bus_type, >> of_node_put(root); >> } >> - if (!np || !of_device_is_compatible(np, "brcm,bcm4329-fmac")) >> - return; >> + clk = devm_clk_get_optional_enabled(dev, "lpo"); >> + if (!IS_ERR_OR_NULL(clk)) { >> + brcmf_dbg(INFO, "enabling 32kHz clock\n"); >> + clk_set_rate(clk, 32768); >> + } else if (PTR_ERR_OR_ZERO(clk) == -EPROBE_DEFER) { > > PTR_ERR should be enough, no? Or better yet move this to the bottom of > the function as was discussed on your previous submission, then you can > return PTR_ERR_OR_ZERO(clk) right away, which would be a bit neater. > >> + return -EPROBE_DEFER; >> + } >> err = brcmf_of_get_country_codes(dev, settings); >> if (err) >> @@ -123,23 +133,25 @@ void brcmf_of_probe(struct device *dev, enum >> brcmf_bus_type bus_type, >> of_get_mac_address(np, settings->mac); >> if (bus_type != BRCMF_BUSTYPE_SDIO) >> - return; >> + return 0; >> if (of_property_read_u32(np, "brcm,drive-strength", &val) == 0) >> sdio->drive_strength = val; >> /* make sure there are interrupts defined in the node */ >> if (!of_property_present(np, "interrupts")) >> - return; >> + return 0; > > -ENOENT? That property is option in the binding so we return silently here as well. >> irq = irq_of_parse_and_map(np, 0); >> if (!irq) { >> brcmf_err("interrupt could not be mapped\n"); >> - return; >> + return 0; > > -ENXIO? This is a bit more gray area. The interrupt is optional, but if it is in the device tree this is clearly intended to succeed. When it does the question is whether the error print is sufficient or should we use WARN() instead or fail the probe entirely. This interrupt is optional because it is only needed in some sleep scenarios where it can wake the host. >> } >> irqf = irqd_get_trigger_type(irq_get_irq_data(irq)); >> sdio->oob_irq_supported = true; >> sdio->oob_irq_nr = irq; >> sdio->oob_irq_flags = irqf; >> + >> + return 0; >> } [...] >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c >> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c >> index 6b38d9de71af6..7d79e2db201b5 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c [...] >> @@ -4446,6 +4448,7 @@ struct brcmf_sdio *brcmf_sdio_probe(struct >> brcmf_sdio_dev *sdiodev) >> struct brcmf_sdio *bus; >> struct workqueue_struct *wq; >> struct brcmf_fw_request *fwreq; >> + int probe_attach_result; >> brcmf_dbg(TRACE, "Enter\n"); >> @@ -4474,7 +4477,8 @@ struct brcmf_sdio *brcmf_sdio_probe(struct >> brcmf_sdio_dev *sdiodev) >> bus->brcmf_wq = wq; >> /* attempt to attach to the dongle */ >> - if (!(brcmf_sdio_probe_attach(bus))) { >> + probe_attach_result = brcmf_sdio_probe_attach(bus); >> + if (probe_attach_result < 0) { >> brcmf_err("brcmf_sdio_probe_attach failed\n"); >> goto fail; >> } >> @@ -4546,6 +4550,8 @@ struct brcmf_sdio *brcmf_sdio_probe(struct >> brcmf_sdio_dev *sdiodev) >> fail: >> brcmf_sdio_remove(bus); >> + if (probe_attach_result < 0) >> + return ERR_PTR(probe_attach_result); >> return NULL; > > See my comment on the bcmsdh.c part above - perhaps initialize > probe_attach_result to -ENODEV by default and just return > ERR_PTR(probe_attach_result) here instead Yup. Let's do that. Thanks, Arend
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@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 <jacobe.zang@wesion.com> >>> --- >>> .../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
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; + } 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); + } } 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; + /* Apple ARM64 platforms have their own idea of board type, passed in * via the device tree. They also have an antenna SKU parameter */ @@ -105,7 +110,7 @@ void brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type, board_type = devm_kstrdup(dev, tmp, GFP_KERNEL); if (!board_type) { of_node_put(root); - return; + return 0; } strreplace(board_type, '/', '-'); settings->board_type = board_type; @@ -113,8 +118,13 @@ void brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type, of_node_put(root); } - if (!np || !of_device_is_compatible(np, "brcm,bcm4329-fmac")) - return; + clk = devm_clk_get_optional_enabled(dev, "lpo"); + if (!IS_ERR_OR_NULL(clk)) { + brcmf_dbg(INFO, "enabling 32kHz clock\n"); + clk_set_rate(clk, 32768); + } else if (PTR_ERR_OR_ZERO(clk) == -EPROBE_DEFER) { + return -EPROBE_DEFER; + } err = brcmf_of_get_country_codes(dev, settings); if (err) @@ -123,23 +133,25 @@ void brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type, of_get_mac_address(np, settings->mac); if (bus_type != BRCMF_BUSTYPE_SDIO) - return; + return 0; if (of_property_read_u32(np, "brcm,drive-strength", &val) == 0) sdio->drive_strength = val; /* make sure there are interrupts defined in the node */ if (!of_property_present(np, "interrupts")) - return; + return 0; irq = irq_of_parse_and_map(np, 0); if (!irq) { brcmf_err("interrupt could not be mapped\n"); - return; + return 0; } irqf = irqd_get_trigger_type(irq_get_irq_data(irq)); sdio->oob_irq_supported = true; sdio->oob_irq_nr = irq; sdio->oob_irq_flags = irqf; + + return 0; } diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.h index 10bf52253337e..ae124c73fc3b7 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.h +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.h @@ -3,11 +3,12 @@ * Copyright (c) 2014 Broadcom Corporation */ #ifdef CONFIG_OF -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); #else -static void brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type, - struct brcmf_mp_device *settings) +static int brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type, + struct brcmf_mp_device *settings) { + return 0; } #endif /* CONFIG_OF */ diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c index 06698a714b523..c34405a6d38b8 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c @@ -2457,6 +2457,9 @@ brcmf_pcie_probe(struct pci_dev *pdev, const struct pci_device_id *id) ret = -ENOMEM; goto fail; } + ret = PTR_ERR_OR_ZERO(devinfo->settings); + if (ret < 0) + goto fail; bus = kzalloc(sizeof(*bus), GFP_KERNEL); if (!bus) { diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c index 6b38d9de71af6..7d79e2db201b5 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c @@ -3943,7 +3943,7 @@ static const struct brcmf_buscore_ops brcmf_sdio_buscore_ops = { .write32 = brcmf_sdio_buscore_write32, }; -static bool +static int brcmf_sdio_probe_attach(struct brcmf_sdio *bus) { struct brcmf_sdio_dev *sdiodev; @@ -3953,6 +3953,7 @@ brcmf_sdio_probe_attach(struct brcmf_sdio *bus) u32 reg_val; u32 drivestrength; u32 enum_base; + int ret = -EBADE; sdiodev = bus->sdiodev; sdio_claim_host(sdiodev->func1); @@ -4001,8 +4002,9 @@ brcmf_sdio_probe_attach(struct brcmf_sdio *bus) BRCMF_BUSTYPE_SDIO, bus->ci->chip, bus->ci->chiprev); - if (!sdiodev->settings) { + if (IS_ERR_OR_NULL(sdiodev->settings)) { brcmf_err("Failed to get device parameters\n"); + ret = PTR_ERR_OR_ZERO(sdiodev->settings); goto fail; } /* platform specific configuration: @@ -4071,7 +4073,7 @@ brcmf_sdio_probe_attach(struct brcmf_sdio *bus) /* allocate header buffer */ bus->hdrbuf = kzalloc(MAX_HDR_READ + bus->head_align, GFP_KERNEL); if (!bus->hdrbuf) - return false; + return -ENOMEM; /* Locate an appropriately-aligned portion of hdrbuf */ bus->rxhdr = (u8 *) roundup((unsigned long)&bus->hdrbuf[0], bus->head_align); @@ -4082,11 +4084,11 @@ brcmf_sdio_probe_attach(struct brcmf_sdio *bus) if (bus->poll) bus->pollrate = 1; - return true; + return 0; fail: sdio_release_host(sdiodev->func1); - return false; + return ret; } static int @@ -4446,6 +4448,7 @@ struct brcmf_sdio *brcmf_sdio_probe(struct brcmf_sdio_dev *sdiodev) struct brcmf_sdio *bus; struct workqueue_struct *wq; struct brcmf_fw_request *fwreq; + int probe_attach_result; brcmf_dbg(TRACE, "Enter\n"); @@ -4474,7 +4477,8 @@ struct brcmf_sdio *brcmf_sdio_probe(struct brcmf_sdio_dev *sdiodev) bus->brcmf_wq = wq; /* attempt to attach to the dongle */ - if (!(brcmf_sdio_probe_attach(bus))) { + probe_attach_result = brcmf_sdio_probe_attach(bus); + if (probe_attach_result < 0) { brcmf_err("brcmf_sdio_probe_attach failed\n"); goto fail; } @@ -4546,6 +4550,8 @@ struct brcmf_sdio *brcmf_sdio_probe(struct brcmf_sdio_dev *sdiodev) fail: brcmf_sdio_remove(bus); + if (probe_attach_result < 0) + return ERR_PTR(probe_attach_result); return NULL; } diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c index 9a105e6debe1f..f7db46ae44906 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c @@ -1272,6 +1272,9 @@ static int brcmf_usb_probe_cb(struct brcmf_usbdev_info *devinfo, ret = -ENOMEM; goto fail; } + ret = PTR_ERR_OR_ZERO(devinfo->settings); + if (ret < 0) + goto fail; if (!brcmf_usb_dlneeded(devinfo)) { ret = brcmf_alloc(devinfo->dev, devinfo->settings);