Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1445067 > unrolled thread

[PATCH 0/9] staging: ks7010: Fine-tuning for a SDIO card driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 0/9] staging: ks7010: Fine-tuning for a SDIO card driver SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-17 20:10 +0200
    [PATCH 2/9] staging: ks7010: Delete unnecessary assignments for  buffer variables SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-17 20:20 +0200
      Re: [PATCH 2/9] staging: ks7010: Delete unnecessary assignments for  buffer variables Wolfram Sang <wsa@the-dreams.de> - 2016-07-20 17:50 +0200
    [PATCH 1/9] staging: ks7010: Delete unnecessary checks before the  function call "kfree" SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-17 20:20 +0200
      Re: [PATCH 1/9] staging: ks7010: Delete unnecessary checks before  the function call "kfree" Wolfram Sang <wsa@the-dreams.de> - 2016-07-20 17:50 +0200
    [PATCH 3/9] staging: ks7010: Return directly after a failed kmalloc() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-17 20:20 +0200
      Re: [PATCH 3/9] staging: ks7010: Return directly after a failed  kmalloc() Julia Lawall <julia.lawall@lip6.fr> - 2016-07-17 21:00 +0200
        Re: [PATCH 3/9] staging: ks7010: Return directly after a failed  kmalloc() Wolfram Sang <wsa@the-dreams.de> - 2016-07-20 17:50 +0200
          Re: [PATCH 3/9] staging: ks7010: Return directly after a failed  kmalloc() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-20 20:50 +0200
            Re: [PATCH 3/9] staging: ks7010: Return directly after a failed  kmalloc() Wolfram Sang <wsa@the-dreams.de> - 2016-07-21 08:30 +0200
              Re: staging: ks7010: Return directly after a failed kmalloc() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-22 09:40 +0200
                Re: staging: ks7010: Return directly after a failed kmalloc() Wolfram Sang <wsa@the-dreams.de> - 2016-07-22 09:50 +0200
    [PATCH 7/9] staging: ks7010: Replace three printk() calls by pr_err() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-17 20:30 +0200
      Re: [PATCH 7/9] staging: ks7010: Replace three printk() calls by  pr_err() Joe Perches <joe@perches.com> - 2016-07-17 22:30 +0200
        Re: [PATCH 7/9] staging: ks7010: Replace three printk() calls by  pr_err() Wolfram Sang <wsa@the-dreams.de> - 2016-07-20 18:00 +0200
    [PATCH 5/9] staging: ks7010: Delete unnecessary uses of the variable  "retval" SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-17 20:30 +0200
      Re: [PATCH 5/9] staging: ks7010: Delete unnecessary uses of the  variable "retval" Julia Lawall <julia.lawall@lip6.fr> - 2016-07-17 21:00 +0200
        Re: [PATCH 5/9] staging: ks7010: Delete unnecessary uses of the  variable "retval" Wolfram Sang <wsa@the-dreams.de> - 2016-07-20 18:00 +0200
          Re: [PATCH 5/9] staging: ks7010: Delete unnecessary uses of the  variable "retval" SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-20 20:50 +0200
            Re: [PATCH 5/9] staging: ks7010: Delete unnecessary uses of the  variable "retval" Wolfram Sang <wsa@the-dreams.de> - 2016-07-21 08:30 +0200
              Re: [PATCH 5/9] staging: ks7010: Delete unnecessary uses of the  variable "retval" Wolfram Sang <wsa@the-dreams.de> - 2016-07-21 09:30 +0200
              Re: [PATCH 5/9] staging: ks7010: Delete unnecessary uses of the  variable "retval" Julia Lawall <julia.lawall@lip6.fr> - 2016-07-21 09:30 +0200
                Re: staging: ks7010: Delete unnecessary uses of the variable "retval" SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-21 14:50 +0200
                  Re: staging: ks7010: Delete unnecessary uses of the variable  "retval" Julia Lawall <julia.lawall@lip6.fr> - 2016-07-21 15:00 +0200
                    Re: staging: ks7010: Delete unnecessary uses of the variable "retval" Wolfram Sang <wsa@the-dreams.de> - 2016-07-21 18:40 +0200
              Re: staging: ks7010: Delete unnecessary uses of the variable "retval" SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-21 15:40 +0200
                Re: staging: ks7010: Delete unnecessary uses of the variable "retval" Wolfram Sang <wsa@the-dreams.de> - 2016-07-21 18:40 +0200
                  Re: staging: ks7010: Delete unnecessary uses of the variable "retval" SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-21 23:30 +0200
                    Re: staging: ks7010: Delete unnecessary uses of the variable "retval" Wolfram Sang <wsa@the-dreams.de> - 2016-07-22 08:00 +0200
    [PATCH 8/9] staging: ks7010: Delete a variable in write_to_device() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-17 20:30 +0200
      Re: [PATCH 8/9] staging: ks7010: Delete a variable in  write_to_device() Wolfram Sang <wsa@the-dreams.de> - 2016-07-20 18:00 +0200
    [PATCH 4/9] staging: ks7010: Rename jump labels SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-17 20:30 +0200
      Re: [PATCH 4/9] staging: ks7010: Rename jump labels Wolfram Sang <wsa@the-dreams.de> - 2016-07-20 18:00 +0200
        Re: staging: ks7010: Rename jump labels SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-20 20:30 +0200
          Re: staging: ks7010: Rename jump labels Jean Delvare <jdelvare@suse.de> - 2016-07-20 23:20 +0200
            Re: staging: ks7010: Rename jump labels Wolfram Sang <wsa@the-dreams.de> - 2016-07-21 08:30 +0200
            Re: staging: ks7010: Rename jump labels "SF Markus Elfring" <elfring@users.sourceforge.net> - 2016-07-21 10:00 +0200
              Re: staging: ks7010: Rename jump labels Jean Delvare <jdelvare@suse.de> - 2016-07-25 14:40 +0200
            Re: staging: ks7010: Rename jump labels SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-21 17:40 +0200
              Re: staging: ks7010: Rename jump labels Jean Delvare <jdelvare@suse.de> - 2016-07-21 21:20 +0200
                Re: staging: ks7010: Rename jump labels SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-21 22:30 +0200
                  Re: staging: ks7010: Rename jump labels Jean Delvare <jdelvare@suse.de> - 2016-07-25 14:40 +0200
                    Re: staging: ks7010: Rename jump labels SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-25 18:20 +0200
                      Re: staging: ks7010: Rename jump labels Jean Delvare <jdelvare@suse.de> - 2016-07-25 23:10 +0200
    [PATCH 6/9] staging: ks7010: Delete unnecessary braces SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-17 20:30 +0200
      Re: [PATCH 6/9] staging: ks7010: Delete unnecessary braces Julia Lawall <julia.lawall@lip6.fr> - 2016-07-17 21:00 +0200
        Re: [PATCH 6/9] staging: ks7010: Delete unnecessary braces Wolfram Sang <wsa@the-dreams.de> - 2016-07-20 18:00 +0200
    [PATCH 9/9] staging: ks7010: Delete three unnecessary variable  initialisations SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-17 20:40 +0200
      Re: [PATCH 9/9] staging: ks7010: Delete three unnecessary variable  initialisations Julia Lawall <julia.lawall@lip6.fr> - 2016-07-17 21:00 +0200
        Re: [PATCH 9/9] staging: ks7010: Delete three unnecessary variable  initialisations Wolfram Sang <wsa@the-dreams.de> - 2016-07-20 18:00 +0200
        Re: [PATCH 9/9] staging: ks7010: Delete three unnecessary variable  initialisations SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-21 15:50 +0200
          Re: [PATCH 9/9] staging: ks7010: Delete three unnecessary variable  initialisations Julia Lawall <julia.lawall@lip6.fr> - 2016-07-21 16:00 +0200
            Re: staging: ks7010: Delete three unnecessary variable  initialisations SF Markus Elfring <elfring@users.sourceforge.net> - 2016-07-21 16:20 +0200

