Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41943 )
Change subject: sb/intel/lynxpoint: Clean up code ......................................................................
Patch Set 6:
(7 comments)
Patch Set 6:
(4 comments)
Sorry, there was one thing I do care about: 96 column limit is for code and can hurt readability of text.
Thanks for letting me know.
https://review.coreboot.org/c/coreboot/+/41943/6/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/chip.h:
https://review.coreboot.org/c/coreboot/+/41943/6/src/southbridge/intel/lynxp... PS6, Line 108: #endif /* SOUTHBRIDGE_INTEL_LYNXPOINT_CHIP_H */
Nit, it was indented on purpose.
Which purpose?
https://review.coreboot.org/c/coreboot/+/41943/6/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/early_usb.c:
https://review.coreboot.org/c/coreboot/+/41943/6/src/southbridge/intel/lynxp... PS6, Line 24: */
Seems easier to read.
Will change back
https://review.coreboot.org/c/coreboot/+/41943/6/src/southbridge/intel/lynxp... PS6, Line 27: pci_or_config16(dev, PCI_COMMAND, PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY);
Off-topic; check if there is errata about _MASTER being compulsory for _MEMORY to take effect on som […]
I can check if this is required
https://review.coreboot.org/c/coreboot/+/41943/6/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/elog.c:
https://review.coreboot.org/c/coreboot/+/41943/6/src/southbridge/intel/lynxp... PS6, Line 33: u32 gpe0_en = inl(get_pmbase() + gpe0_en_reg);
This even (space) looks odd to me. Pun intended.
I'll drop it
https://review.coreboot.org/c/coreboot/+/41943/6/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/41943/6/src/southbridge/intel/lynxp... PS6, Line 148: u32 viddid, const u32 **verb)
Looks more balanced.
Will change back
https://review.coreboot.org/c/coreboot/+/41943/6/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/lp_gpio.h:
https://review.coreboot.org/c/coreboot/+/41943/6/src/southbridge/intel/lynxp... PS6, Line 155: /* Get GPIO pin value */
Is this and the comment above worth a period at the end?
I was asked in other changes to remove trailing periods on single-line comments. In this case, the comments did not have a period to begin with, so I'd rather leave them as-is.
Another trick would be to turn these comments into multiline doxygen comments, which would then appear here: https://doxygen.coreboot.org/
https://review.coreboot.org/c/coreboot/+/41943/6/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/lpc.c:
https://review.coreboot.org/c/coreboot/+/41943/6/src/southbridge/intel/lynxp... PS6, Line 464: RCBA32_AND_OR(0x333c, 0xffcfffff, 0x00c00000); // SATA : : RCBA32_OR(0x38c0, 0x3c07); // SPI Dynamic :
Not convinced this makes it better when there is nothing to align to.
Will change back