Patchwork [v1,1/3] char/serial: cosmetic fixes.

login
register
mail settings
Submitter Peter Crosthwaite
Date June 3, 2013, 5:12 a.m.
Message ID <b61bfd22e26fd08d9cca73e875c6074a9664c49c.1370236013.git.peter.crosthwaite@xilinx.com>
Download mbox | patch
Permalink /patch/248172/
State New
Headers show

Comments

Peter Crosthwaite - June 3, 2013, 5:12 a.m.
From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Some cosmetic fixes to char/serial fixing some checkpatch errors.

Cc: qemu-trivial@nongnu.org

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
Needed for the next patch to pass checkpatch. Done as sep patch to not
obscure that patch.

 hw/char/serial.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)
Andreas Färber - June 10, 2013, 1:32 p.m.
Hi Peter,

Am 03.06.2013 07:12, schrieb peter.crosthwaite@xilinx.com:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> 
> Some cosmetic fixes to char/serial fixing some checkpatch errors.
> 
> Cc: qemu-trivial@nongnu.org
> 
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> Needed for the next patch to pass checkpatch. Done as sep patch to not
> obscure that patch.
> 
>  hw/char/serial.c | 30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index 66b6348..bd6813e 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -263,8 +263,9 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
>      if (s->tsr_retry <= 0) {
>          if (s->fcr & UART_FCR_FE) {
>              s->tsr = fifo_get(s,XMIT_FIFO);
> -            if (!s->xmit_fifo.count)
> +            if (!s->xmit_fifo.count) {
>                  s->lsr |= UART_LSR_THRE;
> +            }
>          } else if ((s->lsr & UART_LSR_THRE)) {
>              return FALSE;
>          } else {
> @@ -461,10 +462,11 @@ static uint64_t serial_ioport_read(void *opaque, hwaddr addr, unsigned size)
>          } else {
>              if(s->fcr & UART_FCR_FE) {
>                  ret = fifo_get(s,RECV_FIFO);
> -                if (s->recv_fifo.count == 0)
> +                if (s->recv_fifo.count == 0) {
>                      s->lsr &= ~(UART_LSR_DR | UART_LSR_BI);
> -                else
> +                } else {
>                      qemu_mod_timer(s->fifo_timeout_timer, qemu_get_clock_ns (vm_clock) + s->char_transmit_time * 4);

Wanna rebreak this one too in case you respin/pull?

> +                }
>                  s->timeout_ipending = 0;
>              } else {
>                  ret = s->rbr;
> @@ -534,15 +536,21 @@ static uint64_t serial_ioport_read(void *opaque, hwaddr addr, unsigned size)
>  static int serial_can_receive(SerialState *s)
>  {
>      if(s->fcr & UART_FCR_FE) {
> -        if(s->recv_fifo.count < UART_FIFO_LENGTH)
> -        /* Advertise (fifo.itl - fifo.count) bytes when count < ITL, and 1 if above. If UART_FIFO_LENGTH - fifo.count is
> -        advertised the effect will be to almost always fill the fifo completely before the guest has a chance to respond,
> -        effectively overriding the ITL that the guest has set. */
> -             return (s->recv_fifo.count <= s->recv_fifo.itl) ? s->recv_fifo.itl - s->recv_fifo.count : 1;
> -        else
> -             return 0;
> +        if (s->recv_fifo.count < UART_FIFO_LENGTH) {
> +            /*
> +             * Advertise (fifo.itl - fifo.count) bytes when count < ITL, and 1
> +             * if above. If UART_FIFO_LENGTH - fifo.count is advertised the
> +             * effect will be to almost always fill the fifo completely before
> +             * the guest has a chance to respond, effectively overriding the ITL
> +             * that the guest has set.
> +             */
> +            return (s->recv_fifo.count <= s->recv_fifo.itl) ?
> +                        s->recv_fifo.itl - s->recv_fifo.count : 1;

Here I stumbled over the indentation being 5 chars from '(' or 4 chars
within, but the latter doesn't make sense since it's terminated before.
I would've expected 4 chars from block or aligned below '(' or
4-char-indented from there. But I'm not sure if there are any clear
recommendations, so since it's apparently not using tabs (my initial
suspicion), no objection.

Cheers,
Andreas

> +        } else {
> +            return 0;
> +        }
>      } else {
> -    return !(s->lsr & UART_LSR_DR);
> +        return !(s->lsr & UART_LSR_DR);
>      }
>  }
>  
>
Michael Tokarev - June 10, 2013, 3:17 p.m.
03.06.2013 09:12, peter.crosthwaite@xilinx.com wrote:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> 
> Some cosmetic fixes to char/serial fixing some checkpatch errors.

These are all cosmetic fixes indeed.  I weren't sure we're
applying stylistic changes by its own.  But apparently
people don't object so I don't see why not, either, except
that I don't really want to open a can of worms with other
stylistic patches to come ;)

Applied all 3 to the trivial patches queue.

Thank you!

/mjt
Michael Tokarev - June 10, 2013, 3:29 p.m.
10.06.2013 19:17, Michael Tokarev пишет:
> 03.06.2013 09:12, peter.crosthwaite@xilinx.com wrote:
>> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>
>> Some cosmetic fixes to char/serial fixing some checkpatch errors.
> 
> These are all cosmetic fixes indeed.  I weren't sure we're
> applying stylistic changes by its own.  But apparently
> people don't object so I don't see why not, either, except
> that I don't really want to open a can of worms with other
> stylistic patches to come ;)
> 
> Applied all 3 to the trivial patches queue.

Actually I didn't apply these.  Since patch 2/3 weren't sent to
-trivial, and 3/3 does not apply without it, I'm not sure I want
to apply either of these.  Maybe all 3 should be sent to the same
tree?  Should I pick the 2/3 too, which wasn't submitted to
-trivial?

Thank you!

/mjt
Peter Crosthwaite - June 10, 2013, 10:24 p.m.
Hi Michael,

On Tue, Jun 11, 2013 at 1:29 AM, Michael Tokarev <mjt@tls.msk.ru> wrote:
> 10.06.2013 19:17, Michael Tokarev пишет:
>> 03.06.2013 09:12, peter.crosthwaite@xilinx.com wrote:
>>> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>>
>>> Some cosmetic fixes to char/serial fixing some checkpatch errors.
>>
>> These are all cosmetic fixes indeed.  I weren't sure we're
>> applying stylistic changes by its own.  But apparently
>> people don't object so I don't see why not, either, except
>> that I don't really want to open a can of worms with other
>> stylistic patches to come ;)
>>
>> Applied all 3 to the trivial patches queue.
>
> Actually I didn't apply these.  Since patch 2/3 weren't sent to
> -trivial, and 3/3 does not apply without it, I'm not sure I want
> to apply either of these.  Maybe all 3 should be sent to the same
> tree?  Should I pick the 2/3 too, which wasn't submitted to
> -trivial?
>

