Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45372 )
Change subject: ec/kontron/kempld: Reflow long lines ......................................................................
Patch Set 4:
(15 comments)
Thanks for the review!
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.
I reworked this patch according to your comments.
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.
Ack
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.
Ack
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: […]
Done
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
Done
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
Ack
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.
Ack
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.
Ack
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.
Ack
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?
canceled
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 15:
Same as before
canceled
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...
canceled
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.
canceled
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?
canceled
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.
Ack