Change in coreboot[master]: drivers/i2c/cs42l42: Add driver for generating device in SSDT
Attention is currently required from: Furquan Shaikh, Martin Roth, Paul Menzel, Zhuohao Lee, Karthik Ramasubramanian, EricR Lai. Vitaly Rodionov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52395 )
Change subject: drivers/i2c/cs42l42: Add driver for generating device in SSDT ......................................................................
Patch Set 5:
(11 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/52395/comment/5155b9c2_8be604ae PS4, Line 13: contained the equivalent parameters that are provided by the current DSDT object.
Please wrap lines after 75 characters in commit messages.
Done
File src/drivers/i2c/cs42l42/chip.h:
https://review.coreboot.org/c/coreboot/+/52395/comment/1dbde792_288c3351 PS4, Line 22: private parameters
Just curious: What do you mean by "private parameters"?
OK, not really private
https://review.coreboot.org/c/coreboot/+/52395/comment/ed6fcbf7_b2c97400 PS4, Line 23: /*
https://doc.coreboot.org/contributing/coding_style.html#commenting […]
Done
https://review.coreboot.org/c/coreboot/+/52395/comment/0ba41093_8d29bf12 PS4, Line 30: unsigned int
I think use of "bool" would be better here to indicate that this is boolean.
Agreed
https://review.coreboot.org/c/coreboot/+/52395/comment/7d113b65_433ad630 PS4, Line 35: unsigned int
Looks like there are a specific set of values that are allowed. […]
Done
https://review.coreboot.org/c/coreboot/+/52395/comment/00cb4f79_cab5fdea PS4, Line 40: unsigned int
same here: […]
Done
https://review.coreboot.org/c/coreboot/+/52395/comment/c25e5391_1e58a61d PS4, Line 49: unsigned int
same here: […]
Value can be from 0 - 200 ms, step 1 ms, do we really need enum with 200 members?
https://review.coreboot.org/c/coreboot/+/52395/comment/e19805c1_61f92f76 PS4, Line 58: unsigned int
same here.
Value can be from 0 - 200 ms, step 1 ms, do we really need enum with 200 members?
https://review.coreboot.org/c/coreboot/+/52395/comment/ce86d324_538f8425 PS4, Line 74: /* headset bias ramp rate */
As mentioned earlier, it is better to have a enum so that the configuration in the devicetree are re […]
Done
File src/drivers/i2c/cs42l42/cs42l42.c:
https://review.coreboot.org/c/coreboot/+/52395/comment/463c4b37_074e599d PS4, Line 59: reset-gpios
This should be added only if reset GPIO is added to _CRS.
Done
https://review.coreboot.org/c/coreboot/+/52395/comment/172548f5_2ce5dc82 PS4, Line 60: There is a single GPIO entry in _CRS
This is not true if IRQ GPIO is written on line 46.
Done
-
Vitaly Rodionov (Code Review)