[OPW,kernel,3/3] Staging: dgnc: Remove multiple variable assignments in one statement
diff mbox

Message ID 20131012151110.GA10210@gmail.com
State Rejected
Headers show

Commit Message

Agnieszka Tabaka Oct. 12, 2013, 3:11 p.m. UTC
Signed-off-by: Agnieszka Tabaka <nufcia@gmail.com>
---
 drivers/staging/dgnc/dgnc_cls.c | 15 ++++++++++-----
 drivers/staging/dgnc/dgnc_neo.c | 15 ++++++++++-----
 drivers/staging/dgnc/dgnc_tty.c |  9 ++++++---
 3 files changed, 26 insertions(+), 13 deletions(-)

Comments

Greg Kroah-Hartman Oct. 14, 2013, 4:09 p.m. UTC | #1
On Sat, Oct 12, 2013 at 05:11:10PM +0200, Agnieszka Tabaka wrote:
> Signed-off-by: Agnieszka Tabaka <nufcia@gmail.com>
> ---
>  drivers/staging/dgnc/dgnc_cls.c | 15 ++++++++++-----
>  drivers/staging/dgnc/dgnc_neo.c | 15 ++++++++++-----
>  drivers/staging/dgnc/dgnc_tty.c |  9 ++++++---
>  3 files changed, 26 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/staging/dgnc/dgnc_cls.c b/drivers/staging/dgnc/dgnc_cls.c
> index fa1bb63..6e19df9 100644
> --- a/drivers/staging/dgnc/dgnc_cls.c
> +++ b/drivers/staging/dgnc/dgnc_cls.c
> @@ -507,9 +507,12 @@ static void cls_param(struct tty_struct *tty)
>  	 * If baud rate is zero, flush queues, and set mval to drop DTR.
>  	 */
>  	if ((ch->ch_c_cflag & (CBAUD)) == 0) {
> -		ch->ch_r_head = ch->ch_r_tail = 0;
> -		ch->ch_e_head = ch->ch_e_tail = 0;
> -		ch->ch_w_head = ch->ch_w_tail = 0;
> +		ch->ch_r_head = 0;
> +		ch->ch_r_tail = 0;
> +		ch->ch_e_head = 0;
> +		ch->ch_e_tail = 0;
> +		ch->ch_w_head = 0;
> +		ch->ch_w_tail = 0;

Why make this change?  What is wrong with the existing code?  I don't
see checkpatch complaining about it, or am I missing something?

thanks,

greg k-h
Josh Triplett Oct. 14, 2013, 5:08 p.m. UTC | #2
On Mon, Oct 14, 2013 at 09:09:04AM -0700, Greg KH wrote:
> On Sat, Oct 12, 2013 at 05:11:10PM +0200, Agnieszka Tabaka wrote:
> > Signed-off-by: Agnieszka Tabaka <nufcia@gmail.com>
> > ---
> >  drivers/staging/dgnc/dgnc_cls.c | 15 ++++++++++-----
> >  drivers/staging/dgnc/dgnc_neo.c | 15 ++++++++++-----
> >  drivers/staging/dgnc/dgnc_tty.c |  9 ++++++---
> >  3 files changed, 26 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/staging/dgnc/dgnc_cls.c b/drivers/staging/dgnc/dgnc_cls.c
> > index fa1bb63..6e19df9 100644
> > --- a/drivers/staging/dgnc/dgnc_cls.c
> > +++ b/drivers/staging/dgnc/dgnc_cls.c
> > @@ -507,9 +507,12 @@ static void cls_param(struct tty_struct *tty)
> >  	 * If baud rate is zero, flush queues, and set mval to drop DTR.
> >  	 */
> >  	if ((ch->ch_c_cflag & (CBAUD)) == 0) {
> > -		ch->ch_r_head = ch->ch_r_tail = 0;
> > -		ch->ch_e_head = ch->ch_e_tail = 0;
> > -		ch->ch_w_head = ch->ch_w_tail = 0;
> > +		ch->ch_r_head = 0;
> > +		ch->ch_r_tail = 0;
> > +		ch->ch_e_head = 0;
> > +		ch->ch_e_tail = 0;
> > +		ch->ch_w_head = 0;
> > +		ch->ch_w_tail = 0;
> 
> Why make this change?  What is wrong with the existing code?  I don't
> see checkpatch complaining about it, or am I missing something?

~/src/linux/Documentation$ grep -A1 assign CodingStyle
Don't put multiple assignments on a single line either.  Kernel coding style
is super simple.  Avoid tricky expressions.

Seems like a perfectly reasonable cleanup to me.

- Josh Triplett
Greg Kroah-Hartman Oct. 14, 2013, 5:22 p.m. UTC | #3
On Mon, Oct 14, 2013 at 10:08:25AM -0700, Josh Triplett wrote:
> On Mon, Oct 14, 2013 at 09:09:04AM -0700, Greg KH wrote:
> > On Sat, Oct 12, 2013 at 05:11:10PM +0200, Agnieszka Tabaka wrote:
> > > Signed-off-by: Agnieszka Tabaka <nufcia@gmail.com>
> > > ---
> > >  drivers/staging/dgnc/dgnc_cls.c | 15 ++++++++++-----
> > >  drivers/staging/dgnc/dgnc_neo.c | 15 ++++++++++-----
> > >  drivers/staging/dgnc/dgnc_tty.c |  9 ++++++---
> > >  3 files changed, 26 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/staging/dgnc/dgnc_cls.c b/drivers/staging/dgnc/dgnc_cls.c
> > > index fa1bb63..6e19df9 100644
> > > --- a/drivers/staging/dgnc/dgnc_cls.c
> > > +++ b/drivers/staging/dgnc/dgnc_cls.c
> > > @@ -507,9 +507,12 @@ static void cls_param(struct tty_struct *tty)
> > >  	 * If baud rate is zero, flush queues, and set mval to drop DTR.
> > >  	 */
> > >  	if ((ch->ch_c_cflag & (CBAUD)) == 0) {
> > > -		ch->ch_r_head = ch->ch_r_tail = 0;
> > > -		ch->ch_e_head = ch->ch_e_tail = 0;
> > > -		ch->ch_w_head = ch->ch_w_tail = 0;
> > > +		ch->ch_r_head = 0;
> > > +		ch->ch_r_tail = 0;
> > > +		ch->ch_e_head = 0;
> > > +		ch->ch_e_tail = 0;
> > > +		ch->ch_w_head = 0;
> > > +		ch->ch_w_tail = 0;
> > 
> > Why make this change?  What is wrong with the existing code?  I don't
> > see checkpatch complaining about it, or am I missing something?
> 
> ~/src/linux/Documentation$ grep -A1 assign CodingStyle
> Don't put multiple assignments on a single line either.  Kernel coding style
> is super simple.  Avoid tricky expressions.

That's not "tricky" here, it's pretty obvious as to what is going on.

Given all of the "real" issues in this file, I'd prefer fixing up those
first before any of these "trivial" issues are addressed.

thanks,

greg k-h

Patch
diff mbox

diff --git a/drivers/staging/dgnc/dgnc_cls.c b/drivers/staging/dgnc/dgnc_cls.c
index fa1bb63..6e19df9 100644
--- a/drivers/staging/dgnc/dgnc_cls.c
+++ b/drivers/staging/dgnc/dgnc_cls.c
@@ -507,9 +507,12 @@  static void cls_param(struct tty_struct *tty)
 	 * If baud rate is zero, flush queues, and set mval to drop DTR.
 	 */
 	if ((ch->ch_c_cflag & (CBAUD)) == 0) {
-		ch->ch_r_head = ch->ch_r_tail = 0;
-		ch->ch_e_head = ch->ch_e_tail = 0;
-		ch->ch_w_head = ch->ch_w_tail = 0;
+		ch->ch_r_head = 0;
+		ch->ch_r_tail = 0;
+		ch->ch_e_head = 0;
+		ch->ch_e_tail = 0;
+		ch->ch_w_head = 0;
+		ch->ch_w_tail = 0;
 
 		cls_flush_uart_write(ch);
                 cls_flush_uart_read(ch);
@@ -640,7 +643,8 @@  static void cls_param(struct tty_struct *tty)
 		break;
 	}
 
-	ier = uart_ier = readb(&ch->ch_cls_uart->ier);
+	ier = readb(&ch->ch_cls_uart->ier);
+	uart_ier = ier;
 	uart_lcr = readb(&ch->ch_cls_uart->lcr);
 
 	if (baud == 0)
@@ -929,7 +933,8 @@  static void cls_copy_data_from_uart_to_queue(struct channel_t *ch)
 			DPR_READ(("Queue full, dropping DATA:%x LSR:%x\n",
 				ch->ch_rqueue[tail], ch->ch_equeue[tail]));
 
-			ch->ch_r_tail = tail = (tail + 1) & RQUEUEMASK;
+			tail = (tail + 1) & RQUEUEMASK;
+			ch->ch_r_tail = tail;
 			ch->ch_err_overrun++;
 			qleft++;
 		}
diff --git a/drivers/staging/dgnc/dgnc_neo.c b/drivers/staging/dgnc/dgnc_neo.c
index 0e2a5e1..7d5ea7e 100644
--- a/drivers/staging/dgnc/dgnc_neo.c
+++ b/drivers/staging/dgnc/dgnc_neo.c
@@ -680,9 +680,12 @@  static void neo_param(struct tty_struct *tty)
 	 * If baud rate is zero, flush queues, and set mval to drop DTR.
 	 */
 	if ((ch->ch_c_cflag & (CBAUD)) == 0) {
-		ch->ch_r_head = ch->ch_r_tail = 0;
-		ch->ch_e_head = ch->ch_e_tail = 0;
-		ch->ch_w_head = ch->ch_w_tail = 0;
+		ch->ch_r_head = 0;
+		ch->ch_r_tail = 0;
+		ch->ch_e_head = 0;
+		ch->ch_e_tail = 0;
+		ch->ch_w_head = 0;
+		ch->ch_w_tail = 0;
 
 		neo_flush_uart_write(ch);
 		neo_flush_uart_read(ch);
@@ -813,7 +816,8 @@  static void neo_param(struct tty_struct *tty)
 		break;
 	}
 
-	ier = uart_ier = readb(&ch->ch_neo_uart->ier);
+	ier = readb(&ch->ch_neo_uart->ier);
+	uart_ier = ier;
 	uart_lcr = readb(&ch->ch_neo_uart->lcr);
 
 	if (baud == 0)
@@ -1361,7 +1365,8 @@  static void neo_copy_data_from_uart_to_queue(struct channel_t *ch)
 			DPR_READ(("Queue full, dropping DATA:%x LSR:%x\n",
 				ch->ch_rqueue[tail], ch->ch_equeue[tail]));
 
-			ch->ch_r_tail = tail = (tail + 1) & RQUEUEMASK;
+			tail = (tail + 1) & RQUEUEMASK;
+			ch->ch_r_tail = tail;
 			ch->ch_err_overrun++;
 			qleft++;
 		}
diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
index a6c6aba..57bf8d4 100644
--- a/drivers/staging/dgnc/dgnc_tty.c
+++ b/drivers/staging/dgnc/dgnc_tty.c
@@ -1363,9 +1363,12 @@  static int dgnc_tty_open(struct tty_struct *tty, struct file *file)
 		/*
 		 * Flush input queues.
 		 */
-		ch->ch_r_head = ch->ch_r_tail = 0;
-		ch->ch_e_head = ch->ch_e_tail = 0;
-		ch->ch_w_head = ch->ch_w_tail = 0;
+		ch->ch_r_head = 0;
+		ch->ch_r_tail = 0;
+		ch->ch_e_head = 0;
+		ch->ch_e_tail = 0;
+		ch->ch_w_head = 0;
+		ch->ch_w_tail = 0;
 
 		brd->bd_ops->flush_uart_write(ch);
 		brd->bd_ops->flush_uart_read(ch);