Name of user not set #1003143 has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46694 )
Change subject: lib/edid: Add missing name descriptor presence flag. ......................................................................
lib/edid: Add missing name descriptor presence flag.
EDID parser internal flah c->has_name_descriptor flag was never set. It was causing decode_edid() function to return NON_CONFORMANT instead of CONFORMANT even when EDID frame was correct.
Signed-off-by: Jakub Czapiga jacz@semihalf.com Change-Id: Ifdc723b892a0885cfca08dab1a5ef961463da289 --- M src/lib/edid.c 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/46694/1
diff --git a/src/lib/edid.c b/src/lib/edid.c index f20d239..cd7a47a 100644 --- a/src/lib/edid.c +++ b/src/lib/edid.c @@ -261,6 +261,7 @@ extract_string(x + 5, &c->has_valid_string_termination, EDID_ASCII_STRING_LENGTH)); + c->has_name_descriptor = 1; return 1; case 0xFD: {
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46694 )
Change subject: lib/edid: Add missing name descriptor presence flag. ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46694/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46694/1//COMMIT_MSG@9 PS1, Line 9: flah flash
https://review.coreboot.org/c/coreboot/+/46694/1//COMMIT_MSG@13 PS1, Line 13: Is there a bug number for this? I know that coreboot doesn't really care about bug numbers, but when the upstream CL lands in the chromium tree, it will be nice to have a bug number associated with it.
Even better is if there is a unit test for this module (I know there isn't one yet), then we add a test case that fails before this fix is applied and passes after it is applied.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46694 )
Change subject: lib/edid: Add missing name descriptor presence flag. ......................................................................
Patch Set 1: Code-Review+2
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46694 )
Change subject: lib/edid: Add missing name descriptor presence flag. ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46694/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46694/1//COMMIT_MSG@9 PS1, Line 9: flah
flash
sorry, no, that should be "flag" I had just read another email where the topic was flash memory, so I thought an "s" was missing here.
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46694 )
Change subject: lib/edid: Add missing name descriptor presence flag. ......................................................................
Patch Set 1: Code-Review+2
Also, it looks good, just the minor spelling nit in the commit message, a request for a bug number, and a commentary on how unit testing can help improve this next time.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46694 )
Change subject: lib/edid: Add missing name descriptor presence flag. ......................................................................
Patch Set 1: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/46694/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46694/1//COMMIT_MSG@7 PS1, Line 7: . nit: drop trailing period in commit summary
https://review.coreboot.org/c/coreboot/+/46694/1//COMMIT_MSG@11 PS1, Line 11: nit: only one space is enough
Hello build bot (Jenkins), Patrick Georgi, Paul Fagerburg, Angel Pons, Julius Werner, Jan Dabros,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46694
to look at the new patch set (#2).
Change subject: lib/edid: Add missing name descriptor presence flag ......................................................................
lib/edid: Add missing name descriptor presence flag
EDID parser internal flag c->has_name_descriptor was never set. It was causing decode_edid() function to return NON_CONFORMANT instead of CONFORMANT even when EDID frame was correct.
Signed-off-by: Jakub Czapiga jacz@semihalf.com Change-Id: Ifdc723b892a0885cfca08dab1a5ef961463da289 --- M src/lib/edid.c 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/46694/2
Name of user not set #1003143 has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46694 )
Change subject: lib/edid: Add missing name descriptor presence flag ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/46694/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46694/1//COMMIT_MSG@7 PS1, Line 7: .
nit: drop trailing period in commit summary
Done
https://review.coreboot.org/c/coreboot/+/46694/1//COMMIT_MSG@9 PS1, Line 9: flah
sorry, no, that should be "flag" […]
Done
https://review.coreboot.org/c/coreboot/+/46694/1//COMMIT_MSG@11 PS1, Line 11:
nit: only one space is enough
Done
https://review.coreboot.org/c/coreboot/+/46694/1//COMMIT_MSG@13 PS1, Line 13:
Is there a bug number for this? I know that coreboot doesn't really care about bug numbers, but when […]
I do not know of any bug number associated with this. I found this bug while creating tests for this module. I checked previous versions of lib/edid.c in repository and found the exact place where this flag had to be, but was scrapped in commit 2536c1ac76790b12554fe8e277ef2dcbc5f58242 probably by accident.
Paul Fagerburg has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46694 )
Change subject: lib/edid: Add missing name descriptor presence flag ......................................................................
lib/edid: Add missing name descriptor presence flag
EDID parser internal flag c->has_name_descriptor was never set. It was causing decode_edid() function to return NON_CONFORMANT instead of CONFORMANT even when EDID frame was correct.
Signed-off-by: Jakub Czapiga jacz@semihalf.com Change-Id: Ifdc723b892a0885cfca08dab1a5ef961463da289 Reviewed-on: https://review.coreboot.org/c/coreboot/+/46694 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Julius Werner jwerner@chromium.org Reviewed-by: Paul Fagerburg pfagerburg@chromium.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/lib/edid.c 1 file changed, 1 insertion(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved Angel Pons: Looks good to me, but someone else must approve Paul Fagerburg: Looks good to me, approved
diff --git a/src/lib/edid.c b/src/lib/edid.c index f20d239..cd7a47a 100644 --- a/src/lib/edid.c +++ b/src/lib/edid.c @@ -261,6 +261,7 @@ extract_string(x + 5, &c->has_valid_string_termination, EDID_ASCII_STRING_LENGTH)); + c->has_name_descriptor = 1; return 1; case 0xFD: {
-
Angel Pons (Code Review) -
Julius Werner (Code Review)
-
Name of user not set (Code Review)
-
Paul Fagerburg (Code Review)