omapfb-main.c: check result of simple_strtoul

Tomi,

In drivers/video/omap2/omapfb/omapfb-main.c:

static int omapfb_parse_vram_param(const char *param, int max_entries,
unsigned long *sizes, unsigned long *paddrs)
{
int fbnum;
unsigned long size;
unsigned long paddr = 0;
char *p, *start;
start = (char *)param;
while (1) {
p = start;
fbnum = simple_strtoul(p, &p, 10);
- if (p == param)
+ if (p == start)

correct?

Hein

Yes, looks like a correct fix. I'll cook up a patch.

How did you encounter the bug? What was the outcome?

Tomi

I was reading the source, adding some printk's to get more debugging.

It would fail to detect a syntax error like:

     "omapfb.vram=0:2M,:5M"

If I may ask: why aren't "mode" and "modes" filled in:

/sys/devices/platform/omapfb/graphics/fb0 ?

Only "modes" will get an entry when a new mode becomes active.

I thought of adding something like this in hdmi.c:

+static void omap_to_fbmode (struct fb_videomode *fb_mode, const struct hdmi_config *cfg) +{
+ char name[64];
+ unsigned hsync;
+ unsigned vsync;
+ unsigned refresh;
+ snprintf (name, sizeof name, "%d_%s_%d.%d",
+ cfg->cm.code,
+ cfg->cm.mode == HDMI_HDMI ? "hdmi" : "dvi",
+ cfg->timings.x_res,
+ cfg->timings.y_res);
+
+ hsync = cfg->timings.hfp + cfg->timings.hsw + cfg->timings.hbp + cfg->timings.x_res;
+ vsync = cfg->timings.vfp + cfg->timings.vsw + cfg->timings.vbp + cfg->timings.y_res;
+ refresh = (unsigned) ((1000U * cfg->timings.pixel_clock + (hsync * vsync-1) ) / (hsync * vsync));
+
+ fb_mode->name = kstrdup(name, GFP_KERNEL); /* optional */
+ fb_mode->refresh = refresh; /* optional */
+ fb_mode->xres = cfg->timings.x_res;
+ fb_mode->yres = cfg->timings.y_res;
+ fb_mode->pixclock = DIV_ROUND_UP (1000000000UL, cfg->timings.pixel_clock);
+ fb_mode->left_margin = cfg->timings.hfp;
+ fb_mode->right_margin = cfg->timings.hbp;
+ fb_mode->upper_margin = cfg->timings.vfp;
+ fb_mode->lower_margin = cfg->timings.vbp;
+ fb_mode->hsync_len = cfg->timings.hsw;
+ fb_mode->vsync_len = cfg->timings.vsw;
+ fb_mode->sync =
+ (cfg->timings.vsync_pol ? FB_SYNC_VERT_HIGH_ACT : 0) |
+ (cfg->timings.hsync_pol ? FB_SYNC_HOR_HIGH_ACT : 0);
+ fb_mode->vmode = 0;
+ fb_mode->flag = 0;
+}
+
+int omap2_add_video_modes (struct fb_info *fbi)
+{
+ struct fb_videomode fb_mode;
+ int i;
+ int count = 0;
+
+ INIT_LIST_HEAD(&fbi->modelist);
+
+ for (i = 0; i < ARRAY_SIZE(cea_timings); i++) {
+ omap_to_fbmode (&fb_mode, &cea_timings[i]);
+ fb_add_videomode(&fb_mode, &fbi->modelist);
+ count++;
+ }
+ for (i = 0; i < ARRAY_SIZE(vesa_timings); i++) {
+ omap_to_fbmode (&fb_mode, &vesa_timings[i]);
+ fb_add_videomode(&fb_mode, &fbi->modelist);
+ count++;
+ }
+ return count;
+}

and call it from omapfb-main.c:

static int omapfb_create_framebuffers(struct omapfb2_device *fbdev)

{
	...

clear_fb_info(fbi);

fbdev->fbs[i] = fbi;
+ omap2_add_video_modes (fbi);

With this patch I get 34 working entries from "cat modes":

U:1024x768p-59
U:1280x800p-67
U:1680x1050p-59
U:1400x1050p-59
U:1280x768p-59
U:1920x1080p-60
U:1366x768p-59
...

which makes testing a bit easier.

But maybe I'm reinventing some wheel?

Hein

Ok, thanks.

No particular reason, nobody just has implemented it.