Page 3 of 3 — ← Prev page 1 2 [3]

#1448119 — Re: staging: ks7010: Rename jump labels

>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/scripts/checkpatch.pl?id=92d21ac74a9e3c09b0b01c764e530657e4c85c49#n4326
> 
> "#goto labels aren't indented, allow a single space however"
> 
> Can't be clearer :-)

Should such information from a comment in this script be also
explicitly mentioned in the document "CodingStyle" anyhow?

How should the support for indentation of jump labels with
a single space be expressed in all relevant Linux development documentation?


>> How do you generally think about jump label renaming?
> 
> Renaming from "out0:", "out1:" etc to something meaningful, yes.

I suggest to take another look at such identifiers.

Would you like to support the renaming of a label like "error_out1"
(in the function "ks7010_upload_firmware" for example)?


> Did you have anything else in mind?

Not really for this update suggestion.

Will the software evolution be continued also with information from the topic
"Source code review around jump label usage"?
https://lkml.org/lkml/2015/12/11/378
http://article.gmane.org/gmane.linux.kernel/2106190

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1449449 — Re: staging: ks7010: Rename jump labels

Hi Markus,

On Thu, 21 Jul 2016 22:11:53 +0200, SF Markus Elfring wrote:
> >> How do you generally think about jump label renaming?
> > 
> > Renaming from "out0:", "out1:" etc to something meaningful, yes.
> 
> I suggest to take another look at such identifiers.
> 
> Would you like to support the renaming of a label like "error_out1"
> (in the function "ks7010_upload_firmware" for example)?

They should be renamed too. Anything using numbers instead of explicit
labels should be updated. I included the reasons in the patch I just
sent, hopefully the documentation is clearer now.

> Will the software evolution be continued also with information from the topic
> "Source code review around jump label usage"?
> https://lkml.org/lkml/2015/12/11/378
> http://article.gmane.org/gmane.linux.kernel/2106190

Personally I see no value in such statistics. Either labels are wrong
(either wrong indentation or wrong name) and should be fixed, or they
are correct and you should not touch them. Whether the same label name
is used somewhere else is irrelevant. Labels are local by nature, so
uniqueness isn't a goal at all, only correctness.

-- 
Jean Delvare
SUSE L3 Support

[toc] | [prev] | [next] | [standalone]

#1449630 — Re: staging: ks7010: Rename jump labels

>> Would you like to support the renaming of a label like "error_out1"
>> (in the function "ks7010_upload_firmware" for example)?
> 
> They should be renamed too. Anything using numbers instead of explicit

Interesting …


> Anything using numbers instead of explicit labels should be updated.

Would you dare to search for corresponding update candidates explicitly
by special semantic patch scripts?


> I included the reasons in the patch I just sent,
> hopefully the documentation is clearer now.

