Re: [Qemu-devel] [PATCH] hw/arm/stellaris: Implement watchdog timer

[Eric Blake <eblake@redhat.com>]

On 2/22/19 5:38 PM, michelheily@gmail.com wrote:
> From: Michel Heily <michelheily@gmail.com>
> 
> Signed-off-by: Michel Heily <michelheily@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