Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33582 )
Change subject: libpayload/storage: Add NVMe driver ......................................................................
Patch Set 16:
(4 comments)
https://review.coreboot.org/c/coreboot/+/33582/15/payloads/libpayload/driver... File payloads/libpayload/drivers/storage/nvme.c:
https://review.coreboot.org/c/coreboot/+/33582/15/payloads/libpayload/driver... PS15, Line 60: : static storage_poll_t nvme_poll(struct storage_dev *dev); : static void nvme_detach_device(struct storage_dev *dev); : static ssize_t nvme_read_blocks512( : struct storage_dev *dev, lba_t start, size_t count, unsigned char *buf); : : static int create_admin_queues(struct nvme_dev *nvme); : static int create_io_submission_queue(struct nvme_dev *nvme); : static int create_io_completion_queue(struct nvme_dev *nvme); : static int delete_admin_queues(struct nvme_dev *nvme); : static int delete_io_submission_queue(struct nvme_dev *nvme); : static int delete_io_completion_queue(struct nvme_dev *nvme); : static int nvme_cmd( : struct nvme_dev *nvme, enum nvme_queue q, const struct nvme_s_queue_entry *cmd); : static int read(struct nvme_dev *nvme, unsigned char *buffer, uint64_t base, uint16_t count); :
It would contribute to the readability of the code to reorder the functions so that this is not need […]
Done
https://review.coreboot.org/c/coreboot/+/33582/15/payloads/libpayload/driver... PS15, Line 130: static int read
even though this is static, but please don't use posix/bsd/sysv reserved function names (unless you […]
Done
https://review.coreboot.org/c/coreboot/+/33582/15/payloads/libpayload/driver... PS15, Line 390: FATAL ERROR
How are tgese two functions more fatal than the other ones goto-ing to abort:?
I unified the error message in the abort section. This could be enough
https://review.coreboot.org/c/coreboot/+/33582/15/payloads/libpayload/driver... File payloads/libpayload/drivers/storage/storage.c:
https://review.coreboot.org/c/coreboot/+/33582/15/payloads/libpayload/driver... PS15, Line 31: #if CONFIG(LP_STORAGE_AHCI) : # include <storage/ahci.h> : #endif : #if CONFIG(LP_STORAGE_NVME) : #include <storage/nvme.h> : #endif
With these only containing one prototype each, are we sure that wrapping the includes in CONFIG() is […]
Done