I am curious on how feedback will evolve for your suggestion
"CodingStyle: Clarify and complete chapter 7".
https://lkml.org/lkml/2016/7/25/207

How do you think about to show a shorter label like "free_bar"
(instead of "err_free_bar") as an example?


>> "Source code review around jump label usage"?
>> https://lkml.org/lkml/2015/12/11/378
>> http://article.gmane.org/gmane.linux.kernel/2106190
> 
> Personally I see no value in such statistics.

Do they indicate any code smells eventually?


> Either labels are wrong (either wrong indentation or wrong name)
> and should be fixed, or they are correct and you should not touch them.

Do you find such changes worthwhile (without touching also any surrounding
source code)?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1449839 — Re: staging: ks7010: Rename jump labels

Hello Markus,

On lun., 2016-07-25 at 18:19 +0200, SF Markus Elfring wrote:
> >> Would you like to support the renaming of a label like "error_out1"
> >> (in the function "ks7010_upload_firmware" for example)?
> > 
> > They should be renamed too. Anything using numbers instead of explicit
> 
> Interesting …
> 
> 
> > Anything using numbers instead of explicit labels should be updated.
> 
> Would you dare to search for corresponding update candidates explicitly
> by special semantic patch scripts?

No. You started it all, and I do not have more time to devote to it. I
do not find it all particularly interesting, to be honest. I have a lot
of other things to work on, of much greater interest (to me.)

> > I included the reasons in the patch I just sent,
> > hopefully the documentation is clearer now.
> 
> I am curious on how feedback will evolve for your suggestion
> "CodingStyle: Clarify and complete chapter 7".
> https://lkml.org/lkml/2016/7/25/207
> 
> How do you think about to show a shorter label like "free_bar"
> (instead of "err_free_bar") as an example?

Up to whoever writes and maintains the code. As most things should be in
the absence of a compelling reason to normalize.

> >> "Source code review around jump label usage"?
> >> https://lkml.org/lkml/2015/12/11/378
> >> http://article.gmane.org/gmane.linux.kernel/2106190
> > 
> > Personally I see no value in such statistics.
> 
> Do they indicate any code smells eventually?

I have no idea what you mean, sorry.

> > Either labels are wrong (either wrong indentation or wrong name)
> > and should be fixed, or they are correct and you should not touch them.
> 
> Do you find such changes worthwhile (without touching also any surrounding
> source code)?

You keep asking more and more from me. May I remind you this is your
"project" in the first place, not mine? If you have no idea what should
be done, or even whether anything should be done, then just move on to
something else. I have already expressed all my views on this topic and
am not willing to say anything more about it.

Thanks,
-- 
Jean Delvare
SUSE L3 Support

[toc] | [prev] | [next] | [standalone]

#1445078 — [PATCH 6/9] staging: ks7010: Delete unnecessary braces

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 17 Jul 2016 18:39:03 +0200

Do not use curly brackets at some source code places
where a single statement should be sufficient.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/ks7010/ks7010_sdio.c | 48 ++++++++++++------------------------
 1 file changed, 16 insertions(+), 32 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.c b/drivers/staging/ks7010/ks7010_sdio.c
index b3ca8e2..1e072e3 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -109,9 +109,8 @@ void ks_wlan_hw_sleep_doze_request(struct ks_wlan_private *priv)
 		DPRINTK(3, "sleep_mode=SLP_SLEEP\n");
 		atomic_set(&priv->sleepstatus.status, 1);
 		priv->last_doze = jiffies;
-	} else {
+	} else
 		DPRINTK(1, "sleep_mode=%d\n", priv->sleep_mode);
-	}
 
 out:
 	priv->sleep_mode = atomic_read(&priv->sleepstatus.status);
@@ -140,9 +139,8 @@ void ks_wlan_hw_sleep_wakeup_request(struct ks_wlan_private *priv)
 		atomic_set(&priv->sleepstatus.status, 0);
 		priv->last_wakeup = jiffies;
 		++priv->wakeup_count;
-	} else {
+	} else
 		DPRINTK(1, "sleep_mode=%d\n", priv->sleep_mode);
-	}
 
 out:
 	priv->sleep_mode = atomic_read(&priv->sleepstatus.status);
@@ -164,10 +162,9 @@ void ks_wlan_hw_wakeup_request(struct ks_wlan_private *priv)
 		DPRINTK(4, "wake up : WAKEUP=%02X\n", rw_data);
 		priv->last_wakeup = jiffies;
 		++priv->wakeup_count;
-	} else {
+	} else
 		DPRINTK(1, "psstatus=%d\n",
 			atomic_read(&priv->psstatus.status));
-	}
 }
 
 int _ks_wlan_hw_power_save(struct ks_wlan_private *priv)
@@ -380,10 +377,9 @@ int ks_wlan_hw_tx(struct ks_wlan_private *priv, void *p, unsigned long size,
 	result = enqueue_txdev(priv, p, size, complete_handler, arg1, arg2);
 	spin_unlock(&priv->tx_dev.tx_dev_lock);
 
-	if (cnt_txqbody(priv) > 0) {
+	if (cnt_txqbody(priv) > 0)
 		queue_delayed_work(priv->ks_wlan_hw.ks7010sdio_wq,
 				   &priv->ks_wlan_hw.rw_wq, 0);
-	}
 	return result;
 }
 
@@ -399,9 +395,8 @@ static void rx_event_task(unsigned long dev)
 		hostif_receive(priv, rp->data, rp->size);
 		inc_rxqhead(priv);
 
