[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 8/8] xen/arm: introduce a driver for the ARM HDLCD controller
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |