On 8/14/2024 11:48 AM, Alexey Charkov wrote:
On Wed, Aug 14, 2024 at 12:27 PM Jacobe Zang <jacobe.zang@xxxxxxxxxx> wrote:
On 2024/8/14 16:47, Alexey Charkov wrote:
Hi Arend, Jacobe,
On Tuesday, August 13, 2024 2:57:28 PM GMT+3 Arend van Spriel wrote:
On 8/13/2024 10:20 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.
I was going to say it looks good to me, but....
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>
Reviewed-by: Sai Krishna <saikrishnag@xxxxxxxxxxx>
Signed-off-by: Jacobe Zang <jacobe.zang@xxxxxxxxxx>
---
.../broadcom/brcm80211/brcmfmac/bcmsdh.c | 4 +-
.../broadcom/brcm80211/brcmfmac/common.c | 3 +-
.../wireless/broadcom/brcm80211/brcmfmac/of.c | 53 +++++++++++--------
.../wireless/broadcom/brcm80211/brcmfmac/of.h | 9 ++--
.../broadcom/brcm80211/brcmfmac/pcie.c | 3 ++
.../broadcom/brcm80211/brcmfmac/sdio.c | 22 +++++---
.../broadcom/brcm80211/brcmfmac/usb.c | 3 ++
7 files changed, 61 insertions(+), 36 deletions(-)
[...]
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c index
e406e11481a62..f19dc7355e0e8 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
[...]
@@ -113,33 +118,39 @@ 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;
-
err = brcmf_of_get_country_codes(dev, settings);
if (err)
brcmf_err("failed to get OF country code map (err=%d)
\n", err);
of_get_mac_address(np, settings->mac);
- if (bus_type != BRCMF_BUSTYPE_SDIO)
- return;
+ if (bus_type == BRCMF_BUSTYPE_SDIO) {
Don't like the fact that this now has an extra indentation level and it
offers no extra benefit. Just keep the original if-statement and return
0. Consequently the LPO clock code should move just before the if-statement.
+ if (of_property_read_u32(np, "brcm,drive-strength",
&val) == 0)
+ sdio->drive_strength = val;
- 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 0;
- /* make sure there are interrupts defined in the node */
- if (!of_property_present(np, "interrupts"))
- return;
+ irq = irq_of_parse_and_map(np, 0);
+ if (!irq) {
+ brcmf_err("interrupt could not be
mapped\n");
+ 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;
+ }
- irq = irq_of_parse_and_map(np, 0);
- if (!irq) {
- brcmf_err("interrupt could not be mapped\n");
- return;
+ clk = devm_clk_get_optional_enabled(dev, "lpo");
+ if (!IS_ERR_OR_NULL(clk)) {
+ brcmf_dbg(INFO, "enabling 32kHz clock\n");
+ return clk_set_rate(clk, 32768);
+ } else {
+ return PTR_ERR_OR_ZERO(clk);
}
Change this to:
> + clk = devm_clk_get_optional_enabled(dev, "lpo");
> + if (IS_ERR_OR_NULL(clk)) {
> + return PTR_ERR_OR_ZERO(clk);
Perhaps in this case we should go for IS_ERR and PTR_ERR respectively.
devm_clk_get_optional_enabled would return NULL when the optional clock is not
found, so NULL is not an error state but serves as a dummy clock that can be> used with clk_set_rate.
I think we don't need to set clock rate for clock is NULL. So it should
be changed to:
+ clk = devm_clk_get_optional_enabled(dev, "lpo");
+ if (IS_ERR(clk)) {
+ return PTR_ERR(clk);
+ } else if (clk) {
+ brcmf_dbg(INFO, "enabling 32kHz clock\n");
+ clk_set_rate(clk, 32768);
+ }
If clk is NULL then clk_set_rate returns immediately with status zero,
so there is little difference from whether you wrap it into another if
(clk) or not. You can probably drop the debug statement altogether and
call clk_set_rate unconditionally - this will look neater.
The construct above is indeed only needed to get the debug statement
correct given the behavior of clk_set_rate(). However, for debugging it
is useful to know that the LPO clock is defined and used or not. Maybe
do this:
clk = devm_clk_get_optional_enabled(dev, "lpo");
if (IS_ERR(clk))
return PTR_ERR(clk);
brcmf_dbg(INFO, "%s LPO clock\n", clk ? "enable" : "no");
clk_set_rate(clk, 32768);
Regards,
Arend
