Attention is currently required from: Paul Menzel, cong yang.
Ruihai Zhou has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74051 )
Change subject: mb/google/corsola: Add support for mipi panel ......................................................................
Patch Set 12:
(9 comments)
This change is ready for review.
Commit Message:
https://review.coreboot.org/c/coreboot/+/74051/comment/3ba3caeb_76043174 PS11, Line 12:
Please mention the datasheet name and revision.
Done
File src/mainboard/google/corsola/display.c:
https://review.coreboot.org/c/coreboot/+/74051/comment/80c1c40b_0d6123a9 PS11, Line 122: u32
unsigned int […]
Done
https://review.coreboot.org/c/coreboot/+/74051/comment/76fb8bd5_90bdc95d PS11, Line 124: u8 msg = 0;
Why initialize it to 0?
Just want to be consistent with other users
https://review.coreboot.org/c/coreboot/+/74051/comment/c0b2dbfc_7b00f322 PS11, Line 127: printk(BIOS_ERR, "Failed to read i2c: addr(%u)\n", addr);
Please use a more elaborate error message, so (normal) users know what to do.
add the function name and bus, do you think this is okay?
https://review.coreboot.org/c/coreboot/+/74051/comment/05c1e0cf_ff6652f6 PS11, Line 139: u8 value = 0; : u8 value1 = 0;
Why initialize it to 0?
Just want to be consistent with other users
https://review.coreboot.org/c/coreboot/+/74051/comment/396faf54_8813aec9 PS11, Line 156: Just set
Set
Done
https://review.coreboot.org/c/coreboot/+/74051/comment/153635c0_bd022df0 PS11, Line 172: mdelay(50);
Where is that long delay documented?
TPS65132-Single-Inductor-Dual-Output-Power-Supply.pdf pages 18 8.6 Register Maps.
This delay is only called when the board is turned on for the first time, so I think it should be fine.
File src/mainboard/google/corsola/panel_starmie.c:
https://review.coreboot.org/c/coreboot/+/74051/comment/32a30c0c_d9ba654a PS11, Line 31: },
Should } be aligned differently?
Done
https://review.coreboot.org/c/coreboot/+/74051/comment/e84739c1_acdaecb3 PS11, Line 36: uint32_t id = panel_id() & 0xF;
Why limit the size?
We use only one ADC channel for the panel id, so only need 4bit