Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32692 )
Change subject: drivers/i2c: Add LP5562 LED driver ......................................................................
Patch Set 11:
(32 comments)
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... File src/drivers/i2c/lp5562/led_lp5562.h:
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... PS9, Line 33: int led_lp5562_display_pattern(unsigned i2c_bus, enum display_pattern pattern);
Prefer 'unsigned int' to bare use of 'unsigned'
Done
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... File src/drivers/i2c/lp5562/led_lp5562.c:
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... PS9, Line 110: * The controller has 96 bytes of SRAM for code/data, availabe as three 32 byte
'availabe' may be misspelled - perhaps 'available'?
Done
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... PS9, Line 124: unsigned i2c_bus;
Prefer 'unsigned int' to bare use of 'unsigned'
Done
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... PS9, Line 129: static void led_lp5562_init(unsigned i2c_bus);
Prefer 'unsigned int' to bare use of 'unsigned'
Done
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... PS9, Line 154: /* Accessing reset regsiter is expected to fail. */
'regsiter' may be misspelled - perhaps 'register'?
Done
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... PS9, Line 177: const uint8_t *data, unsigned count)
Prefer 'unsigned int' to bare use of 'unsigned'
Done
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... PS9, Line 254: const uint8_t *program, unsigned count)
Prefer 'unsigned int' to bare use of 'unsigned'
Done
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... PS9, Line 288: enable_reg &= ~LP5562_ENABLE_ALL_MASK;
please, no spaces at the start of a line
Done
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... PS9, Line 288: enable_reg &= ~LP5562_ENABLE_ALL_MASK;
code indent should use tabs where possible
Done
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... PS9, Line 289: enable_reg |= LP5562_ENABLE_CHIP_EN;
please, no spaces at the start of a line
Done
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... PS9, Line 289: enable_reg |= LP5562_ENABLE_CHIP_EN;
code indent should use tabs where possible
Done
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... PS9, Line 290: enable_reg |= LP5562_ENABLE_ALL_HOLD;
code indent should use tabs where possible
Done
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... PS9, Line 290: enable_reg |= LP5562_ENABLE_ALL_HOLD;
please, no spaces at the start of a line
Done
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... PS9, Line 293: /* All engines in LOAD mode. */
code indent should use tabs where possible
Done
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... PS9, Line 294: ledc_write_opmode(ledc, LP5562_OPMODE_ALL_LOAD);
code indent should use tabs where possible
Done
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... PS9, Line 294: ledc_write_opmode(ledc, LP5562_OPMODE_ALL_LOAD);
please, no spaces at the start of a line
Done
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... PS9, Line 296: /* Inject program code for each engine and start address */
code indent should use tabs where possible
Done
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... PS9, Line 297: load_addr = LP5562_PROG_MEM_ENG1;
code indent should use tabs where possible
Done
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... PS9, Line 297: load_addr = LP5562_PROG_MEM_ENG1;
please, no spaces at the start of a line
Done
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... PS9, Line 298: for (i = 0; i < LED_LP5562_NUM_OF_ENGINES; i++) {
code indent should use tabs where possible
Done
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... PS9, Line 298: for (i = 0; i < LED_LP5562_NUM_OF_ENGINES; i++) {
please, no spaces at the start of a line
Done
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... PS9, Line 306: }
please, no spaces at the start of a line
Done
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... PS9, Line 306: }
code indent should use tabs where possible
Done
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... PS9, Line 308: /* All engines on run. */
code indent should use tabs where possible
Done
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... PS9, Line 309: ledc_write_opmode(ledc, LP5562_OPMODE_ALL_RUN);
code indent should use tabs where possible
Done
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... PS9, Line 309: ledc_write_opmode(ledc, LP5562_OPMODE_ALL_RUN);
please, no spaces at the start of a line
Done
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... PS9, Line 311: enable_reg &= ~LP5562_ENABLE_ALL_MASK;
code indent should use tabs where possible
Done
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... PS9, Line 311: enable_reg &= ~LP5562_ENABLE_ALL_MASK;
please, no spaces at the start of a line
Done
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... PS9, Line 312: enable_reg |= LP5562_ENABLE_ALL_RUN;
please, no spaces at the start of a line
Done
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... PS9, Line 312: enable_reg |= LP5562_ENABLE_ALL_RUN;
code indent should use tabs where possible
Done
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... PS9, Line 359: int led_lp5562_display_pattern(unsigned i2c_bus, enum display_pattern pattern)
Prefer 'unsigned int' to bare use of 'unsigned'
Done
https://review.coreboot.org/c/coreboot/+/32692/9/src/drivers/i2c/lp5562/led_... PS9, Line 403: static void led_lp5562_init(unsigned i2c_bus)
Prefer 'unsigned int' to bare use of 'unsigned'
Done