[XL] Change in coreboot[main]: soc/mediatek/mt8196: Add PMIC MT6316 driver

[Hope Wang (Code Review)]

Attention is currently required from: Hung-Te Lin, Jarried Lin, Yidi Lin, Yu-Ping Wu.

Hope Wang has posted comments on this change by Jarried Lin. ( https://review.coreboot.org/c/coreboot/+/85630?usp=email )

Change subject: soc/mediatek/mt8196: Add PMIC MT6316 driver
......................................................................

Patch Set 5:

(16 comments)

File src/soc/mediatek/common/include/soc/mt6316.h:

https://review.coreboot.org/c/coreboot/+/85630/comment/13674336_0582257d?usp... :
PS3, Line 21: max
> MAX

Done

File src/soc/mediatek/common/mt6316.c:

https://review.coreboot.org/c/coreboot/+/85630/comment/91b654e7_e1dc025f?usp... :
PS2, Line 14: 	assert(pmif_arb);
            : 	pmif_arb->read(pmif_arb, slvid, reg, data);
> Is `data` supposed to be `u8`?

There is no read api for u8, how about u16?

File src/soc/mediatek/common/mt6316.c:

https://review.coreboot.org/c/coreboot/+/85630/comment/5b005af7_1a1bbb91?usp... :
PS3, Line 12: enum spmi_slave
> Please consistently use either `u32` or `enum spmi_slave` in all of the functions.

Done

https://review.coreboot.org/c/coreboot/+/85630/comment/15019cfe_285f34dc?usp... :
PS3, Line 77: (
> remove

Done

https://review.coreboot.org/c/coreboot/+/85630/comment/b8b4c835_9b0aed0d?usp... :
PS3, Line 78: (
> remove

Done

https://review.coreboot.org/c/coreboot/+/85630/comment/f742cef5_89c2f2c0?usp... :
PS3, Line 107: (
> remove

Done

https://review.coreboot.org/c/coreboot/+/85630/comment/2f2af29c_aa175614?usp... :
PS3, Line 108: (
> remove

Done

https://review.coreboot.org/c/coreboot/+/85630/comment/7bfa628b_06b642b0?usp... :
PS3, Line 111: u8
> Is this supposed to be a boolean?

Done

https://review.coreboot.org/c/coreboot/+/85630/comment/4d1ef617_386ef0be?usp... :
PS3, Line 160: con9
> What does con9 mean?

The register name of address 0x222 is TEST_CON9

https://review.coreboot.org/c/coreboot/+/85630/comment/e9f8d670_819189c1?usp... :
PS3, Line 164: 0x222
> 0x222 is `reg`, not the mask. Please define a macro for it.

Yes, 0x222 is the register of test_con9, has 8 bit, only need to set bit5.

https://review.coreboot.org/c/coreboot/+/85630/comment/7dd2b072_720a9681?usp... :
PS3, Line 169: reg_val_6
> +1. Please change the type of `data` to u8 for both `mt6316_read` and `mt6316_write`.

Yes, read ont byte, read api do not have u8 but only u16, how about mt6316_read8 get u16 from pmif_arb->read16 and return u8 implicitly.

https://review.coreboot.org/c/coreboot/+/85630/comment/34213a5b_317ef605?usp... :
PS3, Line 181: /* for N*/
> remove. […]

N means project liber

https://review.coreboot.org/c/coreboot/+/85630/comment/25290ccd_961f1b81?usp... :
PS3, Line 202: mt6316_wdt_enable(SPMI_SLAVE_6);
             : 	mt6316_wdt_enable(SPMI_SLAVE_7);
             : 	mt6316_wdt_enable(SPMI_SLAVE_8);
             : 	mt6316_wdt_enable(SPMI_SLAVE_15);
             :
> Define a global static const array for `[SPMI_SLAVE_6, SPMI_SLAVE_7, SPMI_SLAVE_8, SPMI_SLAVE_15]` a […]

mt6316_init_setting not have only slave id, but also init_setting array, do we need to optimize these arrays?

File src/soc/mediatek/mt8196/mt6316.c:

https://review.coreboot.org/c/coreboot/+/85630/comment/efa4f888_37611d3c?usp... :
PS3, Line 885: 	for (int i = 0; i < ARRAY_SIZE(init_setting_s8); i++)
> Can we make the order consistent: 6, 7, 8, 15 (instead of 8, 6, 7, 15)?

Done

https://review.coreboot.org/c/coreboot/+/85630/comment/ef476d08_1195dfc1?usp... :
PS3, Line 886: mt6316_write_field(SPMI_SLAVE_8,
             : 				   init_setting_s8[i].addr, init_setting_s8[i].val,
             : 				   init_setting_s8[i].mask, init_setting_s8[i].shift);
> Extract it to […]

Done

https://review.coreboot.org/c/coreboot/+/85630/comment/c4c3723b_1c2ff306?usp... :
PS3, Line 889: 	printk(BIOS_INFO, "%s S8 done\n", __func__);
> Do we really need these logs? `mt6316_write_field` won't fail, so these logs will always be printed.

Done