On 20 January 2013 15:54, Blue Swirl <blauwir...@gmail.com> wrote:
This patch is a bit big to usefully review. A few comments on bits
I happened to notice:
> diff --git a/hw/arm_sysctl.c b/hw/arm_sysctl.c
> index a196fcc..2066ef3 100644
> --- a/hw/arm_sysctl.c
> +++ b/hw/arm_sysctl.c
> @@ -199,6 +199,7 @@ static void arm_sysctl_write(void *opaque, hwaddr offset,
> switch (offset) {
> case 0x08: /* LED */
> s->leds = val;
> + /* XXX: questionable fallthrough */
Should have its own 'break' but it's safe currently as the following
case is just 'break' anyway.
> case 0x0c: /* OSC0 */
> case 0x10: /* OSC1 */
> case 0x14: /* OSC2 */
> @@ -295,6 +296,7 @@ static void arm_sysctl_write(void *opaque, hwaddr offset,
> /* On VExpress this register is unimplemented and will RAZ/WI */
> break;
> }
> + /* XXX: questionable fallthrough */
Ditto.
> case 0x54: /* CLCDSER */
> case 0x64: /* DMAPSR0 */
> case 0x68: /* DMAPSR1 */
> --- a/hw/es1370.c
> +++ b/hw/es1370.c
> @@ -537,8 +537,10 @@ IO_WRITE_PROTO (es1370_writew)
>
> case ES1370_REG_ADC_SCOUNT:
> d++;
> + /* XXX: questionable fallthrough */
> case ES1370_REG_DAC2_SCOUNT:
> d++;
> + /* XXX: questionable fallthrough */
> case ES1370_REG_DAC1_SCOUNT:
> d->scount = (d->scount & ~0xffff) | (val & 0xffff);
> break;
These fallthroughs are clearly intentional (similar cases
elsewhere in your patch).
> --- a/hw/stellaris.c
> +++ b/hw/stellaris.c
> @@ -182,8 +182,10 @@ static uint64_t gptm_read(void *opaque, hwaddr offset,
> case 0x48: /* TAR */
> if (s->control == 1)
> return s->rtc;
> + /* XXX: questionable fallthrough */
> case 0x4c: /* TBR */
> hw_error("TODO: Timer value read\n");
> + /* XXX: questionable fallthrough */
This isn't a fallthrough at all, hw_error() never returns.
> default:
> hw_error("gptm_read: Bad offset 0x%x\n", (int)offset);
> return 0;
(...so this return 0 is unreachable, but hey.)
I don't think there's much point adding tons of "XXX" comments
when a bunch of these aren't actually wrong code. If you want to fix
this I think a better approach would be more focused patches aimed
at adding 'break;' or "/* fallthrough */" based on actual human
examination of the surrounding code.
-- PMM
