Re: [PATCH 2/3] pinctrl: mediatek: Add driver for MT6858

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

On 7/10/26 17:00, Nikolai Burov via B4 Relay wrote:
> From: Nikolai Burov <nikolai.burov@jolla.com>
> 
> Add a pinctrl driver for the MT6858 (MediaTek Dimensity 7100) SoC.
> 
> Signed-off-by: Nikolai Burov <nikolai.burov@jolla.com>

Thanks for the patch!

However, there are a few things to improve; check below.

> ---
>   drivers/pinctrl/mediatek/Kconfig              |   10 +
>   drivers/pinctrl/mediatek/Makefile             |    1 +
>   drivers/pinctrl/mediatek/pinctrl-mt6858.c     | 1408 +++++++++++++++
>   drivers/pinctrl/mediatek/pinctrl-mtk-mt6858.h | 2301 +++++++++++++++++++++++++
>   4 files changed, 3720 insertions(+)
> 
> diff --git a/drivers/pinctrl/mediatek/Kconfig b/drivers/pinctrl/mediatek/Kconfig
> index 97980cc28b9c..95c5250150d7 100644
> --- a/drivers/pinctrl/mediatek/Kconfig
> +++ b/drivers/pinctrl/mediatek/Kconfig
> @@ -166,6 +166,16 @@ config PINCTRL_MT6797
>   	default ARM64 && ARCH_MEDIATEK
>   	select PINCTRL_MTK_PARIS
>   
> +config PINCTRL_MT6858
> +	bool "MediaTek MT6858 pin control"
> +	depends on OF
> +	depends on ARM64 || COMPILE_TEST
> +	default ARM64 && ARCH_MEDIATEK
> +	select PINCTRL_MTK_PARIS
> +	help
> +	  Say yes here to support pin controller and gpio driver
> +	  on the MediaTek MT6858 SoC.
> +
>   config PINCTRL_MT6878
>   	bool "MediaTek MT6878 pin control"
>   	depends on OF
> diff --git a/drivers/pinctrl/mediatek/Makefile b/drivers/pinctrl/mediatek/Makefile
> index 6dc17b0c23f9..6ee3833dc6e2 100644
> --- a/drivers/pinctrl/mediatek/Makefile
> +++ b/drivers/pinctrl/mediatek/Makefile
> @@ -22,6 +22,7 @@ obj-$(CONFIG_PINCTRL_MT6765)		+= pinctrl-mt6765.o
>   obj-$(CONFIG_PINCTRL_MT6779)		+= pinctrl-mt6779.o
>   obj-$(CONFIG_PINCTRL_MT6795)		+= pinctrl-mt6795.o
>   obj-$(CONFIG_PINCTRL_MT6797)		+= pinctrl-mt6797.o
> +obj-$(CONFIG_PINCTRL_MT6858)		+= pinctrl-mt6858.o
>   obj-$(CONFIG_PINCTRL_MT6878)		+= pinctrl-mt6878.o
>   obj-$(CONFIG_PINCTRL_MT6893)		+= pinctrl-mt6893.o
>   obj-$(CONFIG_PINCTRL_MT7622)		+= pinctrl-mt7622.o
> diff --git a/drivers/pinctrl/mediatek/pinctrl-mt6858.c b/drivers/pinctrl/mediatek/pinctrl-mt6858.c
> new file mode 100644
> index 000000000000..eeb4a6bbc0fe
> --- /dev/null
> +++ b/drivers/pinctrl/mediatek/pinctrl-mt6858.c
> @@ -0,0 +1,1408 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2025 MediaTek Inc.
> + *               Alice Chao <alice.chao@mediatek.com>
> + * Copyright (c) 2026 Jolla Mobile Ltd
> + *               Nikolai Burov <nikolai.burov@jolla.com>
> + */
> +
> +#include <linux/module.h>
> +#include "pinctrl-mtk-mt6858.h"
> +#include "pinctrl-paris.h"
> +

..snip..

> +static const struct mtk_pin_reg_calc mt6858_reg_cals[PINCTRL_PIN_REG_MAX] = {
> +	[PINCTRL_PIN_REG_MODE] = MTK_RANGE(mt6858_pin_mode_range),
> +	[PINCTRL_PIN_REG_DIR] = MTK_RANGE(mt6858_pin_dir_range),
> +	[PINCTRL_PIN_REG_DI] = MTK_RANGE(mt6858_pin_di_range),
> +	[PINCTRL_PIN_REG_DO] = MTK_RANGE(mt6858_pin_do_range),
> +	[PINCTRL_PIN_REG_SR] = MTK_RANGE(mt6858_pin_dir_range),

This is Slew Rate, not Direction... And I think this is wrong.

If you don't know where the slew rate registers are, or if that is an unsupported
operation, just omit PINCTRL_PIN_REG_SR.

> +	[PINCTRL_PIN_REG_SMT] = MTK_RANGE(mt6858_pin_smt_range),
> +	[PINCTRL_PIN_REG_IES] = MTK_RANGE(mt6858_pin_ies_range),
> +	[PINCTRL_PIN_REG_PU] = MTK_RANGE(mt6858_pin_pu_range),
> +	[PINCTRL_PIN_REG_PD] = MTK_RANGE(mt6858_pin_pd_range),
> +	[PINCTRL_PIN_REG_DRV] = MTK_RANGE(mt6858_pin_drv_range),
> +	[PINCTRL_PIN_REG_PUPD] = MTK_RANGE(mt6858_pin_pupd_range),
> +	[PINCTRL_PIN_REG_R0] = MTK_RANGE(mt6858_pin_r0_range),
> +	[PINCTRL_PIN_REG_R1] = MTK_RANGE(mt6858_pin_r1_range),
> +	[PINCTRL_PIN_REG_DRV_ADV] = MTK_RANGE(mt6858_pin_drv_adv_range),
> +	[PINCTRL_PIN_REG_RSEL] = MTK_RANGE(mt6858_pin_rsel_range),
> +};
> +
> +static const char * const mt6858_pinctrl_register_base_names[] = {
> +	"base", "lm", "rb", "bm2", "bm", "bm1", "lt", "lt1", "rt", "rt1",

Fix ordering:

"base", "lm", "rb", "bm", "bm1", "bm2", "lt", "lt1", "rt", "rt1",

> +};
> +
> +static const struct mtk_eint_hw mt6858_eint_hw = {
> +	.port_mask = 0xf,
> +	.ports     = 3,
> +	.ap_num    = 217,
> +	.db_cnt    = 36,
> +	.db_time   = debounce_time_mt6878,
> +};
> +
> +static const struct mtk_pin_soc mt6858_data = {
> +	.reg_cal = mt6858_reg_cals,
> +	.pins = mtk_pins_mt6858,
> +	.npins = ARRAY_SIZE(mtk_pins_mt6858),
> +	.ngrps = ARRAY_SIZE(mtk_pins_mt6858),
> +	.eint_hw = &mt6858_eint_hw,
> +	.eint_pin = eint_pins_mt6858,
> +	.nfuncs = 16,
> +	.gpio_m = 0,
> +	.base_names = mt6858_pinctrl_register_base_names,
> +	.nbase_names = ARRAY_SIZE(mt6858_pinctrl_register_base_names),
> +	.pull_type = mt6858_pull_type,
> +	.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_drive_get = mtk_pinconf_adv_drive_get_raw,
> +	.adv_drive_set = mtk_pinconf_adv_drive_set_raw,
> +};
> +
> +static const struct of_device_id mt6858_pinctrl_of_match[] = {
> +	{ .compatible = "mediatek,mt6858-pinctrl", .data = &mt6858_data },
> +	{ }

	{ /* sentinel */ }

> +};
> +
> +static struct platform_driver mt6858_pinctrl_driver = {
> +	.driver = {
> +		.name = "mt6858-pinctrl",
> +		.of_match_table = mt6858_pinctrl_of_match,
> +		.pm = &mtk_paris_pinctrl_pm_ops,

.pm = pm_sleep_ptr(&mtk_paris_pinctrl_pm_ops),

> +	},
> +	.probe = mtk_paris_pinctrl_probe,
> +};
> +
> +static int __init mt6858_pinctrl_init(void)
> +{
> +	return platform_driver_register(&mt6858_pinctrl_driver);
> +}
> +arch_initcall(mt6858_pinctrl_init);
> +
> +MODULE_DESCRIPTION("MediaTek MT6858 Pinctrl Driver");
> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-mt6858.h b/drivers/pinctrl/mediatek/pinctrl-mtk-mt6858.h
> new file mode 100644
> index 000000000000..f6d2ae2fd07e
> --- /dev/null
> +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-mt6858.h
> @@ -0,0 +1,2301 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2025 MediaTek Inc.
> + *               Alice Chao <alice.chao@mediatek.com>
> + * Copyright (c) 2026 Jolla Mobile Ltd
> + *               Nikolai Burov <nikolai.burov@jolla.com>
> + */
> +
> +#ifndef __PINCTRL_MTK_MT6858_H
> +#define __PINCTRL_MTK_MT6858_H
> +
> +#include "pinctrl-paris.h"
> +
> +#define MTK_PIN_VEINT(_number)			\
> +	MTK_PIN(				\
> +		_number, "veint" #_number,	\
> +		MTK_EINT_FUNCTION(0, _number),	\
> +		DRV_GRP4,			\
> +		MTK_FUNCTION(0, NULL)		\
> +	)
> +
> +static const struct mtk_pin_desc mtk_pins_mt6858[] = {

..snip..

> +
> +static struct mtk_eint_pin eint_pins_mt6858[] = {

..snip..

> +	MTK_EINT_PIN(119, 0, 72, 0),
> +	MTK_EINT_PIN(120, 0, 73, 0),
> +	MTK_EINT_PIN(121, 0, 74, 0),

Looks like you're missing ~70 pins here?

If those have no EINT support, you can declare them as

	MTK_EINT_PIN(122, EINT_INVALID_BASE, 0, 0),

...or you can add a macro to add a (sequential) range of pins with no EINT support.

Cheers,
Angelo

> +	MTK_EINT_PIN(197, 3, 0, 0),
> +	MTK_EINT_PIN(198, 3, 1, 0),
> +	MTK_EINT_PIN(199, 3, 2, 0),
> +	MTK_EINT_PIN(200, 3, 3, 0),
> +	MTK_EINT_PIN(201, 3, 4, 0),
> +	MTK_EINT_PIN(202, 3, 5, 0),
> +	MTK_EINT_PIN(203, 3, 6, 0),
> +	MTK_EINT_PIN(204, 3, 7, 0),
> +	MTK_EINT_PIN(205, 3, 8, 0),
> +	MTK_EINT_PIN(206, 3, 9, 0),
> +	MTK_EINT_PIN(207, 3, 10, 0),
> +	MTK_EINT_PIN(208, 3, 11, 0),
> +	MTK_EINT_PIN(209, 3, 12, 0),
> +	MTK_EINT_PIN(210, 3, 13, 0),
> +	MTK_EINT_PIN(211, 3, 14, 0),
> +	MTK_EINT_PIN(212, 3, 15, 0),
> +	MTK_EINT_PIN(213, 3, 16, 0),
> +	MTK_EINT_PIN(214, 3, 17, 0),
> +	MTK_EINT_PIN(215, 3, 18, 0),
> +	MTK_EINT_PIN(216, 3, 19, 0),
> +};
> +
> +#endif /* __PINCTRL_MTK_MT6858_H */
>