| Message ID | 20240729070102.3770318-5-jacobe.zang@wesion.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | Add AP6275P wireless support | expand |
Hello Jacobe, Please see one comment below. On 2024-07-29 09:01, Jacobe Zang wrote: > WiFi modules often require 32kHz clock to function. Add support to > enable the clock to PCIe driver. > > Co-developed-by: Ondrej Jirman <megi@xff.cz> > Signed-off-by: Ondrej Jirman <megi@xff.cz> > Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com> > --- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c > b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c > index e406e11481a62..4db188a41fd52 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,6 +71,7 @@ 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; > @@ -113,6 +115,13 @@ void brcmf_of_probe(struct device *dev, enum > brcmf_bus_type bus_type, > of_node_put(root); > } > > + clk = devm_clk_get_optional_enabled(dev, "lpo"); > + if (IS_ERR(clk)) > + if (clk) { These two lines looks really confusing. Shouldn't it be just a single "if (!IS_ERR(clk)) {" line instead? > + brcmf_dbg(INFO, "enabling 32kHz clock\n"); > + clk_set_rate(clk, 32768); > + } > + > if (!np || !of_device_is_compatible(np, "brcm,bcm4329-fmac")) > return;
>> + clk = devm_clk_get_optional_enabled(dev, "lpo"); >> + if (IS_ERR(clk)) >> + if (clk) { > > These two lines looks really confusing. Shouldn't it be just a single > "if (!IS_ERR(clk)) {" line instead? Thanks for the correction... I forgot to organize this... --- Best Regards Jacobe
On Mon, Jul 29, 2024 at 09:12:20AM GMT, Dragan Simic wrote: > Hello Jacobe, > > [...] > > > > > + clk = devm_clk_get_optional_enabled(dev, "lpo"); > > + if (IS_ERR(clk)) > > + if (clk) { > > These two lines looks really confusing. Shouldn't it be just a single > "if (!IS_ERR(clk)) {" line instead? It should be `!IS_ERR(clk) && clk` otherwise the debug message will be incorrect. Kind regards, o. > > + brcmf_dbg(INFO, "enabling 32kHz clock\n"); > > + clk_set_rate(clk, 32768); > > + } > > + > > if (!np || !of_device_is_compatible(np, "brcm,bcm4329-fmac")) > > return;
Hello Ondrej, On 2024-07-29 10:44, Ondřej Jirman wrote: > On Mon, Jul 29, 2024 at 09:12:20AM GMT, Dragan Simic wrote: >> Hello Jacobe, >> >> [...] >> >> > >> > + clk = devm_clk_get_optional_enabled(dev, "lpo"); >> > + if (IS_ERR(clk)) >> > + if (clk) { >> >> These two lines looks really confusing. Shouldn't it be just a single >> "if (!IS_ERR(clk)) {" line instead? > > It should be `!IS_ERR(clk) && clk` otherwise the debug message will be > incorrect. Ah, I see now, thanks. There's also IS_ERR_OR_NULL, so the condition can actually be "!IS_ERR_OR_NULL(clk)". >> > + brcmf_dbg(INFO, "enabling 32kHz clock\n"); >> > + clk_set_rate(clk, 32768); >> > + } >> > + >> > if (!np || !of_device_is_compatible(np, "brcm,bcm4329-fmac")) >> > return;
On 7/29/2024 11:12 AM, Dragan Simic wrote: > Hello Ondrej, > > On 2024-07-29 10:44, Ondřej Jirman wrote: >> On Mon, Jul 29, 2024 at 09:12:20AM GMT, Dragan Simic wrote: >>> Hello Jacobe, >>> >>> [...] >>> >>> > >>> > + clk = devm_clk_get_optional_enabled(dev, "lpo"); >>> > + if (IS_ERR(clk)) >>> > + if (clk) { >>> >>> These two lines looks really confusing. Shouldn't it be just a single >>> "if (!IS_ERR(clk)) {" line instead? >> >> It should be `!IS_ERR(clk) && clk` otherwise the debug message will be >> incorrect. > > Ah, I see now, thanks. There's also IS_ERR_OR_NULL, so the condition > can actually be "!IS_ERR_OR_NULL(clk)". ++ best suggestion >>> > + brcmf_dbg(INFO, "enabling 32kHz clock\n"); >>> > + clk_set_rate(clk, 32768); >>> > + } >>> > + >>> > if (!np || !of_device_is_compatible(np, "brcm,bcm4329-fmac")) >>> > return;
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c index e406e11481a62..4db188a41fd52 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,6 +71,7 @@ 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; @@ -113,6 +115,13 @@ void brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type, of_node_put(root); } + clk = devm_clk_get_optional_enabled(dev, "lpo"); + if (IS_ERR(clk)) + if (clk) { + brcmf_dbg(INFO, "enabling 32kHz clock\n"); + clk_set_rate(clk, 32768); + } + if (!np || !of_device_is_compatible(np, "brcm,bcm4329-fmac")) return;