Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32692 )
Change subject: drivers/i2c: Add LP5562 LED driver ......................................................................
Patch Set 3:
(6 comments)
https://review.coreboot.org/c/coreboot/+/32692/3/src/drivers/i2c/Kconfig File src/drivers/i2c/Kconfig:
https://review.coreboot.org/c/coreboot/+/32692/3/src/drivers/i2c/Kconfig@1 PS3, Line 1: source src/drivers/i2c/adm1026/Kconfig 1) What does this have to do with adding the LP5562 driver? 2) You can replace all of this with a single line:
source src/drivers/i2c/*/Kconfig
https://review.coreboot.org/c/coreboot/+/32692/3/src/drivers/i2c/lp5562/led_... File src/drivers/i2c/lp5562/led_lp5562.h:
https://review.coreboot.org/c/coreboot/+/32692/3/src/drivers/i2c/lp5562/led_... PS3, Line 33: int led_lp5562_display_pattern(unsigned i2c_bus, enum display_pattern pattern);
Prefer 'unsigned int' to bare use of 'unsigned'
As checkpatch says, please don't use just "unsigned", use "unsigned int"
https://review.coreboot.org/c/coreboot/+/32692/3/src/drivers/i2c/lp5562/led_... File src/drivers/i2c/lp5562/led_lp5562.c:
https://review.coreboot.org/c/coreboot/+/32692/3/src/drivers/i2c/lp5562/led_... PS3, Line 35: /* Key lp5562 registers. */ Put all of the #defines into the header file?
https://review.coreboot.org/c/coreboot/+/32692/3/src/drivers/i2c/lp5562/led_... PS3, Line 127: TiLp5562 No camelcase please.
https://review.coreboot.org/c/coreboot/+/32692/3/src/drivers/i2c/lp5562/led_... File src/drivers/i2c/lp5562/led_lp5562_programs.h:
https://review.coreboot.org/c/coreboot/+/32692/3/src/drivers/i2c/lp5562/led_... PS3, Line 28: THIRD_PARTY_COREBOOT_SRC We really only need enough to make it unique, this might be overkill
https://review.coreboot.org/c/coreboot/+/32692/3/src/drivers/i2c/lp5562/led_... PS3, Line 60: Led5562StateProg Again, no camelcase, here and above