Re: [Qemu-devel] [PATCH v5 11/11] Exynos4210: added display controller implementation

[Peter Maydell]

On 23 December 2011 11:40, Evgeny Voevodin <address@hidden> wrote:

> +/* Perform byte/halfword/word swap of data according to WINCON */
> +static inline void fimd_swap_data(unsigned int swap_ctl, uint64_t *data)
> +{
> +    int i;
> +    uint64_t res;
> +    uint64_t x = *data;
> +
> +    if (swap_ctl & FIMD_WINCON_SWAP_BITS) {
> +        res = 0;
> +        for (i = 0; i < 64; i++) {
> +            if (x & (1ULL << (64 - i))) {
> +                res |= (1ULL << i);
> +            }
> +        }
> +        x = res;
> +    }
> +    if (swap_ctl & FIMD_WINCON_SWAP_BYTE) {
> +        x = ((x & 0x00000000000000FFULL) << 56) |
> +            ((x & 0x000000000000FF00ULL) << 40) |
> +            ((x & 0x0000000000FF0000ULL) << 24) |
> +            ((x & 0x00000000FF000000ULL) <<  8) |
> +            ((x & 0x000000FF00000000ULL) >>  8) |
> +            ((x & 0x0000FF0000000000ULL) >> 24) |
> +            ((x & 0x00FF000000000000ULL) >> 40) |
> +            ((x & 0xFF00000000000000ULL) >> 56);
> +    }
> +    if (swap_ctl & FIMD_WINCON_SWAP_HWORD) {
> +        x = ((x & 0x000000000000FFFFULL) << 48) |
> +            ((x & 0x00000000FFFF0000ULL) << 16) |
> +            ((x & 0x0000FFFF00000000ULL) >> 16) |
> +            ((x & 0xFFFF000000000000ULL) >> 48);
> +    }
> +    if (swap_ctl & FIMD_WINCON_SWAP_WORD) {
> +        x = ((x & 0x00000000FFFFFFFFULL) << 32) |
> +            ((x & 0xFFFFFFFF00000000ULL) >> 32);
> +    }

bswap.h provides bswap_64() which you can use for the first of these.

> +/* Draw line with index in palette table in RAM frame buffer data */
> +#define DEF_DRAW_LINE_PALLETE(N) \
> +static void glue(draw_line_palette_, N)(Exynos4210fimdWindow *w, uint8_t 
> *src, \
> +               uint8_t *dst, bool blend) \

The comment and the parameter are correct about the spelling of 'palette';
please fix the macro name.

exynos4210_fimd_get_bppmode() has the same typo -- global search-n-replace
seems like a good idea.


> +static void exynos4210_fimd_write(void *opaque, target_phys_addr_t offset,
> +                              uint64_t val, unsigned size)
> +{
> +    Exynos4210fimdState *s = (Exynos4210fimdState *)opaque;
> +    int w, i;
> +
> +    DPRINT_L2("write offset 0x%08x, value=%ld(0x%08lx)\n", offset, val, val);
> +
> +    if (offset == FIMD_VIDCON0) {
> +        if ((val & FIMD_VIDCON0_ENVID_MASK) == FIMD_VIDCON0_ENVID_MASK) {
> +            exynos4210_fimd_enable(s, true);
> +        } else {
> +            if ((val & FIMD_VIDCON0_ENVID) == 0) {
> +                exynos4210_fimd_enable(s, false);
> +            }
> +        }
> +        s->vidcon[0] = val;
> +    } else if (offset == FIMD_VIDCON1) {

is using an if-else ladder here rather than a switch statement a deliberate
decision ?

Otherwise looks OK I guess, though I've only scanned it rather superficially.

-- PMM