Re: [PATCH] pinctrl: mediatek: Add support for MT8196

Cathy Xu (许华婷)
2025-01-14T07:47:29Z

On Wed, 2024-12-11 at 08:56 +0100, Krzysztof Kozlowski wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>
>
> On 11/12/2024 06:54, ot907280 wrote:
> > From: Guodong Liu <guodong.liu@mediatek.corp-partner.google.com>
> >
> > Add pinctrl driver for mt8196
> >
> > Signed-off-by: Guodong Liu <guodong.liu@mediatek.com>
> > Signed-off-by: ot907280 <ot_cathy.xu@mediatek.com>
>
>
> Please use real names, not logins.
>
>
> Please use scripts/get_maintainers.pl to get a list of necessary
> people
> and lists to CC. It might happen, that command when run on an older
> kernel, gives you outdated entries. Therefore please be sure you base
> your patches on recent Linux kernel.
>
> Tools like b4 or scripts/get_maintainer.pl provide you proper list of
> people, so fix your workflow. Tools might also fail if you work on
> some
> ancient tree (don't, instead use mainline) or work on fork of kernel
> (don't, instead use mainline). Just use b4 and everything should be
> fine, although remember about `b4 prep --auto-to-cc` if you added new
> patches to the patchset.
>
> You missed at least devicetree list (maybe more), so this won't be
> tested by automated tooling. Performing review on untested code might
> be
> a waste of time.
>
> Please kindly resend and include all necessary To/Cc entries.
>
Thank you for your review. I will use real names and run
scripts/get_maintainers.pl on recent Linux kernel in next version.

>
>
> > ---
> > drivers/pinctrl/mediatek/Kconfig | 12 +
> > drivers/pinctrl/mediatek/Makefile | 1 +
> > drivers/pinctrl/mediatek/pinctrl-mt8196.c | 1757 +++++++++++
> > drivers/pinctrl/mediatek/pinctrl-mtk-mt8196.h | 2791
> > +++++++++++++++++
> > include/dt-bindings/pinctrl/mt8196-pinfunc.h | 1572 ++++++++++
> > 5 files changed, 6133 insertions(+)
> > create mode 100644 drivers/pinctrl/mediatek/pinctrl-mt8196.c
> > create mode 100644 drivers/pinctrl/mediatek/pinctrl-mtk-mt8196.h
> > create mode 100644 include/dt-bindings/pinctrl/mt8196-pinfunc.h
>
> Please run scripts/checkpatch.pl and fix reported warnings. Then
> please
> run `scripts/checkpatch.pl --strict` and (probably) fix more
> warnings.
> Some warnings can be ignored, especially from --strict run, but the
> code
> here looks like it needs a fix. Feel free to get in touch if the
> warning
> is not clear.
Sorry, the other warnings reported by run `scripts/checkpatch.pl --
strict` will be fixed in next version, but it didn't report the warning
here. Could you please tell me what warning you are referring to here?
Thanks~

>
>
>
> > +static const char * const mt8196_pinctrl_register_base_names[] = {
> > + "iocfg0", "iocfg_rt", "iocfg_rm1", "iocfg_rm2",
> > + "iocfg_rb", "iocfg_bm1", "iocfg_bm2", "iocfg_bm3",
> > + "iocfg_lt", "iocfg_lm1", "iocfg_lm2", "iocfg_lb1",
> > + "iocfg_lb2", "iocfg_tm1", "iocfg_tm2", "iocfg_tm3",
> > +};
> > +
> > +static const struct mtk_pin_soc mt8196_data = {
> > + .reg_cal = mt8196_reg_cals,
> > + .pins = mtk_pins_mt8196,
> > + .npins = ARRAY_SIZE(mtk_pins_mt8196),
> > + .ngrps = ARRAY_SIZE(mtk_pins_mt8196),
> > + .nfuncs = 8,
> > + .gpio_m = 0,
> > + .base_names = mt8196_pinctrl_register_base_names,
> > + .nbase_names =
> > ARRAY_SIZE(mt8196_pinctrl_register_base_names),
> > + .pull_type = mt8196_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 mt8196_pinctrl_of_match[] = {
> > + { .compatible = "mediatek,mt8196-pinctrl", .data =
> > &mt8196_data },
>
>
> Please run scripts/checkpatch.pl and fix reported warnings. Then
> please
> run `scripts/checkpatch.pl --strict` and (probably) fix more
> warnings.
> Some warnings can be ignored, especially from --strict run, but the
> code
> here looks like it needs a fix. Feel free to get in touch if the
> warning
> is not clear.
Sorry, it's the same here, no warnings were reported here. Could you
please point it out? Thank you~

>
> > + { }
> > +};
> > +
> > +static struct platform_driver mt8196_pinctrl_driver = {
> > + .driver = {
> > + .name = "mt8196-pinctrl",
> > + .of_match_table = mt8196_pinctrl_of_match,
> > + .pm = &mtk_paris_pinctrl_pm_ops,
> > + },
> > + .probe = mtk_paris_pinctrl_probe,
> > +};
> > +
> > +static int __init mt8196_pinctrl_init(void)
> > +{
> > + return platform_driver_register(&mt8196_pinctrl_driver);
> > +}
> > +arch_initcall(mt8196_pinctrl_init);
> > +
> > +MODULE_LICENSE("GPL");
>
>
> ...
>
> > +#endif /* __PINCTRL_MTK_MT8196_H */
> > diff --git a/include/dt-bindings/pinctrl/mt8196-pinfunc.h
> > b/include/dt-bindings/pinctrl/mt8196-pinfunc.h
> > new file mode 100644
> > index 000000000000..717351b12255
> > --- /dev/null
> > +++ b/include/dt-bindings/pinctrl/mt8196-pinfunc.h
> > @@ -0,0 +1,1572 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
> > +/*
> > + * Copyright (C) 2024 Mediatek Inc.
> > + * Author: Guodong Liu <Guodong.Liu@mediatek.com>
> > + */
> > +
> > +#ifndef __MT8196_PINFUNC_H
> > +#define __MT8196_PINFUNC_H
> > +
> > +#include <dt-bindings/pinctrl/mt65xx.h>
> > +
> > +#define PINMUX_GPIO0__FUNC_GPIO0 (MTK_PIN_NO(0) | 0)
> > +#define PINMUX_GPIO0__FUNC_DMIC1_CLK (MTK_PIN_NO(0) | 1)
> > +#define PINMUX_GPIO0__FUNC_SPI3_A_MO (MTK_PIN_NO(0) | 3)
> > +#define PINMUX_GPIO0__FUNC_FMI2S_B_LRCK (MTK_PIN_NO(0) | 4)
> > +#define PINMUX_GPIO0__FUNC_SCP_DMIC1_CLK (MTK_PIN_NO(0) | 5)
> > +#define PINMUX_GPIO0__FUNC_TP_GPIO14_AO (MTK_PIN_NO(0) | 6)
>
>
> These aren't really bindings. If you disagree, point me to the usage
> in
> the driver.
Sorry, I misunderstood the meaning of the bindings earlier. The
really bindings will be uploaded in next version.

>
>
>
> Best regards,
> Krzysztof