[S] Change in coreboot[master]: src/mb/purism/librem_cnl: Enable Librem 14 jack detect with fixed EC
Attention is currently required from: Tim Crawford, Jeremy Soller, Paul Menzel, Angel Pons, Felix Held.
Jonathon Hall has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74364 )
Change subject: src/mb/purism/librem_cnl: Enable Librem 14 jack detect with fixed EC ......................................................................
Patch Set 2:
(13 comments)
Patchset:
PS1:
If possible, it’d be nice to split the mainboard change out into a separate commit.
I split this up.
File src/ec/purism/librem-ec/librem_ec.h:
https://review.coreboot.org/c/coreboot/+/74364/comment/2c87809d_64cbdd78 PS1, Line 9: update
If you know the version, please mention it.
Done
https://review.coreboot.org/c/coreboot/+/74364/comment/623a1c91_c350247a PS1, Line 8: /* Check whether librem-ec has working jack detect. This was fixed in an : * update, so we only use the verbs with jack detect if the EC has been updated. : */
Please use the recommended multi-line comment style.
Done
File src/ec/purism/librem-ec/librem_ec.c:
https://review.coreboot.org/c/coreboot/+/74364/comment/b0671d70_43ffb523 PS1, Line 11: /* The 'flags' field in the probe command reply was added in an update. : * Send 4 bytes of zeroes in the "request" to zero out the field if the : * EC does not set it for its reply. */
Please use the recommended multi-line comment style.
Done
https://review.coreboot.org/c/coreboot/+/74364/comment/2527546d_5d70e3f5 PS1, Line 17: reply_data, ARRAY_SIZE(reply_data))) : return false;
Aligning both lines the same is a little confusing to me. […]
Hmm, yeah this looked odd, adding braces did not help either. I moved the condition out to a separate statement which I think looks better.
File src/ec/system76/ec/system76_ec.h:
https://review.coreboot.org/c/coreboot/+/74364/comment/541a728b_13b13721 PS1, Line 9: /* Send a command to the EC. request_data/request_size are the request payload, : * request_data can be NULL if request_size is 0. reply_data/reply_size are : * the reply payload, reply_data can be NULL if reply_size is 0. */
Please use the recommended multi-line comment style.
Done
File src/ec/system76/ec/system76_ec.c:
https://review.coreboot.org/c/coreboot/+/74364/comment/babb15a0_760b0122 PS1, Line 84: uint8_t i;
No need to limit the length. […]
system76_ec_write() takes a uint8_t for the offset since it is limited to 256 bytes. If I use an unsigned int, it has to be truncated for each call to system76_ec_write().
https://review.coreboot.org/c/coreboot/+/74364/comment/719d54a9_16f35508 PS1, Line 86: REG_DATA+i
Please spaces around the operator.
Done
https://review.coreboot.org/c/coreboot/+/74364/comment/d42ddd6c_5144bd62 PS1, Line 96: ret = false;
Log an error?
Done
File src/mainboard/purism/librem_cnl/variants/librem_14/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/74364/comment/cd23ae52_e94352d3 PS1, Line 64: /* Older verbs with no jack detect - needed if an older Librem EC is in use that : * lacks jack detect. Headphones can be selected manually. */
Please use the recommended multi-line comment style.
Done
https://review.coreboot.org/c/coreboot/+/74364/comment/2aa4b755_c83c9422 PS1, Line 74: /* Now that the codec is configured, we can check if the EC has : * jack detect. */
Please use the recommended multi-line comment style.
Done
https://review.coreboot.org/c/coreboot/+/74364/comment/aef006f8_a21d1cee PS1, Line 79: printk(BIOS_WARNING, "EC firmware lacks jack detect, applying workaround\n");
Add a comment to update the EC firmware?
I added a log message, if any end users see this in the logs they will know to update the EC firmware.
https://review.coreboot.org/c/coreboot/+/74364/comment/aa84f47d_121fe234 PS1, Line 80: /* The EC firmware lacks jack detect. Program the : * older workaround verbs with no jack detect. */
Please use the recommended multi-line comment style.
Done
-
Jonathon Hall (Code Review)