Attention is currently required from: Alexander Couzens, Nicolas Provost.
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75992?usp=email )
Change subject: new port for Lenovo L420 laptop ......................................................................
Patch Set 4:
(14 comments)
Patchset:
PS4: Hi Nicolas. Thanks for your contribution and welcome to coreboot! 😊
Looks pretty good from the first look. I suggested some clean ups.
File src/mainboard/lenovo/l420/Kconfig:
https://review.coreboot.org/c/coreboot/+/75992/comment/4fcfa03b_9ba88927 : PS4, Line 24: # Workaround for EC/KBC. remove the empty line and this comment
https://review.coreboot.org/c/coreboot/+/75992/comment/7ba87d71_6c4c6b66 : PS4, Line 27: # Realtek 8168 remove the empty line and this comment
https://review.coreboot.org/c/coreboot/+/75992/comment/9a525310_06e0f965 : PS4, Line 55: int no need to redefine the type, please remove
https://review.coreboot.org/c/coreboot/+/75992/comment/ab2bc0d4_4fe487c0 : PS4, Line 59: int no need to redefine the type, please remove
https://review.coreboot.org/c/coreboot/+/75992/comment/025da419_861673cf : PS4, Line 63: string no need to redefine the type, please remove
https://review.coreboot.org/c/coreboot/+/75992/comment/598978b6_67f1e484 : PS4, Line 75: config WARNINGS_ARE_ERRORS : default n : : config PAYLOAD_CONFIGFILE : default These shouldn't be in the mainboard Kconfig, please remove.
File src/mainboard/lenovo/l420/board_info.txt:
https://review.coreboot.org/c/coreboot/+/75992/comment/d1285b42_e6dd4597 : PS4, Line 5: Flashrom support: n probably y?
File src/mainboard/lenovo/l420/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/75992/comment/440a4750_8467a6b1 : PS4, Line 29: # IGD Displays (spread spectrum on) : register "gfx" = "GMA_STATIC_DISPLAYS(1)" : : # Enable DisplayPort Hotplug with 6ms pulse : register "gpu_dp_d_hotplug" = "0x06" : : # Enable Panel as LVDS and configure power delays : register "gpu_panel_port_select" = "PANEL_PORT_LVDS" : register "gpu_panel_power_cycle_delay" = "6" : register "gpu_panel_power_up_delay" = "300" # T1+T2: 30ms : register "gpu_panel_power_down_delay" = "300" # T5+T6: 30ms : register "gpu_panel_power_backlight_on_delay" = "2500" # T3: 200ms : register "gpu_panel_power_backlight_off_delay" = "2500" # T4: 200ms : register "gpu_cpu_backlight" = "0x1155" : r Move these under the IGD controller
https://review.coreboot.org/c/coreboot/+/75992/comment/73834483_1c052463 : PS4, Line 45: device domain 0 on Sandybridge has a chipset device tree (at src/northbridge/intel/sandybridge/chipset.cb) with alias names for the PCI devices. Please use the names for your configuration, which allows to remove most of the comments like "Integrated Graphics Controller" since the names are self explaining.
PCI devices which are turned off can simply be left out.
Example usage for 02.0: ``` device ref igd on end ```
File src/mainboard/lenovo/l420/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/75992/comment/b117fbb4_70a82205 : PS4, Line 24: // global NVS and variables seems superfluous, remove
https://review.coreboot.org/c/coreboot/+/75992/comment/35fbb98b_6b5597ec : PS4, Line 28: Scope (_SB) { : Device (PCI0) : { : #include <northbridge/intel/sandybridge/acpi/sandybridge.asl> : #include <southbridge/intel/bd82x6x/acpi/pch.asl> : #include <drivers/intel/gma/acpi/default_brightness_levels.asl> : } : } ``` Device (_SB.PCI0) { ... } ```
File src/mainboard/lenovo/l420/mainboard.c:
https://review.coreboot.org/c/coreboot/+/75992/comment/c46e7ac3_837afb27 : PS4, Line 9: // mainboard_enable is executed as first thing after : // enumerate_buses(). seems superfluous, remove
File src/mainboard/lenovo/l420/smihandler.c:
https://review.coreboot.org/c/coreboot/+/75992/comment/7510ab47_92acaacd : PS4, Line 13: /* FIXME: check this */ checked?