Attention is currently required from: Subrata Banik, Tim Wawrzynczak, Paul Menzel, Shon Wang. Robert Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/60331 )
Change subject: mb/google/brya/var/vell: Add AMP driver setting ......................................................................
Patch Set 20:
(19 comments)
Patchset:
PS7:
Please split this into to commits. […]
cs35l53 driver transfer to cirruslogic's commit. https://review.coreboot.org/c/coreboot/+/61448
PS7:
I will review in more details tomorrow. […]
Done
PS7:
Style nit: Your Gerrit user name has a dot in it: Shon. […]
Done
File src/drivers/i2c/cs35l53/Kconfig:
https://review.coreboot.org/c/coreboot/+/60331/comment/c2e59ac5_440fa8ef PS7, Line 4:
This would be a good place to point to the kernel's firmware bindings documentation
Done
File src/drivers/i2c/cs35l53/chip.h:
https://review.coreboot.org/c/coreboot/+/60331/comment/4cf9049f_1d18e5e1 PS7, Line 2:
Can you please help me to get the product datasheet? I have tried getting from product site but its […]
cs35l53 driver transfer to cirruslogic's commit. https://review.coreboot.org/c/coreboot/+/61448
https://review.coreboot.org/c/coreboot/+/60331/comment/72969147_78145af9 PS7, Line 36:
nit: please remove extra space?
Done
https://review.coreboot.org/c/coreboot/+/60331/comment/af109488_312e99ea PS7, Line 39:
nit: extra blank line
cs35l53 driver transfer to cirruslogic's commit. https://review.coreboot.org/c/coreboot/+/61448
https://review.coreboot.org/c/coreboot/+/60331/comment/2bfe13d2_9dcbfea8 PS7, Line 41: /* ACPI Dev
nit: move this to line above to maintain the parity with other comments in this file ?
Done
https://review.coreboot.org/c/coreboot/+/60331/comment/f80ac238_4ddccb18 PS7, Line 132:
Blank line not needed.
Done
File src/drivers/i2c/cs35l53/cs35l53.c:
https://review.coreboot.org/c/coreboot/+/60331/comment/6b068b34_5d43e22f PS7, Line 2:
@Tim/Nick, what do you think ?
Done
https://review.coreboot.org/c/coreboot/+/60331/comment/3d56aa76_b49fb248 PS7, Line 24: .speed = config->bus_speed ? : I2C_SPEED_FAST,
Mostly its coming from here […]
Done
https://review.coreboot.org/c/coreboot/+/60331/comment/d8316aea_f787056c PS7, Line 57: AAD
Advanced Accessory Detection (AAD) […]
Done
https://review.coreboot.org/c/coreboot/+/60331/comment/78457ac9_68a55c01 PS7, Line 70: if ((config->boost_peak_milliamp > 4500) || : (config->boost_peak_milliamp < 1600) || : (config->boost_peak_milliamp % 50)){
please reflow it till 96 line?
Done
https://review.coreboot.org/c/coreboot/+/60331/comment/c4cc8eb4_7969fd41 PS7, Line 82: acpi_dp_add_integer(dsd, "cirrus,gpio1-polarity-invert", config->gpio1_polarity_invert ? 1 : 0);
line over 96 characters […]
Done
https://review.coreboot.org/c/coreboot/+/60331/comment/f395f2a1_4c3fe293 PS7, Line 85: acpi_dp_add_integer(dsd, "cirrus,gpio2-polarity-invert", config->gpio2_polarity_invert ? 1 : 0);
line over 96 characters […]
Done
https://review.coreboot.org/c/coreboot/+/60331/comment/61ade70c_e745632d PS7, Line 86: acpi_dp_add_integer(dsd, "cirrus,gpio2-output-enable", config->gpio2_output_enable ? 1 : 0);
line over 96 characters […]
Done
https://review.coreboot.org/c/coreboot/+/60331/comment/ead6beff_5085a715 PS7, Line 102: // return CS35L53_ACPI_NAME;
while not just return the ACPI name. […]
Done
https://review.coreboot.org/c/coreboot/+/60331/comment/8d4f749c_6b1939e7 PS7, Line 100: static const char *cs35l53_acpi_name(const struct device *dev) : { : // return CS35L53_ACPI_NAME; : struct drivers_i2c_cs35l53_config *config = dev->chip_info; : static char name[5]; : : : : if (config->name){ : return config->name; : } : : : snprintf(name, sizeof(name), "D%03.3X", dev->path.i2c.device); : return name; : }
please reformat this one
Done
File src/mainboard/google/brya/variants/vell/overridetree.cb:
PS7:
will there be a corresponding FW_CONFIG option for this in AUDIO? […]
Yes, vell only use cs35l53 smart amp
-
Robert Chen (Code Review)