On 3 August 2011 13:31, Engin AYDOGAN <en...@bzzzt.biz> wrote:
> Based on the work http://patchwork.ozlabs.org/patch/31469/ , here's the
> patch for HEAD.
Thanks very much for updating this patch. I only have a couple of
review comments.
Patch formatting issues first:
It's better to submit patches with git-send-email, because this
will get the format right (for instance your email wrapped a number
of long lines which meant the patch didn't apply cleanly).
Also it's nice to have a slightly longer commit message
in addition to the initial summary line. In particular in
this case since the patch is based on somebody else's work it's
polite to say something like "Based on an earlier patch by
Vijay Kumar B" (and cc him when mailing qemu-devel). So you
might have something like:
hw/stellaris: Add support for RCC2 register
Add support for the RCC2 register on Fury class devices.
Based on a patch...
Signed-off-by: ...
> +static bool ssys_is_sandstorm(ssys_state *s)
> +{
> + uint32_t did0 = s->board->did0;
> +
> + switch (did0 & DID0_VER_MASK) {
> + case DID0_VER_0:
> + return 1;
> + case DID0_VER_1:
> + return ((did0 & DID0_CLASS_MASK) == DID0_CLASS_SANDSTORM);
> + default:
> + return 0;
> + }
> +}
> +
> +static bool ssys_is_fury(ssys_state *s)
> +{
> + uint32_t did0 = s->board->did0;
> +
> + return (((did0 & DID0_VER_MASK) == DID0_VER_1)
> + && ((did0 & DID0_CLASS_MASK) == DID0_CLASS_FURY));
> +}
These functions kind of inversely duplicate each other.
I think it would be nicer to have:
static int ssys_board_class(ssys_state *s)
{
uint32_t did0 = s->board->did0;
if ((did0 & DID0_VER_MASK) == DID0_VER_0) {
return DID0_CLASS_SANDSTORM;
}
return did0 & DID0_CLASS_MASK;
}
and then do
if (ssys_board_class(s) == DID0_CLASS_SANDSTORM) ...
(or ... == DID0_CLASS_FURY). This will continue to be
useful if we ever add another board class later.
-- PMM
