Attention is currently required from: Bill XIE. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48771 )
Change subject: mainboard/lenovo: Add Thinkpad Edge S220 (a.k.a E220s) ......................................................................
Patch Set 2:
(29 comments)
File Documentation/mainboard/lenovo/s220.md:
https://review.coreboot.org/c/coreboot/+/48771/comment/c58bfa17_78a49077 PS2, Line 37: Two empty lines
File Documentation/mainboard/lenovo/s220_ready_to_flash.jpg:
PS2: Please reduce the size of this image. I'd suggest cropping it to only show the area around the flash chip: https://imgur.com/Ehqn38w.png
File src/mainboard/hp/snb_ivb_laptops/cmos.default:
https://review.coreboot.org/c/coreboot/+/48771/comment/837585a3_e568ae37 PS2, Line 5: volume=0x3 Seems unrelated?
File src/mainboard/lenovo/s220/Kconfig:
https://review.coreboot.org/c/coreboot/+/48771/comment/6578537f_af9eff57 PS2, Line 9: C216 Schematics say Cougar Point (aka BD82X6X)
https://review.coreboot.org/c/coreboot/+/48771/comment/eddbe0ce_c97822a2 PS2, Line 43: default 8 No longer necessary
https://review.coreboot.org/c/coreboot/+/48771/comment/4bc67af8_33553b79 PS2, Line 51: default 0xff000000 Is this reserved anywhere?
File src/mainboard/lenovo/s220/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/48771/comment/41eb64fe_28d77537 PS2, Line 17: // Nit: use consistent comment style?
https://review.coreboot.org/c/coreboot/+/48771/comment/4dd7be21_a21c7789 PS2, Line 130: Store (One, PWRS) Please use ASL 2.0 syntax:
PWRS = 1
Applies to all .asl files.
https://review.coreboot.org/c/coreboot/+/48771/comment/d025ed50_8310df01 PS2, Line 244: * Remove this asterisk to match the comment style in the guidelines
File src/mainboard/lenovo/s220/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/48771/comment/f2b7e468_91d2f2ec PS2, Line 2: /* This file is part of the coreboot project. */ Please drop this comment
https://review.coreboot.org/c/coreboot/+/48771/comment/0e1b22aa_9b716302 PS2, Line 5: #include <southbridge/intel/bd82x6x/nvs.h> Needs an update
File src/mainboard/lenovo/s220/board_info.txt:
https://review.coreboot.org/c/coreboot/+/48771/comment/c8594f62_ead6d25e PS2, Line 4: FIXME FIXME
File src/mainboard/lenovo/s220/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/48771/comment/389b756c_5e675951 PS2, Line 6: register "gpu_dp_d_hotplug" = "4" DP is not used?
https://review.coreboot.org/c/coreboot/+/48771/comment/48223f4a_1a1eff09 PS2, Line 21: register "c3_battery" = "5" Needs an update
https://review.coreboot.org/c/coreboot/+/48771/comment/0c006051_c4440ba3 PS2, Line 30: Host bridge Host bridge One "Host bridge" is enough
https://review.coreboot.org/c/coreboot/+/48771/comment/4700ce4d_c517991b PS2, Line 32: end nit: please align the `end` keywords
https://review.coreboot.org/c/coreboot/+/48771/comment/d1e46bd1_8fa45b3b PS2, Line 36: register "gen1_dec" = "0x00000000" Zero, please drop
File src/mainboard/lenovo/s220/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/48771/comment/db8725c2_70c52828 PS2, Line 8: One blank line is enough
https://review.coreboot.org/c/coreboot/+/48771/comment/df2d2558_c74eb39b PS2, Line 19: { Missing #include <acpi/dsdt_top.asl>
File src/mainboard/lenovo/s220/early_init.c:
https://review.coreboot.org/c/coreboot/+/48771/comment/5a127056_299b812a PS2, Line 2: /* This file is part of the coreboot project. */ Drop this comment
https://review.coreboot.org/c/coreboot/+/48771/comment/0b3dc169_4219db4c PS2, Line 4: FIXME Should be easy to fix
https://review.coreboot.org/c/coreboot/+/48771/comment/63dcf15c_2295f0c8 PS2, Line 42: CONFIG_EC_BASE_ADDRESS | 1); You have to reserve this memory range somewhere.
https://review.coreboot.org/c/coreboot/+/48771/comment/014ea40b_cdbeb73c PS2, Line 50: FIXME Hm?
File src/mainboard/lenovo/s220/ec.c:
https://review.coreboot.org/c/coreboot/+/48771/comment/4237d0a1_0b7ed541 PS2, Line 20: s230u s220
File src/mainboard/lenovo/s220/gma-mainboard.ads:
https://review.coreboot.org/c/coreboot/+/48771/comment/04e9e2c3_884152c0 PS2, Line 2: -- This file is part of the coreboot project. Please drop
File src/mainboard/lenovo/s220/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/48771/comment/c2476dda_09f0ace4 PS2, Line 2: /* This file is part of the coreboot project. */ Drop
File src/mainboard/lenovo/s220/mainboard.c:
https://review.coreboot.org/c/coreboot/+/48771/comment/bcea5293_0a7bdceb PS2, Line 63: lenovo_s230u_ec_init hm?
File src/mainboard/lenovo/s220/smihandler.c:
https://review.coreboot.org/c/coreboot/+/48771/comment/bf7e1137_c612e21f PS2, Line 18: enum sleep_states { src/include/acpi/acpi.h
https://review.coreboot.org/c/coreboot/+/48771/comment/65a9a17b_d941ccf5 PS2, Line 76: BIOS_INFO _INFO? I'd say _DEBUG or _SPEW