diff mbox

[target-arm,v4,10/16] char: cadence_uart: Clean up variable names

Message ID f865c90b0ca53fce8d7e2a8a1970dd2e30986966.1427108387.git.peter.crosthwaite@xilinx.com
State New
Headers show

Commit Message

Peter Crosthwaite March 23, 2015, 11:05 a.m. UTC 
In preparation for migrating the state struct and type cast macro to a public
header. The acronym "UART" on it's own is not specific enough to be used in a
more global namespace so preface with "cadence". Fix the capitalisation of
"uart" in the state type while touching the typename. Preface macros
used by the state struct itself with CADENCE_UART so they don't conflict
in namespace either.

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
 hw/char/cadence_uart.c | 102 ++++++++++++++++++++++++++-----------------------
 1 file changed, 54 insertions(+), 48 deletions(-)

Comments

Peter Maydell April 23, 2015, 5:59 p.m. UTC | #1 
On 23 March 2015 at 11:05, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> In preparation for migrating the state struct and type cast macro to a public
> header. The acronym "UART" on it's own is not specific enough to be used in a
> more global namespace so preface with "cadence". Fix the capitalisation of
> "uart" in the state type while touching the typename. Preface macros
> used by the state struct itself with CADENCE_UART so they don't conflict
> in namespace either.

This is another non-standalone commit message. Other than that:
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

> -    s->r[R_SR] |= s->rx_count == RX_FIFO_SIZE ? UART_SR_INTR_RFUL : 0;
> +    s->r[R_SR] |= s->rx_count == CADENCE_UART_RX_FIFO_SIZE ? UART_SR_INTR_RFUL
> +                                                           : 0;

These look kind of ugly as the line length got long enough to wrap.
    if (s->rx_count == CADENCE_UART_RX_FIFO_SIZE) {
        s->r[R_SR] |= UART_SR_INTR_RFUL;
    }
