Re: [Qemu-devel] [PATCH v2 4/5] vmstateify tsc2005
On 22 September 2016 at 16:53, Dr. David Alan Gilbert wrote: > * Peter Maydell (peter.mayd...@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
Re: [Qemu-devel] [PATCH v2 4/5] vmstateify tsc2005
On 24 August 2016 at 11:40, Dr. David Alan Gilbert (git)
wrote:
> From: "Dr. David Alan Gilbert"
>
> 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
> ---
> 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
Re: [Qemu-devel] [PATCH v2 4/5] vmstateify tsc2005
* Peter Maydell (peter.mayd...@linaro.org) wrote:
> On 24 August 2016 at 11:40, Dr. David Alan Gilbert (git)
> wrote:
> > From: "Dr. David Alan Gilbert"
> >
> > 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
> > ---
> > 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 / dgilb...@redhat.com / Manchester, UK
