Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45398 )
Change subject: soc/mediatek/mt8192: add pmif driver ......................................................................
Patch Set 28:
(29 comments)
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/in... File src/soc/mediatek/mt8192/include/soc/pmif.h:
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/in... PS28, Line 165: Use tabs and align all of these comments.
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/in... File src/soc/mediatek/mt8192/include/soc/pmif_spi.h:
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/in... PS28, Line 45: #define mtk_iocfg_tl ((struct mtk_iocfg_tl_regs *)IOCFG_TL_BASE) Why not use a global variable just like mtk_pmicspi_mst?
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/in... PS28, Line 51: #define mtk_modem_temp_share ((struct mtk_modem_temp_share_regs *)SPM_BASE) Why not use a global variable just like mtk_pmicspi_mst?
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/in... PS28, Line 75: ( No need for parentheses here.
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/in... PS28, Line 107: ( No need for parentheses.
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/in... PS28, Line 124: X Use lowercase "x"?
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/in... File src/soc/mediatek/mt8192/include/soc/pmif_spmi.h:
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/in... PS28, Line 96: ( No need for parentheses.
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... File src/soc/mediatek/mt8192/pmif.c:
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 15: unsigned int Use "long" to be consistent with the signature of stopwatch_init_usecs_expire().
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 15: unsigned int int
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 16: unsigned int u32
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 18: unsigned int u32
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 31: unsigned int If it's boolean, use "int". Otherwise, consider u32, u16 or u8. Same for other similar arguments.
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 46: { No need for these.
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 46: if (write == 1) if (write)
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 55: if (write == 0) if (!write)
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 74: *data = 0; Is this for timeout case in pmif_send_cmd()?
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 89: ( No need for these. Better to write
data >>= shift;
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 122: ( No need for these. Better to write
data >>= shift;
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 175: if (inf == PMIF_SPMI) Check "mstid < ARRAY_SIZE(pmif_spmi_arb)".
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 191: Remove this blank line.
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... File src/soc/mediatek/mt8192/pmif_spi.c:
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... PS7, Line 171: };
After confirming with DE, they are random numbers in order to verify the read-back data.
Ack
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... File src/soc/mediatek/mt8192/pmif_spi.c:
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 59: u32 int
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 59: u32 void
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 74: s32 Can we use int?
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 172: s32 Can we use int?
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 176: const u32 test_data[30] = { Add a comment
/* Random data for testing */
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 206: size_t int
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... File src/soc/mediatek/mt8192/pmif_spmi.c:
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 104: b What does this "b" mean?
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 108: } },