Attention is currently required from: Paul Menzel, Angel Pons, Wenbin Mei, Yu-Ping Wu. Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/51966 )
Change subject: soc/mediatek: add new driver 'msdc' for eMMC ......................................................................
Patch Set 9:
(6 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/51966/comment/9390b8a7_1a0d1d49 PS3, Line 10:
Done
Ack
File src/soc/mediatek/common/include/soc/msdc.h:
https://review.coreboot.org/c/coreboot/+/51966/comment/6143eae3_82371bcb PS5, Line 96: #define SDC_CMD_WR BIT(13)
nit: For consistency with the other macros, I wouldn't use `BIT(x)` here: […]
Ack
https://review.coreboot.org/c/coreboot/+/51966/comment/ff819e0c_afa7fd65 PS5, Line 143: #define MSDC_TIMEOUT (1000*1000) /* 1S */
I would add a `_MS` or `_US` suffix to indicate the units the value is expressed in: […]
Ack
File src/soc/mediatek/common/msdc.c:
https://review.coreboot.org/c/coreboot/+/51966/comment/4fad5620_c780d508 PS2, Line 349: host
I think it's a pretty bad idea to recast (struct sd_mmc_ctrlr *) to (struct msdc_ctrlr *). […]
Ack
File src/soc/mediatek/common/msdc.c:
https://review.coreboot.org/c/coreboot/+/51966/comment/20a618ba_d295f2ad PS5, Line 269: if (cmd->cmdidx != MMC_CMD_AUTO_TUNING_SEQUENCE)
nit: I'd add braces here. […]
Ack
File src/soc/mediatek/common/msdc.c:
https://review.coreboot.org/c/coreboot/+/51966/comment/9d0b0910_ec1fa3af PS7, Line 380: (struct msdc_ctrlr *)ctrlr;
this should also be container_of
Ack
-
Hung-Te Lin (Code Review)