diff mbox series

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

Message ID 20240813082007.2625841-5-jacobe.zang@wesion.com (mailing list archive)
State New
Headers show
Series Add AP6275P wireless support | expand

Commit Message

Jacobe Zang Aug. 13, 2024, 8:20 a.m. UTC
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.

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(-)

Comments

Arend van Spriel Aug. 13, 2024, 11:57 a.m. UTC | #1
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;
>   }

[...]
Arend van Spriel Aug. 13, 2024, 6:31 p.m. UTC | #2
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
Alexey Charkov Aug. 14, 2024, 8:47 a.m. UTC | #3
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
Jacobe Zang Aug. 14, 2024, 9:26 a.m. UTC | #4
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;
>>>
>>>    }
> 
>
Alexey Charkov Aug. 14, 2024, 9:48 a.m. UTC | #5
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
Arend van Spriel Aug. 14, 2024, 10:44 a.m. UTC | #6
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 mbox series

Patch

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);