Re: [Qemu-devel] [PATCH v2 4/5] vmstateify tsc2005

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* 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