Im happy for you to take all three as they have had a week of list
time and 2/3 is cleanup only with no functional change. It's
borderline as to whether its trivial.

Regards,
Peter

> Thank you!
>
> /mjt
>
>

Patch

diff --git a/hw/char/serial.c b/hw/char/serial.c
index 66b6348..bd6813e 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -263,8 +263,9 @@  static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
     if (s->tsr_retry <= 0) {
         if (s->fcr & UART_FCR_FE) {
             s->tsr = fifo_get(s,XMIT_FIFO);
-            if (!s->xmit_fifo.count)
+            if (!s->xmit_fifo.count) {
                 s->lsr |= UART_LSR_THRE;
+            }
         } else if ((s->lsr & UART_LSR_THRE)) {
             return FALSE;
         } else {
@@ -461,10 +462,11 @@  static uint64_t serial_ioport_read(void *opaque, hwaddr addr, unsigned size)
         } else {
             if(s->fcr & UART_FCR_FE) {
                 ret = fifo_get(s,RECV_FIFO);
-                if (s->recv_fifo.count == 0)
+                if (s->recv_fifo.count == 0) {
                     s->lsr &= ~(UART_LSR_DR | UART_LSR_BI);
-                else
+                } else {
                     qemu_mod_timer(s->fifo_timeout_timer, qemu_get_clock_ns (vm_clock) + s->char_transmit_time * 4);
+                }
                 s->timeout_ipending = 0;
             } else {
                 ret = s->rbr;
@@ -534,15 +536,21 @@  static uint64_t serial_ioport_read(void *opaque, hwaddr addr, unsigned size)
 static int serial_can_receive(SerialState *s)
 {
     if(s->fcr & UART_FCR_FE) {
-        if(s->recv_fifo.count < UART_FIFO_LENGTH)
-        /* Advertise (fifo.itl - fifo.count) bytes when count < ITL, and 1 if above. If UART_FIFO_LENGTH - fifo.count is
-        advertised the effect will be to almost always fill the fifo completely before the guest has a chance to respond,
-        effectively overriding the ITL that the guest has set. */
-             return (s->recv_fifo.count <= s->recv_fifo.itl) ? s->recv_fifo.itl - s->recv_fifo.count : 1;
-        else
-             return 0;
+        if (s->recv_fifo.count < UART_FIFO_LENGTH) {
+            /*
+             * Advertise (fifo.itl - fifo.count) bytes when count < ITL, and 1
+             * if above. If UART_FIFO_LENGTH - fifo.count is advertised the
+             * effect will be to almost always fill the fifo completely before
+             * the guest has a chance to respond, effectively overriding the ITL
+             * that the guest has set.
+             */
+            return (s->recv_fifo.count <= s->recv_fifo.itl) ?
+                        s->recv_fifo.itl - s->recv_fifo.count : 1;
+        } else {
+            return 0;
+        }
     } else {
-    return !(s->lsr & UART_LSR_DR);
+        return !(s->lsr & UART_LSR_DR);
     }
 }