Re: [Qemu-devel] [PATCH v11 1/9] ARM: exynos4210: IRQ subsystem support.

["Andreas Färber" <afaerber@suse.de>]

Am 30.01.2012 08:38, schrieb Evgeny Voevodin:
> Signed-off-by: Evgeny Voevodin <e.voevodin@samsung.com>
> ---

> diff --git a/hw/exynos4210_combiner.c b/hw/exynos4210_combiner.c
> new file mode 100644
> index 0000000..4d41a1a
> --- /dev/null
> +++ b/hw/exynos4210_combiner.c

> +static const VMStateDescription VMState_Exynos4210CombinerGroupState = {

> +static const VMStateDescription VMState_Exynos4210Combiner = {

Here the variable names should not be CamelCase.

> +static DeviceInfo exynos4210_combiner_info = {

This will need to be updated to TypeInfo after Anthony's series removes
DeviceInfo.

> diff --git a/hw/exynos4210_gic.c b/hw/exynos4210_gic.c
> new file mode 100644
> index 0000000..bd37e86
> --- /dev/null
> +++ b/hw/exynos4210_gic.c

> +#define    EXT_GIC_ID_TVENC   127
> +#define    EXT_GIC_ID_MFC 126
> +#define    EXT_GIC_ID_HDMI_I2C    125
> +#define    EXT_GIC_ID_HDMI    124
> +#define    EXT_GIC_ID_MIXER   123
> +#define    EXT_GIC_ID_PCIe    122
> +#define    EXT_GIC_ID_2D  121
> +#define    EXT_GIC_ID_JPEG    120
> +#define    EXT_GIC_ID_FIMC3   119
> +#define    EXT_GIC_ID_FIMC2   118
> +#define    EXT_GIC_ID_FIMC1   117
> +#define    EXT_GIC_ID_FIMC0   116
> +#define    EXT_GIC_ID_ROTATOR 115
> +#define    EXT_GIC_ID_ONENAND_AUDI    114
> +#define    EXT_GIC_ID_MIPI_DSI_2LANE  113
> +#define    EXT_GIC_ID_MIPI_CSI_2LANE  112
> +#define    EXT_GIC_ID_MIPI_DSI_4LANE  111
> +#define    EXT_GIC_ID_MIPI_CSI_4LANE  110
> +#define    EXT_GIC_ID_SDMMC   109
> +#define    EXT_GIC_ID_HSMMC3  108
> +#define    EXT_GIC_ID_HSMMC2  107
> +#define    EXT_GIC_ID_HSMMC1  106
> +#define    EXT_GIC_ID_HSMMC0  105
> +#define    EXT_GIC_ID_MODEMIF 104
> +#define    EXT_GIC_ID_USB_DEVICE  103
> +#define    EXT_GIC_ID_USB_HOST    102
> +#define    EXT_GIC_ID_MCT_G1  101
> +#define    EXT_GIC_ID_SPI2    100
> +#define    EXT_GIC_ID_SPI1    99
> +#define    EXT_GIC_ID_SPI0    98
> +#define    EXT_GIC_ID_I2C7    97
> +#define    EXT_GIC_ID_I2C6    96
> +#define    EXT_GIC_ID_I2C5    95
> +#define    EXT_GIC_ID_I2C4    94
> +#define    EXT_GIC_ID_I2C3    93
> +#define    EXT_GIC_ID_I2C2    92
> +#define    EXT_GIC_ID_I2C1    91
> +#define    EXT_GIC_ID_I2C0    90
> +#define    EXT_GIC_ID_MCT_G0  89
> +#define    EXT_GIC_ID_UART4   88
> +#define    EXT_GIC_ID_UART3   87
> +#define    EXT_GIC_ID_UART2   86
> +#define    EXT_GIC_ID_UART1   85
> +#define    EXT_GIC_ID_UART0    84
> +#define    EXT_GIC_ID_NFC      83
> +#define    EXT_GIC_ID_IEM_IEC 82
> +#define    EXT_GIC_ID_IEM_APC 81
> +#define    EXT_GIC_ID_MCT_L1  80
> +#define    EXT_GIC_ID_GPIO_XA 79
> +#define    EXT_GIC_ID_GPIO_XB 78
> +#define    EXT_GIC_ID_RTC_TIC 77
> +#define    EXT_GIC_ID_RTC_ALARM   76
> +#define    EXT_GIC_ID_WDT 75
> +#define    EXT_GIC_ID_MCT_L0  74
> +#define    EXT_GIC_ID_TIMER4  73
> +#define    EXT_GIC_ID_TIMER3  72
> +#define    EXT_GIC_ID_TIMER2  71
> +#define    EXT_GIC_ID_TIMER1  70
> +#define    EXT_GIC_ID_TIMER0  69
> +#define    EXT_GIC_ID_PDMA1   68
> +#define    EXT_GIC_ID_PDMA0   67
> +#define    EXT_GIC_ID_MDMA_LCD0   66

The formatting looks interesting here... copied from an external header?
Since they're in descending order maybe just inverse them and use an
enum (gdb in mind)?

> +
> +enum ext_int {

CamelCase :)

> +static uint32_t
> +combiner_grp_to_gic_id[64-EXYNOS4210_MAX_EXT_COMBINER_OUT_IRQ][8] = {
> +    /* int combiner groups 16-19 */
> +    {}, {}, {}, {},
> +    /* int combiner group 20 */
> +    {0, EXT_GIC_ID_MDMA_LCD0},

Usually we use spaces inside braces.

> +static const VMStateDescription VMState_Exynos4210IRQGate = {

Not CamelCase ;)

But seriously, I really think review makes more sense when the patches
can actually be applied unmodified. Since Peter, as I understood, does
not have a kernel to test these machines himself, this will need to be
rebased by you guys - either onto Anthony's designated qom-upstream.X
branch or wait til next week for the patches to arrive in master.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg