Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41586 )
Change subject: drivers/vpd: rename VPD_ANY to VPD_RO_OR_RW ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41586/3/src/drivers/vpd/vpd.h File src/drivers/vpd/vpd.h:
https://review.coreboot.org/c/coreboot/+/41586/3/src/drivers/vpd/vpd.h@18 PS3, Line 18: VPD_RO_OR_RW I think I would prefer something like VPD_RO_THEN_RW to make more clear that it's intentionally ordered.
https://review.coreboot.org/c/coreboot/+/41586/3/src/drivers/vpd/vpd.c File src/drivers/vpd/vpd.c:
https://review.coreboot.org/c/coreboot/+/41586/3/src/drivers/vpd/vpd.c@179 PS3, Line 179: consumed = 0; This is all... getting pretty hairy. I think this could look a lot cleaner by packing the actual lookup in a separate function like I did in https://review.coreboot.org/c/coreboot/+/41757/2/src/drivers/vpd/vpd.c#214. Then you could just do
if (region == VPD_RW_THEN_RO) vpd_find_in(&rw_vpd, &arg);
if (!arg.matched && region != VPD_RW) vpd_find_in(&ro_vpd, &arg);
if (!arg.matched && (region == VPD_RW || region == VPD_RO_THEN_RW)) vpd_find_in(&rw_vpd, &arg);
Alternatively, a little more wordy but just to make the ordering super clear it could be written as:
switch (region) { case VPD_RO: vpd_find_in(&ro_vpd, &arg); break; case VPD_RW: vpd_find_in(&rw_vpd, &arg); break; case VPD_RO_THEN_RW: vpd_find_in(&ro_vpd, &arg); if (!arg.matched) vpd_find_in(&rw_vpd, &arg); break; case VPD_RW_THEN_RO: vpd_find_in(&rw_vpd, &arg); if (!arg.matched) vpd_find_in(&ro_vpd, &arg); break; }
If you're not in a hurry maybe it would make things easier to get my patch in first?