Re: [Qemu-devel] [PATCH v12 03/19] i.MX:Fix Coding style for UART emulator.

[Peter Crosthwaite]

On Fri, Jul 10, 2015 at 4:31 PM, Jean-Christophe Dubois
<address@hidden> wrote:
> Signed-off-by: Jean-Christophe Dubois <address@hidden>

Reviewed-by: Peter Crosthwaite <address@hidden>

One minor optional nit below.

> ---
>
> Changes since v1:
>     * not present on v1
>
> Changes since v2:
>     * not present on v2
>
> Changes since v3:
>     * not present on v3
>
> Changes since v4:
>     * not present on v4
>
> Changes since v5:
>     * not present on v5
>
> Changes since v6:
>     * not present on v6
>
> Changes since v7:
>     * not preset on v7
>
> Changes since v8:
>     * Fix coding style
>
> Changes since v9:
>     * no change
>
> Changes since v10:
>     * no change
>
> Changes since v11:
>     * no change
>
>  hw/char/imx_serial.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/hw/char/imx_serial.c b/hw/char/imx_serial.c
> index 180bc45..f9da59f 100644
> --- a/hw/char/imx_serial.c
> +++ b/hw/char/imx_serial.c
> @@ -26,7 +26,7 @@
>  //#define DEBUG_SERIAL 1
>  #ifdef DEBUG_SERIAL
>  #define DPRINTF(fmt, args...) \
> -do { printf("imx_serial: " fmt , ##args); } while (0)
> +do { printf("%s: " fmt , TYPE_IMX_SERIAL, ##args); } while (0)
>  #else
>  #define DPRINTF(fmt, args...) do {} while (0)
>  #endif
> @@ -164,13 +164,13 @@ static uint64_t imx_serial_read(void *opaque, hwaddr 
> offset,
>          return 0x0; /* TODO */
>
>      default:
> -        IPRINTF("imx_serial_read: bad offset: 0x%x\n", (int)offset);
> +        IPRINTF("%s: bad offset: 0x%x\n", __func__, (int)offset);
>          return 0;
>      }
>  }
>
>  static void imx_serial_write(void *opaque, hwaddr offset,
> -                      uint64_t value, unsigned size)
> +                             uint64_t value, unsigned size)
>  {
>      IMXSerialState *s = (IMXSerialState *)opaque;
>      unsigned char ch;
> @@ -220,25 +220,25 @@ static void imx_serial_write(void *opaque, hwaddr 
> offset,
>
>      case 0x25: /* USR1 */
>          value &= USR1_AWAKE | USR1_AIRINT | USR1_DTRD | USR1_AGTIM |
> -            USR1_FRAMERR | USR1_ESCF | USR1_RTSD | USR1_PARTYER;
> +                 USR1_FRAMERR | USR1_ESCF | USR1_RTSD | USR1_PARTYER;
>          s->usr1 &= ~value;
>          break;
>
>      case 0x26: /* USR2 */
> -       /*
> -        * Writing 1 to some bits clears them; all other
> -        * values are ignored
> -        */
> +        /*
> +         * Writing 1 to some bits clears them; all other
> +         * values are ignored
> +         */
>          value &= USR2_ADET | USR2_DTRF | USR2_IDLE | USR2_ACST |
> -            USR2_RIDELT | USR2_IRINT | USR2_WAKE |
> -            USR2_DCDDELT | USR2_RTSF | USR2_BRCD | USR2_ORE;
> +                 USR2_RIDELT | USR2_IRINT | USR2_WAKE |
> +                 USR2_DCDDELT | USR2_RTSF | USR2_BRCD | USR2_ORE;
>          s->usr2 &= ~value;
>          break;
>
> -        /*
> -         * Linux expects to see what it writes to these registers
> -         * We don't currently alter the baud rate
> -         */
> +    /*
> +     * Linux expects to see what it writes to these registers
> +     * We don't currently alter the baud rate
> +     */

This change is correct, in that comment goes with the case so needs a
de-indent for current position ...

>      case 0x29: /* UBIR */

... but re-locating the comment here will be more consistent with
surrounding code and is a more adopted style.

Regards,
Peter

>          s->ubrc = value & 0xffff;
>          break;
> @@ -266,7 +266,7 @@ static void imx_serial_write(void *opaque, hwaddr offset,
>          break;
>
>      default:
> -        IPRINTF("imx_serial_write: Bad offset 0x%x\n", (int)offset);
> +        IPRINTF("%s: Bad offset 0x%x\n", __func__, (int)offset);
>      }
>  }
>
> --
> 2.1.4
>
>