On 2/22/19 5:38 PM, michelhe...@gmail.com wrote: > From: Michel Heily <michelhe...@gmail.com> > > Signed-off-by: Michel Heily <michelhe...@gmail.com>
This appears to be your first contribution to qemu. Welcome to the community! Your commit message doesn't give any details beyond the "what" in the subject line. The body of the commit message should explain the "why" (what bug are you fixing, how to reproduce it), so that a reviewer stands a chance of determining if the code matches the description you gave, and if the issue you describe really does warrant the inclusion of your patch. In particular, when implementing a particular piece of hardware, giving a URL to the datasheet you used as your reference will let reviewers check if your software appears to match what the actual hardware would do. For more patch submission hints, see: https://wiki.qemu.org/Contribute/SubmitAPatch I'm not an expert on this hardware, but will at least give a cursory glance for style issues: > + case WDT_O_LOCK: > + return s->locked ? 1 : 0; > + case WDT_O_PeriphID4: /* fallthrough */ > + case WDT_O_PeriphID5: I don't think that particular /* fallthrough */ comment is needed. It matters when you have a case with code, but not for consecutive case labels. > + > +static void wdtimer_write(void *opaque, hwaddr offset, > + uint64_t value, unsigned size) Alignment looks off. > +{ > + wdtimer_state *s = (wdtimer_state *)opaque; This is C, where void* implicitly converts to other types; you don't need the cast. > @@ -1243,7 +1478,7 @@ static void stellaris_init(MachineState *ms, > stellaris_board_info *board) > * Stellaris LM3S6965 Microcontroller Data Sheet (rev I) > * http://www.ti.com/lit/ds/symlink/lm3s6965.pdf > * > - * 40000000 wdtimer (unimplemented) > + * 40000000 wdtimer Okay, so there is a datasheet. But it may still help to mention it in the commit message in addition to the comment already in the code. > * 40002000 i2c (unimplemented) > * 40004000 GPIO > * 40005000 GPIO > @@ -1335,6 +1570,12 @@ static void stellaris_init(MachineState *ms, > stellaris_board_info *board) > } > } > > + if (board->dc1 & (1 << 3)) { /* watchdog present */ Spacing looks off. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org