hsin-hsiung wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45398 )
Change subject: soc/mediatek/mt8192: add pmif driver ......................................................................
Patch Set 31:
(27 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.
Done
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?
Done
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?
remove it because our chip won't use modem feature.
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/in... PS28, Line 75: (
No need for parentheses here.
Done
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/in... PS28, Line 124: X
Use lowercase "x"?
Done
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.
Done
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
int
Done
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().
Done
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 16: unsigned int
u32
Done
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 18: unsigned int
u32
Done
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.
Done
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 46: if (write == 1)
if (write)
Done
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 46: {
No need for these.
Done
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 55: if (write == 0)
if (!write)
Done
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()?
yes, it is.
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 89: (
No need for these. Better to write […]
Done
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 122: (
No need for these. Better to write […]
Done
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)".
Done
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 191:
Remove this blank line.
Done
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
void
Done
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 59: u32
int
Done
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 74: s32
Can we use int?
Done
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 172: s32
Can we use int?
Done
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 176: const u32 test_data[30] = {
Add a comment […]
Done
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 206: size_t
int
Done
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?
Done
https://review.coreboot.org/c/coreboot/+/45398/28/src/soc/mediatek/mt8192/pm... PS28, Line 108: }
},
Done