Change in coreboot[master]: src/soc/intel/commmon/itss: Add support to get interrupt table for PC...
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34089 )
Change subject: src/soc/intel/commmon/itss: Add support to get interrupt table for PCI devices ......................................................................
Patch Set 7:
(11 comments)
https://review.coreboot.org/c/coreboot/+/34089/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34089/7//COMMIT_MSG@7 PS7, Line 7: commmon Nit: Typo error
https://review.coreboot.org/c/coreboot/+/34089/7/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/irq.h:
https://review.coreboot.org/c/coreboot/+/34089/7/src/soc/intel/common/block/... PS7, Line 37: get_soc_irq_devices Prefer to have SoC specific function names with the format soc_*
https://review.coreboot.org/c/coreboot/+/34089/7/src/soc/intel/common/block/... File src/soc/intel/common/block/itss/irq.c:
https://review.coreboot.org/c/coreboot/+/34089/7/src/soc/intel/common/block/... PS7, Line 34: struct irq_node *node; Please leave a blank line between the declaration block and function body.
https://review.coreboot.org/c/coreboot/+/34089/7/src/soc/intel/common/block/... PS7, Line 35: node = (struct irq_node *)malloc(sizeof(struct irq_node)); Nit: Can we not use calloc and just return (struct irq_node *)calloc(1, sizeof(struct irq_node));
Then this whole function is just one line.
https://review.coreboot.org/c/coreboot/+/34089/7/src/soc/intel/common/block/... PS7, Line 44: struct irq_node *p; Please leave a blank line between the declaration block and function body.
https://review.coreboot.org/c/coreboot/+/34089/7/src/soc/intel/common/block/... PS7, Line 45: temp = create_device_node(); malloc & calloc can return NULL unless it dies internally if the dynamic allocation fails. I am not sure about the behavior. If it does not die, please check the validity of the buffer before accessing it.
https://review.coreboot.org/c/coreboot/+/34089/7/src/soc/intel/common/block/... PS7, Line 50: if (dev == NULL) It is better to have a brace around the if block, if the else block has a brace(better for readability).
https://review.coreboot.org/c/coreboot/+/34089/7/src/soc/intel/common/block/... PS7, Line 77: } else Same comment regarding brace as earlier.
https://review.coreboot.org/c/coreboot/+/34089/7/src/soc/intel/common/block/... PS7, Line 94: static const int *devlist; Please leave a blank line
https://review.coreboot.org/c/coreboot/+/34089/7/src/soc/intel/common/block/... PS7, Line 102: } Please leave a blank line between 2 function definitions.
https://review.coreboot.org/c/coreboot/+/34089/7/src/soc/intel/common/block/... PS7, Line 114: /* : * Add device IRQ information following below rules: : * : * 1. For single function PCI device capable of generating : * interrupt, use INTA(IRQ16) : * 2. For multi function PCI device capable of generating : * interrupt, alteast one should use INTA(IRQ16) : * 3. LPSS controllers need to be assigned unique IRQs : */ Add this as a function header above the function definition.
-
Karthik Ramasubramanian (Code Review)