looks nicer to me (and makes it clearer that the bit only updates
if the condition is true, rather than leaving you to figure it out
from the combination of the OR and the ternary operator with a zero
operand), but that kind of change doesn't belong in this
no-content-change patch. You could do it in a later patch, or
not do it at all if you don't think it's important enough (I
don't care strongly either way).

-- PMM
Peter Crosthwaite April 24, 2015, 12:25 a.m. UTC | #2 
On Thu, Apr 23, 2015 at 10:59 AM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 23 March 2015 at 11:05, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> In preparation for migrating the state struct and type cast macro to a public
>> header. The acronym "UART" on it's own is not specific enough to be used in a
>> more global namespace so preface with "cadence". Fix the capitalisation of
>> "uart" in the state type while touching the typename. Preface macros
>> used by the state struct itself with CADENCE_UART so they don't conflict
>> in namespace either.
>
> This is another non-standalone commit message. Other than that:

Fixed.

> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Thanks.

>
>> -    s->r[R_SR] |= s->rx_count == RX_FIFO_SIZE ? UART_SR_INTR_RFUL : 0;
>> +    s->r[R_SR] |= s->rx_count == CADENCE_UART_RX_FIFO_SIZE ? UART_SR_INTR_RFUL
>> +                                                           : 0;
>
> These look kind of ugly as the line length got long enough to wrap.
>     if (s->rx_count == CADENCE_UART_RX_FIFO_SIZE) {
>         s->r[R_SR] |= UART_SR_INTR_RFUL;
>     }
> looks nicer to me (and makes it clearer that the bit only updates
> if the condition is true, rather than leaving you to figure it out
> from the combination of the OR and the ternary operator with a zero
> operand), but that kind of change doesn't belong in this
> no-content-change patch. You could do it in a later patch, or
> not do it at all if you don't think it's important enough (I
> don't care strongly either way).
>

Thinking we do this later to minimize scope of the series.

Regards,
Peter

> -- PMM
>
diff mbox

Patch

diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index a5dc2a4..efc6576 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -85,8 +85,8 @@ 
 #define LOCAL_LOOPBACK         (0x2 << UART_MR_CHMODE_SH)
 #define REMOTE_LOOPBACK        (0x3 << UART_MR_CHMODE_SH)
 
-#define RX_FIFO_SIZE           16
-#define TX_FIFO_SIZE           16
+#define CADENCE_UART_RX_FIFO_SIZE           16
+#define CADENCE_UART_TX_FIFO_SIZE           16
 #define UART_INPUT_CLK         50000000
 
 #define R_CR       (0x00/4)
@@ -108,10 +108,11 @@ 
 #define R_PWID     (0x40/4)
 #define R_TTRIG    (0x44/4)
 
-#define R_MAX (R_TTRIG + 1)
+#define CADENCE_UART_R_MAX (0x48/4)
 
 #define TYPE_CADENCE_UART "cadence_uart"
-#define CADENCE_UART(obj) OBJECT_CHECK(UartState, (obj), TYPE_CADENCE_UART)
+#define CADENCE_UART(obj) OBJECT_CHECK(CadenceUARTState, (obj), \
+                                       TYPE_CADENCE_UART)
 
 typedef struct {
     /*< private >*/
@@ -119,9 +120,9 @@  typedef struct {
     /*< public >*/
 
     MemoryRegion iomem;
-    uint32_t r[R_MAX];
-    uint8_t rx_fifo[RX_FIFO_SIZE];
-    uint8_t tx_fifo[TX_FIFO_SIZE];
+    uint32_t r[CADENCE_UART_R_MAX];
+    uint8_t rx_fifo[CADENCE_UART_RX_FIFO_SIZE];
+    uint8_t tx_fifo[CADENCE_UART_TX_FIFO_SIZE];
     uint32_t rx_wpos;
     uint32_t rx_count;
     uint32_t tx_count;
@@ -129,17 +130,19 @@  typedef struct {
     CharDriverState *chr;
     qemu_irq irq;
     QEMUTimer *fifo_trigger_handle;
-} UartState;
+} CadenceUARTState;
 
-static void uart_update_status(UartState *s)
+static void uart_update_status(CadenceUARTState *s)
 {
     s->r[R_SR] = 0;
 
-    s->r[R_SR] |= s->rx_count == RX_FIFO_SIZE ? UART_SR_INTR_RFUL : 0;
+    s->r[R_SR] |= s->rx_count == CADENCE_UART_RX_FIFO_SIZE ? UART_SR_INTR_RFUL
+                                                           : 0;
     s->r[R_SR] |= !s->rx_count ? UART_SR_INTR_REMPTY : 0;
     s->r[R_SR] |= s->rx_count >= s->r[R_RTRIG] ? UART_SR_INTR_RTRIG : 0;
 
-    s->r[R_SR] |= s->tx_count == TX_FIFO_SIZE ? UART_SR_INTR_TFUL : 0;
+    s->r[R_SR] |= s->tx_count == CADENCE_UART_TX_FIFO_SIZE ? UART_SR_INTR_TFUL
+                                                           : 0;
     s->r[R_SR] |= !s->tx_count ? UART_SR_INTR_TEMPTY : 0;
     s->r[R_SR] |= s->tx_count >= s->r[R_TTRIG] ? UART_SR_TTRIG : 0;
 
@@ -150,14 +153,14 @@  static void uart_update_status(UartState *s)
 
 static void fifo_trigger_update(void *opaque)
 {
-    UartState *s = (UartState *)opaque;
+    CadenceUARTState *s = opaque;
 
     s->r[R_CISR] |= UART_INTR_TIMEOUT;
 
     uart_update_status(s);
 }
 
-static void uart_rx_reset(UartState *s)
+static void uart_rx_reset(CadenceUARTState *s)
 {
     s->rx_wpos = 0;
     s->rx_count = 0;
@@ -166,12 +169,12 @@  static void uart_rx_reset(UartState *s)
     }
 }
 
-static void uart_tx_reset(UartState *s)
+static void uart_tx_reset(CadenceUARTState *s)
 {
     s->tx_count = 0;
 }
 
-static void uart_send_breaks(UartState *s)
+static void uart_send_breaks(CadenceUARTState *s)
 {
     int break_enabled = 1;
 
@@ -181,7 +184,7 @@  static void uart_send_breaks(UartState *s)
     }
 }
 
-static void uart_parameters_setup(UartState *s)
+static void uart_parameters_setup(CadenceUARTState *s)
 {
     QEMUSerialSetParams ssp;
     unsigned int baud_rate, packet_size;
@@ -236,20 +239,20 @@  static void uart_parameters_setup(UartState *s)
 
 static int uart_can_receive(void *opaque)
 {
-    UartState *s = (UartState *)opaque;
-    int ret = MAX(RX_FIFO_SIZE, TX_FIFO_SIZE);
+    CadenceUARTState *s = opaque;
+    int ret = MAX(CADENCE_UART_RX_FIFO_SIZE, CADENCE_UART_TX_FIFO_SIZE);
     uint32_t ch_mode = s->r[R_MR] & UART_MR_CHMODE;
 
     if (ch_mode == NORMAL_MODE || ch_mode == ECHO_MODE) {
-        ret = MIN(ret, RX_FIFO_SIZE - s->rx_count);
+        ret = MIN(ret, CADENCE_UART_RX_FIFO_SIZE - s->rx_count);
     }
     if (ch_mode == REMOTE_LOOPBACK || ch_mode == ECHO_MODE) {
-        ret = MIN(ret, TX_FIFO_SIZE - s->tx_count);
+        ret = MIN(ret, CADENCE_UART_TX_FIFO_SIZE - s->tx_count);
     }
     return ret;
 }
 
-static void uart_ctrl_update(UartState *s)
+static void uart_ctrl_update(CadenceUARTState *s)
 {
     if (s->r[R_CR] & UART_CR_TXRST) {
         uart_tx_reset(s);
@@ -268,7 +271,7 @@  static void uart_ctrl_update(UartState *s)
 
 static void uart_write_rx_fifo(void *opaque, const uint8_t *buf, int size)
 {
-    UartState *s = (UartState *)opaque;
+    CadenceUARTState *s = opaque;
     uint64_t new_rx_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
     int i;
 
@@ -276,12 +279,12 @@  static void uart_write_rx_fifo(void *opaque, const uint8_t *buf, int size)
         return;
     }
 
-    if (s->rx_count == RX_FIFO_SIZE) {
+    if (s->rx_count == CADENCE_UART_RX_FIFO_SIZE) {
         s->r[R_CISR] |= UART_INTR_ROVR;
     } else {
         for (i = 0; i < size; i++) {
             s->rx_fifo[s->rx_wpos] = buf[i];
-            s->rx_wpos = (s->rx_wpos + 1) % RX_FIFO_SIZE;
+            s->rx_wpos = (s->rx_wpos + 1) % CADENCE_UART_RX_FIFO_SIZE;
             s->rx_count++;
         }
         timer_mod(s->fifo_trigger_handle, new_rx_time +
@@ -293,7 +296,7 @@  static void uart_write_rx_fifo(void *opaque, const uint8_t *buf, int size)
 static gboolean cadence_uart_xmit(GIOChannel *chan, GIOCondition cond,
                                   void *opaque)
 {
-    UartState *s = opaque;
+    CadenceUARTState *s = opaque;
     int ret;
 
     /* instant drain the fifo when there's no back-end */
@@ -320,14 +323,15 @@  static gboolean cadence_uart_xmit(GIOChannel *chan, GIOCondition cond,
     return FALSE;
 }
 
-static void uart_write_tx_fifo(UartState *s, const uint8_t *buf, int size)
+static void uart_write_tx_fifo(CadenceUARTState *s, const uint8_t *buf,
+                               int size)
 {
     if ((s->r[R_CR] & UART_CR_TX_DIS) || !(s->r[R_CR] & UART_CR_TX_EN)) {
         return;
     }
 
-    if (size > TX_FIFO_SIZE - s->tx_count) {
-        size = TX_FIFO_SIZE - s->tx_count;
+    if (size > CADENCE_UART_TX_FIFO_SIZE - s->tx_count) {
+        size = CADENCE_UART_TX_FIFO_SIZE - s->tx_count;
         /*
          * This can only be a guest error via a bad tx fifo register push,
          * as can_receive() should stop remote loop and echo modes ever getting
@@ -345,7 +349,7 @@  static void uart_write_tx_fifo(UartState *s, const uint8_t *buf, int size)
 
 static void uart_receive(void *opaque, const uint8_t *buf, int size)
 {
-    UartState *s = (UartState *)opaque;
+    CadenceUARTState *s = opaque;
     uint32_t ch_mode = s->r[R_MR] & UART_MR_CHMODE;
 
     if (ch_mode == NORMAL_MODE || ch_mode == ECHO_MODE) {
@@ -358,7 +362,7 @@  static void uart_receive(void *opaque, const uint8_t *buf, int size)
 
 static void uart_event(void *opaque, int event)
 {
-    UartState *s = (UartState *)opaque;
+    CadenceUARTState *s = opaque;
     uint8_t buf = '\0';
 
     if (event == CHR_EVENT_BREAK) {
@@ -368,15 +372,15 @@  static void uart_event(void *opaque, int event)
     uart_update_status(s);
 }
 
-static void uart_read_rx_fifo(UartState *s, uint32_t *c)
+static void uart_read_rx_fifo(CadenceUARTState *s, uint32_t *c)
 {
     if ((s->r[R_CR] & UART_CR_RX_DIS) || !(s->r[R_CR] & UART_CR_RX_EN)) {
         return;
     }
 
     if (s->rx_count) {
-        uint32_t rx_rpos =
-                (RX_FIFO_SIZE + s->rx_wpos - s->rx_count) % RX_FIFO_SIZE;
+        uint32_t rx_rpos = (CADENCE_UART_RX_FIFO_SIZE + s->rx_wpos -
+                            s->rx_count) % CADENCE_UART_RX_FIFO_SIZE;
         *c = s->rx_fifo[rx_rpos];
         s->rx_count--;
 
@@ -393,7 +397,7 @@  static void uart_read_rx_fifo(UartState *s, uint32_t *c)
 static void uart_write(void *opaque, hwaddr offset,
                           uint64_t value, unsigned size)
 {
-    UartState *s = (UartState *)opaque;
+    CadenceUARTState *s = opaque;
 
     DB_PRINT(" offset:%x data:%08x\n", (unsigned)offset, (unsigned)value);
     offset >>= 2;
@@ -437,11 +441,11 @@  static void uart_write(void *opaque, hwaddr offset,
 static uint64_t uart_read(void *opaque, hwaddr offset,
         unsigned size)
 {
-    UartState *s = (UartState *)opaque;
+    CadenceUARTState *s = opaque;
     uint32_t c = 0;
 
     offset >>= 2;
-    if (offset >= R_MAX) {
+    if (offset >= CADENCE_UART_R_MAX) {
         c = 0;
     } else if (offset == R_TX_RX) {
         uart_read_rx_fifo(s, &c);
@@ -461,7 +465,7 @@  static const MemoryRegionOps uart_ops = {
 
 static void cadence_uart_reset(DeviceState *dev)
 {
-    UartState *s = CADENCE_UART(dev);
+    CadenceUARTState *s = CADENCE_UART(dev);
 
     s->r[R_CR] = 0x00000128;
     s->r[R_IMR] = 0;
@@ -478,7 +482,7 @@  static void cadence_uart_reset(DeviceState *dev)
 
 static void cadence_uart_realize(DeviceState *dev, Error **errp)
 {
-    UartState *s = CADENCE_UART(dev);
+    CadenceUARTState *s = CADENCE_UART(dev);
 
     s->fifo_trigger_handle = timer_new_ns(QEMU_CLOCK_VIRTUAL,
                                           fifo_trigger_update, s);
@@ -494,7 +498,7 @@  static void cadence_uart_realize(DeviceState *dev, Error **errp)
 static void cadence_uart_init(Object *obj)
 {
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
-    UartState *s = CADENCE_UART(obj);
+    CadenceUARTState *s = CADENCE_UART(obj);
 
     memory_region_init_io(&s->iomem, obj, &uart_ops, s, "uart", 0x1000);
     sysbus_init_mmio(sbd, &s->iomem);
@@ -505,7 +509,7 @@  static void cadence_uart_init(Object *obj)
 
 static int cadence_uart_post_load(void *opaque, int version_id)
 {
-    UartState *s = opaque;
+    CadenceUARTState *s = opaque;
 
     uart_parameters_setup(s);
     uart_update_status(s);
@@ -518,13 +522,15 @@  static const VMStateDescription vmstate_cadence_uart = {
     .minimum_version_id = 2,
     .post_load = cadence_uart_post_load,
     .fields = (VMStateField[]) {
-        VMSTATE_UINT32_ARRAY(r, UartState, R_MAX),
-        VMSTATE_UINT8_ARRAY(rx_fifo, UartState, RX_FIFO_SIZE),
-        VMSTATE_UINT8_ARRAY(tx_fifo, UartState, RX_FIFO_SIZE),
-        VMSTATE_UINT32(rx_count, UartState),
-        VMSTATE_UINT32(tx_count, UartState),
-        VMSTATE_UINT32(rx_wpos, UartState),
-        VMSTATE_TIMER_PTR(fifo_trigger_handle, UartState),
+        VMSTATE_UINT32_ARRAY(r, CadenceUARTState, CADENCE_UART_R_MAX),
+        VMSTATE_UINT8_ARRAY(rx_fifo, CadenceUARTState,
+                            CADENCE_UART_RX_FIFO_SIZE),
+        VMSTATE_UINT8_ARRAY(tx_fifo, CadenceUARTState,
+                            CADENCE_UART_TX_FIFO_SIZE),
+        VMSTATE_UINT32(rx_count, CadenceUARTState),
+        VMSTATE_UINT32(tx_count, CadenceUARTState),
+        VMSTATE_UINT32(rx_wpos, CadenceUARTState),
+        VMSTATE_TIMER_PTR(fifo_trigger_handle, CadenceUARTState),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -541,7 +547,7 @@  static void cadence_uart_class_init(ObjectClass *klass, void *data)
 static const TypeInfo cadence_uart_info = {
     .name          = TYPE_CADENCE_UART,
     .parent        = TYPE_SYS_BUS_DEVICE,
-    .instance_size = sizeof(UartState),
+    .instance_size = sizeof(CadenceUARTState),
     .instance_init = cadence_uart_init,
     .class_init    = cadence_uart_class_init,
 };