[coreboot-gerrit] [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 9:

(7 comments)

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

https://review.coreboot.org/c/coreboot/+/85630/comment/cb23bbb9_39015139?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);
             :
> mt6316_init_setting not have only slave id, but also init_setting array, do we need to optimize thes […]

PS9 looks good to me!

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

https://review.coreboot.org/c/coreboot/+/85630/comment/c23a591a_49d8d092?usp... :
PS9, Line 20: read16
This should be `read` (which is for reading 1 byte). `read16` is for reading 2 bytes. Also see b/381782630 for future refactoring. For now, the data argument of `read()` is u32, so we should write:

```
u32 data = 0;

assert(...);
pmif_arb->read(..., &data);

return (u8)data;
```

https://review.coreboot.org/c/coreboot/+/85630/comment/1202dc64_b1298515?usp... :
PS9, Line 114: vol * 5000 + vol_l * 2500
Use a local variable to to store this.

https://review.coreboot.org/c/coreboot/+/85630/comment/520c11b7_ae2f87a7?usp... :
PS9, Line 118: mode
Does this mean enable/disable? Can we rename this to `enable`?

https://review.coreboot.org/c/coreboot/+/85630/comment/d8370d15_f63655f3?usp... :
PS9, Line 161: mod = mt6316_read8(slvid, MT6316_BUCK_TOP_4PHASE_TOP_ANA_CON0);
             : 	mod >>= mod_shift;
             : 	mod &= 0x1;
Isn't this `mod = mt6316_read_field(slvid, MT6316_BUCK_TOP_4PHASE_TOP_ANA_CON0, 0x1, mod_shift)`?

https://review.coreboot.org/c/coreboot/+/85630/comment/721277e1_ab7ee4be?usp... :
PS9, Line 165: mod ? true : false
`!!mod`

https://review.coreboot.org/c/coreboot/+/85630/comment/2ea5958e_28717afb?usp... :
PS9, Line 186: mt6316_slave_id[i], mt6316_read_field(mt6316_slave_id[i],
             : 		       MT6316_PMIC_SWCID_H_ADDR, 0xFF, 0x0));
```
mt6316_slave_id[i],
mt6316_read_field(mt6316_slave_id[i], MT6316_PMIC_SWCID_H_ADDR,
                  0xFF, 0x0));
```