diff mbox

[v2,4/5] vmstateify tsc2005

Message ID 1472035246-12483-5-git-send-email-dgilbert@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Dr. David Alan Gilbert Aug. 24, 2016, 10:40 a.m. UTC 
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

I've converted the fields in it's main data structure
to fixed size types in ways that look sane, but I don't actually
know the details of this hardware.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/input/tsc2005.c | 154 ++++++++++++++++++++---------------------------------
 1 file changed, 57 insertions(+), 97 deletions(-)

Comments

Peter Maydell Sept. 22, 2016, 3:41 p.m. UTC | #1 
On 24 August 2016 at 11:40, Dr. David Alan Gilbert (git)
<dgilbert@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> I've converted the fields in it's main data structure
> to fixed size types in ways that look sane, but I don't actually
> know the details of this hardware.

This is the kind of thing that should go below the '---',
not in the commit message proper (at least not in this phrasing,
in my opinion).

The TSC2005 datasheet is http://www.ti.com/lit/ds/symlink/tsc2005.pdf .

> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  hw/input/tsc2005.c | 154 ++++++++++++++++++++---------------------------------
>  1 file changed, 57 insertions(+), 97 deletions(-)
>
> diff --git a/hw/input/tsc2005.c b/hw/input/tsc2005.c
> index 9b359aa..b66dc50 100644
> --- a/hw/input/tsc2005.c
> +++ b/hw/input/tsc2005.c
> @@ -31,30 +31,31 @@ typedef struct {
>      QEMUTimer *timer;
>      uint16_t model;
>
> -    int x, y;
> -    int pressure;
> +    int32_t x, y;
> +    bool pressure;
>
> -    int state, reg, irq, command;
> +    uint8_t reg, state;
> +    bool irq, command;
>      uint16_t data, dav;
>
> -    int busy;
> -    int enabled;
> -    int host_mode;
> -    int function;
> -    int nextfunction;
> -    int precision;
> -    int nextprecision;
> -    int filter;
> -    int pin_func;
> -    int timing[2];
> -    int noise;
> -    int reset;
> -    int pdst;
> -    int pnd0;
> +    bool busy;
> +    bool enabled;

These changes mean the code is now doing bitwise-logic on
bool variables, for instance:
            s->busy &= s->enabled;

We also assign '0' and '1' to them rather than 'true' and 'false'.

> +    bool host_mode;
> +    int8_t function;
> +    int8_t nextfunction;
> +    bool precision;
> +    bool nextprecision;
> +    uint16_t filter;
> +    uint8_t pin_func;
> +    int16_t timing[2];

Why not uint16_t ?

> +    uint8_t noise;
> +    bool reset;
> +    bool pdst;
> +    bool pnd0;
>      uint16_t temp_thr[2];
>      uint16_t aux_thr[2];
>
> -    int tr[8];
> +    int32_t tr[8];
>  } TSC2005State;

Looks ok otherwise.

thanks
-- PMM
Dr. David Alan Gilbert Sept. 22, 2016, 3:53 p.m. UTC | #2 
* Peter Maydell (peter.maydell@linaro.org) wrote:
> On 24 August 2016 at 11:40, Dr. David Alan Gilbert (git)
> <dgilbert@redhat.com> wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > I've converted the fields in it's main data structure
> > to fixed size types in ways that look sane, but I don't actually
> > know the details of this hardware.
> 
> This is the kind of thing that should go below the '---',
> not in the commit message proper (at least not in this phrasing,
> in my opinion).

I can fix that.

