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

[Yu-Ping Wu (Code Review)]

Attention is currently required from: Hope Wang, Hung-Te Lin, Jarried Lin.

Yu-Ping Wu 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 3:

(18 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/85630/comment/88e1dada_3b5ed226?usp... :
PS3, Line 9: Add MT6316 driver in SoC folder
Missing `.`

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

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

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

https://review.coreboot.org/c/coreboot/+/85630/comment/1bcffe99_f1b87596?usp... :
PS2, Line 14: 	assert(pmif_arb);
            : 	pmif_arb->read(pmif_arb, slvid, reg, data);
> Please follow other PMICs' read/write semantic. […]

Is `data` supposed to be `u8`?

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

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

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

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

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

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

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

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

https://review.coreboot.org/c/coreboot/+/85630/comment/b0fc3387_5105dbc5?usp... :
PS3, Line 164: 0x222
> Is the mask correct ? Doesn't `pmif_arb->write` send one byte per call ?

0x222 is `reg`, not the mask. Please define a macro for it.

https://review.coreboot.org/c/coreboot/+/85630/comment/6d7d712b_a9215b5c?usp... :
PS3, Line 169: reg_val_6
> We expect to read one byte back, right ?

+1. Please change the type of `data` to u8 for both `mt6316_read` and `mt6316_write`.

https://review.coreboot.org/c/coreboot/+/85630/comment/04050582_2b815af3?usp... :
PS3, Line 181: /* for N*/
remove. What does that mean BTW?

https://review.coreboot.org/c/coreboot/+/85630/comment/6ff087d9_8f8354d4?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]` and then use for loops in `mt6316_set_all_test_con9`, `mt6316_init` and here.

Also, can we pass `SPMI_SLAVE_*` as an argument to `mt6316_init_setting(enum spmi_slave slvid)`?

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

https://review.coreboot.org/c/coreboot/+/85630/comment/85b3d2fb_993c7155?usp... :
PS3, Line 6: MeidaTek
MediaTek

https://review.coreboot.org/c/coreboot/+/85630/comment/e34309a7_eacd9494?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)?

https://review.coreboot.org/c/coreboot/+/85630/comment/0979c2ec_ff63cb55?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

```
static inline void write_field(u32 slvid, const struct mt6316_setting *setting)
{
    mt6316_write_field(slvid, settings->addr, ...);
}
```

https://review.coreboot.org/c/coreboot/+/85630/comment/90b70e89_2a3e5c48?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.