[coreboot-gerrit] Change in coreboot[master]: soc/intel/cannonlake: Make use of is_devfn_enabled() function

[Angel Pons (Code Review)]

Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Subrata Banik, Patrick Rudolph.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55328 )

Change subject: soc/intel/cannonlake: Make use of is_devfn_enabled() function
......................................................................

Patch Set 1: Code-Review+1

(4 comments)

File src/soc/intel/cannonlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/55328/comment/7167bfa1_bc83f30d 
PS1, Line 315: 			dev->enabled = 0;
This also disabled the device in the devicetree

https://review.coreboot.org/c/coreboot/+/55328/comment/c509c559_62ddbee7 
PS1, Line 150: 	if (params->SataEnable) {
Strictly speaking, this changes behavior when the device exists in the devicetree but is disabled:

Before: All SATA-related UPDs would be set.
 After:  Only SataEnable would be set.

I don't think it's a big deal though.

https://review.coreboot.org/c/coreboot/+/55328/comment/dc5b2f8d_ef816617 
PS1, Line 173: if (params->PchLanEnable) {
             : 		if (config->s0ix_enable && params->PchLanEnable) {
Hmmm, do you notice anything weird here? A redundant check, maybe? 😉

if (params->PchLanEnable) {
    	if (config->s0ix_enable) {

I'd still keep the two nested if-blocks, for consistency with the other devices.

https://review.coreboot.org/c/coreboot/+/55328/comment/6978f183_0d2e57a5 
PS1, Line 481: if (CONFIG(RUN_FSP_GOP) && is_devfn_enabled(SA_DEVFN_IGD))
             : 		params->PeiGraphicsPeimInit = 1;
             : 	else
             : 		params->PeiGraphicsPeimInit = 0;
nit: This could be a single statement, but it would be longer than 96 characters