Name of user not set #1003058 has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44302 )
Change subject: mb/lenovo/thinkcentre_m91p add ThinkCentre M91p ......................................................................
Patch Set 4:
(11 comments)
https://review.coreboot.org/c/coreboot/+/44302/1/src/mainboard/lenovo/thinkc... File src/mainboard/lenovo/thinkcentre_m91p/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/44302/1/src/mainboard/lenovo/thinkc... PS1, Line 9: /* The lid is open by default. */ : gnvs->lids = 1;
Which lid? This isn't a laptop, so this can be dropped. […]
Done
https://review.coreboot.org/c/coreboot/+/44302/1/src/mainboard/lenovo/thinkc... File src/mainboard/lenovo/thinkcentre_m91p/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/44302/1/src/mainboard/lenovo/thinkc... PS1, Line 1: # FIXME: GPU registers may not always apply. : register "gfx" = "GMA_STATIC_DISPLAYS(0)" : register "gpu_cpu_backlight" = "0x00000000" : register "gpu_dp_b_hotplug" = "0" : register "gpu_dp_c_hotplug" = "0" : register "gpu_dp_d_hotplug" = "0" : register "gpu_panel_port_select" = "0" : register "gpu_panel_power_backlight_off_delay" = "0" : register "gpu_panel_power_backlight_on_delay" = "0" : register "gpu_panel_power_cycle_delay" = "0" : register "gpu_panel_power_down_delay" = "0" : register "gpu_panel_power_up_delay" = "0" : register "gpu_pch_backlight" = "0x00000000"
This can be dropped completely (starting from the FIXME up until `gpu_pch_backlight` (inclusive).
Done
https://review.coreboot.org/c/coreboot/+/44302/1/src/mainboard/lenovo/thinkc... PS1, Line 23: end
nit: I usually `pick up` these lone `end` keywords and put them on the same line if there's nothing […]
Done
https://review.coreboot.org/c/coreboot/+/44302/1/src/mainboard/lenovo/thinkc... PS1, Line 31: register "docking_supported" = "0"
Any devicetree field that is zero and does not provide any useful info can be removed. […]
Done
https://review.coreboot.org/c/coreboot/+/44302/1/src/mainboard/lenovo/thinkc... PS1, Line 33: register "gen2_dec" = "0x00000000" : register "gen3_dec" = "0x00000000" : register "gen4_dec" = "0x00000000" : register "pcie_hotplug_map" = "{ 0, 0, 0, 0, 0, 0, 0, 0 }"
Also zero, can be dropped
Done
https://review.coreboot.org/c/coreboot/+/44302/1/src/mainboard/lenovo/thinkc... PS1, Line 60: device pci 1c.0 off # PCIe Port #1 : end : device pci 1c.1 off # PCIe Port #2 : end : device pci 1c.2 off # PCIe Port #3 : end : device pci 1c.3 off # PCIe Port #4 : end : device pci 1c.4 off # PCIe Port #5 : end : device pci 1c.5 off # PCIe Port #6 : end : device pci 1c.6 off # PCIe Port #7 : end : device pci 1c.7 off # PCIe Port #8 : end
this is weird.
Done
https://review.coreboot.org/c/coreboot/+/44302/1/src/mainboard/lenovo/thinkc... PS1, Line 96: Host bridge Host bridge
One `Host bridge` is enough, isn't it? 😄
Done
https://review.coreboot.org/c/coreboot/+/44302/1/src/mainboard/lenovo/thinkc... PS1, Line 96: device pci 00.0 on # Host bridge Host bridge : subsystemid 0x17aa 0x3070 : end : device pci 01.0 off # PEG : end : device pci 02.0 on # iGPU : subsystemid 0x17aa 0x3070 : end
These devices correspond to northbridge functions. […]
Done
https://review.coreboot.org/c/coreboot/+/44302/1/src/mainboard/lenovo/thinkc... File src/mainboard/lenovo/thinkcentre_m91p/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/44302/1/src/mainboard/lenovo/thinkc... PS1, Line 1: #define BRIGHTNESS_UP _SB.PCI0.GFX0.INCB : #define BRIGHTNESS_DOWN _SB.PCI0.GFX0.DECB
These definitions are unused and can be removed.
Done
https://review.coreboot.org/c/coreboot/+/44302/1/src/mainboard/lenovo/thinkc... PS1, Line 25: #include <drivers/intel/gma/acpi/default_brightness_levels.asl>
No backlight on this board, so this isn't necessary.
Done
https://review.coreboot.org/c/coreboot/+/44302/1/src/mainboard/lenovo/thinkc... File src/mainboard/lenovo/thinkcentre_m91p/early_init.c:
https://review.coreboot.org/c/coreboot/+/44302/1/src/mainboard/lenovo/thinkc... PS1, Line 3:
nit: double blank line
Done