Attention is currently required from: Hope Wang, Hung-Te Lin, Jarried Lin, Yidi Lin.
Yu-Ping Wu has posted comments on this change by Jarried Lin. ( https://review.coreboot.org/c/coreboot/+/85127?usp=email )
Change subject: soc/mediatek/mt8196: Add PMIC MT6363 driver ......................................................................
Patch Set 7:
(13 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/85127/comment/1988d7f6_2cf855ec?usp... : PS7, Line 9: SOC SoC
https://review.coreboot.org/c/coreboot/+/85127/comment/50d84645_562b3f91?usp... : PS7, Line 12: MT6363_sdmadc This isn't added in this patch.
File src/soc/mediatek/common/include/soc/mt6363.h:
https://review.coreboot.org/c/coreboot/+/85127/comment/0fe1e97f_18da9f68?usp... : PS7, Line 11: a Let's use all upper cases for consistency.
https://review.coreboot.org/c/coreboot/+/85127/comment/16175467_704bad2b?usp... : PS7, Line 28: 0x24e Should this be sorted?
https://review.coreboot.org/c/coreboot/+/85127/comment/1303f3f4_0039aeff?usp... : PS7, Line 115: #endif /* __SOC_MEDIATEK_MT6363_H__ */ One blank line above.
File src/soc/mediatek/common/mt6363.c:
https://review.coreboot.org/c/coreboot/+/85127/comment/8d858757_d309e58e?usp... : PS7, Line 11: MT6363_UDELAY_200US MT6363_SET_DELAY_US
I assume this is for the delay after setting voltage.
https://review.coreboot.org/c/coreboot/+/85127/comment/b8bd23dd_25a2ca25?usp... : PS7, Line 141: ) remove
https://review.coreboot.org/c/coreboot/+/85127/comment/69f9a078_98716f86?usp... : PS7, Line 260: mt6363_sdmadc_init This doesn't exist (yet). Remove it.
File src/soc/mediatek/mt8196/mt6363.c:
https://review.coreboot.org/c/coreboot/+/85127/comment/839a46d1_2f16a229?usp... : PS7, Line 13: 0 If `shift` is always 0, we can remove the field from the `pmic_setting` struct.
https://review.coreboot.org/c/coreboot/+/85127/comment/048ecd8a_f246d3fa?usp... : PS7, Line 380: /* Suspend */ Is this a TODO comment? Can we remove this in this patch, if not needed?
https://review.coreboot.org/c/coreboot/+/85127/comment/cebe2f26_eda2f4ab?usp... : PS7, Line 390: /* We should disable unused modem power in a separate function and Is this a TODO comment? If so, change it to `TODO: Disable unused ...`
https://review.coreboot.org/c/coreboot/+/85127/comment/ec094e36_da3ec423?usp... : PS7, Line 390: /* Wrong format.
https://review.coreboot.org/c/coreboot/+/85127/comment/b75488f2_3ecc79c1?usp... : PS7, Line 396: pmic_lp_setting Where is this called?