-		if (cnt_rxqbody(priv) > 0) {
+		if (cnt_rxqbody(priv) > 0)
 			tasklet_schedule(&priv->ks_wlan_hw.rx_bh_task);
-		}
 	}
 
 	return;
@@ -463,12 +458,11 @@ static void ks_wlan_hw_rx(void *dev, uint16_t size)
 		DPRINTK(1, " error : READ_STATUS=%02X\n", read_status);
 	DPRINTK(4, "READ_STATUS=%02X\n", read_status);
 
-	if (atomic_read(&priv->psstatus.confirm_wait)) {
+	if (atomic_read(&priv->psstatus.confirm_wait))
 		if (IS_HIF_CONF(event)) {
 			DPRINTK(4, "IS_HIF_CONF true !!\n");
 			atomic_dec(&priv->psstatus.confirm_wait);
 		}
-	}
 
 	/* rx_event_task((void *)priv); */
 	tasklet_schedule(&priv->ks_wlan_hw.rx_bh_task);
@@ -537,13 +531,11 @@ static void ks7010_rw_function(struct work_struct *work)
 	}
 	DPRINTK(4, "WSTATUS_RSIZE=%02X\n", rw_data);
 
-	if (rw_data & RSIZE_MASK) {	/* Read schedule */
+	if (rw_data & RSIZE_MASK)	/* Read schedule */
 		ks_wlan_hw_rx((void *)priv,
 			      (uint16_t) (((rw_data & RSIZE_MASK) << 4)));
-	}
-	if ((rw_data & WSTATUS_MASK)) {
+	if (rw_data & WSTATUS_MASK)
 		tx_device_task((void *)priv);
-	}
 	_ks_wlan_hw_power_save(priv);
 release_host:
 	sdio_release_host(priv->ks_wlan_hw.sdio_card->func);
@@ -611,10 +603,9 @@ static void ks_sdio_interrupt(struct sdio_func *func)
 			}
 			DPRINTK(4, "WSTATUS_RSIZE=%02X\n", rw_data);
 			rsize = rw_data & RSIZE_MASK;
-			if (rsize) {	/* Read schedule */
+			if (rsize)	/* Read schedule */
 				ks_wlan_hw_rx((void *)priv,
 					      (uint16_t) (((rsize) << 4)));
-			}
 			if (rw_data & WSTATUS_MASK) {
 #if 0
 				if (status & INT_WRITE_STATUS
@@ -625,11 +616,10 @@ static void ks_sdio_interrupt(struct sdio_func *func)
 					    ks7010_sdio_write(priv, DATA_WINDOW,
 							      &rw_data,
 							      sizeof(rw_data));
-					if (retval) {
+					if (retval)
 						DPRINTK(1,
 							"write DATA_WINDOW Failed!!(%d)\n",
 							retval);
-					}
 					status &= ~INT_WRITE_STATUS;
 				} else {
 #endif
@@ -870,13 +860,11 @@ static void ks7010_card_init(struct ks_wlan_private *priv)
 	DPRINTK(5, "hostif_sme_enqueu()\n");
 
 	if (!wait_for_completion_interruptible_timeout
-	    (&priv->confirm_wait, 5 * HZ)) {
+	    (&priv->confirm_wait, 5 * HZ))
 		DPRINTK(1, "wait time out!! SME_START\n");
-	}
 
-	if (priv->mac_address_valid && priv->version_size) {
+	if (priv->mac_address_valid && priv->version_size)
 		priv->dev_state = DEVICE_STATE_PREINIT;
-	}
 
 	hostif_sme_enqueue(priv, SME_GET_EEPROM_CKSUM);
 
@@ -898,16 +886,14 @@ static void ks7010_card_init(struct ks_wlan_private *priv)
 	hostif_sme_enqueue(priv, SME_START_REQUEST);
 
 	if (!wait_for_completion_interruptible_timeout
-	    (&priv->confirm_wait, 5 * HZ)) {
+	    (&priv->confirm_wait, 5 * HZ))
 		DPRINTK(1, "wait time out!! wireless parameter set\n");
-	}
 
 	if (priv->dev_state >= DEVICE_STATE_PREINIT) {
 		DPRINTK(1, "DEVICE READY!!\n");
 		priv->dev_state = DEVICE_STATE_READY;
-	} else {
+	} else
 		DPRINTK(1, "dev_state=%d\n", priv->dev_state);
-	}
 }
 
 static void ks7010_init_defaults(struct ks_wlan_private *priv)
