[Qemu-trivial] [PATCH v1 1/3] char/serial: cosmetic fixes.

[Qemu-trivial] [PATCH v1 1/3] char/serial: cosmetic fixes.

[peter.crosthwaite]

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);
+                }
                 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);
     }
 }
 
-- 
1.8.3.rc1.44.gb387c77.dirty

[Qemu-trivial] [PATCH v1 3/3] char/serial: serial_ioport_write: Factor out common code

[peter.crosthwaite]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

These three lines are common to both FIFO and regular mode. Just factor
them out to outside the if rather than replicate the same lines inside
both if and else.

Cc: qemu-trivial@nongnu.org

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/char/serial.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index 0a2b6c9..017610e 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -285,15 +285,11 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
                     fifo8_pop(&s->xmit_fifo);
                 }
                 fifo8_push(&s->xmit_fifo, s->thr);
-                s->thr_ipending = 0;
                 s->lsr &= ~UART_LSR_TEMT;
-                s->lsr &= ~UART_LSR_THRE;
-                serial_update_irq(s);
-            } else {
-                s->thr_ipending = 0;
-                s->lsr &= ~UART_LSR_THRE;
-                serial_update_irq(s);
             }
+            s->thr_ipending = 0;
+            s->lsr &= ~UART_LSR_THRE;
+            serial_update_irq(s);
             serial_xmit(NULL, G_IO_OUT, s);
         }
         break;
-- 
1.8.3.rc1.44.gb387c77.dirty

Re: [Qemu-trivial] [PATCH v1 0/3] Serial cleanup

[Peter Crosthwaite]

Ping!

Any objections to this one going in? perhaps even via trivial queue?

Regards,
Peter

On Mon, Jun 3, 2013 at 3:11 PM,  <peter.crosthwaite@xilinx.com> wrote:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
> Some cosmetics, refactored to use util/fifo8 for the FIFO8, then
> factored out some common code.
>
> Tested as working on petalogix-ml605 machine model + Linux (has
> coverage of serial fifo usage).
>
>
> Peter Crosthwaite (3):
>   char/serial: cosmetic fixes.
>   char/serial: Use generic Fifo8
>   char/serial: serial_ioport_write: Factor out common code
>
>  hw/char/serial.c         | 128 +++++++++++++++++++----------------------------
>  include/hw/char/serial.h |  15 ++----
>  2 files changed, 56 insertions(+), 87 deletions(-)
>
> --
> 1.8.3.rc1.44.gb387c77.dirty
>

Re: [Qemu-trivial] [Qemu-devel] [PATCH v1 0/3] Serial cleanup

[Andreas Färber]

Am 10.06.2013 12:23, schrieb Peter Crosthwaite:
> Ping!
> 
> Any objections to this one going in? perhaps even via trivial queue?

No strong objection, but you are using an unusual 12-char indentation in
some places that you may want to check.

Otherwise the cosmetic cleanup looks fine to me.

Cheers,
Andreas

> On Mon, Jun 3, 2013 at 3:11 PM,  <peter.crosthwaite@xilinx.com> wrote:
>> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>
>> Some cosmetics, refactored to use util/fifo8 for the FIFO8, then
>> factored out some common code.
>>
>> Tested as working on petalogix-ml605 machine model + Linux (has
>> coverage of serial fifo usage).
>>
>>
>> Peter Crosthwaite (3):
>>   char/serial: cosmetic fixes.
>>   char/serial: Use generic Fifo8
>>   char/serial: serial_ioport_write: Factor out common code
>>
>>  hw/char/serial.c         | 128 +++++++++++++++++++----------------------------
>>  include/hw/char/serial.h |  15 ++----
>>  2 files changed, 56 insertions(+), 87 deletions(-)
>>
>> --
>> 1.8.3.rc1.44.gb387c77.dirty
>>
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-trivial] [Qemu-devel] [PATCH v1 0/3] Serial cleanup

[Peter Crosthwaite]

Hi Andreas,

On Mon, Jun 10, 2013 at 9:49 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 10.06.2013 12:23, schrieb Peter Crosthwaite:
>> Ping!
>>
>> Any objections to this one going in? perhaps even via trivial queue?
>
> No strong objection, but you are using an unusual 12-char indentation in
> some places that you may want to check.
>

I use that indentation when a ? : operator continues to the next line.
What indentation scheme should be used in this instance?

Regards,
Peter

> Otherwise the cosmetic cleanup looks fine to me.
>
> Cheers,
> Andreas
>
>> On Mon, Jun 3, 2013 at 3:11 PM,  <peter.crosthwaite@xilinx.com> wrote:
>>> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>>
>>> Some cosmetics, refactored to use util/fifo8 for the FIFO8, then
>>> factored out some common code.
>>>
>>> Tested as working on petalogix-ml605 machine model + Linux (has
>>> coverage of serial fifo usage).
>>>
>>>
>>> Peter Crosthwaite (3):
>>>   char/serial: cosmetic fixes.
>>>   char/serial: Use generic Fifo8
>>>   char/serial: serial_ioport_write: Factor out common code
>>>
>>>  hw/char/serial.c         | 128 +++++++++++++++++++----------------------------
>>>  include/hw/char/serial.h |  15 ++----
>>>  2 files changed, 56 insertions(+), 87 deletions(-)
>>>
>>> --
>>> 1.8.3.rc1.44.gb387c77.dirty
>>>
>>
>
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>

Re: [Qemu-trivial] [Qemu-devel] [PATCH v1 1/3] char/serial: cosmetic fixes.

[Andreas Färber]

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);
>      }
>  }
>  
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-trivial] [Qemu-devel] [PATCH v1 3/3] char/serial: serial_ioport_write: Factor out common code

[Andreas Färber]

Am 03.06.2013 07:14, schrieb peter.crosthwaite@xilinx.com:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> 
> These three lines are common to both FIFO and regular mode. Just factor
> them out to outside the if rather than replicate the same lines inside
> both if and else.
> 
> Cc: qemu-trivial@nongnu.org
> 
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Reviewed-by: Andreas Färber <afaerber@suse.de>

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-trivial] [PATCH v1 1/3] char/serial: cosmetic fixes.

[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.

Thank you!

/mjt

Re: [Qemu-trivial] [PATCH v1 1/3] char/serial: cosmetic fixes.

[Michael Tokarev]

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

Re: [Qemu-trivial] [Qemu-devel] [PATCH v1 1/3] char/serial: cosmetic fixes.

[Peter Crosthwaite]

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
>
>

Re: [Qemu-trivial] [PATCH v1 0/3] Serial cleanup

[Michael Tokarev]

10.06.2013 14:23, Peter Crosthwaite wrote:
> Ping!
> 
> Any objections to this one going in? perhaps even via trivial queue?

Actually applied all 3 (including 2/3 which weren't submitted to -trivial --
I had some fun verifying it :) to the trivial-patches queue.

Thanks!

/mjt