On 7/31/2024 12:16 PM, Alexey Charkov wrote:
Hi Jacobe,
On 31/07/2024 9:11 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
>
> Co-developed-by: Ondrej Jirman <megi@xxxxxx>
> Signed-off-by: Ondrej Jirman <megi@xxxxxx>
> Signed-off-by: Jacobe Zang <jacobe.zang@xxxxxxxxxx>
> ---
> .../net/wireless/broadcom/brcm80211/brcmfmac/of.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
> index e406e11481a62..7e0a2ad5c7c8a 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"
> @@ -70,12 +71,16 @@ void brcmf_of_probe(struct device *dev, enum
brcmf_bus_type bus_type,
> {
> 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;
Did you test this? The DTS patch you sent as part of this series doesn't
list "brcm,bcm4329-fmac" in the compatible, so this will probably return
right here, skipping over the rest of your patch.
Please test before resending, both with and without the driver for the
Bluetooth part of the chip (since it also touches clocks).
You are also changing the behavior for other systems by putting this
check further up the probe path, which might break things for no reason.
Better put your clk-related addition below where this check was
originally, rather than reorder stuff you don't have to reorder.
That was upon my suggestion. That check was originally at the top of the
function, but people added stuff before that. I agree that makes the
compatible "brcm,brcm4329-fmac" required which is what the textual
binding stated before the switch to YAML was made:
"""
Broadcom BCM43xx Fullmac wireless SDIO devices
This node provides properties for controlling the Broadcom wireless
device. The
node is expected to be specified as a child node to the SDIO controller that
connects the device to the system.
Required properties:
- compatible : Should be "brcm,bcm4329-fmac".
"""
Not sure whether this is still true for YAML version (poor YAML reading
skills ;-) ), but it should as the switch from textual to YAML should
not have changed the bindings specification.
> +
> /* Apple ARM64 platforms have their own idea of board type,
passed in
> * via the device tree. They also have an antenna SKU parameter
> */
> @@ -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"))
> + 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 {
> return;
Why return here? If the clock is optional, a lot of systems will not
have it - that shouldn't prevent the driver from probing. And you are
still not handling the -EPROBE_DEFER case which was mentioned on your
previous submission.
Right. The else statement above could/should be:
} else if (clk && PTR_ERR(clk) == -EPROBE_DEFER) {
return PTR_ERR(clk);
}
Regards,
Arend
