Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31427 )
Change subject: superio/nsc: Use common defines for NSC PC8xxxx chips ......................................................................
Patch Set 1:
(3 comments)
only had a brief look and probably missed some stuff, but I wonder if it would be better to move the definitions that are common for multiple SIOs in tree to a common file, keep the device-specific files and include the common header in the specific headers and keep using the specific headers in the mainboards
https://review.coreboot.org/#/c/31427/1/src/superio/nsc/common/superio_defs.... File src/superio/nsc/common/superio_defs.h:
https://review.coreboot.org/#/c/31427/1/src/superio/nsc/common/superio_defs.... PS1, Line 34: FSD HWM maybe? at least on SIOs from other vendors that function block is often called HWM
https://review.coreboot.org/#/c/31427/1/src/superio/nsc/common/superio_defs.... PS1, Line 34: #define PC8XXXX_FSD 0x08 /* Fan speed device */ : #define PC8XXXX_ACB 0x08 : #define PC8XXXX_FSCM 0x09 : #define PC8XXXX_FMC 0x09 leave different LDNs with same LDN number in the SIO-specific header iles and include the common definition header file in those header files?
https://review.coreboot.org/#/c/31427/1/src/superio/nsc/pc87417/pc87417.h File src/superio/nsc/pc87417/pc87417.h:
https://review.coreboot.org/#/c/31427/1/src/superio/nsc/pc87417/pc87417.h@a3... PS1, Line 30: this is the only device with this LDN in the tree, so leave this definition in this header file and include the common header file here?