Re: [Xen-devel] [PATCH v3 8/8] xen/arm: introduce a driver for the ARM HDLCD controller

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2012-12-18 at 18:46 +0000, Stefano Stabellini wrote:
> +static void set_color_masks(int bpp,
> +                       int *red_shift, int *green_shift, int *blue_shift,
> +                       int *red_size, int *green_size, int *blue_size)

This is crying out for a pointer to a struct.

> +{
> +    switch (bpp) {
> +        case 2:
> +            *red_shift = 0;
> +            *green_shift = 5;
> +            *blue_shift = 11;
> +            *red_size = 5;
> +            *green_size = 6;
> +            *blue_size = 5;
> +            break;
> +        case 3:
> +        case 4:
> +            *red_shift = 0;
> +            *green_shift = 8;
> +            *blue_shift = 16;
> +            *red_size = 8;
> +            *green_size = 8;
> +            *blue_size = 8;
> +            break;
> +        default:
> +            BUG();
> +            break;
> +    }
> +}
> +
> +static void set_pixclock(uint32_t pixclock)
> +{

Doesn't there need to be some sort of "are we running on a vexpress"
check here? e.g. a DTB compatibility node check or something?

> +    vexpress_syscfg(1, V2M_SYS_CFG_OSC_FUNC, V2M_SYS_CFG_OSC5, &pixclock);
> +}
> +
> +void __init video_init(void)
> +{
> +    int node, depth;
> +    u32 address_cells, size_cells;
> +    struct fb_prop fbp;
> +    unsigned char *lfb;
> +    paddr_t hdlcd_start, hdlcd_size;
> +    paddr_t framebuffer_start, framebuffer_size;
> +    const struct fdt_property *prop;
> +    const u32 *cell;
> +    const char *mode_string;
> +    char _mode_string[16];
> +    int bpp;
> +    int red_shift, green_shift, blue_shift;
> +    int red_size, green_size, blue_size;
> +    struct modeline *videomode = NULL;
> +    int i;
> +
> +    if ( find_compatible_node("arm,hdlcd", &node, &depth,
> +                &address_cells, &size_cells) <= 0 )
> +        return;
> +
> +    prop = fdt_get_property(device_tree_flattened, node, "reg", NULL);
> +    if ( !prop )
> +        return;
> +
> +    cell = (const u32 *)prop->data;
> +    device_tree_get_reg(&cell, address_cells, size_cells,
> +            &hdlcd_start, &hdlcd_size);
> +
> +    prop = fdt_get_property(device_tree_flattened, node, "framebuffer", 
> NULL);
> +    if ( !prop )
> +        return;
> +
> +    cell = (const u32 *)prop->data;
> +    device_tree_get_reg(&cell, address_cells, size_cells,
> +            &framebuffer_start, &framebuffer_size);
> +
> +    mode_string = fdt_getprop(device_tree_flattened, node, "mode", NULL);
> +    if ( !mode_string )
> +    {
> +        bpp = 4;
> +        set_color_masks(bpp, &red_shift, &green_shift, &blue_shift,
> +                &red_size, &green_size, &blue_size);
> +        memcpy(_mode_string, "1280x1024@60", strlen("1280x1024@60"));

What associates mode_string with this _mode_string?

> +    }

Or should there be an else here? Otherwise can't mode_string be NULL?

> +    if ( strlen(mode_string) < strlen("800x600@60") )
> +    {
> +        printk("HDLCD: invalid modeline=%s\n", mode_string);
> +        return;
> +    } else {
> +        char *s = strchr(mode_string, '-');
> +        if ( !s )
> +        {
> +            printk("HDLCD: bpp not found in modeline %s, assume 32 bpp\n",
> +                    mode_string);
> +            bpp = 4;
> +            set_color_masks(bpp, &red_shift, &green_shift, &blue_shift,
> +                    &red_size, &green_size, &blue_size);
> +            memcpy(_mode_string, mode_string, strlen(mode_string) + 1);

What if strlen(mode_string)+1 > 16 ?

> +        } else {
> +                       if ( strlen(s) < 6 )
> +                       {
> +                               printk("HDLCD: invalid mode %s\n", 
> mode_string);
> +                               return;
> +                       }

Indentation

> +            s++;
> +            if ( !strncmp(s, "16", 2) )
> +            {
> +                bpp = 2;
> +                set_color_masks(bpp, &red_shift, &green_shift, &blue_shift,
> +                        &red_size, &green_size, &blue_size);
> +            }
> +            else if ( !strncmp(s, "24", 2) )
> +            {
> +                bpp = 3;
> +                set_color_masks(bpp, &red_shift, &green_shift, &blue_shift,
> +                        &red_size, &green_size, &blue_size);
> +            }
> +            else if ( !strncmp(s, "32", 2) )
> +            {
> +                bpp = 4;
> +                set_color_masks(bpp, &red_shift, &green_shift, &blue_shift,
> +                        &red_size, &green_size, &blue_size);

This all smells like a lookup table to me.

> +            } else  {

extra space here.

> +                printk("HDLCD: unsupported bpp %s\n", s);
> +                return;
> +            }
> +            i = s - mode_string - 1;
> +            memcpy(_mode_string, mode_string, i);
> +            memcpy(_mode_string + i, mode_string + i + 3, 4);
> +        }
> +    }
> +
> +    for ( i = 0; i < ARRAY_SIZE(videomodes); i++ )
> +    {
> +        if ( !strcmp(_mode_string, videomodes[i].mode) )
> +        {
> +            videomode = &videomodes[i];
> +            break;
> +        }
> +    }
> +    if ( !videomode )
> +    {
> +        printk("HDLCD: unsupported videomode %s\n", _mode_string);
> +        return;
> +    }
> +
> +
> +    if ( !hdlcd_start || !framebuffer_start )

Worth a message? Also couldn't you have checked this much earlier
(before the mode parsing stuff).

> +        return;
> +
> +    if ( framebuffer_size < bpp * videomode->xres * videomode->yres )
> +    {
> +        printk("HDLCD: the framebuffer is too small, disable the HDLCD 
> driver\n");

"disable" or "disabling" ?

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel