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

[Peter Maydell]

On 24 August 2016 at 11:40, Dr. David Alan Gilbert (git)
<address@hidden> wrote:
> From: "Dr. David Alan Gilbert" <address@hidden>
>
> 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 <address@hidden>
> ---
>  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