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

[Cathy Xu (许华婷) <ot_cathy.xu@xxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
> > 
> > Add pinctrl driver for mt8196
> > 
> > Signed-off-by: Guodong Liu <guodong.liu@xxxxxxxxxxxx>
> > Signed-off-by: ot907280 <ot_cathy.xu@xxxxxxxxxxxx>
> 
> 
> Please use real names, not logins.
> 
> <form letter>
> 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.
> </form letter>
  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@xxxxxxxxxxxx>
> > + */
> > +
> > +#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