Re: [Qemu-devel] [PATCH 1/4] ARM: exynos4210_pmu: just formatting changes

[Peter Maydell]

On 6 July 2012 16:49, Maksim Kozlov <address@hidden> wrote:
> Mainly to make 'exynos4210_pmu_regs' array more readable.

>
>  static const Exynos4210PmuReg exynos4210_pmu_regs[] = {
> -    {"OM_STAT", OM_STAT, 0x00000000},

> +    { "OM_STAT",                      OM_STAT,                     
> 0x00000000 },

If you really want to improve the readability of this you
could avoid the repetition of the register name:

#define PMUREG(NAME,RESETVALUE) \
    { #NAME, NAME, RESETVALUE }

static const Exynos4210PmuReg exynos4210_pmu_regs[] = {
    PMUREG(OM_STAT, 0x00000000),
    PMUREG(RTC_CLK0_SEL, 0x00000000),
[etc]
};
#undef PMUREG

(which would also avoid the annoying handful of cases which
currently overflow a single 80 column line).

>  typedef struct Exynos4210PmuState {
>      SysBusDevice busdev;
>      MemoryRegion iomem;
> -    uint32_t reg[PMU_NUM_OF_REGISTERS];
> +    uint32_t     reg[PMU_NUM_OF_REGISTERS];
>  } Exynos4210PmuState;

I wouldn't bother with this change. (Generally trying to line
up struct field names etc is not a good idea in my opinion --
it means that later additions to the struct either cause wholesale
realignment of unrelated existing fields, or they're not lined up.)

> @@ -468,7 +472,7 @@ static const VMStateDescription exynos4210_pmu_vmstate = {
>      .name = "exynos4210.pmu",
>      .version_id = 1,
>      .minimum_version_id = 1,
> -    .fields      = (VMStateField[]) {
> +    .fields = (VMStateField[]) {
>          VMSTATE_UINT32_ARRAY(reg, Exynos4210PmuState, PMU_NUM_OF_REGISTERS),
>          VMSTATE_END_OF_LIST()
>      }

This change is OK, though, since it's removing pointless extra spacing.

> @@ -479,9 +483,9 @@ static void exynos4210_pmu_class_init(ObjectClass *klass, 
> void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>
> -    k->init = exynos4210_pmu_init;
> +    k->init   = exynos4210_pmu_init;
>      dc->reset = exynos4210_pmu_reset;
> -    dc->vmsd = &exynos4210_pmu_vmstate;
> +    dc->vmsd  = &exynos4210_pmu_vmstate;
>  }

...but this one's not really worthwhile.

-- PMM