@@ -1057,9 +1043,8 @@ static int ks7010_sdio_probe(struct sdio_func *func,
 	sdio_claim_host(func);
 	ret = ks7010_sdio_write(priv, INT_PENDING, &rw_data, sizeof(rw_data));
 	sdio_release_host(func);
-	if (ret) {
+	if (ret)
 		DPRINTK(1, " error : INT_PENDING=%02X\n", rw_data);
-	}
 	DPRINTK(4, " clear Interrupt : INT_PENDING=%02X\n", rw_data);
 
 	/* enable ks7010sdio interrupt (INT_GCR_B|INT_READ_STATUS|INT_WRITE_STATUS) */
@@ -1067,9 +1052,8 @@ static int ks7010_sdio_probe(struct sdio_func *func,
 	sdio_claim_host(func);
 	ret = ks7010_sdio_write(priv, INT_ENABLE, &rw_data, sizeof(rw_data));
 	sdio_release_host(func);
-	if (ret) {
+	if (ret)
 		DPRINTK(1, " error : INT_ENABLE=%02X\n", rw_data);
-	}
 	DPRINTK(4, " enable Interrupt : INT_ENABLE=%02X\n", rw_data);
 	priv->dev_state = DEVICE_STATE_BOOT;
 
-- 
2.9.1

[toc] | [prev] | [next] | [standalone]

#1445085 — Re: [PATCH 6/9] staging: ks7010: Delete unnecessary braces

On Sun, 17 Jul 2016, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 17 Jul 2016 18:39:03 +0200
>
> Do not use curly brackets at some source code places
> where a single statement should be sufficient.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/staging/ks7010/ks7010_sdio.c | 48 ++++++++++++------------------------
>  1 file changed, 16 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/staging/ks7010/ks7010_sdio.c b/drivers/staging/ks7010/ks7010_sdio.c
> index b3ca8e2..1e072e3 100644
> --- a/drivers/staging/ks7010/ks7010_sdio.c
> +++ b/drivers/staging/ks7010/ks7010_sdio.c
> @@ -109,9 +109,8 @@ void ks_wlan_hw_sleep_doze_request(struct ks_wlan_private *priv)
>  		DPRINTK(3, "sleep_mode=SLP_SLEEP\n");
>  		atomic_set(&priv->sleepstatus.status, 1);
>  		priv->last_doze = jiffies;
> -	} else {
> +	} else
>  		DPRINTK(1, "sleep_mode=%d\n", priv->sleep_mode);
> -	}

This is a violation of CodingStyle:

This does not apply if only one branch of a conditional statement is a
single
statement; in the latter case use braces in both branches:

	if (condition) {
		do_this();
		do_that();
	} else {
		otherwise();
	}

julia

