7 comments:
File src/drivers/aspeed/common/ast_i2c.c:
backup = software_i2c[ASPEED_BUS];
software_i2c[ASPEED_BUS] = &ast_ops;
It's not exactly dead? It's just a debug feature that you normally wouldn't enable in committed code […]
Well, I see what this code is doing. But it's just weird and a bad
example, IMHO, for anyone who's going to look in existing code how
to use it. Making a backup of nothing for the case that somebody
activates incompatible debug code? shouldn't it be the other way
around, the code that requires software_i2c is clean and the debug
code looks weird?
Patch Set #5, Line 133: ast = ast_priv;
Don't you need to do this before you call set_clock() and set_data() above?
I see that too.
File src/drivers/aspeed/common/ast_mode_corebootfb.c:
Patch Set #6, Line 40: expexts
expects
Patch Set #6, Line 51: dev_err(dev->pdev, "BAR0 resource not found.\n");
eh, just wasted a lot of time to figure out why this line compiles...
because `dev` doesn't exist. Can we make it a function or just use
printk() here?
Patch Set #6, Line 81: mode->crtc_hblank_start = edid->mode.ha + edid->mode.hso;
Why add `hso`? (sync offset == front porch, which is blank too?)
I'll try to draw a picture of my EDID interpretation:
+++++++++++++++++++++++++++++++++++---------------------------------------------------,
| active | blank |
|<-border->|<-display->|<-border->|<-front porch->|<-sync-pulse width->|<-back porch->|
|<-sync offset->|
Border is mostly ignored, though.
Patch Set #6, Line 82: mode->crtc_hblank_end = edid->mode.ha + edid->mode.hso + edid->mode.hbl;
This should be just active + blank?
Patch Set #6, Line 85: mode->crtc_htotal = MAX(mode->crtc_hblank_end, mode->crtc_hsync_end);
Rather bail out if something is wrong?
To view, visit change 35726. To unsubscribe, or for help writing mail filters, visit settings.