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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 19 Dec 2012, Ian Campbell wrote:
> 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.

OK


> > +{
> > +    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?

Yes. TBH the ideal thing to have would be a set of generic platform
functions that get initialized early in start_xen and then used lated
on; but considering that at the moment we only have one platform, it is
even difficult to figure out what functions we would need to export.


> > +    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?

_mode_string is RW while mode_string is RO.
This particular mode string (1280x1024@60) is chosen arbitrarily.


> > +    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 ?

in theory it can't be if it follows the DT spec, but I'll add a check
for it

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