Attention is currently required from: Tim Wawrzynczak, Robert Chen, Nick Vaccaro, Shon Wang. Subrata Banik 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 7:
(11 comments)
File src/drivers/i2c/cs35l53/chip.h:
https://review.coreboot.org/c/coreboot/+/60331/comment/f3508d55_61a1ed6d PS7, Line 2: Can you please help me to get the product datasheet? I have tried getting from product site but its asking for purchasing details. Possible to create a partner bug and upload the doc?
https://www.cirrus.com/products/cs35l45/
https://review.coreboot.org/c/coreboot/+/60331/comment/a6e1797d_772d4268 PS7, Line 36: nit: please remove extra space?
https://review.coreboot.org/c/coreboot/+/60331/comment/5af78710_debc3e2d PS7, Line 41: /* ACPI Dev nit: move this to line above to maintain the parity with other comments in this file ?
File src/drivers/i2c/cs35l53/cs35l53.c:
https://review.coreboot.org/c/coreboot/+/60331/comment/714dcf54_326f5fde PS7, Line 2: one interesting observation that I have post reviewing this Cirrus Logic 35L53 and Cirrus Logic CS42L42 (existing code in coreboot) is that, except the _DSD implementation, rest of the code is exactly same. One just need to pass the configuration options like _HID.
https://review.coreboot.org/c/coreboot/+/60331/comment/6053974a_154e99ca PS7, Line 24: .speed = config->bus_speed ? : I2C_SPEED_FAST,
shouldn't this be […]
Mostly its coming from here
https://github.com/coreboot/coreboot/blob/master/src/drivers/i2c/cs42l42/cs4...
When nothing appears between ?:, then the value of the comparison is used in the true case.
Therefore, the expression
x ? : y
has the value of x if that is nonzero; otherwise, the value of y.
This example is perfectly equivalent to
x ? x : y
https://review.coreboot.org/c/coreboot/+/60331/comment/ce932540_8f047491 PS7, Line 57: AAD
what is AAD?
Advanced Accessory Detection (AAD)
This is what I found while Googling on kernel code:
The Advanced Accessory Detection (AAD) will detect a jack insertion/removal, detect the polarity of the jack (CTIA/OMTP), and will support the detection of impedance based buttons in parallel with the connected accessory microphone.
https://review.coreboot.org/c/coreboot/+/60331/comment/5ac3bad8_fe6a3cce 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?
https://review.coreboot.org/c/coreboot/+/60331/comment/3ca079f2_f480d67b PS7, Line 82: acpi_dp_add_integer(dsd, "cirrus,gpio1-polarity-invert", config->gpio1_polarity_invert ? 1 : 0);
line over 96 characters
Please fix.
https://review.coreboot.org/c/coreboot/+/60331/comment/6c7c6fff_4b6bf62b PS7, Line 85: acpi_dp_add_integer(dsd, "cirrus,gpio2-polarity-invert", config->gpio2_polarity_invert ? 1 : 0);
line over 96 characters
Please fix.
https://review.coreboot.org/c/coreboot/+/60331/comment/8fa5829f_7d94aefd PS7, Line 86: acpi_dp_add_integer(dsd, "cirrus,gpio2-output-enable", config->gpio2_output_enable ? 1 : 0);
line over 96 characters
Please fix.
https://review.coreboot.org/c/coreboot/+/60331/comment/6297e707_dfce7847 PS7, Line 102: // return CS35L53_ACPI_NAME; while not just return the ACPI name. if you are not planning to use CS35L53_ACPI_NAME then there is no need for this macro?
return CS35L53_ACPI_NAME;
-
Subrata Banik (Code Review)