[coreboot-gerrit] Change in coreboot[master]: soc/intel/cannonlake: Steal no memory for disabled IGD

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )

Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD
......................................................................

Patch Set 13:

(5 comments)

https://review.coreboot.org/c/coreboot/+/39454/13//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/39454/13//COMMIT_MSG@9 
PS13, Line 9: It is
nit: use `it's`  so that this line is under 72 chars

https://review.coreboot.org/c/coreboot/+/39454/13//COMMIT_MSG@13 
PS13, Line 13: of
ofF

https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... 
File src/soc/intel/cannonlake/romstage/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... 
PS13, Line 42: dev
> Why? We should only do this if dev. If dev is not enabled anyways we do need to read this.

The `||` operator is short-circuiting. If `dev` is null, the first check for `!dev` will succeed and the block of code will be run. If the check is false, it means `dev` is true, so it is redundant.

https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... 
PS13, Line 42: pci_read_config16(SA_DEV_IGD, PCI_VENDOR_ID) == 0xffff))
> If dev, but there is no IGD (Read from PCI_VENDOR_ID == 0xffff) it should be disabled, otherwise it  […]

Ack

https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... 
PS13, Line 98: dev
> > "dev" was declared const above... […]

Ack, forgot my own rule to understand const on pointers... Silly me.