On 23 December 2011 11:40, Evgeny Voevodin <e.voevo...@samsung.com> 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
