Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45372 )
Change subject: ec/kontron/kempld: Fix code style ......................................................................
Patch Set 2:
(16 comments)
https://review.coreboot.org/c/coreboot/+/45372/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45372/2//COMMIT_MSG@7 PS2, Line 7: Fix I'm afraid that this isn't the case here.
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/chip.... File src/ec/kontron/kempld/chip.h:
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/chip.... PS2, Line 6: #define KEMPLD_NUM_UARTS 2 I doubt using a tab here provides any visual benefit.
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/early... File src/ec/kontron/kempld/early_kempld.c:
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/early... PS2, Line 6: This newline is intentional, since both types of headers are different.
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/early... PS2, Line 45: KEMPLD_UART_ENABLE | KEMPLD_UART_3F8 << KEMPLD_UART_IO_SHIFT); Please keep the line aligned:
kempld_write8(KEMPLD_UART_0, KEMPLD_UART_ENABLE | KEMPLD_UART_3F8 << KEMPLD_UART_IO_SHIFT);
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/early... PS2, Line 49: KEMPLD_UART_ENABLE | KEMPLD_UART_2F8 << KEMPLD_UART_IO_SHIFT); Same here
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... File src/ec/kontron/kempld/kempld.c:
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 5: Same as before
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 13: This blank line separates fixed constants from dev-dependent constants.
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 15: This one separates regular variable assignments from function calls.
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 58: const unsigned int uart = dev->path.generic.subid; I'd rather keep these where they were before. If we can't get the mutex, we don't need these.
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... File src/ec/kontron/kempld/kempld_i2c.c:
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 7: */ Why is this gone?
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 15: Same as before
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 40: #define EBUSY 16 What does this fix? With the spaces, the numbers were aligned: ones with ones, tens with tens...
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 66: }; This was aligned with tabs so that the difference between types and member names was clearer.
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 225: .ops_i2c_bus = &kempld_i2c_bus_ops, Why break the alignment?
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... File src/ec/kontron/kempld/kempld_internal.h:
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 11: #define KEMPLD_DAT 0xa81 These aren't indexed registers, so there's no need to align them with those.
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 29: #define KEMPLD_CLK 33333333 /* 33MHz */ I don't think this should be aligned with the indexed registers either.