Re: [Qemu-devel] Re: [PATCH v3 2/7] wdt_ib700: Don't use SoftFloat int64 type

[Andreas Färber]

Am 19.12.2010 um 15:38 schrieb Blue Swirl:

> On Sun, Dec 19, 2010 at 2:28 PM, Andreas Färber <address@hidden 
> > wrote:
> > Am 19.12.2010 um 15:16 schrieb Richard W.M. Jones:
> >
> > > On Sun, Dec 19, 2010 at 01:51:22PM +0100, Andreas Färber wrote:
> > > > Am 18.12.2010 um 17:47 schrieb Richard W.M. Jones:
> > > >
> > > > > On Sat, Dec 18, 2010 at 05:25:26PM +0100, Andreas Färber wrote:
> > > > > > softfloat.h's int64 type has least-width semantics,
> > > > > > but this doesn't seem intended here, so use plain int64_t.
> > > > > >
> > > > > > v3:
> > > > > > * Split off.
> > > > > >
> > > > > > Cc: Richard W.M. Jones <address@hidden>
> > > > > > Signed-off-by: Andreas Färber <address@hidden>
> > > > > > ---
> > > > > > hw/wdt_ib700.c |    2 +-
> > > > > > 1 files changed, 1 insertions(+), 1 deletions(-)
> > > > > >
> > > > > > diff --git a/hw/wdt_ib700.c b/hw/wdt_ib700.c
> > > > > > index b6235eb..1248464 100644
> > > > > > --- a/hw/wdt_ib700.c
> > > > > > +++ b/hw/wdt_ib700.c
> > > > > > @@ -53,7 +53,7 @@ static void ib700_write_enable_reg(void *vp,
> > > > > > uint32_t addr, uint32_t data)
> > > > > >    30, 28, 26, 24, 22, 20, 18, 16,
> > > > > >    14, 12, 10,  8,  6,  4,  2,  0
> > > > > > };
> > > > > > -    int64 timeout;
> > > > > > +    int64_t timeout;
> > > > > >
> > > > > > ib700_debug("addr = %x, data = %x\n", addr, data);
> > > > >
> > > > > The use of int64(_t) was just so that the timeout calculation in  
> > > > > the
> > > > > next two lines would not overflow:
> > > > >
> > > > > timeout = (int64_t) time_map[data & 0xF] * get_ticks_per_sec();
> > > > > qemu_mod_timer(s->timer, qemu_get_clock (vm_clock) + timeout);
> > > > >
> > > > > and from you say it does seem like it was a mistake to use int64
> > > > > instead of int64_t.
> > > >
> > > > int64_t should be the right choice then.
> > > >
> > > > > ACK.
> > > > >
> > > > > In more general terms, am I doing the timeout correctly in this  
> > > > > code?
> > > >
> > > > Being unfamiliar with both the timer code and this device, hard to
> > > > say for me.
> > > > You're taking the lower nibble of uint32_t data and indexing
> > > > time_map[] with it, which contains 16 elements, so okay, upcast it
> > > > to 64-bit and multiply it to ticks. Assuming that vm_clock works in
> > > > ticks, adding the calculated timeout for the next expiry  
> > > > technically
> > > > looks good. Except for the extra space. ;)
> > > >
> > > > Care to provide a formal Reviewed-by or Acked-by? I'd respin it for
> > > > Blue with updated description.
> > >
> > > Is it enough just to write this:
> > >
> > > Reviewed-by: Richard W.M. Jones <address@hidden>
> > > Acked-by: Richard W.M. Jones <address@hidden>
> > >
> > > or do you want me to send the updated patch with this added?
> >
> > Thanks, that's sufficient! I think of Acked-by as a superset of  
> > Reviewed-by
> > so I'll go with the former.
>
> No, Acked-by tag does not mean much but Reviewed-by carries a lot of  
> weight:
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/SubmittingPatches;h=689e2371095cc5dfea9927120009341f369159aa;hb=HEAD#l423

So, should I add both or just Reviewed-by or relieve him of the  
weight? :)