Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45398 )
Change subject: soc/mediatek/mt8192: add pmif driver ......................................................................
Patch Set 7:
(23 comments)
https://review.coreboot.org/c/coreboot/+/45398/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45398/7//COMMIT_MSG@11 PS7, Line 11: Where is the interface documented? Please add a datasheet name and revision.
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 99: Print out as a debug message, how long it took?
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... PS7, Line 171: }; In that pattern documented somewhere?
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... PS7, Line 217: __func__, rdata, DEFAULT_VALUE_READ_TEST); Please use more elaborate error messages.
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... PS7, Line 234: /* Wait for completion of sending the commands */ : do { : rdata = read32(&arb->mtk_pmif->inf_busy_sta); : } while ((rdata & PMIF_SPI_AP) != 0x0); : : do { : rdata = read32(&arb->mtk_pmif->other_busy_sta_0); : } while (GET_CMDISSUE_BUSY(rdata) != 0x0); : : do { : rdata = read32(&mtk_pmicspi_mst->other_busy_sta_0); : } while (GET_PMICSPI_BUSY(rdata) != 0x0); Could these be endless loops?
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... PS7, Line 254: Setup Set up
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... PS7, Line 257: Setup Set up
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... PS7, Line 268: int ret = 0; Use CB_SUCCESS and friends?
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... PS7, Line 287: if (ret != 0) { Use CB_SUCCESS and friends.
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... PS7, Line 288: printk(BIOS_ERR, "[%s] data calibration fail,ret=%d\n", Please elaborate and add a space after the comma.
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... PS7, Line 298: Checking checking
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... PS7, Line 298: 1. Signature Checking using CRC (CRC 0 only) : * 2. EINT update : * 3. Read back Auxadc thermal data for GPS
- Check signature using CRC (CRC 0 only)
- Update EINT
- …
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... PS7, Line 300: Auxadc AUXADC
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... PS7, Line 302: initstaupd `init_staupd()` for consistency with `init_sistrobe()`.
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... PS7, Line 321: udelay(100); How is that done by a delay?
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... File src/soc/mediatek/mt8192/pmif_spmi.c:
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... PS7, Line 52: }, { Should { be on the next line?
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... PS7, Line 102: u32 i = 0; The native type should be enough for a counting variable.
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... PS7, Line 131: printk(BIOS_ERR, "%s: Null argument", __func__); Error messages should be more elaborate and helpful.
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... PS7, Line 162: u32 swinf_no) Fits into one line.
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... PS7, Line 200: write32(&arb->mtk_pmif->inf_cmd_per_1, cmd_per); Why does a different value have to be programmed here?
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... PS7, Line 210: */ Please use one of the recommended coding styles [1].
[1]: https://doc.coreboot.org/coding_style.html#commenting
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... PS7, Line 224: 0x2F7 Add enums or macros?
https://review.coreboot.org/c/coreboot/+/45398/7/src/soc/mediatek/mt8192/pmi... PS7, Line 251: return -E_NODEV; Add a debug message?