Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35726 )
Change subject: drivers/aspeed/common: Add support for high resolution framebuffer ......................................................................
Patch Set 6:
(7 comments)
https://review.coreboot.org/c/coreboot/+/35726/5/src/drivers/aspeed/common/a... File src/drivers/aspeed/common/ast_i2c.c:
https://review.coreboot.org/c/coreboot/+/35726/5/src/drivers/aspeed/common/a... PS5, Line 123: 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?
https://review.coreboot.org/c/coreboot/+/35726/5/src/drivers/aspeed/common/a... PS5, 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.
https://review.coreboot.org/c/coreboot/+/35726/6/src/drivers/aspeed/common/a... File src/drivers/aspeed/common/ast_mode_corebootfb.c:
https://review.coreboot.org/c/coreboot/+/35726/6/src/drivers/aspeed/common/a... PS6, Line 40: expexts expects
https://review.coreboot.org/c/coreboot/+/35726/6/src/drivers/aspeed/common/a... PS6, 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?
https://review.coreboot.org/c/coreboot/+/35726/6/src/drivers/aspeed/common/a... PS6, 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.
https://review.coreboot.org/c/coreboot/+/35726/6/src/drivers/aspeed/common/a... PS6, Line 82: mode->crtc_hblank_end = edid->mode.ha + edid->mode.hso + edid->mode.hbl; This should be just active + blank?
https://review.coreboot.org/c/coreboot/+/35726/6/src/drivers/aspeed/common/a... PS6, Line 85: mode->crtc_htotal = MAX(mode->crtc_hblank_end, mode->crtc_hsync_end); Rather bail out if something is wrong?