Re: [PATCH v8 6/6] pinctrl: mediatek: Add MT6735 pinctrl driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Missing `.pull_type` field initialization in `mt6735m_data` causes an out-of-bounds array read during pin configuration.
- [High] The `mt6735_pull_type` array is smaller than the total number of registered pins, causing an out-of-bounds read when virtual EINT GPIOs are accessed.
- [High] The `WB SPI` pins incorrectly share RDSEL register bits with `MSDC0_CMD` due to a missing cross-address stride, overwriting its configuration.
- [Medium] The `mt6735_eint_hw` struct specifies `.db_cnt = 16` but completely omits the `.db_time` array initialization, breaking external interrupt debounce.
- [Low] The `PINCTRL_MT6735` configuration symbol lacks architecture-specific dependencies, violating cross-config consistency rules.
- [Low] The MAINTAINERS file assigns the device tree binding file for the unrelated MT6779 platform to the MT6735 maintainer section.
--

commit 6bc2f83078b0f10dd52d6f0637645ffc40df6f25
Author: Yassine Oudjana <y.oudjana@protonmail.com>
Subject: pinctrl: mediatek: Add MT6735 pinctrl driver

This commit introduces a new pin controller driver for the MediaTek MT6735
and MT6735M SoCs. It includes register mappings, pin definitions, and device
tree matching data.

> diff --git a/MAINTAINERS b/MAINTAINERS
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21087,6 +21087,9 @@ PIN CONTROLLER - MEDIATEK MT6735
>  M:	Yassine Oudjana <y.oudjana@protonmail.com>
>  L:	linux-mediatek@lists.infradead.org (moderated for non-subscribers)
>  S:	Maintained

[Severity: Low]
Is this the correct binding file for the MT6735 section? The path targets
mediatek,mt6779-pinctrl.yaml instead of the MT6735 binding, which might
misdirect get_maintainer.pl routing.

> +F:	Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml
> +F:	drivers/pinctrl/mediatek/pinctrl-mt6735.c

[ ... ]

> diff --git a/drivers/pinctrl/mediatek/Kconfig b/drivers/pinctrl/mediatek/Kconfig
> --- a/drivers/pinctrl/mediatek/Kconfig
> +++ b/drivers/pinctrl/mediatek/Kconfig
> @@ -148,6 +148,12 @@ config PINCTRL_MT2712
>  	default ARM64 && ARCH_MEDIATEK
>  	select PINCTRL_MTK
>  

[Severity: Low]
Does this symbol need an architecture dependency? Other similar MediaTek
drivers use depends on ARM64 || COMPILE_TEST. Without it, this option
might clutter menuconfig on unrelated architectures that support OF.

> +config PINCTRL_MT6735
> +	bool "MediaTek MT6735(M) pin control"
> +	depends on OF
> +	default ARM64 && ARCH_MEDIATEK
> +	select PINCTRL_MTK_PARIS

[ ... ]

> diff --git a/drivers/pinctrl/mediatek/pinctrl-mt6735.c b/drivers/pinctrl/mediatek/pinctrl-mt6735.c
> --- /dev/null
> +++ b/drivers/pinctrl/mediatek/pinctrl-mt6735.c

[ ... ]

> +static const struct mtk_pin_field_calc mt6735_pin_rdsel_range[] = {
> +	PINS_FIELD_BASE(0, 4, 2, GPIO_RDSEL, 32, 0x04, 24, 2),		/* EINT0~4 */

[ ... ]

> +	PINS_FIELD_BASE(183, 183, 6, GPIO_RDSEL, 32, 0, 24, 6),		/* MSDC0_RSTB */
> +	PINS_FIELD_BASE(184, 185, 6, GPIO_RDSEL, 32, 0, 30, 2),		/* FM */

[Severity: High]
Could the missing cross-address stride here cause configuration overwrites?
In mt6735_pin_rdsel_range, the WB SPI pins specify an s_bit of 32 but
leave _x_addrs as 0. Because of the wrap-around in the offset calculation,
writing RDSEL for WB SPI appears to target bits 0-1 of GPIO_RDSEL, which
would clobber the configuration for MSDC0_CMD.

> +	PINS_FIELD_BASE(186, 189, 6, GPIO_RDSEL, 32, 0, 32, 2),		/* WB SPI */
> +	PINS_FIELD_BASE(198, 198, 1, GPIO_RDSEL, 32, 0, 4, 6),		/* MSDC2_CMD */

[ ... ]

[Severity: High]
Is it possible for this array to cause an out-of-bounds read?
The driver registers 213 total pins in mtk_pins_mt6735, but this
mt6735_pull_type array only explicitly defines up to index 203.
If the pinctrl framework queries the bias configuration for any virtual
EINT GPIO (indexes 204 and above), it looks like it will read past the end
of the array.

> +static const unsigned int mt6735_pull_type[] = {
> +	MTK_PULL_PULLSEL_TYPE,		/*   0 */
> +	MTK_PULL_PULLSEL_TYPE,		/*   1 */

[ ... ]

> +static const char * const mt6735_pinctrl_register_base_names[] = {
> +	"gpio", "iocfg0", "iocfg1", "iocfg2", "iocfg3", "iocfg4", "iocfg5"
> +};
> +
> +static const struct mtk_eint_hw mt6735_eint_hw = {
> +	.port_mask = 7,
> +	.ports     = 6,
> +	.ap_num    = 224,

[Severity: Medium]
Will external interrupt debounce work without the .db_time array defined?
The mtk-eint driver seems to explicitly check if eint->hw->db_time is
present, returning -EOPNOTSUPP if it is omitted. This might leave the
hardware debounce feature non-functional.

> +	.db_cnt    = 16,
> +};
> +
> +static const struct mtk_pin_soc mt6735_data = {

[ ... ]

> +static const struct mtk_pin_soc mt6735m_data = {
> +	.reg_cal = mt6735m_reg_cals,
> +	.pins = mtk_pins_mt6735m,
> +	.npins = ARRAY_SIZE(mtk_pins_mt6735m),
> +	.ngrps = ARRAY_SIZE(mtk_pins_mt6735m),
> +	.eint_hw = &mt6735_eint_hw,
> +	.gpio_m = 0,
> +	.ies_present = true,
> +	.base_names = mt6735_pinctrl_register_base_names,
> +	.nbase_names = ARRAY_SIZE(mt6735_pinctrl_register_base_names),

[Severity: High]
Does the absence of the .pull_type field here create a memory safety issue?
Without the pull_type pointer in mt6735m_data, Paris pinctrl framework
functions like mtk_pinconf_bias_get_combo will fall back to
MTK_PULL_TYPE_MASK and read the PINCTRL_PIN_REG_RSEL (index 23) register
configuration. Since mt6735m_reg_cals stops at PINCTRL_PIN_REG_PULLSEL
(index 18), this fallback path will read out of bounds.

> +	.bias_set_combo = mtk_pinconf_bias_set_combo,
> +	.bias_get_combo = mtk_pinconf_bias_get_combo,
> +	.drive_set = mtk_pinconf_drive_set_rev1,
> +	.drive_get = mtk_pinconf_drive_get_rev1,
> +	.adv_pull_get = mtk_pinconf_adv_pull_get,
> +	.adv_pull_set = mtk_pinconf_adv_pull_set,
> +};

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260530145800.1029920-1-y.oudjana@protonmail.com?part=6