| Message ID | 20240813082007.2625841-5-jacobe.zang@wesion.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add AP6275P wireless support | expand |
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@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> > Reviewed-by: Sai Krishna <saikrishnag@marvell.com> > Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com> > --- > .../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); > + } > + brcmf_dbg(INFO, "enabling 32kHz clock\n"); > + clk_set_rate(clk, 32768); As said above this should be moved before the if-statement: > - if (bus_type != BRCMF_BUSTYPE_SDIO) > - 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; > } [...]
On 8/13/2024 1:57 PM, 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@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> >> Reviewed-by: Sai Krishna <saikrishnag@marvell.com> >> Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com> >> --- >> .../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); > > + } > > + brcmf_dbg(INFO, "enabling 32kHz clock\n"); > > + clk_set_rate(clk, 32768); Do we really need to set the clock rate or could that be defined in the devicetree? Just wondering. I have looked at the clock.yaml schema, but not really an idea what is needed in the devicetree spec. Any pointers from DT devs would be appreciated or a hard no-do-not-do-that is also fine ;-) Regards, Arend
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@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> > > Reviewed-by: Sai Krishna <saikrishnag@marvell.com> > > Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com> > > --- > > > > .../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. This way we won't skip over the interrupts initialization below in case the clock is absent. > > + } > > + brcmf_dbg(INFO, "enabling 32kHz clock\n"); > > + clk_set_rate(clk, 32768); > > As said above this should be moved before the if-statement: > > - if (bus_type != BRCMF_BUSTYPE_SDIO) > > - 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; > > > > } Best regards, Alexey
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@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> >>> Reviewed-by: Sai Krishna <saikrishnag@marvell.com> >>> Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com> >>> --- >>> >>> .../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); + } > > This way we won't skip over the interrupts initialization below in case the > clock is absent. > >> > + } >> > + brcmf_dbg(INFO, "enabling 32kHz clock\n"); >> > + clk_set_rate(clk, 32768); >> >> As said above this should be moved before the if-statement: >> > - if (bus_type != BRCMF_BUSTYPE_SDIO) >> > - 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; >>> >>> } > >
On Wed, Aug 14, 2024 at 12:27 PM Jacobe Zang <jacobe.zang@wesion.com> 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@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> > >>> Reviewed-by: Sai Krishna <saikrishnag@marvell.com> > >>> Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com> > >>> --- > >>> > >>> .../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. Best regards, Alexey
On 8/14/2024 11:48 AM, Alexey Charkov wrote: > On Wed, Aug 14, 2024 at 12:27 PM Jacobe Zang <jacobe.zang@wesion.com> 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@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> >>>>> Reviewed-by: Sai Krishna <saikrishnag@marvell.com> >>>>> Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com> >>>>> --- >>>>> >>>>> .../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
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c index 13391c2d82aae..b2ede4e579c5c 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c @@ -947,8 +947,8 @@ int brcmf_sdiod_probe(struct brcmf_sdio_dev *sdiodev) /* try to attach to the target device */ sdiodev->bus = brcmf_sdio_probe(sdiodev); - if (!sdiodev->bus) { - ret = -ENODEV; + if (IS_ERR(sdiodev->bus)) { + ret = PTR_ERR(sdiodev->bus); goto out; } brcmf_sdiod_host_fixup(sdiodev->func2->card->host); diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c index b24faae35873d..58d50918dd177 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c @@ -561,7 +561,8 @@ 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; 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 @@ -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,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) { + 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); } - 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..462fc669b959c 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 @@ -4451,8 +4453,10 @@ struct brcmf_sdio *brcmf_sdio_probe(struct brcmf_sdio_dev *sdiodev) /* Allocate private bus interface state */ bus = kzalloc(sizeof(struct brcmf_sdio), GFP_ATOMIC); - if (!bus) + if (!bus) { + ret = -ENOMEM; goto fail; + } bus->sdiodev = sdiodev; sdiodev->bus = bus; @@ -4467,6 +4471,7 @@ struct brcmf_sdio *brcmf_sdio_probe(struct brcmf_sdio_dev *sdiodev) dev_name(&sdiodev->func1->dev)); if (!wq) { brcmf_err("insufficient memory to create txworkqueue\n"); + ret = -ENOMEM; goto fail; } brcmf_sdiod_freezer_count(sdiodev); @@ -4474,7 +4479,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))) { + ret = brcmf_sdio_probe_attach(bus); + if (ret < 0) { brcmf_err("brcmf_sdio_probe_attach failed\n"); goto fail; } @@ -4546,7 +4552,7 @@ struct brcmf_sdio *brcmf_sdio_probe(struct brcmf_sdio_dev *sdiodev) fail: brcmf_sdio_remove(bus); - return NULL; + return ERR_PTR(ret); } /* Detach and free everything */ 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);