Cheng-Yi Chiang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36029 )
Change subject: drivers/i2c/rt1011: Add a driver for RT1011 ......................................................................
Patch Set 8:
(5 comments)
Thanks for reviewing!
https://review.coreboot.org/c/coreboot/+/36029/7/src/drivers/i2c/rt1011/chip... File src/drivers/i2c/rt1011/chip.h:
https://review.coreboot.org/c/coreboot/+/36029/7/src/drivers/i2c/rt1011/chip... PS7, Line 23: enum i2c_speed speed; /* Bus speed in Hz, default is I2C_SPEED_FAST */
Is this something that is configurable? If not, can we just assume I2C_SPEED_FAST in the driver and […]
Done From the spec of ALC1011, the supported speed is 400K, that is, I2C_SPEED_FAST, so we can get rid of this member.
https://review.coreboot.org/c/coreboot/+/36029/7/src/drivers/i2c/rt1011/chip... PS7, Line 25: /* ACPI _HID (required) */
Now that this isn't using the generic driver you might consider removing the hid setting and have th […]
I see. Removed hid in config. Thanks for explanation.
https://review.coreboot.org/c/coreboot/+/36029/7/src/drivers/i2c/rt1011/rt10... File src/drivers/i2c/rt1011/rt1011.c:
https://review.coreboot.org/c/coreboot/+/36029/7/src/drivers/i2c/rt1011/rt10... PS7, Line 59:
nit: empty line
Done
https://review.coreboot.org/c/coreboot/+/36029/7/src/drivers/i2c/rt1011/rt10... PS7, Line 94: RT1011_ACPI_NAME
If this same driver ends up getting used by multiple devices under the same parent, then there will […]
Done. I used the same name as generic driver in case the name is not set from config.
https://review.coreboot.org/c/coreboot/+/36029/7/src/drivers/i2c/rt1011/rt10... PS7, Line 119: CHIP_NAME("Realtek RT1011 Codec").enable_dev
CHIP_NAME(x) is just ".name = x," so when expanded this looks a bit odd without a newline. […]
It was made by clang-format. I agree that splitting them to 2 lines looks better.