> The TSC2005 datasheet is http://www.ti.com/lit/ds/symlink/tsc2005.pdf .
> 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  hw/input/tsc2005.c | 154 ++++++++++++++++++++---------------------------------
> >  1 file changed, 57 insertions(+), 97 deletions(-)
> >
> > diff --git a/hw/input/tsc2005.c b/hw/input/tsc2005.c
> > index 9b359aa..b66dc50 100644
> > --- a/hw/input/tsc2005.c
> > +++ b/hw/input/tsc2005.c
> > @@ -31,30 +31,31 @@ typedef struct {
> >      QEMUTimer *timer;
> >      uint16_t model;
> >
> > -    int x, y;
> > -    int pressure;
> > +    int32_t x, y;
> > +    bool pressure;
> >
> > -    int state, reg, irq, command;
> > +    uint8_t reg, state;
> > +    bool irq, command;
> >      uint16_t data, dav;
> >
> > -    int busy;
> > -    int enabled;
> > -    int host_mode;
> > -    int function;
> > -    int nextfunction;
> > -    int precision;
> > -    int nextprecision;
> > -    int filter;
> > -    int pin_func;
> > -    int timing[2];
> > -    int noise;
> > -    int reset;
> > -    int pdst;
> > -    int pnd0;
> > +    bool busy;
> > +    bool enabled;
> 
> These changes mean the code is now doing bitwise-logic on
> bool variables, for instance:
>             s->busy &= s->enabled;

> We also assign '0' and '1' to them rather than 'true' and 'false'.

Yes, I went through and as far as I can tell they're always
used as booleans already.


> > +    bool host_mode;
> > +    int8_t function;
> > +    int8_t nextfunction;
> > +    bool precision;
> > +    bool nextprecision;
> > +    uint16_t filter;
> > +    uint8_t pin_func;
> > +    int16_t timing[2];
> 
> Why not uint16_t ?

Yep, I'll fix that.

> > +    uint8_t noise;
> > +    bool reset;
> > +    bool pdst;
> > +    bool pnd0;
> >      uint16_t temp_thr[2];
> >      uint16_t aux_thr[2];
> >
> > -    int tr[8];
> > +    int32_t tr[8];
> >  } TSC2005State;
> 
> Looks ok otherwise.
> 
> thanks
> -- PMM
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Peter Maydell Sept. 22, 2016, 4 p.m. UTC | #3 
On 22 September 2016 at 16:53, Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
> * Peter Maydell (peter.maydell@linaro.org) wrote:
>> These changes mean the code is now doing bitwise-logic on
>> bool variables, for instance:
>>             s->busy &= s->enabled;
>
>> We also assign '0' and '1' to them rather than 'true' and 'false'.
>
> Yes, I went through and as far as I can tell they're always
> used as booleans already.

Yeah, but using bitwise ops on booleans is the kind of
thing that prompts compilers and static analysers and
linters to complain, so we should fix those expressions
if we're changing the type.

thanks
-- PMM
diff mbox

Patch

