[PATCH 1/2] brcmfmac: rename brcmf_bus_start function to brcmf_attach_phase2

[PATCH 1/2] brcmfmac: rename brcmf_bus_start function to brcmf_attach_phase2

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

This change intends to make init/attach process easier to follow.

What driver were doing in brcmf_bus_start wasn't bus specific at all and
function brcmf_bus_stop wasn't undoing things done there. It seems
brcmf_detach was actually undoing things done in both: brcmf_attach and
brcmf_bus_start.

To me it makes more sense to call these two function in a similar way as
they both handle some part of attaching process. It also makes
brcmf_detach more meaningful.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
This patchset is based on top of
[PATCH 2/2] brcmfmac: move function declarations to proper headers
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c | 2 +-
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c   | 4 ++--
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h   | 4 ++--
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c   | 6 +++---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c   | 6 +++---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c    | 6 +++---
 6 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
index 3e15d64..6fb7d12 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
@@ -74,7 +74,7 @@ module_param_named(roamoff, brcmf_roamoff, int, S_IRUSR);
 MODULE_PARM_DESC(roamoff, "Do not use internal roaming engine");
 
 #ifdef DEBUG
-/* always succeed brcmf_bus_start() */
+/* always succeed brcmf_attach_phase2() */
 static int brcmf_ignore_probe_fail;
 module_param_named(ignore_probe_fail, brcmf_ignore_probe_fail, int, 0);
 MODULE_PARM_DESC(ignore_probe_fail, "always succeed probe for debugging");
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index 9e6f60a..adf8eb1 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -893,7 +893,7 @@ static int brcmf_inet6addr_changed(struct notifier_block *nb,
 }
 #endif
 
-int brcmf_attach(struct device *dev, struct brcmf_mp_device *settings)
+int brcmf_attach_phase1(struct device *dev, struct brcmf_mp_device *settings)
 {
 	struct brcmf_pub *drvr = NULL;
 	int ret = 0;
@@ -966,7 +966,7 @@ static int brcmf_revinfo_read(struct seq_file *s, void *data)
 	return 0;
 }
 
-int brcmf_bus_start(struct device *dev)
+int brcmf_attach_phase2(struct device *dev)
 {
 	int ret = -1;
 	struct brcmf_bus *bus_if = dev_get_drvdata(dev);
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
index d92beca..df4189e 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
@@ -226,8 +226,8 @@ void brcmf_rx_event(struct device *dev, struct sk_buff *rxp);
 void brcmf_txcomplete(struct device *dev, struct sk_buff *txp, bool success);
 void brcmf_net_setcarrier(struct brcmf_if *ifp, bool on);
 /* Indication from bus module regarding presence/insertion of dongle. */
-int brcmf_attach(struct device *dev, struct brcmf_mp_device *settings);
-int brcmf_bus_start(struct device *dev);
+int brcmf_attach_phase1(struct device *dev, struct brcmf_mp_device *settings);
+int brcmf_attach_phase2(struct device *dev);
 void brcmf_bus_add_txhdrlen(struct device *dev, uint len);
 /* Indication from bus module that dongle should be reset */
 void brcmf_dev_reset(struct device *dev);
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
index 048027f..bbc3ded 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
@@ -1568,11 +1568,11 @@ static int brcmf_pcie_attach_bus(struct brcmf_pciedev_info *devinfo)
 	int ret;
 
 	/* Attach to the common driver interface */
-	ret = brcmf_attach(&devinfo->pdev->dev, devinfo->settings);
+	ret = brcmf_attach_phase1(&devinfo->pdev->dev, devinfo->settings);
 	if (ret) {
-		brcmf_err("brcmf_attach failed\n");
+		brcmf_err("brcmf_attach_phase1 failed\n");
 	} else {
-		ret = brcmf_bus_start(&devinfo->pdev->dev);
+		ret = brcmf_attach_phase2(&devinfo->pdev->dev);
 		if (ret)
 			brcmf_err("dongle is not responding\n");
 	}
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index dfb0658..5307223 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -4065,7 +4065,7 @@ static void brcmf_sdio_firmware_callback(struct device *dev,
 
 	sdio_release_host(sdiodev->func[1]);
 
-	err = brcmf_bus_start(dev);
+	err = brcmf_attach_phase2(dev);
 	if (err != 0) {
 		brcmf_err("dongle is not responding\n");
 		goto fail;
@@ -4150,9 +4150,9 @@ struct brcmf_sdio *brcmf_sdio_probe(struct brcmf_sdio_dev *sdiodev)
 	bus->tx_hdrlen = SDPCM_HWHDR_LEN + SDPCM_SWHDR_LEN;
 
 	/* Attach to the common layer, reserve hdr space */
-	ret = brcmf_attach(bus->sdiodev->dev, bus->sdiodev->settings);
+	ret = brcmf_attach_phase1(bus->sdiodev->dev, bus->sdiodev->settings);
 	if (ret != 0) {
-		brcmf_err("brcmf_attach failed\n");
+		brcmf_err("brcmf_attach_phase1 failed\n");
 		goto fail;
 	}
 
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
index 2f978a3..e031fea 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
@@ -1138,9 +1138,9 @@ static int brcmf_usb_bus_setup(struct brcmf_usbdev_info *devinfo)
 	int ret;
 
 	/* Attach to the common driver interface */
-	ret = brcmf_attach(devinfo->dev, devinfo->settings);
+	ret = brcmf_attach_phase1(devinfo->dev, devinfo->settings);
 	if (ret) {
-		brcmf_err("brcmf_attach failed\n");
+		brcmf_err("brcmf_attach_phase1 failed\n");
 		return ret;
 	}
 
@@ -1148,7 +1148,7 @@ static int brcmf_usb_bus_setup(struct brcmf_usbdev_info *devinfo)
 	if (ret)
 		goto fail;
 
-	ret = brcmf_bus_start(devinfo->dev);
+	ret = brcmf_attach_phase2(devinfo->dev);
 	if (ret)
 		goto fail;
 
-- 
2.10.1

[PATCH 2/2] brcmfmac: drop brcmf_bus_detach and inline its code

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Driver used to call brcmf_bus_detach only from one place and it already
contained a check for drvr not being NULL. We can get rid of this extra
function, call brcmf_bus_stop directly and simplify the code.
There also isn't brcmf_bus_attach function which one could expect so it
looks more consistent this way.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index adf8eb1..122c79d 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -1075,16 +1075,6 @@ void brcmf_bus_add_txhdrlen(struct device *dev, uint len)
 	}
 }
 
-static void brcmf_bus_detach(struct brcmf_pub *drvr)
-{
-	brcmf_dbg(TRACE, "Enter\n");
-
-	if (drvr) {
-		/* Stop the bus module */
-		brcmf_bus_stop(drvr->bus_if);
-	}
-}
-
 void brcmf_dev_reset(struct device *dev)
 {
 	struct brcmf_bus *bus_if = dev_get_drvdata(dev);
@@ -1131,7 +1121,7 @@ void brcmf_detach(struct device *dev)
 
 	brcmf_fws_deinit(drvr);
 
-	brcmf_bus_detach(drvr);
+	brcmf_bus_stop(drvr->bus_if);
 
 	brcmf_proto_detach(drvr);
 
-- 
2.10.1

Re: [PATCH 1/2] brcmfmac: rename brcmf_bus_start function to brcmf_attach_phase2

[Arend Van Spriel]

On 17-1-2017 20:23, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> This change intends to make init/attach process easier to follow.
> 
> What driver were doing in brcmf_bus_start wasn't bus specific at all and
> function brcmf_bus_stop wasn't undoing things done there. It seems
> brcmf_detach was actually undoing things done in both: brcmf_attach and
> brcmf_bus_start.
> 
> To me it makes more sense to call these two function in a similar way as
> they both handle some part of attaching process. It also makes
> brcmf_detach more meaningful.

To me this is all bike-shedding and I have two options 1) what's in a
name and let it pass, or 2) explain the naming. Let's try option 2). It
seems your expectation was different because of the name
brcmf_*bus*_start(). The function brcmf_attach() is called by
bus-specific code, ie. sdio, pcie, or usb, to instantiate the common
driver structures and essential common facilities, eg. debugfs, and
brcmf_bus_start() is called by bus-specific code when everything is
ready for use. For PCIe there is nothing between those calls but for
SDIO there are several steps before the party can start, hence the name.

Regards,
Arend

> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> This patchset is based on top of
> [PATCH 2/2] brcmfmac: move function declarations to proper headers
> ---
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c | 2 +-
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c   | 4 ++--
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h   | 4 ++--
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c   | 6 +++---
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c   | 6 +++---
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c    | 6 +++---
>  6 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> index 3e15d64..6fb7d12 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> @@ -74,7 +74,7 @@ module_param_named(roamoff, brcmf_roamoff, int, S_IRUSR);
>  MODULE_PARM_DESC(roamoff, "Do not use internal roaming engine");
>  
>  #ifdef DEBUG
> -/* always succeed brcmf_bus_start() */
> +/* always succeed brcmf_attach_phase2() */
>  static int brcmf_ignore_probe_fail;
>  module_param_named(ignore_probe_fail, brcmf_ignore_probe_fail, int, 0);
>  MODULE_PARM_DESC(ignore_probe_fail, "always succeed probe for debugging");
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> index 9e6f60a..adf8eb1 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> @@ -893,7 +893,7 @@ static int brcmf_inet6addr_changed(struct notifier_block *nb,
>  }
>  #endif
>  
> -int brcmf_attach(struct device *dev, struct brcmf_mp_device *settings)
> +int brcmf_attach_phase1(struct device *dev, struct brcmf_mp_device *settings)
>  {
>  	struct brcmf_pub *drvr = NULL;
>  	int ret = 0;
> @@ -966,7 +966,7 @@ static int brcmf_revinfo_read(struct seq_file *s, void *data)
>  	return 0;
>  }
>  
> -int brcmf_bus_start(struct device *dev)
> +int brcmf_attach_phase2(struct device *dev)
>  {
>  	int ret = -1;
>  	struct brcmf_bus *bus_if = dev_get_drvdata(dev);
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
> index d92beca..df4189e 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
> @@ -226,8 +226,8 @@ void brcmf_rx_event(struct device *dev, struct sk_buff *rxp);
>  void brcmf_txcomplete(struct device *dev, struct sk_buff *txp, bool success);
>  void brcmf_net_setcarrier(struct brcmf_if *ifp, bool on);
>  /* Indication from bus module regarding presence/insertion of dongle. */
> -int brcmf_attach(struct device *dev, struct brcmf_mp_device *settings);
> -int brcmf_bus_start(struct device *dev);
> +int brcmf_attach_phase1(struct device *dev, struct brcmf_mp_device *settings);
> +int brcmf_attach_phase2(struct device *dev);
>  void brcmf_bus_add_txhdrlen(struct device *dev, uint len);
>  /* Indication from bus module that dongle should be reset */
>  void brcmf_dev_reset(struct device *dev);
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> index 048027f..bbc3ded 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> @@ -1568,11 +1568,11 @@ static int brcmf_pcie_attach_bus(struct brcmf_pciedev_info *devinfo)
>  	int ret;
>  
>  	/* Attach to the common driver interface */
> -	ret = brcmf_attach(&devinfo->pdev->dev, devinfo->settings);
> +	ret = brcmf_attach_phase1(&devinfo->pdev->dev, devinfo->settings);
>  	if (ret) {
> -		brcmf_err("brcmf_attach failed\n");
> +		brcmf_err("brcmf_attach_phase1 failed\n");
>  	} else {
> -		ret = brcmf_bus_start(&devinfo->pdev->dev);
> +		ret = brcmf_attach_phase2(&devinfo->pdev->dev);
>  		if (ret)
>  			brcmf_err("dongle is not responding\n");
>  	}
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> index dfb0658..5307223 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> @@ -4065,7 +4065,7 @@ static void brcmf_sdio_firmware_callback(struct device *dev,
>  
>  	sdio_release_host(sdiodev->func[1]);
>  
> -	err = brcmf_bus_start(dev);
> +	err = brcmf_attach_phase2(dev);
>  	if (err != 0) {
>  		brcmf_err("dongle is not responding\n");
>  		goto fail;
> @@ -4150,9 +4150,9 @@ struct brcmf_sdio *brcmf_sdio_probe(struct brcmf_sdio_dev *sdiodev)
>  	bus->tx_hdrlen = SDPCM_HWHDR_LEN + SDPCM_SWHDR_LEN;
>  
>  	/* Attach to the common layer, reserve hdr space */
> -	ret = brcmf_attach(bus->sdiodev->dev, bus->sdiodev->settings);
> +	ret = brcmf_attach_phase1(bus->sdiodev->dev, bus->sdiodev->settings);
>  	if (ret != 0) {
> -		brcmf_err("brcmf_attach failed\n");
> +		brcmf_err("brcmf_attach_phase1 failed\n");
>  		goto fail;
>  	}
>  
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
> index 2f978a3..e031fea 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
> @@ -1138,9 +1138,9 @@ static int brcmf_usb_bus_setup(struct brcmf_usbdev_info *devinfo)
>  	int ret;
>  
>  	/* Attach to the common driver interface */
> -	ret = brcmf_attach(devinfo->dev, devinfo->settings);
> +	ret = brcmf_attach_phase1(devinfo->dev, devinfo->settings);
>  	if (ret) {
> -		brcmf_err("brcmf_attach failed\n");
> +		brcmf_err("brcmf_attach_phase1 failed\n");
>  		return ret;
>  	}
>  
> @@ -1148,7 +1148,7 @@ static int brcmf_usb_bus_setup(struct brcmf_usbdev_info *devinfo)
>  	if (ret)
>  		goto fail;
>  
> -	ret = brcmf_bus_start(devinfo->dev);
> +	ret = brcmf_attach_phase2(devinfo->dev);
>  	if (ret)
>  		goto fail;
>  
> 

Re: [PATCH 2/2] brcmfmac: drop brcmf_bus_detach and inline its code

[Arend Van Spriel]

On 17-1-2017 20:23, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Driver used to call brcmf_bus_detach only from one place and it already
> contained a check for drvr not being NULL. We can get rid of this extra
> function, call brcmf_bus_stop directly and simplify the code.
> There also isn't brcmf_bus_attach function which one could expect so it
> looks more consistent this way.

Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 12 +-----------
>  1 file changed, 1 insertion(+), 11 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> index adf8eb1..122c79d 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> @@ -1075,16 +1075,6 @@ void brcmf_bus_add_txhdrlen(struct device *dev, uint len)
>  	}
>  }
>  
> -static void brcmf_bus_detach(struct brcmf_pub *drvr)
> -{
> -	brcmf_dbg(TRACE, "Enter\n");
> -
> -	if (drvr) {
> -		/* Stop the bus module */
> -		brcmf_bus_stop(drvr->bus_if);
> -	}
> -}
> -
>  void brcmf_dev_reset(struct device *dev)
>  {
>  	struct brcmf_bus *bus_if = dev_get_drvdata(dev);
> @@ -1131,7 +1121,7 @@ void brcmf_detach(struct device *dev)
>  
>  	brcmf_fws_deinit(drvr);
>  
> -	brcmf_bus_detach(drvr);
> +	brcmf_bus_stop(drvr->bus_if);
>  
>  	brcmf_proto_detach(drvr);
>  
> 

Re: [PATCH 1/2] brcmfmac: rename brcmf_bus_start function to brcmf_attach_phase2

[Rafał Miłecki]

On 18 January 2017 at 10:25, Arend Van Spriel
<arend.vanspriel@broadcom.com> wrote:
> On 17-1-2017 20:23, Rafa=C5=82 Mi=C5=82ecki wrote:
>> From: Rafa=C5=82 Mi=C5=82ecki <rafal@milecki.pl>
>>
>> This change intends to make init/attach process easier to follow.
>>
>> What driver were doing in brcmf_bus_start wasn't bus specific at all and
>> function brcmf_bus_stop wasn't undoing things done there. It seems
>> brcmf_detach was actually undoing things done in both: brcmf_attach and
>> brcmf_bus_start.
>>
>> To me it makes more sense to call these two function in a similar way as
>> they both handle some part of attaching process. It also makes
>> brcmf_detach more meaningful.
>
> To me this is all bike-shedding and I have two options 1) what's in a
> name and let it pass, or 2) explain the naming. Let's try option 2). It
> seems your expectation was different because of the name
> brcmf_*bus*_start(). The function brcmf_attach() is called by
> bus-specific code, ie. sdio, pcie, or usb, to instantiate the common
> driver structures and essential common facilities, eg. debugfs, and
> brcmf_bus_start() is called by bus-specific code when everything is
> ready for use. For PCIe there is nothing between those calls but for
> SDIO there are several steps before the party can start, hence the name.

Sorry you find this cleanup try this way. If you still have some
patience for this /silly/ stuff, I've one more question.

So as you noticed (and confirmed my note) both: brcmf_attach and
brcmf_bus_start are called from bus specific code. They initialize
some *common* stuff. How did you come with the "brcmf_bus_start" name
then?
1) It's called after bus got initialized (started?)
2) It's initializes common stuff
3) It doesn't execute bus specific code

My point is "brcmf_bus_start" name doesn't seem to make much sense. If
you have any better suggestion than "brcmf_bus_start" and
"brcmf_attach_phase2", I'll be happy to use it. What could it be?
brcmf_attach_after_bus_started would be more accurate but too long.

--=20
Rafa=C5=82

Re: [PATCH 1/2] brcmfmac: rename brcmf_bus_start function to brcmf_attach_phase2

[Arend Van Spriel]

On 18-1-2017 10:51, Rafał Miłecki wrote:
> On 18 January 2017 at 10:25, Arend Van Spriel
> <arend.vanspriel@broadcom.com> wrote:
>> On 17-1-2017 20:23, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>
>>> This change intends to make init/attach process easier to follow.
>>>
>>> What driver were doing in brcmf_bus_start wasn't bus specific at all and
>>> function brcmf_bus_stop wasn't undoing things done there. It seems
>>> brcmf_detach was actually undoing things done in both: brcmf_attach and
>>> brcmf_bus_start.
>>>
>>> To me it makes more sense to call these two function in a similar way as
>>> they both handle some part of attaching process. It also makes
>>> brcmf_detach more meaningful.
>>
>> To me this is all bike-shedding and I have two options 1) what's in a
>> name and let it pass, or 2) explain the naming. Let's try option 2). It
>> seems your expectation was different because of the name
>> brcmf_*bus*_start(). The function brcmf_attach() is called by
>> bus-specific code, ie. sdio, pcie, or usb, to instantiate the common
>> driver structures and essential common facilities, eg. debugfs, and
>> brcmf_bus_start() is called by bus-specific code when everything is
>> ready for use. For PCIe there is nothing between those calls but for
>> SDIO there are several steps before the party can start, hence the name.
> 
> Sorry you find this cleanup try this way. If you still have some
> patience for this /silly/ stuff, I've one more question.
> 
> So as you noticed (and confirmed my note) both: brcmf_attach and
> brcmf_bus_start are called from bus specific code. They initialize
> some *common* stuff. How did you come with the "brcmf_bus_start" name
> then?
> 1) It's called after bus got initialized (started?)
> 2) It's initializes common stuff
> 3) It doesn't execute bus specific code
> 
> My point is "brcmf_bus_start" name doesn't seem to make much sense. If
> you have any better suggestion than "brcmf_bus_start" and
> "brcmf_attach_phase2", I'll be happy to use it. What could it be?
> brcmf_attach_after_bus_started would be more accurate but too long.

It is a signal given by bus specific code that common code can "start
using the bus" for communication with the device. Maybe
brcmf_bus_started() is more accurate. The fact that common code uses
that signal to execute more initialization stuff is irrelevant.

Regards,
Arend

Re: [PATCH 1/2] brcmfmac: rename brcmf_bus_start function to brcmf_attach_phase2

[Rafał Miłecki]

On 18 January 2017 at 11:13, Arend Van Spriel
<arend.vanspriel@broadcom.com> wrote:
> On 18-1-2017 10:51, Rafa=C5=82 Mi=C5=82ecki wrote:
>> On 18 January 2017 at 10:25, Arend Van Spriel
>> <arend.vanspriel@broadcom.com> wrote:
>>> On 17-1-2017 20:23, Rafa=C5=82 Mi=C5=82ecki wrote:
>>>> From: Rafa=C5=82 Mi=C5=82ecki <rafal@milecki.pl>
>>>>
>>>> This change intends to make init/attach process easier to follow.
>>>>
>>>> What driver were doing in brcmf_bus_start wasn't bus specific at all a=
nd
>>>> function brcmf_bus_stop wasn't undoing things done there. It seems
>>>> brcmf_detach was actually undoing things done in both: brcmf_attach an=
d
>>>> brcmf_bus_start.
>>>>
>>>> To me it makes more sense to call these two function in a similar way =
as
>>>> they both handle some part of attaching process. It also makes
>>>> brcmf_detach more meaningful.
>>>
>>> To me this is all bike-shedding and I have two options 1) what's in a
>>> name and let it pass, or 2) explain the naming. Let's try option 2). It
>>> seems your expectation was different because of the name
>>> brcmf_*bus*_start(). The function brcmf_attach() is called by
>>> bus-specific code, ie. sdio, pcie, or usb, to instantiate the common
>>> driver structures and essential common facilities, eg. debugfs, and
>>> brcmf_bus_start() is called by bus-specific code when everything is
>>> ready for use. For PCIe there is nothing between those calls but for
>>> SDIO there are several steps before the party can start, hence the name=
.
>>
>> Sorry you find this cleanup try this way. If you still have some
>> patience for this /silly/ stuff, I've one more question.
>>
>> So as you noticed (and confirmed my note) both: brcmf_attach and
>> brcmf_bus_start are called from bus specific code. They initialize
>> some *common* stuff. How did you come with the "brcmf_bus_start" name
>> then?
>> 1) It's called after bus got initialized (started?)
>> 2) It's initializes common stuff
>> 3) It doesn't execute bus specific code
>>
>> My point is "brcmf_bus_start" name doesn't seem to make much sense. If
>> you have any better suggestion than "brcmf_bus_start" and
>> "brcmf_attach_phase2", I'll be happy to use it. What could it be?
>> brcmf_attach_after_bus_started would be more accurate but too long.
>
> It is a signal given by bus specific code that common code can "start
> using the bus" for communication with the device. Maybe
> brcmf_bus_started() is more accurate. The fact that common code uses
> that signal to execute more initialization stuff is irrelevant.

Sounds OK to me!

--=20
Rafa=C5=82