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

[Arend van Spriel]

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