diff --git a/hw/input/tsc2005.c b/hw/input/tsc2005.c
index 9b359aa..b66dc50 100644
--- a/hw/input/tsc2005.c
+++ b/hw/input/tsc2005.c
@@ -31,30 +31,31 @@  typedef struct {
     QEMUTimer *timer;
     uint16_t model;
 
-    int x, y;
-    int pressure;
+    int32_t x, y;
+    bool pressure;
 
-    int state, reg, irq, command;
+    uint8_t reg, state;
+    bool irq, command;
     uint16_t data, dav;
 
-    int busy;
-    int enabled;
-    int host_mode;
-    int function;
-    int nextfunction;
-    int precision;
-    int nextprecision;
-    int filter;
-    int pin_func;
-    int timing[2];
-    int noise;
-    int reset;
-    int pdst;
-    int pnd0;
+    bool busy;
+    bool enabled;
+    bool host_mode;
+    int8_t function;
+    int8_t nextfunction;
+    bool precision;
+    bool nextprecision;
+    uint16_t filter;
+    uint8_t pin_func;
+    int16_t timing[2];
+    uint8_t noise;
+    bool reset;
+    bool pdst;
+    bool pnd0;
     uint16_t temp_thr[2];
     uint16_t aux_thr[2];
 
-    int tr[8];
+    int32_t tr[8];
 } TSC2005State;
 
 enum {
@@ -434,86 +435,9 @@  static void tsc2005_touchscreen_event(void *opaque,
         tsc2005_pin_update(s);
 }
 
-static void tsc2005_save(QEMUFile *f, void *opaque)
+static int tsc2005_post_load(void *opaque, int version_id)
 {
     TSC2005State *s = (TSC2005State *) opaque;
-    int i;
-
-    qemu_put_be16(f, s->x);
-    qemu_put_be16(f, s->y);
-    qemu_put_byte(f, s->pressure);
-
-    qemu_put_byte(f, s->state);
-    qemu_put_byte(f, s->reg);
-    qemu_put_byte(f, s->command);
-
-    qemu_put_byte(f, s->irq);
-    qemu_put_be16s(f, &s->dav);
-    qemu_put_be16s(f, &s->data);
-
-    timer_put(f, s->timer);
-    qemu_put_byte(f, s->enabled);
-    qemu_put_byte(f, s->host_mode);
-    qemu_put_byte(f, s->function);
-    qemu_put_byte(f, s->nextfunction);
-    qemu_put_byte(f, s->precision);
-    qemu_put_byte(f, s->nextprecision);
-    qemu_put_be16(f, s->filter);
-    qemu_put_byte(f, s->pin_func);
-    qemu_put_be16(f, s->timing[0]);
-    qemu_put_be16(f, s->timing[1]);
-    qemu_put_be16s(f, &s->temp_thr[0]);
-    qemu_put_be16s(f, &s->temp_thr[1]);
-    qemu_put_be16s(f, &s->aux_thr[0]);
-    qemu_put_be16s(f, &s->aux_thr[1]);
-    qemu_put_be32(f, s->noise);
-    qemu_put_byte(f, s->reset);
-    qemu_put_byte(f, s->pdst);
-    qemu_put_byte(f, s->pnd0);
-
-    for (i = 0; i < 8; i ++)
-        qemu_put_be32(f, s->tr[i]);
-}
-
-static int tsc2005_load(QEMUFile *f, void *opaque, int version_id)
-{
-    TSC2005State *s = (TSC2005State *) opaque;
-    int i;
-
-    s->x = qemu_get_be16(f);
-    s->y = qemu_get_be16(f);
-    s->pressure = qemu_get_byte(f);
-
-    s->state = qemu_get_byte(f);
-    s->reg = qemu_get_byte(f);
-    s->command = qemu_get_byte(f);
-
-    s->irq = qemu_get_byte(f);
-    qemu_get_be16s(f, &s->dav);
-    qemu_get_be16s(f, &s->data);
-
-    timer_get(f, s->timer);
-    s->enabled = qemu_get_byte(f);
-    s->host_mode = qemu_get_byte(f);
-    s->function = qemu_get_byte(f);
-    s->nextfunction = qemu_get_byte(f);
-    s->precision = qemu_get_byte(f);
-    s->nextprecision = qemu_get_byte(f);
-    s->filter = qemu_get_be16(f);
-    s->pin_func = qemu_get_byte(f);
-    s->timing[0] = qemu_get_be16(f);
-    s->timing[1] = qemu_get_be16(f);
-    qemu_get_be16s(f, &s->temp_thr[0]);
-    qemu_get_be16s(f, &s->temp_thr[1]);
-    qemu_get_be16s(f, &s->aux_thr[0]);
-    qemu_get_be16s(f, &s->aux_thr[1]);
-    s->noise = qemu_get_be32(f);
-    s->reset = qemu_get_byte(f);
-    s->pdst = qemu_get_byte(f);
-    s->pnd0 = qemu_get_byte(f);
-
-    for (i = 0; i < 8; i ++)
-        s->tr[i] = qemu_get_be32(f);
 
     s->busy = timer_pending(s->timer);
     tsc2005_pin_update(s);
@@ -521,6 +445,42 @@  static int tsc2005_load(QEMUFile *f, void *opaque, int version_id)
     return 0;
 }
 
+static const VMStateDescription vmstate_tsc2005 = {
+    .name = "tsc2005",
+    .version_id = 2,
+    .minimum_version_id = 2,
+    .post_load = tsc2005_post_load,
+    .fields      = (VMStateField []) {
+        VMSTATE_BOOL(pressure, TSC2005State),
+        VMSTATE_BOOL(irq, TSC2005State),
+        VMSTATE_BOOL(command, TSC2005State),
+        VMSTATE_BOOL(enabled, TSC2005State),
+        VMSTATE_BOOL(host_mode, TSC2005State),
+        VMSTATE_BOOL(reset, TSC2005State),
+        VMSTATE_BOOL(pdst, TSC2005State),
+        VMSTATE_BOOL(pnd0, TSC2005State),
+        VMSTATE_BOOL(precision, TSC2005State),
+        VMSTATE_BOOL(nextprecision, TSC2005State),
+        VMSTATE_UINT8(reg, TSC2005State),
+        VMSTATE_UINT8(state, TSC2005State),
+        VMSTATE_UINT16(data, TSC2005State),
+        VMSTATE_UINT16(dav, TSC2005State),
+        VMSTATE_UINT16(filter, TSC2005State),
+        VMSTATE_INT8(nextfunction, TSC2005State),
+        VMSTATE_INT8(function, TSC2005State),
+        VMSTATE_INT32(x, TSC2005State),
+        VMSTATE_INT32(y, TSC2005State),
+        VMSTATE_TIMER_PTR(timer, TSC2005State),
+        VMSTATE_UINT8(pin_func, TSC2005State),
+        VMSTATE_INT16_ARRAY(timing, TSC2005State, 2),
+        VMSTATE_UINT8(noise, TSC2005State),
+        VMSTATE_UINT16_ARRAY(temp_thr, TSC2005State, 2),
+        VMSTATE_UINT16_ARRAY(aux_thr, TSC2005State, 2),
+        VMSTATE_INT32_ARRAY(tr, TSC2005State, 8),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 void *tsc2005_init(qemu_irq pintdav)
 {
     TSC2005State *s;
@@ -550,7 +510,7 @@  void *tsc2005_init(qemu_irq pintdav)
                     "QEMU TSC2005-driven Touchscreen");
 
     qemu_register_reset((void *) tsc2005_reset, s);
-    register_savevm(NULL, "tsc2005", -1, 0, tsc2005_save, tsc2005_load, s);
+    vmstate_register(NULL, 0, &vmstate_tsc2005, s);
 
     return s;
 }