>
>  out:
>  	priv->sleep_mode = atomic_read(&priv->sleepstatus.status);
> @@ -140,9 +139,8 @@ void ks_wlan_hw_sleep_wakeup_request(struct ks_wlan_private *priv)
>  		atomic_set(&priv->sleepstatus.status, 0);
>  		priv->last_wakeup = jiffies;
>  		++priv->wakeup_count;
> -	} else {
> +	} else
>  		DPRINTK(1, "sleep_mode=%d\n", priv->sleep_mode);
> -	}
>
>  out:
>  	priv->sleep_mode = atomic_read(&priv->sleepstatus.status);
> @@ -164,10 +162,9 @@ void ks_wlan_hw_wakeup_request(struct ks_wlan_private *priv)
>  		DPRINTK(4, "wake up : WAKEUP=%02X\n", rw_data);
>  		priv->last_wakeup = jiffies;
>  		++priv->wakeup_count;
> -	} else {
> +	} else
>  		DPRINTK(1, "psstatus=%d\n",
>  			atomic_read(&priv->psstatus.status));
> -	}
>  }
>
>  int _ks_wlan_hw_power_save(struct ks_wlan_private *priv)
> @@ -380,10 +377,9 @@ int ks_wlan_hw_tx(struct ks_wlan_private *priv, void *p, unsigned long size,
>  	result = enqueue_txdev(priv, p, size, complete_handler, arg1, arg2);
>  	spin_unlock(&priv->tx_dev.tx_dev_lock);
>
> -	if (cnt_txqbody(priv) > 0) {
> +	if (cnt_txqbody(priv) > 0)
>  		queue_delayed_work(priv->ks_wlan_hw.ks7010sdio_wq,
>  				   &priv->ks_wlan_hw.rw_wq, 0);
> -	}
>  	return result;
>  }
>
> @@ -399,9 +395,8 @@ static void rx_event_task(unsigned long dev)
>  		hostif_receive(priv, rp->data, rp->size);
>  		inc_rxqhead(priv);
>
> -		if (cnt_rxqbody(priv) > 0) {
> +		if (cnt_rxqbody(priv) > 0)
>  			tasklet_schedule(&priv->ks_wlan_hw.rx_bh_task);
> -		}
>  	}
>
>  	return;
> @@ -463,12 +458,11 @@ static void ks_wlan_hw_rx(void *dev, uint16_t size)
>  		DPRINTK(1, " error : READ_STATUS=%02X\n", read_status);
>  	DPRINTK(4, "READ_STATUS=%02X\n", read_status);
>
> -	if (atomic_read(&priv->psstatus.confirm_wait)) {
> +	if (atomic_read(&priv->psstatus.confirm_wait))
>  		if (IS_HIF_CONF(event)) {
>  			DPRINTK(4, "IS_HIF_CONF true !!\n");
>  			atomic_dec(&priv->psstatus.confirm_wait);
>  		}
> -	}
>
>  	/* rx_event_task((void *)priv); */
>  	tasklet_schedule(&priv->ks_wlan_hw.rx_bh_task);
> @@ -537,13 +531,11 @@ static void ks7010_rw_function(struct work_struct *work)
>  	}
>  	DPRINTK(4, "WSTATUS_RSIZE=%02X\n", rw_data);
>
> -	if (rw_data & RSIZE_MASK) {	/* Read schedule */
> +	if (rw_data & RSIZE_MASK)	/* Read schedule */
>  		ks_wlan_hw_rx((void *)priv,
>  			      (uint16_t) (((rw_data & RSIZE_MASK) << 4)));
> -	}
> -	if ((rw_data & WSTATUS_MASK)) {
> +	if (rw_data & WSTATUS_MASK)
>  		tx_device_task((void *)priv);
> -	}
>  	_ks_wlan_hw_power_save(priv);
>  release_host:
>  	sdio_release_host(priv->ks_wlan_hw.sdio_card->func);
> @@ -611,10 +603,9 @@ static void ks_sdio_interrupt(struct sdio_func *func)
>  			}
>  			DPRINTK(4, "WSTATUS_RSIZE=%02X\n", rw_data);
>  			rsize = rw_data & RSIZE_MASK;
> -			if (rsize) {	/* Read schedule */
> +			if (rsize)	/* Read schedule */
>  				ks_wlan_hw_rx((void *)priv,
>  					      (uint16_t) (((rsize) << 4)));
> -			}
>  			if (rw_data & WSTATUS_MASK) {
>  #if 0
>  				if (status & INT_WRITE_STATUS
> @@ -625,11 +616,10 @@ static void ks_sdio_interrupt(struct sdio_func *func)
>  					    ks7010_sdio_write(priv, DATA_WINDOW,
>  							      &rw_data,
>  							      sizeof(rw_data));
> -					if (retval) {
> +					if (retval)
>  						DPRINTK(1,
>  							"write DATA_WINDOW Failed!!(%d)\n",
>  							retval);
> -					}
>  					status &= ~INT_WRITE_STATUS;
>  				} else {
>  #endif
> @@ -870,13 +860,11 @@ static void ks7010_card_init(struct ks_wlan_private *priv)
>  	DPRINTK(5, "hostif_sme_enqueu()\n");
>
>  	if (!wait_for_completion_interruptible_timeout
> -	    (&priv->confirm_wait, 5 * HZ)) {
> +	    (&priv->confirm_wait, 5 * HZ))
>  		DPRINTK(1, "wait time out!! SME_START\n");
> -	}
>
> -	if (priv->mac_address_valid && priv->version_size) {
> +	if (priv->mac_address_valid && priv->version_size)
>  		priv->dev_state = DEVICE_STATE_PREINIT;
> -	}
>
>  	hostif_sme_enqueue(priv, SME_GET_EEPROM_CKSUM);
>
> @@ -898,16 +886,14 @@ static void ks7010_card_init(struct ks_wlan_private *priv)
>  	hostif_sme_enqueue(priv, SME_START_REQUEST);
>
>  	if (!wait_for_completion_interruptible_timeout
> -	    (&priv->confirm_wait, 5 * HZ)) {
> +	    (&priv->confirm_wait, 5 * HZ))
>  		DPRINTK(1, "wait time out!! wireless parameter set\n");
> -	}
>
>  	if (priv->dev_state >= DEVICE_STATE_PREINIT) {
>  		DPRINTK(1, "DEVICE READY!!\n");
>  		priv->dev_state = DEVICE_STATE_READY;
> -	} else {
> +	} else
>  		DPRINTK(1, "dev_state=%d\n", priv->dev_state);
> -	}
>  }
>
>  static void ks7010_init_defaults(struct ks_wlan_private *priv)
> @@ -1057,9 +1043,8 @@ static int ks7010_sdio_probe(struct sdio_func *func,
>  	sdio_claim_host(func);
>  	ret = ks7010_sdio_write(priv, INT_PENDING, &rw_data, sizeof(rw_data));
>  	sdio_release_host(func);
> -	if (ret) {
> +	if (ret)
>  		DPRINTK(1, " error : INT_PENDING=%02X\n", rw_data);
> -	}
>  	DPRINTK(4, " clear Interrupt : INT_PENDING=%02X\n", rw_data);
>
>  	/* enable ks7010sdio interrupt (INT_GCR_B|INT_READ_STATUS|INT_WRITE_STATUS) */
> @@ -1067,9 +1052,8 @@ static int ks7010_sdio_probe(struct sdio_func *func,
>  	sdio_claim_host(func);
>  	ret = ks7010_sdio_write(priv, INT_ENABLE, &rw_data, sizeof(rw_data));
>  	sdio_release_host(func);
> -	if (ret) {
> +	if (ret)
>  		DPRINTK(1, " error : INT_ENABLE=%02X\n", rw_data);
> -	}
>  	DPRINTK(4, " enable Interrupt : INT_ENABLE=%02X\n", rw_data);
>  	priv->dev_state = DEVICE_STATE_BOOT;
>
> --
> 2.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[toc] | [prev] | [next] | [standalone]

#1447324 — Re: [PATCH 6/9] staging: ks7010: Delete unnecessary braces

[Multipart message — attachments visible in raw view] — view raw

On Sun, Jul 17, 2016 at 08:55:41PM +0200, Julia Lawall wrote:
> 
> 
> On Sun, 17 Jul 2016, SF Markus Elfring wrote:
> 
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Sun, 17 Jul 2016 18:39:03 +0200
> >
> > Do not use curly brackets at some source code places
> > where a single statement should be sufficient.
> >
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > ---
> >  drivers/staging/ks7010/ks7010_sdio.c | 48 ++++++++++++------------------------
> >  1 file changed, 16 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/staging/ks7010/ks7010_sdio.c b/drivers/staging/ks7010/ks7010_sdio.c
> > index b3ca8e2..1e072e3 100644
> > --- a/drivers/staging/ks7010/ks7010_sdio.c
> > +++ b/drivers/staging/ks7010/ks7010_sdio.c
> > @@ -109,9 +109,8 @@ void ks_wlan_hw_sleep_doze_request(struct ks_wlan_private *priv)
> >  		DPRINTK(3, "sleep_mode=SLP_SLEEP\n");
> >  		atomic_set(&priv->sleepstatus.status, 1);
> >  		priv->last_doze = jiffies;
> > -	} else {
> > +	} else
> >  		DPRINTK(1, "sleep_mode=%d\n", priv->sleep_mode);
> > -	}
> 
> This is a violation of CodingStyle:

