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

[Alexey Charkov]

On Wed, Aug 14, 2024 at 12:27 PM Jacobe Zang <jacobe.zang at 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 at xff.cz>
> >>> Signed-off-by: Ondrej Jirman <megi at xff.cz>
> >>> Co-developed-by: Arend van Spriel <arend.vanspriel at broadcom.com>
> >>> Signed-off-by: Arend van Spriel <arend.vanspriel at broadcom.com>
> >>> Reviewed-by: Sai Krishna <saikrishnag at marvell.com>
> >>> Signed-off-by: Jacobe Zang <jacobe.zang at 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