Attention is currently required from: Michael Büchler.
Patch set 6:Code-Review +1
16 comments:
Patchset:
I added documentation and a way to configure the fan header for CPUFANIN (see comment below). […]
I think I suggested this change because Windows hates NCT6776 ACPI
Just some minor things here and there, and this will be ready to go!
File Documentation/mainboard/asrock/h77pro4-m.md:
Patch Set #6, Line 24: header
I'd mention it's RS-232 (and not TTL UART)
Patch Set #6, Line 45: SATA at 6 Gb/s
Doesn't this repeat part of what the two preceding lines say? I'd remove it
Patch Set #6, Line 147: work, check if your bracket expects a different assignment.
I'm worried about people connecting things that doesn't use RS-232 voltages to this header, and frying them. But I wouldn't expect this kind of people to read warnings anyway.
File src/mainboard/asrock/h77pro4-m/Kconfig:
Patch Set #6, Line 22: string
Type no longer needed, see CB:56553
Patch Set #6, Line 26: string
Type no longer needed, see CB:56554
File src/mainboard/asrock/h77pro4-m/cmos.layout:
Patch Set #6, Line 28: cpu_fan_header
Since this option is about the tachometer signal source, how about naming it `cpu_fan_tach_src` instead?
File src/mainboard/asrock/h77pro4-m/devicetree.cb:
Patch Set #6, Line 75: # parallel port
Comment seems redundant
File src/mainboard/asrock/h77pro4-m/dsdt.asl:
Patch Set #6, Line 10: /* OEM revision */
This comment comes from autoport, right? I'd remove it.
File src/mainboard/asrock/h77pro4-m/early_init.c:
/* Configure GPIO7 */
pnp_set_logical_device(GPIO6789_DEV);
pnp_write_config(GPIO6789_DEV, 0xe0, 0x3f);
pnp_write_config(GPIO6789_DEV, 0xe1, 0x7f);
I copied the CMOS option tables from mb/asus/p8z77-v_lx2 and looked at mb/kontron/986lcd-m/mainboard […]
Sounds great!
File src/mainboard/asrock/h77pro4-m/early_init.c:
#include <bootblock_common.h>
#include <device/pci_ops.h>
#include <device/pnp_ops.h>
#include <northbridge/intel/sandybridge/raminit_native.h>
#include <southbridge/intel/bd82x6x/pch.h>
#include <superio/nuvoton/nct6776/nct6776.h>
#include <superio/nuvoton/common/nuvoton.h>
#include <option.h>
nit: maybe order includes alphabetically?
#define CPU_FAN_HEADER_NONE 0
#define CPU_FAN_HEADER_1 1
#define CPU_FAN_HEADER_2 2
#define CPU_FAN_HEADER_BOTH 3
nit: Use an enum?
#include <stdint.h>
cpu_fan_header = get_uint_option("cpu_fan_header", CPU_FAN_HEADER_1);
switch (cpu_fan_header) {
case CPU_FAN_HEADER_NONE:
reg = 0xff;
break;
case CPU_FAN_HEADER_1:
reg = 0x7f;
break;
case CPU_FAN_HEADER_2:
reg = 0xbf;
break;
case CPU_FAN_HEADER_BOTH:
reg = 0x3f;
break;
}
Idea: place the switch/case statement in a helper function, so that you can replace the assignments and break statements with one return statement:
static u8 get_cpu_fan_header(void)
{
switch (get_uint_option("cpu_fan_header", CPU_FAN_HEADER_1)) {
case CPU_FAN_HEADER_NONE:
return 0xff;
case CPU_FAN_HEADER_1:
default:
return 0x7f;
case CPU_FAN_HEADER_2:
return 0xbf;
case CPU_FAN_HEADER_BOTH:
return 0x3f;
}
}
Note that I added a `default:` case. This is to handle the unusual case where `get_uint_option()` returns something other than the defined values (which could only happen if something goes very wrong).
Feel free to pick a different function name. I'm not very inspired today 😄
File src/mainboard/asrock/h77pro4-m/hda_verb.c:
nit: drop blank line?
To view, visit change 45317. To unsubscribe, or for help writing mail filters, visit settings.