Re: [PATCH] pinctrl: mediatek: Add pinctrl driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

On Mon, 2024-11-11 at 14:39 +0100, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Il 11/11/24 08:40, ot907280 ha scritto:
> > From: Guodong Liu <guodong.liu@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
> > 
> > Add pinctrl driver for mt8196
> > 
> 
> Please fix the commit title, add a meaningful description ... and
> also please fix
> your name, as your email is sent by "ot907280" and not from "Cathy
> Xu".
  Thank you for your review.It will be fixed in next version.

> 
> > Signed-off-by: Guodong Liu <guodong.liu@xxxxxxxxxxxx>
> > Cathy Xu <ot_cathy.xu@xxxxxxxxxxxx>
> 
> You're missing the "Signed-off-by: " part before your name.
  It will be fixed 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
> > +++++++++++++++++
> >   4 files changed, 4561 insertions(+)
> >   create mode 100644 drivers/pinctrl/mediatek/pinctrl-mt8196.c
> >   create mode 100644 drivers/pinctrl/mediatek/pinctrl-mtk-mt8196.h
> > 
> > diff --git a/drivers/pinctrl/mediatek/Kconfig
> > b/drivers/pinctrl/mediatek/Kconfig
> > index a417a031659c..149a78e4216e 100644
> > --- a/drivers/pinctrl/mediatek/Kconfig
> > +++ b/drivers/pinctrl/mediatek/Kconfig
> > @@ -256,6 +256,18 @@ config PINCTRL_MT8195
> >       default ARM64 && ARCH_MEDIATEK
> >       select PINCTRL_MTK_PARIS
> > 
> > +config PINCTRL_MT8196
> > +     bool "MediaTek MT8196 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 MediaTek MT8196 SoC.
> > +       In MTK platform, we support virtual gpio and use it to
> > +       map specific eint which doesn't have real gpio pin.
> > +
> >   config PINCTRL_MT8365
> >       bool "MediaTek MT8365 pin control"
> >       depends on OF
> > diff --git a/drivers/pinctrl/mediatek/Makefile
> > b/drivers/pinctrl/mediatek/Makefile
> > index 1405d434218e..b4a39c1bafb7 100644
> > --- a/drivers/pinctrl/mediatek/Makefile
> > +++ b/drivers/pinctrl/mediatek/Makefile
> > @@ -35,6 +35,7 @@ obj-$(CONFIG_PINCTRL_MT8186)                +=
> > pinctrl-mt8186.o
> >   obj-$(CONFIG_PINCTRL_MT8188)                += pinctrl-mt8188.o
> >   obj-$(CONFIG_PINCTRL_MT8192)                += pinctrl-mt8192.o
> >   obj-$(CONFIG_PINCTRL_MT8195)                += pinctrl-mt8195.o
> > +obj-$(CONFIG_PINCTRL_MT8196)         += pinctrl-mt8196.o
> >   obj-$(CONFIG_PINCTRL_MT8365)                += pinctrl-mt8365.o
> >   obj-$(CONFIG_PINCTRL_MT8516)                += pinctrl-mt8516.o
> >   obj-$(CONFIG_PINCTRL_MT6397)                += pinctrl-mt6397.o
> > diff --git a/drivers/pinctrl/mediatek/pinctrl-mt8196.c
> > b/drivers/pinctrl/mediatek/pinctrl-mt8196.c
> > new file mode 100644
> > index 000000000000..6d2bee706718
> > --- /dev/null
> > +++ b/drivers/pinctrl/mediatek/pinctrl-mt8196.c
> > @@ -0,0 +1,1757 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2024 Mediatek Inc.
> > + * Author: Guodong Liu <Guodong.Liu@xxxxxxxxxxxx>
> > + *
> > + */
> > +
> > +#include <linux/module.h>
> > +#include "pinctrl-mtk-mt8196.h"
> > +#include "pinctrl-paris.h"
> > +
> > +#define PIN_FIELD_BASE(s_pin, e_pin, i_base, s_addr, x_addrs,
> > s_bit, x_bits)  \
> 
> It doesn't look like there's any s_pin with a different number from
> e_pin - unless
> I am misreading something, you can change `s_pin` to be named
> `se_pin`, so that we
> stop declaring the number twice; makes it a little more readable.
  The definitions of the macros 'PIN_FIELD_BASE' and 'PIN_FIELD' both
  come from 'PIN_FIELD_CALC', where the s_pin and e_pin in 'PIN_FIELD' 
  are different. This way, the code only needs one line to represent
  the addresses of all pins.

  PIN_FIELD define:
  #define PIN_FIELD(_s_pin, _e_pin, _s_addr, _x_addrs, _s_bit,   
_x_bits)   \
          PIN_FIELD_CALC(_s_pin, _e_pin, 0, _s_addr, _x_addrs,   
_s_bit, _x_bits, 32, 0)
  PIN_FIELD use:
  static const struct mtk_pin_field_calc mt8196_pin_mode_range[] = {
    PIN_FIELD(0, 270, 0x0300, 0x10, 0, 4),
};

> 
> > +     PIN_FIELD_CALC(s_pin, e_pin, i_base, s_addr, x_addrs, s_bit,
> > x_bits, \
> > +             32, 0)
> > +
> > +#define PINS_FIELD_BASE(s_pin, e_pin, i_base, s_addr, x_addrs,
> > s_bit, x_bits) \
> 
> Same here.
  Same as above.

> 
> > +     PIN_FIELD_CALC(s_pin, e_pin, i_base, s_addr, x_addrs, s_bit,
> > x_bits, \
> > +             32, 1)
> > +
> > +static const struct mtk_pin_field_calc mt8196_pin_mode_range[] = {
> > +     PIN_FIELD(0, 270, 0x0300, 0x10, 0, 4),
> > +};
> > +
> > +static const struct mtk_pin_field_calc mt8196_pin_dir_range[] = {
> > +     PIN_FIELD(0, 270, 0x0000, 0x10, 0, 1),
> > +};
> > +
> > +static const struct mtk_pin_field_calc mt8196_pin_di_range[] = {
> > +     PIN_FIELD(0, 270, 0x0200, 0x10, 0, 1),
> > +};
> > +
> > +static const struct mtk_pin_field_calc mt8196_pin_do_range[] = {
> > +     PIN_FIELD(0, 270, 0x0100, 0x10, 0, 1),
> > +};
> > +
> 
> ..snip..
> 
> > +static const unsigned int mt8196_pull_type[] = {
> > +     MTK_PULL_PU_PD_TYPE,/*0*/               MTK_PULL_PU_PD_TYPE,/
> > *1*/
> > +     MTK_PULL_PU_PD_TYPE,/*2*/               MTK_PULL_PU_PD_TYPE,/
> > *3*/
> > +     MTK_PULL_PU_PD_TYPE,/*4*/               MTK_PULL_PU_PD_TYPE,/
> > *5*/
> > +     MTK_PULL_PU_PD_TYPE,/*6*/               MTK_PULL_PU_PD_TYPE,/
> > *7*/
> > +     MTK_PULL_PU_PD_TYPE,/*8*/               MTK_PULL_PU_PD_TYPE,/
> > *9*/
> > +     MTK_PULL_PU_PD_TYPE,/*10*/              MTK_PULL_PU_PD_TYPE,/
> > *11*/
> > +     MTK_PULL_PU_PD_TYPE,/*12*/              MTK_PULL_PU_PD_TYPE,/
> > *13*/
> > +     MTK_PULL_PU_PD_TYPE,/*14*/              MTK_PULL_PU_PD_TYPE,/
> > *15*/
> > +     MTK_PULL_PU_PD_TYPE,/*16*/              MTK_PULL_PU_PD_TYPE,/
> > *17*/
> > +     MTK_PULL_PU_PD_TYPE,/*18*/              MTK_PULL_PU_PD_TYPE,/
> > *19*/
> > +     MTK_PULL_PU_PD_TYPE,/*20*/              MTK_PULL_PU_PD_TYPE,/
> > *21*/
> > +     MTK_PULL_PU_PD_TYPE,/*22*/              MTK_PULL_PU_PD_TYPE,/
> > *23*/
> > +     MTK_PULL_PU_PD_TYPE,/*24*/              MTK_PULL_PU_PD_TYPE,/
> > *25*/
> > +     MTK_PULL_PU_PD_TYPE,/*26*/              MTK_PULL_PU_PD_TYPE,/
> > *27*/
> > +     MTK_PULL_PU_PD_TYPE,/*28*/              MTK_PULL_PU_PD_TYPE,/
> > *29*/
> > +     MTK_PULL_PU_PD_TYPE,/*30*/              MTK_PULL_PU_PD_TYPE,/
> > *31*/
> > +     MTK_PULL_PU_PD_TYPE,/*32*/              MTK_PULL_PU_PD_TYPE,/
> > *33*/
> > +     MTK_PULL_PU_PD_TYPE,/*34*/              MTK_PULL_PU_PD_TYPE,/
> > *35*/
> > +     MTK_PULL_PU_PD_TYPE,/*36*/              MTK_PULL_PU_PD_TYPE,/
> > *37*/
> > +     MTK_PULL_PU_PD_TYPE,/*38*/              MTK_PULL_PU_PD_TYPE,/
> > *39*/
> > +     MTK_PULL_PU_PD_TYPE,/*40*/              MTK_PULL_PU_PD_TYPE,/
> > *41*/
> > +     MTK_PULL_PU_PD_TYPE,/*42*/              MTK_PULL_PU_PD_TYPE,/
> > *43*/
> > +     MTK_PULL_PU_PD_TYPE,/*44*/              MTK_PULL_PU_PD_TYPE,/
> > *45*/
> > +     MTK_PULL_PU_PD_RSEL_TYPE,/*46*/
> > MTK_PULL_PU_PD_RSEL_TYPE,/*47*/
> > +     MTK_PULL_PU_PD_RSEL_TYPE,/*48*/
> > MTK_PULL_PU_PD_RSEL_TYPE,/*49*/
> > +     MTK_PULL_PU_PD_RSEL_TYPE,/*50*/
> > MTK_PULL_PU_PD_RSEL_TYPE,/*51*/
> 
> Please fix the indentation to be consistent.
  It will be fixed in next version.

> 
> > +     MTK_PULL_PU_PD_RSEL_TYPE,/*52*/
> > MTK_PULL_PU_PD_RSEL_TYPE,/*53*/
> 
> ..snip..
> 
> > +
> > +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),
> 
> Where is eint?!
  It will be add in next version.
> 
> > +     .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 },
> > +     { }
> 
>         { /* sentinel */ }
> 
> > +};
> 
> Regards,
> Angelo
>
[Index of Archives]     [Linux SPI]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux