[PATCH 0/8] staging: ks7010: factor out some functions

[PATCH 0/8] staging: ks7010: factor out some functions

[Sergio Paracuellos]

This patch series factors out some functions to improve
a bit readability in ks7010_sdio source file.

Sergio Paracuellos (8):
  staging: ks7010: remove unnecessary 'out of memory' message
  staging: ks7010: factor out irq enable process to
    ks7010_sdio_init_irqs
  staging: ks7010: fix label to jump to in error case
  staging: ks7010: factor out irq setup process to
    ks7010_sdio_setup_irqs
  staging: ks7010: factor out ks_wlan_private init process into
    ks7010_private_init
  staging: ks7010: factor out initial enqueue process into
    ks7010_sme_enqueue_events
  staging: ks7010: factor out check for firmware running into
    ks7010_is_firmware_running
  staging: ks7010: factor out firmware copy process into
    ks7010_copy_firmware

 drivers/staging/ks7010/ks7010_sdio.c | 252 +++++++++++++++++++++--------------
 1 file changed, 149 insertions(+), 103 deletions(-)

-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/8] staging: ks7010: remove unnecessary 'out of memory' message

[Sergio Paracuellos]

This commit removes unnecessay out of memory message
fixing the following checkpach.pl warning:
WARNING: Possible unnecessary 'out of memory' message

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/ks7010/ks7010_sdio.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.c b/drivers/staging/ks7010/ks7010_sdio.c
index 0cc14ac..19dfbb9 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -957,10 +957,8 @@ static int send_stop_request(struct sdio_func *func)
 	card = sdio_get_drvdata(func);
 
 	pp = kzalloc(hif_align_size(sizeof(*pp)), GFP_KERNEL);
-	if (!pp) {
-		netdev_err(card->priv->net_dev, "allocate memory failed..\n");
+	if (!pp)
 		return -ENOMEM;
-	}
 
 	size = sizeof(*pp) - sizeof(pp->header.size);
 	pp->header.size = cpu_to_le16((uint16_t)size);
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/8] staging: ks7010: factor out irq enable process to ks7010_sdio_init_irqs

[Sergio Paracuellos]

This commit extracts sdio irq enable process to a new function
ks7010_sdio_init_irqs to improve readability.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/ks7010/ks7010_sdio.c | 43 ++++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.c b/drivers/staging/ks7010/ks7010_sdio.c
index 19dfbb9..10374be 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -802,13 +802,38 @@ static void ks7010_init_defaults(struct ks_wlan_private *priv)
 	priv->reg.rate_set.size = 12;
 }
 
+static void ks7010_sdio_init_irqs(struct sdio_func *func,
+				  struct ks_wlan_private *priv)
+{
+	unsigned char byte;
+	int ret;
+
+	/*
+	 * interrupt setting
+	 * clear Interrupt status write
+	 * (ARMtoSD_InterruptPending FN1:00_0024)
+	 */
+	sdio_claim_host(func);
+	ret = ks7010_sdio_writeb(priv, INT_PENDING, 0xff);
+	sdio_release_host(func);
+	if (ret)
+		netdev_err(priv->net_dev, " error : INT_PENDING\n");
+
+	/* enable ks7010sdio interrupt */
+	byte = (INT_GCR_B | INT_READ_STATUS | INT_WRITE_STATUS);
+	sdio_claim_host(func);
+	ret = ks7010_sdio_writeb(priv, INT_ENABLE, byte);
+	sdio_release_host(func);
+	if (ret)
+		netdev_err(priv->net_dev, " err : INT_ENABLE\n");
+}
+
 static int ks7010_sdio_probe(struct sdio_func *func,
 			     const struct sdio_device_id *device)
 {
 	struct ks_wlan_private *priv;
 	struct ks_sdio_card *card;
 	struct net_device *netdev;
-	unsigned char byte;
 	int ret;
 
 	priv = NULL;
@@ -898,21 +923,7 @@ static int ks7010_sdio_probe(struct sdio_func *func,
 		goto err_free_netdev;
 	}
 
-	/* interrupt setting */
-	/* clear Interrupt status write (ARMtoSD_InterruptPending FN1:00_0024) */
-	sdio_claim_host(func);
-	ret = ks7010_sdio_writeb(priv, INT_PENDING, 0xff);
-	sdio_release_host(func);
-	if (ret)
-		netdev_err(priv->net_dev, " error : INT_PENDING\n");
-
-	/* enable ks7010sdio interrupt */
-	byte = (INT_GCR_B | INT_READ_STATUS | INT_WRITE_STATUS);
-	sdio_claim_host(func);
-	ret = ks7010_sdio_writeb(priv, INT_ENABLE, byte);
-	sdio_release_host(func);
-	if (ret)
-		netdev_err(priv->net_dev, " err : INT_ENABLE\n");
+	ks7010_sdio_init_irqs(func, priv);
 
 	priv->dev_state = DEVICE_STATE_BOOT;
 
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 3/8] staging: ks7010: fix label to jump to in error case

[Sergio Paracuellos]

This commit fixs the label to jump to when in case
an error occurs disabling interrupts. At this point
of the code sdio_enable_func() function has been
successfully called.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/ks7010/ks7010_sdio.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.c b/drivers/staging/ks7010/ks7010_sdio.c
index 10374be..a55611f 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -858,7 +858,8 @@ static int ks7010_sdio_probe(struct sdio_func *func,
 	/* interrupt disable */
 	sdio_writeb(func, 0, INT_ENABLE, &ret);
 	if (ret)
-		goto err_free_card;
+		goto err_disable_func;
+
 	sdio_writeb(func, 0xff, INT_PENDING, &ret);
 	if (ret)
 		goto err_disable_func;
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 4/8] staging: ks7010: factor out irq setup process to ks7010_sdio_setup_irqs

[Sergio Paracuellos]

This commit extract sdio irq setup process into a new
function ks7010_sdio_setup_irqs to improve readability.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/ks7010/ks7010_sdio.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.c b/drivers/staging/ks7010/ks7010_sdio.c
index a55611f..5b3e814 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -802,6 +802,26 @@ static void ks7010_init_defaults(struct ks_wlan_private *priv)
 	priv->reg.rate_set.size = 12;
 }
 
+static int ks7010_sdio_setup_irqs(struct sdio_func *func)
+{
+	int ret;
+
+	/* interrupt disable */
+	sdio_writeb(func, 0, INT_ENABLE, &ret);
+	if (ret)
+		goto irq_error;
+
+	sdio_writeb(func, 0xff, INT_PENDING, &ret);
+	if (ret)
+		goto irq_error;
+
+	/* setup interrupt handler */
+	ret = sdio_claim_irq(func, ks_sdio_interrupt);
+
+irq_error:
+	return ret;
+}
+
 static void ks7010_sdio_init_irqs(struct sdio_func *func,
 				  struct ks_wlan_private *priv)
 {
@@ -855,17 +875,7 @@ static int ks7010_sdio_probe(struct sdio_func *func,
 	if (ret)
 		goto err_free_card;
 
-	/* interrupt disable */
-	sdio_writeb(func, 0, INT_ENABLE, &ret);
-	if (ret)
-		goto err_disable_func;
-
-	sdio_writeb(func, 0xff, INT_PENDING, &ret);
-	if (ret)
-		goto err_disable_func;
-
-	/* setup interrupt handler */
-	ret = sdio_claim_irq(func, ks_sdio_interrupt);
+	ret = ks7010_sdio_setup_irqs(func);
 	if (ret)
 		goto err_disable_func;
 
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 5/8] staging: ks7010: factor out ks_wlan_private init process into ks7010_private_init

[Sergio Paracuellos]

This commit extract ks_wlan_private initialization process
into a new function ks7010_private_init to improve a bit
readability.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/ks7010/ks7010_sdio.c | 50 ++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 22 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.c b/drivers/staging/ks7010/ks7010_sdio.c
index 5b3e814..c14721e 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -848,6 +848,33 @@ static void ks7010_sdio_init_irqs(struct sdio_func *func,
 		netdev_err(priv->net_dev, " err : INT_ENABLE\n");
 }
 
+static void ks7010_private_init(struct ks_wlan_private *priv,
+				struct ks_sdio_card *card,
+				struct net_device *netdev)
+{
+	/* private memory initialize */
+	priv->ks_sdio_card = card;
+
+	priv->dev_state = DEVICE_STATE_PREBOOT;
+	priv->net_dev = netdev;
+	priv->firmware_version[0] = '\0';
+	priv->version_size = 0;
+	priv->last_doze = jiffies;
+	priv->last_wakeup = jiffies;
+	memset(&priv->nstats, 0, sizeof(priv->nstats));
+	memset(&priv->wstats, 0, sizeof(priv->wstats));
+
+	/* sleep mode */
+	atomic_set(&priv->sleepstatus.doze_request, 0);
+	atomic_set(&priv->sleepstatus.wakeup_request, 0);
+	atomic_set(&priv->sleepstatus.wakeup_request, 0);
+
+	trx_device_init(priv);
+	hostif_init(priv);
+	ks_wlan_net_start(netdev);
+	ks7010_init_defaults(priv);
+}
+
 static int ks7010_sdio_probe(struct sdio_func *func,
 			     const struct sdio_device_id *device)
 {
@@ -903,28 +930,7 @@ static int ks7010_sdio_probe(struct sdio_func *func,
 	card->priv = priv;
 	SET_NETDEV_DEV(netdev, &card->func->dev);	/* for create sysfs symlinks */
 
-	/* private memory initialize */
-	priv->ks_sdio_card = card;
-
-	priv->dev_state = DEVICE_STATE_PREBOOT;
-	priv->net_dev = netdev;
-	priv->firmware_version[0] = '\0';
-	priv->version_size = 0;
-	priv->last_doze = jiffies;
-	priv->last_wakeup = jiffies;
-	memset(&priv->nstats, 0, sizeof(priv->nstats));
-	memset(&priv->wstats, 0, sizeof(priv->wstats));
-
-	/* sleep mode */
-	atomic_set(&priv->sleepstatus.doze_request, 0);
-	atomic_set(&priv->sleepstatus.wakeup_request, 0);
-	atomic_set(&priv->sleepstatus.wakeup_request, 0);
-
-	trx_device_init(priv);
-	hostif_init(priv);
-	ks_wlan_net_start(netdev);
-
-	ks7010_init_defaults(priv);
+	ks7010_private_init(priv, card, netdev);
 
 	ret = ks7010_upload_firmware(card);
 	if (ret) {
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 6/8] staging: ks7010: factor out initial enqueue process into ks7010_sme_enqueue_events

[Sergio Paracuellos]

This commit extract initial enqueue process into a new
ks7010_sme_enqueue_events function.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/ks7010/ks7010_sdio.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.c b/drivers/staging/ks7010/ks7010_sdio.c
index c14721e..5b6c7a7 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -730,21 +730,8 @@ static int ks7010_upload_firmware(struct ks_sdio_card *card)
 	return ret;
 }
 
-static void ks7010_card_init(struct ks_wlan_private *priv)
+static void ks7010_sme_enqueue_events(struct ks_wlan_private *priv)
 {
-	init_completion(&priv->confirm_wait);
-
-	/* get mac address & firmware version */
-	hostif_sme_enqueue(priv, SME_START);
-
-	if (!wait_for_completion_interruptible_timeout
-	    (&priv->confirm_wait, 5 * HZ)) {
-		netdev_dbg(priv->net_dev, "wait time out!! SME_START\n");
-	}
-
-	if (priv->mac_address_valid && priv->version_size != 0)
-		priv->dev_state = DEVICE_STATE_PREINIT;
-
 	hostif_sme_enqueue(priv, SME_GET_EEPROM_CKSUM);
 
 	/* load initial wireless parameter */
@@ -763,6 +750,24 @@ static void ks7010_card_init(struct ks_wlan_private *priv)
 	hostif_sme_enqueue(priv, SME_RSN_ENABLED_REQUEST);
 	hostif_sme_enqueue(priv, SME_MODE_SET_REQUEST);
 	hostif_sme_enqueue(priv, SME_START_REQUEST);
+}
+
+static void ks7010_card_init(struct ks_wlan_private *priv)
+{
+	init_completion(&priv->confirm_wait);
+
+	/* get mac address & firmware version */
+	hostif_sme_enqueue(priv, SME_START);
+
+	if (!wait_for_completion_interruptible_timeout
+	    (&priv->confirm_wait, 5 * HZ)) {
+		netdev_dbg(priv->net_dev, "wait time out!! SME_START\n");
+	}
+
+	if (priv->mac_address_valid && priv->version_size != 0)
+		priv->dev_state = DEVICE_STATE_PREINIT;
+
+	ks7010_sme_enqueue_events(priv);
 
 	if (!wait_for_completion_interruptible_timeout
 	    (&priv->confirm_wait, 5 * HZ)) {
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 7/8] staging: ks7010: factor out check for firmware running into ks7010_is_firmware_running

[Sergio Paracuellos]

This commit extracts process to check if firmware is running
into a new inline function called ks7010_is_firmware_running.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/ks7010/ks7010_sdio.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.c b/drivers/staging/ks7010/ks7010_sdio.c
index 5b6c7a7..11d5be1 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -639,12 +639,20 @@ static int ks7010_sdio_data_compare(struct ks_wlan_private *priv, u32 address,
 	return ret;
 }
 
+static inline bool ks7010_is_firmware_running(struct ks_wlan_private *priv,
+					      int *ret)
+{
+	unsigned char byte;
+
+	*ret = ks7010_sdio_readb(priv, GCR_A, &byte);
+	return (byte == GCR_A_RUN);
+}
+
 static int ks7010_upload_firmware(struct ks_sdio_card *card)
 {
 	struct ks_wlan_private *priv = card->priv;
 	unsigned int size, offset, n = 0;
 	unsigned char *rom_buf;
-	unsigned char byte = 0;
 	int ret;
 	unsigned int length;
 	const struct firmware *fw_entry = NULL;
@@ -655,9 +663,7 @@ static int ks7010_upload_firmware(struct ks_sdio_card *card)
 
 	sdio_claim_host(card->func);
 
-	/* Firmware running ? */
-	ret = ks7010_sdio_readb(priv, GCR_A, &byte);
-	if (byte == GCR_A_RUN) {
+	if (ks7010_is_firmware_running(priv, &ret)) {
 		netdev_dbg(priv->net_dev, "MAC firmware running ...\n");
 		goto release_host_and_free;
 	}
@@ -706,11 +712,7 @@ static int ks7010_upload_firmware(struct ks_sdio_card *card)
 	/* Firmware running check */
 	for (n = 0; n < 50; ++n) {
 		mdelay(10);	/* wait_ms(10); */
-		ret = ks7010_sdio_readb(priv, GCR_A, &byte);
-		if (ret)
-			goto release_firmware;
-
-		if (byte == GCR_A_RUN)
+		if (ks7010_is_firmware_running(priv, &ret))
 			break;
 	}
 	if ((50) <= n) {
@@ -719,8 +721,6 @@ static int ks7010_upload_firmware(struct ks_sdio_card *card)
 		goto release_firmware;
 	}
 
-	ret = 0;
-
  release_firmware:
 	release_firmware(fw_entry);
  release_host_and_free:
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 8/8] staging: ks7010: factor out firmware copy process into ks7010_copy_firmware

[Sergio Paracuellos]

This commit extracts firmware copy process into a new function
ks7010_copy_firmware. Because rom_buf is only needed for this
process, memory request for it has been also moved to this new
function so the error handling label release_host_and_free has
been renamed to release_host into ks7010_upload_firmware original
function.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/ks7010/ks7010_sdio.c | 71 ++++++++++++++++++++++--------------
 1 file changed, 43 insertions(+), 28 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.c b/drivers/staging/ks7010/ks7010_sdio.c
index 11d5be1..f30dba8 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -648,34 +648,21 @@ static inline bool ks7010_is_firmware_running(struct ks_wlan_private *priv,
 	return (byte == GCR_A_RUN);
 }
 
-static int ks7010_upload_firmware(struct ks_sdio_card *card)
+static int ks7010_copy_firmware(struct ks_wlan_private *priv,
+				const struct firmware *fw_entry)
 {
-	struct ks_wlan_private *priv = card->priv;
-	unsigned int size, offset, n = 0;
-	unsigned char *rom_buf;
 	int ret;
 	unsigned int length;
-	const struct firmware *fw_entry = NULL;
+	unsigned int size;
+	unsigned int offset;
+	unsigned int n = 0;
+	unsigned char *rom_buf;
 
 	rom_buf = kmalloc(ROM_BUFF_SIZE, GFP_KERNEL);
 	if (!rom_buf)
 		return -ENOMEM;
 
-	sdio_claim_host(card->func);
-
-	if (ks7010_is_firmware_running(priv, &ret)) {
-		netdev_dbg(priv->net_dev, "MAC firmware running ...\n");
-		goto release_host_and_free;
-	}
-
-	ret = request_firmware(&fw_entry, ROM_FILE,
-			       &priv->ks_sdio_card->func->dev);
-	if (ret)
-		goto release_host_and_free;
-
 	length = fw_entry->size;
-
-	n = 0;
 	do {
 		if (length >= ROM_BUFF_SIZE) {
 			size = ROM_BUFF_SIZE;
@@ -686,32 +673,61 @@ static int ks7010_upload_firmware(struct ks_sdio_card *card)
 		}
 		if (size == 0)
 			break;
+
 		memcpy(rom_buf, fw_entry->data + n, size);
 
 		offset = n;
-		ret = ks7010_sdio_update_index(priv, KS7010_IRAM_ADDRESS + offset);
+		ret = ks7010_sdio_update_index(priv,
+					       KS7010_IRAM_ADDRESS + offset);
 		if (ret)
-			goto release_firmware;
+			goto copy_error;
 
 		ret = ks7010_sdio_write(priv, DATA_WINDOW, rom_buf, size);
 		if (ret)
-			goto release_firmware;
+			goto copy_error;
 
-		ret = ks7010_sdio_data_compare(priv, DATA_WINDOW, rom_buf, size);
+		ret = ks7010_sdio_data_compare(priv,
+					       DATA_WINDOW, rom_buf, size);
 		if (ret)
-			goto release_firmware;
+			goto copy_error;
 
 		n += size;
 
 	} while (size);
 
-	ret = ks7010_sdio_writeb(priv, GCR_A, GCR_A_REMAP);
+	return ks7010_sdio_writeb(priv, GCR_A, GCR_A_REMAP);
+
+copy_error:
+	kfree(rom_buf);
+	return ret;
+}
+
+static int ks7010_upload_firmware(struct ks_sdio_card *card)
+{
+	struct ks_wlan_private *priv = card->priv;
+	unsigned int n;
+	int ret;
+	const struct firmware *fw_entry = NULL;
+
+	sdio_claim_host(card->func);
+
+	if (ks7010_is_firmware_running(priv, &ret)) {
+		netdev_dbg(priv->net_dev, "MAC firmware running ...\n");
+		goto release_host;
+	}
+
+	ret = request_firmware(&fw_entry, ROM_FILE,
+			       &priv->ks_sdio_card->func->dev);
+	if (ret)
+		goto release_host;
+
+	ret = ks7010_copy_firmware(priv, fw_entry);
 	if (ret)
 		goto release_firmware;
 
 	/* Firmware running check */
 	for (n = 0; n < 50; ++n) {
-		mdelay(10);	/* wait_ms(10); */
+		mdelay(10);
 		if (ks7010_is_firmware_running(priv, &ret))
 			break;
 	}
@@ -723,9 +739,8 @@ static int ks7010_upload_firmware(struct ks_sdio_card *card)
 
  release_firmware:
 	release_firmware(fw_entry);
- release_host_and_free:
+ release_host:
 	sdio_release_host(card->func);
-	kfree(rom_buf);
 
 	return ret;
 }
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 7/8] staging: ks7010: factor out check for firmware running into ks7010_is_firmware_running

[Dan Carpenter]

On Fri, Mar 30, 2018 at 05:13:20PM +0200, Sergio Paracuellos wrote:
> This commit extracts process to check if firmware is running
> into a new inline function called ks7010_is_firmware_running.
> 
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> ---
>  drivers/staging/ks7010/ks7010_sdio.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/ks7010/ks7010_sdio.c b/drivers/staging/ks7010/ks7010_sdio.c
> index 5b6c7a7..11d5be1 100644
> --- a/drivers/staging/ks7010/ks7010_sdio.c
> +++ b/drivers/staging/ks7010/ks7010_sdio.c
> @@ -639,12 +639,20 @@ static int ks7010_sdio_data_compare(struct ks_wlan_private *priv, u32 address,
>  	return ret;
>  }
>  
> +static inline bool ks7010_is_firmware_running(struct ks_wlan_private *priv,
> +					      int *ret)
> +{
> +	unsigned char byte;
> +
> +	*ret = ks7010_sdio_readb(priv, GCR_A, &byte);
> +	return (byte == GCR_A_RUN);
> +}
> +
>  static int ks7010_upload_firmware(struct ks_sdio_card *card)
>  {
>  	struct ks_wlan_private *priv = card->priv;
>  	unsigned int size, offset, n = 0;
>  	unsigned char *rom_buf;
> -	unsigned char byte = 0;
>  	int ret;
>  	unsigned int length;
>  	const struct firmware *fw_entry = NULL;
> @@ -655,9 +663,7 @@ static int ks7010_upload_firmware(struct ks_sdio_card *card)
>  
>  	sdio_claim_host(card->func);
>  
> -	/* Firmware running ? */
> -	ret = ks7010_sdio_readb(priv, GCR_A, &byte);
> -	if (byte == GCR_A_RUN) {
> +	if (ks7010_is_firmware_running(priv, &ret)) {
>  		netdev_dbg(priv->net_dev, "MAC firmware running ...\n");
>  		goto release_host_and_free;


The original code is obviously buggy with regards to return values so
this doesn't make it noticeably worse, but I hate how mmc code uses the
parameters as a return value.  For example:

void sdio_writeb(struct sdio_func *func, u8 b, unsigned int addr, int *err_ret)

It should just return the error code...  :/

Anyway, if the readb succeeds but the byte isn't GCR_A_RUN then we're
returning ret == sucess here.  To be honest, once we fix the error
handling there propably isn't a lot to be gained from making this a
separate function.

	ret = ks7010_sdio_readb(priv, GCR_A, &status);
	if (ret)
		goto release_host_and_free;
	if (status == GCR_A_RUN) {
		ret = -EBUSY;
		goto release_host_and_free;
	}

regards,
dan carpenter


_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 8/8] staging: ks7010: factor out firmware copy process into ks7010_copy_firmware

[Dan Carpenter]

On Fri, Mar 30, 2018 at 05:13:21PM +0200, Sergio Paracuellos wrote:
> @@ -686,32 +673,61 @@ static int ks7010_upload_firmware(struct ks_sdio_card *card)
>  		}
>  		if (size == 0)
>  			break;
> +

There are a few unrelated white space changes.  This one is easy because
it's a blank line

>  		memcpy(rom_buf, fw_entry->data + n, size);
>  
>  		offset = n;
> -		ret = ks7010_sdio_update_index(priv, KS7010_IRAM_ADDRESS + offset);
> +		ret = ks7010_sdio_update_index(priv,
> +					       KS7010_IRAM_ADDRESS + offset);

but this one eats my review time to have to look at each letter to see
what the difference is and how it relates to factoring out the function.
I've marked all the unrelated white space changes.  Please, move them to
a different patch when you redo these.

>  		if (ret)
> -			goto release_firmware;
> +			goto copy_error;

Ideally, the label name would tell me what the goto does.  This label
name is based on the function name but that's useless because, of course
the label is going to be in the same function as the goto.  That's a
given.  A better name would be:  "goto free_rom_buf;".

>  
>  		ret = ks7010_sdio_write(priv, DATA_WINDOW, rom_buf, size);
>  		if (ret)
> -			goto release_firmware;
> +			goto copy_error;
>  
> -		ret = ks7010_sdio_data_compare(priv, DATA_WINDOW, rom_buf, size);
> +		ret = ks7010_sdio_data_compare(priv,
> +					       DATA_WINDOW, rom_buf, size);

Unrelated change.

>  		if (ret)
> -			goto release_firmware;
> +			goto copy_error;
>  
>  		n += size;
>  
>  	} while (size);
>  
> -	ret = ks7010_sdio_writeb(priv, GCR_A, GCR_A_REMAP);
> +	return ks7010_sdio_writeb(priv, GCR_A, GCR_A_REMAP);

You're leaking "rom_buf" here.  It needs to be:

	ret = ks7010_sdio_writeb(priv, GCR_A, GCR_A_REMAP);

free_rom_buf:
	kfree(rom_buf);
	return ret;

> +
> +copy_error:
> +	kfree(rom_buf);
> +	return ret;
> +}
> +
> +static int ks7010_upload_firmware(struct ks_sdio_card *card)
> +{
> +	struct ks_wlan_private *priv = card->priv;
> +	unsigned int n;
> +	int ret;
> +	const struct firmware *fw_entry = NULL;
> +
> +	sdio_claim_host(card->func);
> +
> +	if (ks7010_is_firmware_running(priv, &ret)) {
> +		netdev_dbg(priv->net_dev, "MAC firmware running ...\n");
> +		goto release_host;
> +	}
> +
> +	ret = request_firmware(&fw_entry, ROM_FILE,
> +			       &priv->ks_sdio_card->func->dev);
> +	if (ret)
> +		goto release_host;
> +
> +	ret = ks7010_copy_firmware(priv, fw_entry);
>  	if (ret)
>  		goto release_firmware;
>  
>  	/* Firmware running check */
>  	for (n = 0; n < 50; ++n) {
> -		mdelay(10);	/* wait_ms(10); */
> +		mdelay(10);

Unrelated change.

regards,
dan carpenter

Re: [PATCH 7/8] staging: ks7010: factor out check for firmware running into ks7010_is_firmware_running

[Sergio Paracuellos]

On Tue, Apr 3, 2018 at 11:23 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Fri, Mar 30, 2018 at 05:13:20PM +0200, Sergio Paracuellos wrote:
>> This commit extracts process to check if firmware is running
>> into a new inline function called ks7010_is_firmware_running.
>>
>> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
>> ---
>>  drivers/staging/ks7010/ks7010_sdio.c | 22 +++++++++++-----------
>>  1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/staging/ks7010/ks7010_sdio.c b/drivers/staging/ks7010/ks7010_sdio.c
>> index 5b6c7a7..11d5be1 100644
>> --- a/drivers/staging/ks7010/ks7010_sdio.c
>> +++ b/drivers/staging/ks7010/ks7010_sdio.c
>> @@ -639,12 +639,20 @@ static int ks7010_sdio_data_compare(struct ks_wlan_private *priv, u32 address,
>>       return ret;
>>  }
>>
>> +static inline bool ks7010_is_firmware_running(struct ks_wlan_private *priv,
>> +                                           int *ret)
>> +{
>> +     unsigned char byte;
>> +
>> +     *ret = ks7010_sdio_readb(priv, GCR_A, &byte);
>> +     return (byte == GCR_A_RUN);
>> +}
>> +
>>  static int ks7010_upload_firmware(struct ks_sdio_card *card)
>>  {
>>       struct ks_wlan_private *priv = card->priv;
>>       unsigned int size, offset, n = 0;
>>       unsigned char *rom_buf;
>> -     unsigned char byte = 0;
>>       int ret;
>>       unsigned int length;
>>       const struct firmware *fw_entry = NULL;
>> @@ -655,9 +663,7 @@ static int ks7010_upload_firmware(struct ks_sdio_card *card)
>>
>>       sdio_claim_host(card->func);
>>
>> -     /* Firmware running ? */
>> -     ret = ks7010_sdio_readb(priv, GCR_A, &byte);
>> -     if (byte == GCR_A_RUN) {
>> +     if (ks7010_is_firmware_running(priv, &ret)) {
>>               netdev_dbg(priv->net_dev, "MAC firmware running ...\n");
>>               goto release_host_and_free;
>
>
> The original code is obviously buggy with regards to return values so
> this doesn't make it noticeably worse, but I hate how mmc code uses the
> parameters as a return value.  For example:
>
> void sdio_writeb(struct sdio_func *func, u8 b, unsigned int addr, int *err_ret)
>
> It should just return the error code...  :/

I agree with you in this, sdio_writeb should just return error code,
but in this moment is the API that exists in the kernel.
I don't know if this is going to be changed but this patch makes a bit
clear the original code now.
If it is nonsense because it is going to be changed just skip this
patch, please.

>
> Anyway, if the readb succeeds but the byte isn't GCR_A_RUN then we're
> returning ret == sucess here.  To be honest, once we fix the error
> handling there propably isn't a lot to be gained from making this a
> separate function.
>
>         ret = ks7010_sdio_readb(priv, GCR_A, &status);
>         if (ret)
>                 goto release_host_and_free;
>         if (status == GCR_A_RUN) {
>                 ret = -EBUSY;
>                 goto release_host_and_free;
>         }
>
> regards,
> dan carpenter

Best regards,
    Sergio Paracuellos

>
>
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 8/8] staging: ks7010: factor out firmware copy process into ks7010_copy_firmware

[Sergio Paracuellos]

On Tue, Apr 3, 2018 at 11:52 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Fri, Mar 30, 2018 at 05:13:21PM +0200, Sergio Paracuellos wrote:
>> @@ -686,32 +673,61 @@ static int ks7010_upload_firmware(struct ks_sdio_card *card)
>>               }
>>               if (size == 0)
>>                       break;
>> +
>
> There are a few unrelated white space changes.  This one is easy because
> it's a blank line

The unrelated changes are because code has been moved to a new
function and I reorder
here a bit the code. Because they are minor changes in the new
function I though it just
be ok to do this. Obviously it seems I was wrong :-).

>
>>               memcpy(rom_buf, fw_entry->data + n, size);
>>
>>               offset = n;
>> -             ret = ks7010_sdio_update_index(priv, KS7010_IRAM_ADDRESS + offset);
>> +             ret = ks7010_sdio_update_index(priv,
>> +                                            KS7010_IRAM_ADDRESS + offset);
>
> but this one eats my review time to have to look at each letter to see
> what the difference is and how it relates to factoring out the function.
> I've marked all the unrelated white space changes.  Please, move them to
> a different patch when you redo these.
>
>>               if (ret)
>> -                     goto release_firmware;
>> +                     goto copy_error;
>
> Ideally, the label name would tell me what the goto does.  This label
> name is based on the function name but that's useless because, of course
> the label is going to be in the same function as the goto.  That's a
> given.  A better name would be:  "goto free_rom_buf;".
>
>>
>>               ret = ks7010_sdio_write(priv, DATA_WINDOW, rom_buf, size);
>>               if (ret)
>> -                     goto release_firmware;
>> +                     goto copy_error;
>>
>> -             ret = ks7010_sdio_data_compare(priv, DATA_WINDOW, rom_buf, size);
>> +             ret = ks7010_sdio_data_compare(priv,
>> +                                            DATA_WINDOW, rom_buf, size);
>
> Unrelated change.
>
>>               if (ret)
>> -                     goto release_firmware;
>> +                     goto copy_error;
>>
>>               n += size;
>>
>>       } while (size);
>>
>> -     ret = ks7010_sdio_writeb(priv, GCR_A, GCR_A_REMAP);
>> +     return ks7010_sdio_writeb(priv, GCR_A, GCR_A_REMAP);
>
> You're leaking "rom_buf" here.  It needs to be:
>
>         ret = ks7010_sdio_writeb(priv, GCR_A, GCR_A_REMAP);

True, thanks for points me out this. I fix this and the label and
resend v2 for this.

>
> free_rom_buf:
>         kfree(rom_buf);
>         return ret;
>
>> +
>> +copy_error:
>> +     kfree(rom_buf);
>> +     return ret;
>> +}
>> +
>> +static int ks7010_upload_firmware(struct ks_sdio_card *card)
>> +{
>> +     struct ks_wlan_private *priv = card->priv;
>> +     unsigned int n;
>> +     int ret;
>> +     const struct firmware *fw_entry = NULL;
>> +
>> +     sdio_claim_host(card->func);
>> +
>> +     if (ks7010_is_firmware_running(priv, &ret)) {
>> +             netdev_dbg(priv->net_dev, "MAC firmware running ...\n");
>> +             goto release_host;
>> +     }
>> +
>> +     ret = request_firmware(&fw_entry, ROM_FILE,
>> +                            &priv->ks_sdio_card->func->dev);
>> +     if (ret)
>> +             goto release_host;
>> +
>> +     ret = ks7010_copy_firmware(priv, fw_entry);
>>       if (ret)
>>               goto release_firmware;
>>
>>       /* Firmware running check */
>>       for (n = 0; n < 50; ++n) {
>> -             mdelay(10);     /* wait_ms(10); */
>> +             mdelay(10);
>
> Unrelated change.

Thanks for the review, Dan.

>
> regards,
> dan carpenter

Best regards,
    Sergio Paracuellos

Re: [PATCH 7/8] staging: ks7010: factor out check for firmware running into ks7010_is_firmware_running

[Dan Carpenter]

On Tue, Apr 03, 2018 at 12:06:59PM +0200, Sergio Paracuellos wrote:
> On Tue, Apr 3, 2018 at 11:23 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Fri, Mar 30, 2018 at 05:13:20PM +0200, Sergio Paracuellos wrote:
> >> @@ -655,9 +663,7 @@ static int ks7010_upload_firmware(struct ks_sdio_card *card)
> >>
> >>       sdio_claim_host(card->func);
> >>
> >> -     /* Firmware running ? */
> >> -     ret = ks7010_sdio_readb(priv, GCR_A, &byte);
> >> -     if (byte == GCR_A_RUN) {
> >> +     if (ks7010_is_firmware_running(priv, &ret)) {
> >>               netdev_dbg(priv->net_dev, "MAC firmware running ...\n");
> >>               goto release_host_and_free;
> >
> >
> > The original code is obviously buggy with regards to return values so
> > this doesn't make it noticeably worse, but I hate how mmc code uses the
> > parameters as a return value.  For example:
> >
> > void sdio_writeb(struct sdio_func *func, u8 b, unsigned int addr, int *err_ret)
> >
> > It should just return the error code...  :/
> 
> I agree with you in this, sdio_writeb should just return error code,
> but in this moment is the API that exists in the kernel.

1)  We could change it if we wanted but we're all too lazy.  I for darn
    sure am too lazy.

2)  At least let's not copy that anti-pattern into new code.

3)  That was just general whinging and complaining...  It was the rest
    of the email which needs to be addressed.  Please fix
    ks7010_upload_firmware() to return an error code on this path.  I
    gave you some code.  Just copy and paste it.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel