[M] Change in coreboot[main]: soc/mediatek: Remove redundant struct pad_func and PAD_* definitions

[Yu-Ping Wu (Code Review)]

Attention is currently required from: Hung-Te Lin, Xi Chen, Yidi Lin.

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

Change subject: soc/mediatek: Remove redundant struct pad_func and PAD_* definitions
......................................................................

Patch Set 2:

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/84222/comment/0636dcde_3fc4e853?usp... :
PS2, Line 59: }
> The driver that uses `PAD_FUNC` either `OMITS` pull_select setting (because it may have external pul […]

I mean, `GPIO_PULL_DOWN` is 0, so the current `PAD_FUNC` macro defined here is equivalent to setting `.select = GPIO_PULL_DOWN`. I'd like to avoid the implicit initialization of the `select` field here.

As you said, some drivers use the `select` field for `gpio_set_pull`, while others (such as mt8192/i2c.c) simply use a constant value for all pad funcs. In the latter case, the origin `pad_func` struct definition doesn't contain the `select` field because it's not used, and I'd like to keep it that way (well, at least not setting it to `GPIO_PULL_DOWN`). I can see a few options.

(1) Add `GPIO_PULL_INVALID = 0` to the `pull_select` enum, so that when the `.select =` is omitted, it will be an invalid value. `gpio_set_pull()` will need to print an error message on seeing `GPIO_PULL_INVALID`. I don't prefer this option.
(2) Have 2 `pad_func` structs, one with `select` and the other without. `PAD_FUNC()` creates the one without `select`, and `PAD_FUNC_SEL()` creates the one with `select`. Each driver must stick to the same type.
(3) Always explicitly specify `.select`, by introducing `PAD_FUNC_DOWN` and `PAD_FUNC_UP`, which are just syntactic sugar for `PAD_FUNC_SEL(..., GPIO_PULL_DOWN)` and `PAD_FUNC_SEL(..., GPIO_PULL_UP)`, respectively. The 2-arg macro `PAD_FUNC` should cease to exist. Considering `mtk_snfc_init_pad_func`, this option might be better than (2).

`PAD_FUNC_GPIO` doesn't have the problem at all, as `.func = 0` is already an invalid value (valid values are 1-7).

https://review.coreboot.org/c/coreboot/+/84222/comment/5844a64e_e9db19cc?usp... :
PS2, Line 59: PAD_##name##_FUNC_##func
> `PIN()` macro of `mt8173` and `mt8183` are different from others SoCs. […]

I didn't notice mt8173 and mt8183 are excluded from CB:84221. I think it's reasonable to use this common `PAD_FUNC` only if the common `PIN` is used as well. So, yes, excluding mt8173 and mt8183 from this patch sounds good to me.

BTW, is the exclusion of mt8173/mt8183 the reason why you created a new file gpio_defs.h, instead of putting those macros in gpio_common.h?