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 -- To view, visit https://review.coreboot.org/c/coreboot/+/48771 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I746798b13e83d1971bb7be93751a15875d16c337 Gerrit-Change-Number: 48771 Gerrit-PatchSet: 2 Gerrit-Owner: Bill XIE <persmule@hardenedlinux.org> Gerrit-Reviewer: Alexander Couzens <lynxis@fe80.eu> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Angel Pons <th3fanbus@gmail.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Attention: Bill XIE <persmule@hardenedlinux.org> Gerrit-Comment-Date: Thu, 25 Feb 2021 15:16:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment