[coreboot-gerrit] [M] Change in coreboot[main]: soc/mediatek/mt8196: Add MT6685 Clock IC driver

[Yu-Ping Wu (Code Review)]

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

Yu-Ping Wu has posted comments on this change by Jarried Lin. ( https://review.coreboot.org/c/coreboot/+/85734?usp=email )

The change is no longer submittable: All-Comments-Resolved is unsatisfied now.

Change subject: soc/mediatek/mt8196: Add MT6685 Clock IC driver
......................................................................

Patch Set 6:

(6 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/85734/comment/6887e772_d7468a87?usp... :
PS6, Line 10: ,
`.`

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

https://review.coreboot.org/c/coreboot/+/85734/comment/e3be3939_cd3151aa?usp... :
PS6, Line 72: 0xFFFF
Are you sure this is correct? `pmif_spmi_read` should only read 1 byte of data, so that (when shift is 0) the mask should be at most 0xFF.

https://review.coreboot.org/c/coreboot/+/85734/comment/6dc7f126_6acffbb5?usp... :
PS6, Line 78: key_protect_setting
We should use a different struct other than `mt6685_setting` if only `addr` and `val` are used. For example

```
static const struct {
    u16 addr;
    u16 val;
} key_protect_setting[] = {
    {0x39E, 0x7A},
    ...
};
```

Please also move the key_protect_setting declaration to right above this function (line #74).

https://review.coreboot.org/c/coreboot/+/85734/comment/6fcdb22f_478a31f7?usp... :
PS6, Line 92: [MT6685]
Remove "MT6685", as it's already included in the function name.

https://review.coreboot.org/c/coreboot/+/85734/comment/5fffbf6a_cd19142f?usp... :
PS6, Line 103: MT6685
Remove "MT6685", as it's already included in the function name.

https://review.coreboot.org/c/coreboot/+/85734/comment/0df96b31_d1f16cb9?usp... :
PS6, Line 104: 0xFFFF
Same here.