Julia is right again, I agree.

[toc] | [prev] | [next] | [standalone]

#1445079 — [PATCH 9/9] staging: ks7010: Delete three unnecessary variable initialisations

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 17 Jul 2016 19:40:47 +0200

Three variables will be set to an appropriate value a bit later.
Thus omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/ks7010/ks7010_sdio.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.c b/drivers/staging/ks7010/ks7010_sdio.c
index e06bc3c..3c62c57 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -323,14 +323,14 @@ static void tx_device_task(void *dev)
 {
 	struct ks_wlan_private *priv = (struct ks_wlan_private *)dev;
 	struct tx_device_buffer *sp;
-	int rc = 0;
 
 	DPRINTK(4, "\n");
 	if (cnt_txqbody(priv) > 0
 	    && atomic_read(&priv->psstatus.status) != PS_SNOOZE) {
 		sp = &priv->tx_dev.tx_dev_buff[priv->tx_dev.qhead];
 		if (priv->dev_state >= DEVICE_STATE_BOOT) {
-			rc = write_to_device(priv, sp->sendp, sp->size);
+			int rc = write_to_device(priv, sp->sendp, sp->size);
+
 			if (rc) {
 				DPRINTK(1, "write_to_device error !!(%d)\n",
 					rc);
@@ -358,7 +358,7 @@ int ks_wlan_hw_tx(struct ks_wlan_private *priv, void *p, unsigned long size,
 		  void (*complete_handler) (void *arg1, void *arg2),
 		  void *arg1, void *arg2)
 {
-	int result = 0;
+	int result;
 	struct hostif_hdr *hdr;
 	hdr = (struct hostif_hdr *)p;
 
@@ -737,7 +737,7 @@ free_buf:
 static int ks7010_upload_firmware(struct ks_wlan_private *priv,
 				  struct ks_sdio_card *card)
 {
-	unsigned int size, offset, n = 0;
+	unsigned int size, offset, n;
 	unsigned char *rom_buf;
 	unsigned char rw_data = 0;
 	int retval, rc = 0;
-- 
2.9.1

[toc] | [prev] | [next] | [standalone]

#1445084 — Re: [PATCH 9/9] staging: ks7010: Delete three unnecessary variable initialisations

On Sun, 17 Jul 2016, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 17 Jul 2016 19:40:47 +0200
>
> Three variables will be set to an appropriate value a bit later.
> Thus omit the explicit initialisation at the beginning.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/staging/ks7010/ks7010_sdio.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/ks7010/ks7010_sdio.c b/drivers/staging/ks7010/ks7010_sdio.c
> index e06bc3c..3c62c57 100644
> --- a/drivers/staging/ks7010/ks7010_sdio.c
> +++ b/drivers/staging/ks7010/ks7010_sdio.c
> @@ -323,14 +323,14 @@ static void tx_device_task(void *dev)
>  {
>  	struct ks_wlan_private *priv = (struct ks_wlan_private *)dev;
>  	struct tx_device_buffer *sp;
> -	int rc = 0;
>
>  	DPRINTK(4, "\n");
>  	if (cnt_txqbody(priv) > 0
>  	    && atomic_read(&priv->psstatus.status) != PS_SNOOZE) {
>  		sp = &priv->tx_dev.tx_dev_buff[priv->tx_dev.qhead];
>  		if (priv->dev_state >= DEVICE_STATE_BOOT) {
> -			rc = write_to_device(priv, sp->sendp, sp->size);
> +			int rc = write_to_device(priv, sp->sendp, sp->size);

This does not look appealing to me, neither the declaration in the middle
of the function, nor the intiialization to the result of a complex
expression, nor the separation of the call and the error checking code by
a blank line.  There is nothing wrong with having the rc variable be
declared at the the top of the function, in its normal place.

julia

> +
>  			if (rc) {
>  				DPRINTK(1, "write_to_device error !!(%d)\n",
>  					rc);
> @@ -358,7 +358,7 @@ int ks_wlan_hw_tx(struct ks_wlan_private *priv, void *p, unsigned long size,
>  		  void (*complete_handler) (void *arg1, void *arg2),
>  		  void *arg1, void *arg2)
>  {
> -	int result = 0;
> +	int result;
>  	struct hostif_hdr *hdr;
>  	hdr = (struct hostif_hdr *)p;
>
> @@ -737,7 +737,7 @@ free_buf:
>  static int ks7010_upload_firmware(struct ks_wlan_private *priv,
>  				  struct ks_sdio_card *card)
>  {
> -	unsigned int size, offset, n = 0;
> +	unsigned int size, offset, n;
>  	unsigned char *rom_buf;
>  	unsigned char rw_data = 0;
>  	int retval, rc = 0;
> --
> 2.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[toc] | [prev] | [next] | [standalone]

#1447319 — Re: [PATCH 9/9] staging: ks7010: Delete three unnecessary variable initialisations

[Multipart message — attachments visible in raw view] — view raw

On Sun, Jul 17, 2016 at 08:51:39PM +0200, Julia Lawall wrote:
> 
> 
> On Sun, 17 Jul 2016, SF Markus Elfring wrote:
> 
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Sun, 17 Jul 2016 19:40:47 +0200
> >
> > Three variables will be set to an appropriate value a bit later.
> > Thus omit the explicit initialisation at the beginning.
> >
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > ---
> >  drivers/staging/ks7010/ks7010_sdio.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/staging/ks7010/ks7010_sdio.c b/drivers/staging/ks7010/ks7010_sdio.c
> > index e06bc3c..3c62c57 100644
> > --- a/drivers/staging/ks7010/ks7010_sdio.c
> > +++ b/drivers/staging/ks7010/ks7010_sdio.c
> > @@ -323,14 +323,14 @@ static void tx_device_task(void *dev)
> >  {
> >  	struct ks_wlan_private *priv = (struct ks_wlan_private *)dev;
> >  	struct tx_device_buffer *sp;
> > -	int rc = 0;
> >
> >  	DPRINTK(4, "\n");
> >  	if (cnt_txqbody(priv) > 0
> >  	    && atomic_read(&priv->psstatus.status) != PS_SNOOZE) {
> >  		sp = &priv->tx_dev.tx_dev_buff[priv->tx_dev.qhead];
> >  		if (priv->dev_state >= DEVICE_STATE_BOOT) {
> > -			rc = write_to_device(priv, sp->sendp, sp->size);
> > +			int rc = write_to_device(priv, sp->sendp, sp->size);
> 
> This does not look appealing to me, neither the declaration in the middle
> of the function, nor the intiialization to the result of a complex
> expression, nor the separation of the call and the error checking code by
> a blank line.  There is nothing wrong with having the rc variable be
> declared at the the top of the function, in its normal place.

+1

[toc] | [prev] | [next] | [standalone]

#1447890 — Re: [PATCH 9/9] staging: ks7010: Delete three unnecessary variable initialisations

>> @@ -323,14 +323,14 @@ static void tx_device_task(void *dev)
>>  {
>>  	struct ks_wlan_private *priv = (struct ks_wlan_private *)dev;
>>  	struct tx_device_buffer *sp;
>> -	int rc = 0;
>>
>>  	DPRINTK(4, "\n");
>>  	if (cnt_txqbody(priv) > 0
>>  	    && atomic_read(&priv->psstatus.status) != PS_SNOOZE) {
>>  		sp = &priv->tx_dev.tx_dev_buff[priv->tx_dev.qhead];
>>  		if (priv->dev_state >= DEVICE_STATE_BOOT) {
>> -			rc = write_to_device(priv, sp->sendp, sp->size);
>> +			int rc = write_to_device(priv, sp->sendp, sp->size);
> 
> This does not look appealing to me, neither the declaration in the middle
> of the function, nor the intiialization to the result of a complex
> expression, nor the separation of the call and the error checking code by
> a blank line.  There is nothing wrong with having the rc variable be
> declared at the the top of the function, in its normal place.

* Do you occasionally care for a refactoring like "Reduce scope of variable"?

  http://refactoring.com/catalog/reduceScopeOfVariable.html

* How do you think about to remove the extra assignment at the beginning
  of this function implementation?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1447896 — Re: [PATCH 9/9] staging: ks7010: Delete three unnecessary variable initialisations

On Thu, 21 Jul 2016, SF Markus Elfring wrote:

> >> @@ -323,14 +323,14 @@ static void tx_device_task(void *dev)
> >>  {
> >>  	struct ks_wlan_private *priv = (struct ks_wlan_private *)dev;
> >>  	struct tx_device_buffer *sp;
> >> -	int rc = 0;
> >>
> >>  	DPRINTK(4, "\n");
> >>  	if (cnt_txqbody(priv) > 0
> >>  	    && atomic_read(&priv->psstatus.status) != PS_SNOOZE) {
> >>  		sp = &priv->tx_dev.tx_dev_buff[priv->tx_dev.qhead];
> >>  		if (priv->dev_state >= DEVICE_STATE_BOOT) {
> >> -			rc = write_to_device(priv, sp->sendp, sp->size);
> >> +			int rc = write_to_device(priv, sp->sendp, sp->size);
> >
> > This does not look appealing to me, neither the declaration in the middle
> > of the function, nor the intiialization to the result of a complex
> > expression, nor the separation of the call and the error checking code by
> > a blank line.  There is nothing wrong with having the rc variable be
> > declared at the the top of the function, in its normal place.
>
> * Do you occasionally care for a refactoring like "Reduce scope of variable"?
>
>   http://refactoring.com/catalog/reduceScopeOfVariable.html

Probably not.  Certainly not in this case.

> * How do you think about to remove the extra assignment at the beginning
>   of this function implementation?

If the value is not useful, then it can go.

julia

> Regards,
> Markus
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[toc] | [prev] | [next] | [standalone]

#1447903 — Re: staging: ks7010: Delete three unnecessary variable initialisations

>> * Do you occasionally care for a refactoring like "Reduce scope of variable"?
>>
>>   http://refactoring.com/catalog/reduceScopeOfVariable.html
> 
> Probably not.  Certainly not in this case.

In which use cases would the suggested change pattern be more interesting
for you?

Regards,
Markus

[toc] | [prev] | [standalone]

Page 3 of 3 — ← Prev page 1 2 [3]

Back to top | Article view | linux.kernel

csiph-web