[v1,1/2] extcon: add Realtek DHC RTD SoC Type-C driver

This patch adds the extcon driver for Realtek DHC (digital home center)
RTD SoCs type-c module. This can be used to detect whether the port is
configured as a downstream or upstream facing port. And notify the status
of extcon to listeners.

Signed-off-by: Stanley Chang <stanley_chang@realtek.com>
---
 drivers/extcon/Kconfig             |    9 +
 drivers/extcon/Makefile            |    1 +
 drivers/extcon/extcon-rtk-type-c.c | 1799 ++++++++++++++++++++++++++++
 3 files changed, 1809 insertions(+)
 create mode 100644 drivers/extcon/extcon-rtk-type-c.c

Hi Stanley,

kernel test robot noticed the following build errors:

[auto build test ERROR on chanwoo-extcon/extcon-next]
[also build test ERROR on linus/master v6.5-rc7 next-20230823]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Stanley-Chang/dt-bindings-extcon-Add-Realtek-DHC-RTD-SoC-Type-C/20230822-183332
base:   https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git extcon-next
patch link:    https://lore.kernel.org/r/20230822102846.4683-1-stanley_chang%40realtek.com
patch subject: [PATCH v1 1/2] extcon: add Realtek DHC RTD SoC Type-C driver
config: loongarch-randconfig-r053-20230823 (https://download.01.org/0day-ci/archive/20230823/202308232214.4VvP0avv-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230823/202308232214.4VvP0avv-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308232214.4VvP0avv-lkp@intel.com/

All errors (new ones prefixed by >>):

   loongarch64-linux-ld: drivers/extcon/extcon-rtk-type-c.o: in function `.L394':
>> extcon-rtk-type-c.c:(.text+0x2a18): undefined reference to `usb_debug_root'
>> loongarch64-linux-ld: extcon-rtk-type-c.c:(.text+0x2a1c): undefined reference to `usb_debug_root'

Hi Chanwoo,

> 
> Almost code looks good to me. I add some comment below. Please check
> them.
> 
Thanks for your review.
> 
> On 23. 8. 22. 19:28, Stanley Chang wrote:
> > This patch adds the extcon driver for Realtek DHC (digital home center)
> > RTD SoCs type-c module. This can be used to detect whether the port is
> > configured as a downstream or upstream facing port. And notify the status
> > of extcon to listeners.
> >
> > Signed-off-by: Stanley Chang <stanley_chang@realtek.com>
> > ---
> >  drivers/extcon/Kconfig             |    9 +
> >  drivers/extcon/Makefile            |    1 +
> >  drivers/extcon/extcon-rtk-type-c.c | 1799
> ++++++++++++++++++++++++++++
> >  3 files changed, 1809 insertions(+)
> >  create mode 100644 drivers/extcon/extcon-rtk-type-c.c
> >
> > diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> > index 0ef1971d22bb..d670434321f2 100644
> > --- a/drivers/extcon/Kconfig
> > +++ b/drivers/extcon/Kconfig
> > @@ -190,4 +190,13 @@ config EXTCON_USBC_TUSB320
> >         Say Y here to enable support for USB Type C cable detection
> extcon
> >         support using a TUSB320.
> >
> > +config EXTCON_RTK_TYPE_C
> > +     tristate "Realtek RTD SoC extcon Type-C Driver"
> > +     depends on ARCH_REALTEK || COMPILE_TEST
> > +     help
> > +       Say Y here to enable extcon support for USB Type C cable detection
> > +       when using the Realtek RTD SoC USB Type-C port.
> > +       The DHC (Digital Home Hub) RTD series SoC contains a type c
> module.
> > +       This driver will detect the status of the type-c port.
> > +
> >  endif
> > diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> > index 1b390d934ca9..f779adb5e4c7 100644
> > --- a/drivers/extcon/Makefile
> > +++ b/drivers/extcon/Makefile
> > @@ -25,3 +25,4 @@ obj-$(CONFIG_EXTCON_SM5502) += extcon-sm5502.o
> >  obj-$(CONFIG_EXTCON_USB_GPIO)        += extcon-usb-gpio.o
> >  obj-$(CONFIG_EXTCON_USBC_CROS_EC) += extcon-usbc-cros-ec.o
> >  obj-$(CONFIG_EXTCON_USBC_TUSB320) += extcon-usbc-tusb320.o
> > +obj-$(CONFIG_EXTCON_RTK_TYPE_C) += extcon-rtk-type-c.o
> > diff --git a/drivers/extcon/extcon-rtk-type-c.c
> b/drivers/extcon/extcon-rtk-type-c.c
> > new file mode 100644
> > index 000000000000..04d4d5128bdb
> > --- /dev/null
> > +++ b/drivers/extcon/extcon-rtk-type-c.c
> > @@ -0,0 +1,1799 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + *  * extcon-rtk-type-c.c - Realtek Extcon Type C driver
> > + *
> > + * Copyright (C) 2023 Realtek Semiconductor Corporation
> > + *
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/slab.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/of_gpio.h>
> > +#include <linux/io.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/syscalls.h>
> > +#include <linux/suspend.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/extcon.h>
> > +#include <linux/extcon-provider.h>
> > +#include <linux/sys_soc.h>
> > +#include <linux/nvmem-consumer.h>
> > +#include <linux/usb/otg.h>
> > +#include <linux/usb/typec.h>
> > +
> > +struct cc_param {
> > +     u32 rp_4p7k_code;
> > +     u32 rp_36k_code;
> > +     u32 rp_12k_code;
> > +     u32 rd_code;
> > +     u32 ra_code;
> > +     u32 vref_2p6v;
> > +     u32 vref_1p23v;
> > +     u32 vref_0p8v;
> > +     u32 vref_0p66v;
> > +     u32 vref_0p4v;
> > +     u32 vref_0p2v;
> > +     u32 vref_1_1p6v;
> > +     u32 vref_0_1p6v;
> > +};
> > +
> > +struct type_c_cfg {
> > +     int parameter_ver; /* Parameter version */
> > +     int cc_dfp_mode;
> > +     struct cc_param cc1_param;
> > +     struct cc_param cc2_param;
> > +
> > +     u32 debounce_val;
> > +     bool use_defalut_parameter;
> > +};
> > +
> > +struct type_c_data {
> > +     void __iomem *reg_base;
> > +     struct device *dev;
> > +     struct extcon_dev *edev;
> > +
> > +     u32 irq;
> > +
> > +     /* rd control GPIO only for rtd1295 */
> > +     unsigned int rd_ctrl_gpio;
> > +
> > +     /* Parameters */
> > +     struct type_c_cfg *type_c_cfg;
> > +     u32 dfp_mode_rp_en;
> > +     u32 ufp_mode_rd_en;
> > +     u32 cc1_code;
> > +     u32 cc2_code;
> > +     u32 cc1_vref;
> > +     u32 cc2_vref;
> > +     u32 debounce; /* 1b,1us 7f,4.7us */
> > +
> > +     /* type_c state */
> > +     int connect_change;
> > +#define CONNECT_CHANGE 1
> > +#define CONNECT_NO_CHANGE 0
> > +     int cc_mode; /* cc is host or device */
> > +#define IN_HOST_MODE 0x10
> > +#define IN_DEVICE_MODE 0x20
> > +     int is_attach;
> > +#define IN_ATTACH 1
> > +#define TO_ATTACH 1
> > +#define IN_DETACH 0
> > +#define TO_DETACH 0
> > +     int at_cc1;
> > +#define AT_CC1 1
> > +#define AT_CC2 0
> > +
> > +     u32 int_status;
> > +     u32 cc_status;
> > +     /* protect the data member */
> > +     spinlock_t lock;
> > +     struct delayed_work delayed_work;
> > +
> > +     bool rd_en_at_first;
> > +
> > +     struct dentry *debug_dir;
> > +
> > +#ifdef CONFIG_TYPEC
> > +     struct typec_port *port;
> > +#endif /* CONFIG_TYPEC */
> > +};
> > +
> > +/* Type C register offset */
> > +#define USB_TYPEC_CTRL_CC1_0 0x0
> > +#define USB_TYPEC_CTRL_CC1_1 0x4
> > +#define USB_TYPEC_CTRL_CC2_0 0x8
> > +#define USB_TYPEC_CTRL_CC2_1 0xC
> > +#define USB_TYPEC_STS        0x10
> > +#define USB_TYPEC_CTRL       0x14
> > +#define USB_DBUS_PWR_CTRL    0x18
> 
> nitpick.
> Above defintions uses 'space' to keep the left-aligned of value.
> But, below defintions doesn't keep the left-aligned of value.
> 
> I recommend that you better to use the same style for the readability.

Ok, I will revise.

> > +
> > +#define ENABLE_CC1 0x1
> > +#define ENABLE_CC2 0x2
> > +#define DISABLE_CC 0x0
> > +
> > +/* Bit mapping USB_TYPEC_CTRL_CC1_0 and USB_TYPEC_CTRL_CC2_0 */
> > +#define PLR_EN BIT(29)
> > +#define CC_SWITCH_MASK (BIT(29) | BIT(28) | BIT(27))
> > +#define CC_CODE_MASK (0xfffff << 7)
> > +#define rp4pk_code(val) ((0x1f & (val)) << 22)
> > +#define code_rp4pk(val) (((val) >> 22) & 0x1f)
> > +#define rp36k_code(val) ((0x1f & (val)) << 17)
> > +#define code_rp36k(val) (((val) >> 17) & 0x1f)
> > +#define rp12k_code(val) ((0x1f & (val)) << 12)
> > +#define code_rp12k(val) (((val) >> 12) & 0x1f)
> > +#define rd_code(val) ((0x1f & (val)) << 7)
> > +#define code_rd(val) (((val) >> 7) & 0x1f)
> > +#define dfp_mode(val) ((0x3 & (val)) << 5)
> > +#define EN_RP4P7K BIT(4)
> > +#define EN_RP36K BIT(3)
> > +#define EN_RP12K BIT(2)
> > +#define EN_RD BIT(1)
> > +#define EN_CC_DET BIT(0)
> > +
> > +#define CC_MODE_UFP 0x0
> > +#define CC_MODE_DFP_USB 0x1
> > +#define CC_MODE_DFP_1_5 0x2
> > +#define CC_MODE_DFP_3_0 0x3
> > +
> > +/*
> > + * PARAMETER_V0:
> > + *  Realtek Kylin    rtd1295
> > + *  Realtek Hercules rtd1395
> > + *  Realtek Thor     rtd1619
> > + *  Realtek Hank     rtd1319
> > + *  Realtek Groot    rtd1312c
> > + * PARAMETER_V1:
> > + *  Realtek Stark    rtd1619b
> > + *  Realtek Parker   rtd1319d
> > + *  Realtek Danvers  rtd1315e
> > + */
> > +enum parameter_version {
> > +     PARAMETER_V0 = 0,
> > +     PARAMETER_V1 = 1,
> > +};
> > +
> > +/* Bit mapping USB_TYPEC_CTRL_CC1_1 and USB_TYPEC_CTRL_CC2_1 */
> > +#define V0_vref_2p6v(val) ((0xf & (val)) << 26) /* Bit 29 for groot */
> > +#define V0_vref_1p23v(val) ((0xf & (val)) << 22)
> > +#define V0_vref_0p8v(val) ((0xf & (val)) << 18)
> > +#define V0_vref_0p66v(val) ((0xf & (val)) << 14)
> > +#define V0_vref_0p4v(val) ((0x7 & (val)) << 11)
> > +#define V0_vref_0p2v(val) ((0x7 & (val)) << 8)
> > +#define V0_vref_1_1p6v(val) ((0xf & (val)) << 4)
> > +#define V0_vref_0_1p6v(val) ((0xf & (val)) << 0)
> > +
> > +#define V0_decode_2p6v(val) (((val) >> 26) & 0xf) /* Bit 29 for groot */
> > +#define V0_decode_1p23v(val) (((val) >> 22) & 0xf)
> > +#define V0_decode_0p8v(val) (((val) >> 18) & 0xf)
> > +#define V0_decode_0p66v(val) (((val) >> 14) & 0xf)
> > +#define V0_decode_0p4v(val) (((val) >> 11) & 0x7)
> > +#define V0_decode_0p2v(val) (((val) >> 8) & 0x7)
> > +#define V0_decode_1_1p6v(val) (((val) >> 4) & 0xf)
> > +#define V0_decode_0_1p6v(val) (((val) >> 0) & 0xf)
> > +
> > +/* new Bit mapping USB_TYPEC_CTRL_CC1_1 and
> USB_TYPEC_CTRL_CC2_1 */
> > +#define V1_vref_2p6v(val) ((0xf & (val)) << 28)
> > +#define V1_vref_1p23v(val) ((0xf & (val)) << 24)
> > +#define V1_vref_0p8v(val) ((0xf & (val)) << 20)
> > +#define V1_vref_0p66v(val) ((0xf & (val)) << 16)
> > +#define V1_vref_0p4v(val) ((0xf & (val)) << 12)
> > +#define V1_vref_0p2v(val) ((0xf & (val)) << 8)
> > +#define V1_vref_1_1p6v(val) ((0xf & (val)) << 4)
> > +#define V1_vref_0_1p6v(val) ((0xf & (val)) << 0)
> > +
> > +#define V1_decode_2p6v(val) (((val) >> 28) & 0xf)
> > +#define V1_decode_1p23v(val) (((val) >> 24) & 0xf)
> > +#define V1_decode_0p8v(val) (((val) >> 20) & 0xf)
> > +#define V1_decode_0p66v(val) (((val) >> 16) & 0xf)
> > +#define V1_decode_0p4v(val) (((val) >> 12) & 0xf)
> > +#define V1_decode_0p2v(val) (((val) >> 8) & 0xf)
> > +#define V1_decode_1_1p6v(val) (((val) >> 4) & 0xf)
> > +#define V1_decode_0_1p6v(val) (((val) >> 0) & 0xf)
> > +
> > +/* Bit mapping USB_TYPEC_STS */
> > +#define DET_STS 0x7
> > +#define CC1_DET_STS (DET_STS)
> > +#define CC2_DET_STS (DET_STS << 3)
> > +#define DET_STS_RA 0x1
> > +#define DET_STS_RD 0x3
> > +#define DET_STS_RP 0x1
> > +#define CC1_DET_STS_RA (DET_STS_RA)
> > +#define CC1_DET_STS_RD (DET_STS_RD)
> > +#define CC1_DET_STS_RP (DET_STS_RP)
> > +#define CC2_DET_STS_RA (DET_STS_RA << 3)
> > +#define CC2_DET_STS_RD (DET_STS_RD << 3)
> > +#define CC2_DET_STS_RP (DET_STS_RP << 3)
> > +
> > +/* Bit mapping USB_TYPEC_CTRL */
> > +#define CC2_INT_EN BIT(11)
> > +#define CC1_INT_EN BIT(10)
> > +#define CC2_INT_STS BIT(9)
> > +#define CC1_INT_STS BIT(8)
> > +#define DEBOUNCE_TIME_MASK 0xff
> > +#define DEBOUNCE_EN BIT(0)
> > +#define ENABLE_TYPE_C_DETECT (CC1_INT_EN | CC2_INT_EN)
> > +#define ALL_CC_INT_STS (CC1_INT_STS | CC2_INT_STS)
> > +
> > +/* Parameter */
> > +#define DETECT_TIME 50 /* ms */
> > +
> > +static const unsigned int usb_type_c_cable[] = {
> > +     EXTCON_USB,
> > +     EXTCON_USB_HOST,
> > +     EXTCON_NONE,
> > +};
> > +
> > +enum usb_data_roles {
> > +     DR_NONE,
> > +     DR_HOST,
> > +     DR_DEVICE,
> > +};
> > +
> > +static const struct soc_device_attribute rtk_soc_kylin[] = {
> > +     { .family = "Realtek Kylin", },
> > +     { /* empty */ }
> > +};
> > +
> > +static int rtd129x_switch_type_c_plug_config(struct type_c_data *type_c,
> > +                                          int dr_mode, int cc)
> > +{
> > +     void __iomem *reg = type_c->reg_base + USB_TYPEC_CTRL_CC1_0;
> > +     int val_cc;
> > +
> > +#define TYPE_C_EN_SWITCH BIT(29)
> > +#define TYPE_C_TXRX_SEL (BIT(28) | BIT(27))
> > +#define TYPE_C_SWITCH_MASK (TYPE_C_EN_SWITCH | TYPE_C_TXRX_SEL)
> > +#define TYPE_C_ENABLE_CC1 TYPE_C_EN_SWITCH
> > +#define TYPE_C_ENABLE_CC2 (TYPE_C_EN_SWITCH | TYPE_C_TXRX_SEL)
> > +#define TYPE_C_DISABLE_CC ~TYPE_C_SWITCH_MASK
> > +
> > +     val_cc = readl(reg);
> 
> I'd like you to use regmap interface to access the register
> by using regmap_read, regmap_write. You can create the regmap instance
> via devm_regmap_init_mmio() on probe instead of using 'type_c->reg_base'
> at the multipe point.
> 
> For example,
>         struct regmap_config rtk_regmap_config = {
>                 .reg_bits = 32,
>                 .val_bits = 32,
>         };
> 
>         void __iomem *base;
> 
>         base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
>         if (IS_ERR(base))
>                 return PTR_ERR(base);
> 
>         regmap = devm_regmap_init_mmio(dev, base,
> &rtk_regmap_config);
> 
>         ---
> 
>         And then just call regmap_read without any calculation between
>         base address and specific register.
> 
>         regmap_read(regmap, USB_TYPEC_CTRL_CC1_0)

I will study this method.

> 
> > +     val_cc &= ~TYPE_C_SWITCH_MASK;
> > +
> > +     if (cc == DISABLE_CC) {
> > +             val_cc &= TYPE_C_DISABLE_CC;
> > +     } else if (cc == ENABLE_CC1) {
> > +             val_cc |= TYPE_C_ENABLE_CC1;
> > +     } else if (cc == ENABLE_CC2) {
> > +             val_cc |= TYPE_C_ENABLE_CC2;
> > +     } else {
> > +             dev_err(type_c->dev, "%s: Error cc setting cc=0x%x\n",
> __func__, cc);
> > +             return -EINVAL;
> > +     }
> > +     writel(val_cc, reg);
> > +
> > +     mdelay(1);
> Maybe, it depends on h/w constraints. Better to explain the comment
> of why adding delay at here. Also, this patch uses mdelay/msleep at
> the multiple point. I recommend that add the comment of reason to add
> mdelay/msleep.

Ok, I will add.
> 
> > +
> > +     dev_dbg(type_c->dev, "%s: cc=0x%x val_cc=0x%x
> usb_typec_ctrl_cc1_0=0x%x\n",
> > +             __func__, cc, val_cc, readl(reg));
> > +
> > +     return 0;
> > +}
> > +
> > +static inline void switch_type_c_plug_config(struct type_c_data *type_c,
> > +                                          int dr_mode, int cc)
> > +{
> > +     int ret = 0;
> > +
> > +     if (soc_device_match(rtk_soc_kylin))
> > +             ret = rtd129x_switch_type_c_plug_config(type_c, dr_mode,
> cc);
> > +
> > +     if (ret < 0)
> > +             dev_err(type_c->dev, "%s: Error set type c plug config\n",
> > +                     __func__);
> > +}
> > +
> > +static void switch_type_c_dr_mode(struct type_c_data *type_c, int
> dr_mode, int cc)
> > +{
> > +     bool is_host = false;
> > +     bool is_device = false;
> > +     bool polarity = false;
> > +     bool vbus = false;
> > +     bool ss = true;
> > +
> > +     switch_type_c_plug_config(type_c, dr_mode, cc);
> > +     if (cc == ENABLE_CC2)
> > +             polarity = true;
> > +
> > +     switch (dr_mode) {
> > +     case USB_DR_MODE_HOST:
> > +             is_host = true;
> > +             break;
> > +     case USB_DR_MODE_PERIPHERAL:
> > +             is_device = true;
> > +             vbus = true;
> > +             break;
> > +     default:
> > +             dev_dbg(type_c->dev, "%s dr_mode=%d ==> no host or
> device\n",
> > +                     __func__, dr_mode);
> > +             break;
> > +     }
> > +
> > +     dev_dbg(type_c->dev, "%s is_host=%d is_device=%d vbus=%d
> polarity=%d\n",
> > +             __func__, is_host, is_device, vbus, polarity);
> > +
> > +     /* for EXTCON_USB device mode */
> > +     extcon_set_state(type_c->edev, EXTCON_USB, is_device);
> > +     extcon_set_property(type_c->edev, EXTCON_USB,
> > +                         EXTCON_PROP_USB_VBUS,
> > +                         (union extcon_property_value)(int)vbus);
> > +     extcon_set_property(type_c->edev, EXTCON_USB,
> > +                         EXTCON_PROP_USB_TYPEC_POLARITY,
> > +                         (union extcon_property_value)(int)polarity);
> > +     extcon_set_property(type_c->edev, EXTCON_USB,
> > +                         EXTCON_PROP_USB_SS,
> > +                         (union extcon_property_value)(int)ss);
> > +
> > +     /* for EXTCON_USB_HOST host mode */
> > +     extcon_set_state(type_c->edev, EXTCON_USB_HOST, is_host);
> > +     extcon_set_property(type_c->edev, EXTCON_USB_HOST,
> > +                         EXTCON_PROP_USB_VBUS,
> > +                         (union extcon_property_value)(int)vbus);
> > +     extcon_set_property(type_c->edev, EXTCON_USB_HOST,
> > +                         EXTCON_PROP_USB_TYPEC_POLARITY,
> > +                         (union extcon_property_value)(int)polarity);
> > +     extcon_set_property(type_c->edev, EXTCON_USB_HOST,
> > +                         EXTCON_PROP_USB_SS,
> > +                         (union extcon_property_value)(int)ss);
> > +
> > +     /* sync EXTCON_USB and EXTCON_USB_HOST */
> > +     extcon_sync(type_c->edev, EXTCON_USB);
> > +     extcon_sync(type_c->edev, EXTCON_USB_HOST);
> > +
> > +#ifdef CONFIG_TYPEC> +       if (type_c->port) {
> > +             switch (dr_mode) {
> > +             case USB_DR_MODE_HOST:
> > +                     typec_set_data_role(type_c->port, TYPEC_HOST);
> > +                     typec_set_pwr_role(type_c->port,
> TYPEC_SOURCE);
> > +                     break;
> > +             case USB_DR_MODE_PERIPHERAL:
> > +                     typec_set_data_role(type_c->port,
> TYPEC_DEVICE);
> > +                     typec_set_pwr_role(type_c->port, TYPEC_SINK);
> > +                     break;
> > +             default:
> > +                     dev_dbg(type_c->dev, "%s unknown
> dr_mode=%d\n",
> > +                             __func__, dr_mode);
> > +                     break;
> > +             }
> > +     }
> > +#endif
> > +}
> > +
> > +/* device attached/detached */
> > +static int device_attached(struct type_c_data *type_c, u32 cc)
> > +{
> > +     void __iomem *reg = type_c->reg_base + USB_TYPEC_CTRL;
> > +
> > +     cancel_delayed_work(&type_c->delayed_work);
> > +
> > +     switch_type_c_dr_mode(type_c, USB_DR_MODE_HOST, cc);
> > +
> > +     writel(ENABLE_TYPE_C_DETECT | readl(reg), reg);
> > +
> > +     return 0;
> > +}
> 
> device_attached() funciton is same with host_connected()
> except for USR_BR_MODE_HOST, USB_DR_MODE_PERIPHERAL setting.
> 
> You can use only one function with 3rd paramer (dr_mode) as following
> in order to reduce the duplicated code.
> For example,
> static int connector_attached(struct type_c_data *type_c, u32 cc, int
> dr_mode)

I will simplify this.

> > +
> > +static int device_detached(struct type_c_data *type_c, u32 cc)
> > +{
> > +     void __iomem *reg = type_c->reg_base + USB_TYPEC_CTRL;
> > +
> > +     writel(~ENABLE_TYPE_C_DETECT & readl(reg), reg);
> > +
> > +     switch_type_c_dr_mode(type_c, 0, cc);
> > +
> > +     schedule_delayed_work(&type_c->delayed_work,
> msecs_to_jiffies(DETECT_TIME));
> > +
> > +     return 0;
> > +}
> 
> device_detached is perfectly same with host_disconnected.
> You can use the only one function to reduce the duplicated code
> 
> > +
> > +/* host connect/disconnect*/
> > +static int host_connected(struct type_c_data *type_c, u32 cc)
> > +{
> > +     void __iomem *reg = type_c->reg_base + USB_TYPEC_CTRL;
> > +
> > +     cancel_delayed_work(&type_c->delayed_work);
> > +
> > +     switch_type_c_dr_mode(type_c, USB_DR_MODE_PERIPHERAL, cc);
> > +
> > +     writel(ENABLE_TYPE_C_DETECT | readl(reg), reg);
> > +     return 0;
> > +}
> > +
> > +static int host_disconnected(struct type_c_data *type_c, u32 cc)
> > +{
> > +     void __iomem *reg = type_c->reg_base + USB_TYPEC_CTRL;
> > +
> > +     writel(~ENABLE_TYPE_C_DETECT & readl(reg), reg);
> > +
> > +     switch_type_c_dr_mode(type_c, 0, cc);
> > +
> > +     schedule_delayed_work(&type_c->delayed_work,
> msecs_to_jiffies(DETECT_TIME));
> > +
> > +     return 0;
> > +}
> > +
> > +/* detect host device switch */
> > +static int __detect_host_device(struct type_c_data *type_c, u32
> rp_or_rd_en)
> > +{
> > +     struct device *dev = type_c->dev;
> > +     void __iomem *reg_base = type_c->reg_base;
> > +     unsigned int gpio = type_c->rd_ctrl_gpio;
> > +     u32 cc1_config, cc2_config, default_ctrl;
> > +     u32 cc1_switch = 0;
> > +
> > +     default_ctrl = readl(reg_base + USB_TYPEC_CTRL) &
> DEBOUNCE_TIME_MASK;
> > +     writel(default_ctrl, reg_base + USB_TYPEC_CTRL);
> > +
> > +     cc1_config = readl(reg_base + USB_TYPEC_CTRL_CC1_0);
> > +     cc2_config = readl(reg_base + USB_TYPEC_CTRL_CC2_0);
> > +
> > +     cc1_config &= ~EN_CC_DET;
> > +     cc2_config &= ~EN_CC_DET;
> > +     writel(cc1_config, reg_base + USB_TYPEC_CTRL_CC1_0);
> > +     writel(cc2_config, reg_base + USB_TYPEC_CTRL_CC2_0);
> > +
> > +     if (soc_device_match(rtk_soc_kylin))
> > +             cc1_switch = cc1_config & CC_SWITCH_MASK;
> > +
> > +     cc1_config &= CC_CODE_MASK;
> > +     cc1_config |= rp_or_rd_en | cc1_switch;
> > +     cc2_config &= CC_CODE_MASK;
> > +     cc2_config |= rp_or_rd_en;
> > +     writel(cc2_config, reg_base + USB_TYPEC_CTRL_CC2_0);
> > +     writel(cc1_config, reg_base + USB_TYPEC_CTRL_CC1_0);
> > +
> > +     /* For kylin to disable external rd control gpio */
> > +     if (soc_device_match(rtk_soc_kylin)) {
> > +             if (gpio != -1 && gpio_is_valid(gpio)) {
> > +                     if (gpio_direction_output(gpio, 1))
> > +                             dev_err(dev, "%s ERROR rd_ctrl_gpio=1
> fail\n", __func__);
> > +             }
> > +     }
> > +
> > +     cc1_config |= EN_CC_DET;
> > +     cc2_config |= EN_CC_DET;
> > +     writel(cc1_config, reg_base + USB_TYPEC_CTRL_CC1_0);
> > +     writel(cc2_config, reg_base + USB_TYPEC_CTRL_CC2_0);
> > +
> > +     return 0;
> > +}
> > +
> > +static int detect_device(struct type_c_data *type_c)
> > +{
> > +     return __detect_host_device(type_c, type_c->dfp_mode_rp_en);
> > +}
> > +
> > +static int detect_host(struct type_c_data *type_c)
> > +{
> > +     return __detect_host_device(type_c, type_c->ufp_mode_rd_en);
> > +}
> > +
> > +static int host_device_switch_detection(struct type_c_data *type_c)
> > +{
> > +     if (type_c->cc_mode == IN_HOST_MODE) {
> > +             type_c->cc_mode = IN_DEVICE_MODE;
> > +             detect_host(type_c);
> > +     } else {
> > +             type_c->cc_mode = IN_HOST_MODE;
> > +             detect_device(type_c);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int detect_type_c_state(struct type_c_data *type_c)
> > +{
> > +     struct device *dev = type_c->dev;
> > +     void __iomem *reg_base = type_c->reg_base;
> > +     u32 int_status, cc_status, cc_status_check;
> > +     unsigned long flags;
> > +
> > +     spin_lock_irqsave(&type_c->lock, flags);
> > +
> > +     int_status = readl(reg_base + USB_TYPEC_CTRL);
> > +     cc_status = readl(reg_base + USB_TYPEC_STS);
> > +
> > +     type_c->connect_change = CONNECT_NO_CHANGE;
> > +
> > +     switch (type_c->cc_mode | type_c->is_attach) {
> > +     case IN_HOST_MODE | IN_ATTACH:
> > +             if (((cc_status & CC1_DET_STS) == CC1_DET_STS) &&
> type_c->at_cc1 == AT_CC1) {
> > +                     dev_dbg(dev, "IN host mode and cc1 device
> detach (cc_status=0x%x)",
> > +                             cc_status);
> > +                     type_c->is_attach = TO_DETACH;
> > +                     type_c->connect_change = CONNECT_CHANGE;
> > +             } else if (((cc_status & CC2_DET_STS) == CC2_DET_STS) &&
> > +                        type_c->at_cc1 == AT_CC2) {
> > +                     dev_dbg(dev, "IN host mode and cc2 device
> detach (cc_status=0x%x)",
> > +                             cc_status);
> > +                     type_c->is_attach = TO_DETACH;
> > +                     type_c->connect_change = CONNECT_CHANGE;
> > +             }
> > +             break;
> > +     case IN_HOST_MODE | IN_DETACH:
> > +             cc_status_check = readl(reg_base + USB_TYPEC_STS);
> > +             if (cc_status_check != (CC1_DET_STS | CC2_DET_STS)) {
> > +                     if (in_interrupt()) {
> > +                             mdelay(300);
> > +                     } else {
> > +                             spin_unlock_irqrestore(&type_c->lock,
> flags);
> > +                             msleep(300);
> > +                             spin_lock_irqsave(&type_c->lock, flags);
> > +                     }
> 
> As I commented above, it seems that there are some hardware constrains
> which requires the some delay when changing the h/w state.
> I recommend that you need to add the comment of why adding the
> delays/sleep.

Ok, I will add.

> > +                     cc_status_check = readl(reg_base +
> USB_TYPEC_STS);
> > +             }
> > +             if (cc_status != cc_status_check) {
> > +                     dev_warn(dev, "IN_HOST_MODE: cc_status
> (0x%x) != cc_status_check (0x%x)\n",
> > +                              cc_status, cc_status_check);
> > +                     cc_status = readl(reg_base + USB_TYPEC_STS);
> > +             }
> > +
> > +             if ((cc_status & CC1_DET_STS) == CC1_DET_STS_RD) {
> > +                     dev_dbg(dev, "IN host mode and cc1 device
> attach (cc_status=0x%x)",
> > +                             cc_status);
> > +                     type_c->is_attach = TO_ATTACH;
> > +                     type_c->at_cc1 = AT_CC1;
> > +                     type_c->connect_change = CONNECT_CHANGE;
> > +             } else if ((cc_status & CC2_DET_STS) == CC2_DET_STS_RD)
> {
> > +                     dev_dbg(dev, "In host mode and cc2 device attach
> (cc_status=0x%x)",
> > +                             cc_status);
> > +                     type_c->is_attach = TO_ATTACH;
> > +                     type_c->at_cc1 = AT_CC2;
> > +                     type_c->connect_change = CONNECT_CHANGE;
> > +             }
> > +             break;
> > +     case IN_DEVICE_MODE | IN_ATTACH:
> > +             if ((cc_status & CC1_DET_STS) < CC1_DET_STS_RP ||
> > +                 (cc_status & CC2_DET_STS) < CC2_DET_STS_RP) {
> > +                     /* Add a sw debounce to filter cc signal sent from
> apple pd adapter */
> > +                     mdelay(5);
> > +                     cc_status_check = readl(reg_base +
> USB_TYPEC_STS);
> > +
> > +                     if (cc_status != cc_status_check) {
> > +                             dev_dbg(dev, "IN_DEVICE_MODE:
> cc_status (0x%x) != cc_status_check (0x%x) maybe use a pd adapter\n",
> > +                                     cc_status, cc_status_check);
> > +                             cc_status = cc_status_check;
> > +                     }
> > +             }
> > +
> > +             if ((cc_status & CC1_DET_STS) < CC1_DET_STS_RP &&
> type_c->at_cc1 == AT_CC1) {
> > +                     dev_dbg(dev, "IN device mode and cc1 host
> disconnect (cc_status=0x%x)",
> > +                             cc_status);
> > +                     type_c->is_attach = TO_DETACH;
> > +                     type_c->connect_change = CONNECT_CHANGE;
> > +             } else if ((cc_status & CC2_DET_STS) < CC2_DET_STS_RP
> &&
> > +                        type_c->at_cc1 == AT_CC2) {
> > +                     dev_dbg(dev, "IN device mode and cc2 host
> disconnect (cc_status=0x%x)",
> > +                             cc_status);
> > +                     type_c->is_attach = TO_DETACH;
> > +                     type_c->connect_change = CONNECT_CHANGE;
> > +             }
> > +             break;
> > +     case IN_DEVICE_MODE | IN_DETACH:
> > +             cc_status_check = readl(reg_base + USB_TYPEC_STS);
> > +             if (cc_status_check != 0x0) {
> > +                     if (in_interrupt()) {
> > +                             mdelay(300);
> > +                     } else {
> > +                             spin_unlock_irqrestore(&type_c->lock,
> flags);
> > +                             msleep(300);
> > +                             spin_lock_irqsave(&type_c->lock, flags);
> > +                     }
> 
> ditto.
> 
> > +                     cc_status_check = readl(reg_base +
> USB_TYPEC_STS);
> > +             }
> > +
> > +             if (cc_status != cc_status_check) {
> > +                     dev_warn(dev, "IN_DEVICE_MODE: cc_status
> (0x%x) != cc_status_check (0x%x)\n",
> > +                              cc_status, cc_status_check);
> > +                     cc_status = readl(reg_base + USB_TYPEC_STS);
> > +             }
> > +
> > +             if ((cc_status & CC1_DET_STS) >= CC1_DET_STS_RP) {
> > +                     dev_dbg(dev, "IN device mode and cc1 host
> connect (cc_status=0x%x)",
> > +                             cc_status);
> > +                     type_c->at_cc1 = AT_CC1;
> > +                     type_c->is_attach = TO_ATTACH;
> > +                     type_c->connect_change = CONNECT_CHANGE;
> > +             } else if ((cc_status & CC2_DET_STS) >= CC2_DET_STS_RP) {
> > +                     dev_dbg(dev, "IN device mode and cc2 host
> connect (cc_status=0x%x)",
> > +                             cc_status);
> > +                     type_c->at_cc1 = AT_CC2;
> > +                     type_c->is_attach = TO_ATTACH;
> > +                     type_c->connect_change = CONNECT_CHANGE;
> > +             }
> > +             break;
> > +     default:
> > +             dev_err(dev, "error host or device mode (cc_mode=%d,
> is_attach=%d) ",
> > +                     type_c->cc_mode, type_c->is_attach);
> > +     }
> > +
> > +     type_c->int_status = int_status;
> > +     type_c->cc_status = cc_status;
> > +
> > +     spin_unlock_irqrestore(&type_c->lock, flags);
> > +     return 0;
> > +}
> > +
> > +static void host_device_switch(struct work_struct *work)
> > +{
> > +     struct type_c_data *type_c = container_of(work, struct type_c_data,
> > +
> delayed_work.work);
> > +     struct device *dev = type_c->dev;
> > +     unsigned long flags;
> > +     int connect_change = 0;
> > +     int cc_mode = 0;
> > +     int is_attach = 0;
> > +     int at_cc1 = 0;
> > +
> > +     spin_lock_irqsave(&type_c->lock, flags);
> > +     if (type_c->connect_change)
> > +             connect_change = type_c->connect_change;
> > +     spin_unlock_irqrestore(&type_c->lock, flags);
> 
> Better to convert this code as following if type_c->connect_change is NULL,
> you can reduce the lock/unlok operation of spin_lock. As you knew, spin_lock is
> expensive.
> 
>         if (type_c->connect_change) {
>                 spin_lock_irqsave(&type_c->lock, flags);
>                 connect_change = type_c->connect_change;
>                 spin_unlock_irqrestore(&type_c->lock, flags);
>         }

I want to protect type_c->connect_change.
Moving the lock to the if statement loses its original purpose.

> > +
> > +     if (!connect_change)
> > +             detect_type_c_state(type_c);
> > +
> > +     spin_lock_irqsave(&type_c->lock, flags);
> > +     if (type_c->connect_change) {
> > +             connect_change = type_c->connect_change;
> > +             cc_mode = type_c->cc_mode;
> > +             is_attach = type_c->is_attach;
> > +             at_cc1 = type_c->at_cc1;
> > +             type_c->connect_change = CONNECT_NO_CHANGE;
> > +     } else {
> > +             host_device_switch_detection(type_c);
> > +
> > +             schedule_delayed_work(&type_c->delayed_work,
> msecs_to_jiffies(DETECT_TIME));
> > +     }
> > +     spin_unlock_irqrestore(&type_c->lock, flags);
> > +
> > +     if (connect_change) {
> 
> nitpick.
> You can reduce the depth of indentation when connector_change is 0
> by changing the code as following: If possible, I always to try
> to reduce the indentation depth for the readability.
> 
>         if (!connect_change)
>                 return
> 
>         dev_dbg(dev, "%s: usb cable connection change\n", __func__);
>         if (cc_mode == IN_HOST_MODE) {
>                 ......
> 
Ok, I will revise.
> > +             dev_dbg(dev, "%s: usb cable connection change\n",
> __func__);
> > +             if (cc_mode == IN_HOST_MODE) {
> > +                     if (is_attach && at_cc1)
> > +                             device_attached(type_c, ENABLE_CC1);
> > +                     else if (is_attach && !at_cc1)
> > +                             device_attached(type_c, ENABLE_CC2);
> > +                     else
> > +                             device_detached(type_c, DISABLE_CC);
> > +             } else if (cc_mode == IN_DEVICE_MODE) {
> > +                     if (is_attach && at_cc1)
> > +                             host_connected(type_c, ENABLE_CC1);
> > +                     else if (is_attach && !at_cc1)
> > +                             host_connected(type_c, ENABLE_CC2);
> > +                     else
> > +                             host_disconnected(type_c,
> DISABLE_CC);
> > +             } else {
> > +                     dev_err(dev, "Error: IN unknown mode %d to %s
> at %s (cc_status=0x%x)\n",
> > +                             cc_mode, is_attach ? "attach" :
> "detach",
> > +                             at_cc1 ? "cc1" : "cc2",
> type_c->cc_status);
> > +             }
> > +             dev_info(dev, "Connection change OK: IN %s mode to %s at
> %s (cc_status=0x%x)\n",
> > +                      cc_mode == IN_HOST_MODE ? "host" : "device",
> > +                      is_attach ? "attach" : "detach",
> > +                      at_cc1 ? "cc1" : "cc2", type_c->cc_status);
> > +     }
> > +}
> > +
> > +static irqreturn_t type_c_detect_irq(int irq, void *__data)
> > +{
> > +     struct type_c_data *type_c = (struct type_c_data *)__data;
> > +     struct device *dev = type_c->dev;
> > +     void __iomem *reg = type_c->reg_base + USB_TYPEC_CTRL;
> > +     unsigned long flags;
> > +
> > +     detect_type_c_state(type_c);
> > +
> > +     spin_lock_irqsave(&type_c->lock, flags);
> > +
> > +     if (type_c->connect_change) {
> > +             dev_dbg(dev, "%s: IN %s mode to %s (at %s interrupt)
> int_status=0x%x, cc_status=0x%x",
> > +                     __func__,
> > +                     type_c->cc_mode == IN_HOST_MODE ? "host" :
> "device",
> > +                     type_c->is_attach ? "attach" : "detach",
> > +                     type_c->at_cc1 ? "cc1" : "cc2",
> > +                     type_c->int_status, type_c->cc_status);
> > +
> > +             /* clear interrupt status */
> > +             writel(~ALL_CC_INT_STS & readl(reg), reg);
> > +
> > +             cancel_delayed_work(&type_c->delayed_work);
> > +             schedule_delayed_work(&type_c->delayed_work,
> msecs_to_jiffies(0));
> > +     } else {
> > +             static int local_count;
> > +
> > +             if (local_count++ > 10) {
> 
> It seems that clear interrupt status once every 10 times.
> I think that it is not generic. You need to add the comment of why trying to
> clear
> interrupt once every 10 times.

I will add the comment.
> > +                     /* clear interrupt status */
> > +                     writel(~ALL_CC_INT_STS & readl(reg), reg);
> > +                     local_count = 0;
> > +             }
> > +     }
> > +
> > +     spin_unlock_irqrestore(&type_c->lock, flags);
> > +
> > +     return IRQ_HANDLED;
> > +}
> 
> (snip)
> 
> > +
> > +static int extcon_rtk_type_c_init(struct type_c_data *type_c)
> > +{
> > +     struct device *dev = type_c->dev;
> > +     unsigned long flags;
> > +     void __iomem *reg;
> > +     int val;
> > +
> > +     if ((type_c->rd_ctrl_gpio != -1) &&
> 
> nitpick.
> After getting the gpio number by using of_get_named_gpio() function,
> type_c->rd_ctrl_gpio is error number which is returned of_get_named_gpio().
> 
> In order to specify the more correctly, how about changing it as following?
> Because I don't use the '-1' constant variable without any constant defintion.
> Someone cannot understand the meaning of '-1'.
> 
>         if (!(type_c->rd_ctrl_gpio < 0))

I will revise.

> > +         gpio_request(type_c->rd_ctrl_gpio, dev->of_node->name))
> > +             dev_err(dev, "%s ERROR Request rd_ctrl_gpio  (id=%d)
> fail\n",
> > +                     __func__, type_c->rd_ctrl_gpio);
> > +
> > +     spin_lock_irqsave(&type_c->lock, flags);
> > +
> > +     /* set parameter */
> > +     reg = type_c->reg_base + USB_TYPEC_CTRL_CC1_0;
> > +     val = readl(reg);
> > +     val = (~CC_CODE_MASK & val) | (type_c->cc1_code &
> CC_CODE_MASK);
> > +     writel(val, reg);
> > +
> > +     reg = type_c->reg_base + USB_TYPEC_CTRL_CC2_0;
> > +     val = readl(reg);
> > +     val = (~CC_CODE_MASK & val) | (type_c->cc2_code &
> CC_CODE_MASK);
> > +
> > +     reg = type_c->reg_base + USB_TYPEC_CTRL_CC1_1;
> > +     writel(type_c->cc1_vref, reg);
> > +
> > +     reg = type_c->reg_base + USB_TYPEC_CTRL_CC2_1;
> > +     writel(type_c->cc2_vref, reg);
> > +
> > +     reg = type_c->reg_base + USB_TYPEC_CTRL;
> > +     val = readl(reg);
> > +     val = (~DEBOUNCE_TIME_MASK & val) | (type_c->debounce &
> DEBOUNCE_TIME_MASK);
> > +
> > +     dev_info(dev, "First check USB_DR_MODE_PERIPHERAL");
> > +     type_c->cc_mode = IN_DEVICE_MODE;
> > +     type_c->is_attach = IN_DETACH;
> > +     type_c->connect_change = CONNECT_NO_CHANGE;
> > +
> > +     detect_host(type_c);
> > +
> > +     spin_unlock_irqrestore(&type_c->lock, flags);
> > +
> > +     schedule_delayed_work(&type_c->delayed_work,
> msecs_to_jiffies(0));
> > +
> > +#ifdef CONFIG_TYPEC
> 
> Don'n need to check 'CONFIG_TYPEC' defintion wiht ifdef.
> 'ifdef' might make the code more complicated.
> 
> If type_c is optional, just print warning message
> when typec_regiser_port returns error.
> 
> This patch is already check whether 'type_c->port' is NULL or not
> to handle the code related to typec.

If I remove check 'CONFIG_TYPEC' definition,
it will fail the build when CONFIG_TYPEC=n.
Another option is to select CONFIG_TYPEC in Kconfig.

> > +     if (!type_c->port) {
> 
> (snip)
> 
> > +
> > +             type_c->port = typec_register_port(type_c->dev,
> &typec_cap);
> > +             if (IS_ERR(type_c->port))
> > +                     return PTR_ERR(type_c->port);
> > +     }> +#endif /* CONFIG_TYPEC */
> > +
> > +     return 0;
> > +}
> > +
> > +static int extcon_rtk_type_c_edev_register(struct type_c_data *type_c)
> > +{
> > +     struct device *dev = type_c->dev;
> > +     int ret = 0;
> > +
> > +     type_c->edev = devm_extcon_dev_allocate(dev, usb_type_c_cable);
> > +     if (IS_ERR(type_c->edev)) {
> > +             dev_err(dev, "failed to allocate extcon device\n");
> > +             return -ENOMEM;
> > +     }
> > +
> > +     ret = devm_extcon_dev_register(dev, type_c->edev);
> > +     if (ret < 0) {
> > +             dev_err(dev, "failed to register extcon device\n");
> > +             return ret;
> > +     }
> > +
> > +     extcon_set_property_capability(type_c->edev, EXTCON_USB,
> > +                                    EXTCON_PROP_USB_VBUS);
> > +     extcon_set_property_capability(type_c->edev, EXTCON_USB,
> > +
> EXTCON_PROP_USB_TYPEC_POLARITY);
> > +     extcon_set_property_capability(type_c->edev, EXTCON_USB,
> > +                                    EXTCON_PROP_USB_SS);
> > +
> > +     extcon_set_property_capability(type_c->edev, EXTCON_USB_HOST,
> > +                                    EXTCON_PROP_USB_VBUS);
> > +     extcon_set_property_capability(type_c->edev, EXTCON_USB_HOST,
> > +
> EXTCON_PROP_USB_TYPEC_POLARITY);
> > +     extcon_set_property_capability(type_c->edev, EXTCON_USB_HOST,
> > +                                    EXTCON_PROP_USB_SS);
> > +
> > +     return ret;
> > +}
> > +
> > +static int extcon_rtk_type_c_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct type_c_data *type_c;
> > +     const struct type_c_cfg *type_c_cfg;
> > +     unsigned int gpio;
> > +     int ret = 0;
> > +
> > +     type_c = devm_kzalloc(dev, sizeof(*type_c), GFP_KERNEL);
> > +     if (!type_c)
> > +             return -ENOMEM;
> > +
> > +     type_c->reg_base = devm_platform_ioremap_resource(pdev, 0);
> > +     if (IS_ERR(type_c->reg_base))
> > +             return PTR_ERR(type_c->reg_base);
> > +
> > +     type_c->dev = dev;
> > +
> > +     type_c->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> > +     if (type_c->irq <= 0) {
> > +             dev_err(&pdev->dev, "Type C driver with no IRQ. Check %s
> setup!\n",
> > +                     dev_name(&pdev->dev));
> > +             ret = -ENODEV;
> > +             goto err1;
> > +     }
> > +
> > +     ret = devm_request_irq(dev, type_c->irq, type_c_detect_irq,
> > +                            IRQF_SHARED, "type_c_detect", type_c);
> > +
> > +     spin_lock_init(&type_c->lock);
> > +
> > +     type_c->rd_ctrl_gpio = -1;
> > +     if (soc_device_match(rtk_soc_kylin)) {
> > +             gpio = of_get_named_gpio(dev->of_node,
> "realtek,rd-ctrl-gpio", 0);
> > +             if (gpio_is_valid(gpio)) {
> > +                     type_c->rd_ctrl_gpio = gpio;
> > +                     dev_dbg(dev, "%s get rd-ctrl-gpio (id=%d) OK\n",
> __func__, gpio);
> > +             } else {
> > +                     dev_err(dev, "Error rd_ctrl-gpio no found");
> > +             }
> > +     }
> > +
> > +     type_c_cfg = of_device_get_match_data(dev);
> > +     if (!type_c_cfg) {
> > +             dev_err(dev, "type_c config are not assigned!\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     type_c->type_c_cfg = devm_kzalloc(dev, sizeof(*type_c_cfg),
> GFP_KERNEL);
> > +
> > +     memcpy(type_c->type_c_cfg, type_c_cfg, sizeof(*type_c_cfg));
> > +
> > +     if (setup_type_c_parameter(type_c)) {
> > +             dev_err(dev, "ERROR: %s to setup type c parameter!!",
> __func__);
> > +             ret = -EINVAL;
> > +             goto err1;
> > +     }
> > +
> > +     INIT_DELAYED_WORK(&type_c->delayed_work, host_device_switch);
> > +
> > +     ret = extcon_rtk_type_c_init(type_c);
> > +     if (ret) {
> > +             dev_err(dev, "%s failed to init type_c\n", __func__);
> > +             goto err1;
> > +     }
> > +
> > +     platform_set_drvdata(pdev, type_c);
> > +
> > +     ret = extcon_rtk_type_c_edev_register(type_c);
> > +
> > +     create_debug_files(type_c);
> > +
> > +     return 0;
> > +
> > +err1:
> > +     dev_err(&pdev->dev, "%s: Probe fail, %d\n", __func__, ret);
> > +
> > +     return ret;
> > +}
> > +
> > +static void extcon_rtk_type_c_remove(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct type_c_data *type_c = dev_get_drvdata(dev);
> > +     u32 default_ctrl;
> > +     unsigned long flags;
> > +
> > +     remove_debug_files(type_c);
> > +
> > +#ifdef CONFIG_TYPEC
> > +     if (type_c->port) {
> > +             typec_unregister_port(type_c->port);
> > +             type_c->port = NULL;
> > +     }
> > +#endif
> > +
> > +     cancel_delayed_work_sync(&type_c->delayed_work);
> > +     flush_delayed_work(&type_c->delayed_work);
> > +     WARN_ON_ONCE(delayed_work_pending(&type_c->delayed_work));
> > +
> > +     spin_lock_irqsave(&type_c->lock, flags);
> > +     /* disable interrupt */
> > +     default_ctrl = readl(type_c->reg_base + USB_TYPEC_CTRL) &
> > +                 DEBOUNCE_TIME_MASK;
> > +     writel(default_ctrl, type_c->reg_base + USB_TYPEC_CTRL);
> > +
> > +     /* disable cc detect, rp, rd */
> > +     writel(PLR_EN, type_c->reg_base + USB_TYPEC_CTRL_CC1_0);
> > +     writel(0, type_c->reg_base + USB_TYPEC_CTRL_CC2_0);
> > +
> > +     spin_unlock_irqrestore(&type_c->lock, flags);
> > +
> > +     free_irq(type_c->irq, type_c);
> > +}
> > +
> > +static const struct type_c_cfg rtd1295_type_c_cfg = {
> > +     .parameter_ver = PARAMETER_V0,
> > +     .cc_dfp_mode = CC_MODE_DFP_3_0,
> > +     .cc1_param = { .rp_4p7k_code = 0xb,
> > +                    .rp_36k_code = 0x17,
> > +                    .rp_12k_code = 0x10,
> > +                    .rd_code = 0,
> > +                    .ra_code = 0,
> > +                    .vref_2p6v = 0x0,
> > +                    .vref_1p23v = 0x0,
> > +                    .vref_0p8v = 0x3,
> > +                    .vref_0p66v = 0x0,
> > +                    .vref_0p4v = 0x0,
> > +                    .vref_0p2v = 0x4,
> > +                    .vref_1_1p6v = 0,
> > +                    .vref_0_1p6v = 0 },
> > +     .cc2_param = { .rp_4p7k_code = 0xc,
> > +                    .rp_36k_code = 0x17,
> > +                    .rp_12k_code = 0x12,
> > +                    .rd_code = 0,
> > +                    .ra_code = 0,
> > +                    .vref_2p6v = 0x2,
> > +                    .vref_1p23v = 0x0,
> > +                    .vref_0p8v = 0x3,
> > +                    .vref_0p66v = 0x0,
> > +                    .vref_0p4v = 0x0,
> > +                    .vref_0p2v = 0x5,
> > +                    .vref_1_1p6v = 0,
> > +                    .vref_0_1p6v = 0 },
> > +     .debounce_val = 0x7f, /* 1b,1us 7f,4.7us */
> > +     .use_defalut_parameter = false,
> > +};
> > +
> > +static const struct type_c_cfg rtd1395_type_c_cfg = {
> > +     .parameter_ver = PARAMETER_V0,
> > +     .cc_dfp_mode = CC_MODE_DFP_3_0,
> > +     .cc1_param = { .rp_4p7k_code = 0xc,
> > +                    .rp_36k_code = 0xb,
> > +                    .rp_12k_code = 0xe,
> > +                    .rd_code = 0x10,
> > +                    .ra_code = 0x0,
> > +                    .vref_2p6v = 0x0,
> > +                    .vref_1p23v = 0x1,
> > +                    .vref_0p8v = 0x0,
> > +                    .vref_0p66v = 0x0,
> > +                    .vref_0p4v = 0x3,
> > +                    .vref_0p2v = 0x0,
> > +                    .vref_1_1p6v = 0x7,
> > +                    .vref_0_1p6v = 0x7 },
> > +     .cc2_param = { .rp_4p7k_code = 0xb,
> > +                    .rp_36k_code = 0x9,
> > +                    .rp_12k_code = 0xe,
> > +                    .rd_code = 0xf,
> > +                    .ra_code = 0x0,
> > +                    .vref_2p6v = 0x1,
> > +                    .vref_1p23v = 0x3,
> > +                    .vref_0p8v = 0x3,
> > +                    .vref_0p66v = 0x2,
> > +                    .vref_0p4v = 0x3,
> > +                    .vref_0p2v = 0x2,
> > +                    .vref_1_1p6v = 0x7,
> > +                    .vref_0_1p6v = 0x7 },
> > +     .debounce_val = 0x7f, /* 1b,1us 7f,4.7us */
> > +     .use_defalut_parameter = false,
> > +};
> > +
> > +static const struct type_c_cfg rtd1619_type_c_cfg = {
> > +     .parameter_ver = PARAMETER_V0,
> > +     .cc_dfp_mode = CC_MODE_DFP_3_0,
> > +     .cc1_param = { .rp_4p7k_code = 0xc,
> > +                    .rp_36k_code = 0xf,
> > +                    .rp_12k_code = 0xe,
> > +                    .rd_code = 0x11,
> > +                    .ra_code = 0x0,
> > +                    .vref_2p6v = 0x5,
> > +                    .vref_1p23v = 0x7,
> > +                    .vref_0p8v = 0xa,
> > +                    .vref_0p66v = 0xa,
> > +                    .vref_0p4v = 0x3,
> > +                    .vref_0p2v = 0x2,
> > +                    .vref_1_1p6v = 0x7,
> > +                    .vref_0_1p6v = 0x7 },
> > +     .cc2_param = { .rp_4p7k_code = 0xc,
> > +                    .rp_36k_code = 0xf,
> > +                    .rp_12k_code = 0xe,
> > +                    .rd_code = 0xf,
> > +                    .ra_code = 0x0,
> > +                    .vref_2p6v = 0x5,
> > +                    .vref_1p23v = 0x8,
> > +                    .vref_0p8v = 0xa,
> > +                    .vref_0p66v = 0xa,
> > +                    .vref_0p4v = 0x3,
> > +                    .vref_0p2v = 0x2,
> > +                    .vref_1_1p6v = 0x7,
> > +                    .vref_0_1p6v = 0x7 },
> > +     .debounce_val = 0x7f, /* 1b,1us 7f,4.7us */
> > +     .use_defalut_parameter = false,
> > +};
> > +
> > +static const struct type_c_cfg rtd1319_type_c_cfg = {
> > +     .parameter_ver = PARAMETER_V0,
> > +     .cc_dfp_mode = CC_MODE_DFP_1_5,
> > +     .cc1_param = { .rp_4p7k_code = 0x9,
> > +                    .rp_36k_code = 0xe,
> > +                    .rp_12k_code = 0x9,
> > +                    .rd_code = 0x9,
> > +                    .ra_code = 0x7,
> > +                    .vref_2p6v = 0x3,
> > +                    .vref_1p23v = 0x7,
> > +                    .vref_0p8v = 0x7,
> > +                    .vref_0p66v = 0x6,
> > +                    .vref_0p4v = 0x2,
> > +                    .vref_0p2v = 0x3,
> > +                    .vref_1_1p6v = 0x4,
> > +                    .vref_0_1p6v = 0x7 },
> > +     .cc2_param = { .rp_4p7k_code = 0x8,
> > +                    .rp_36k_code = 0xe,
> > +                    .rp_12k_code = 0x9,
> > +                    .rd_code = 0x9,
> > +                    .ra_code = 0x7,
> > +                    .vref_2p6v = 0x3,
> > +                    .vref_1p23v = 0x7,
> > +                    .vref_0p8v = 0x7,
> > +                    .vref_0p66v = 0x6,
> > +                    .vref_0p4v = 0x3,
> > +                    .vref_0p2v = 0x3,
> > +                    .vref_1_1p6v = 0x6,
> > +                    .vref_0_1p6v = 0x7 },
> > +     .debounce_val = 0x7f, /* 1b,1us 7f,4.7us */
> > +     .use_defalut_parameter = false,
> > +};
> > +
> > +static const struct type_c_cfg rtd1312c_type_c_cfg = {
> > +     .parameter_ver = PARAMETER_V0,
> > +     .cc_dfp_mode = CC_MODE_DFP_1_5,
> > +     .cc1_param = { .rp_4p7k_code = 0xe,
> > +                    .rp_36k_code = 0xc,
> > +                    .rp_12k_code = 0xc,
> > +                    .rd_code = 0xa,
> > +                    .ra_code = 0x3,
> > +                    .vref_2p6v = 0xa,
> > +                    .vref_1p23v = 0x7,
> > +                    .vref_0p8v = 0x7,
> > +                    .vref_0p66v = 0x7,
> > +                    .vref_0p4v = 0x4,
> > +                    .vref_0p2v = 0x4,
> > +                    .vref_1_1p6v = 0x7,
> > +                    .vref_0_1p6v = 0x7 },
> > +     .cc2_param = { .rp_4p7k_code = 0xe,
> > +                    .rp_36k_code = 0xc,
> > +                    .rp_12k_code = 0xc,
> > +                    .rd_code = 0xa,
> > +                    .ra_code = 0x3,
> > +                    .vref_2p6v = 0xa,
> > +                    .vref_1p23v = 0x7,
> > +                    .vref_0p8v = 0x7,
> > +                    .vref_0p66v = 0x7,
> > +                    .vref_0p4v = 0x4,
> > +                    .vref_0p2v = 0x4,
> > +                    .vref_1_1p6v = 0x7,
> > +                    .vref_0_1p6v = 0x7 },
> > +     .debounce_val = 0x7f, /* 1b,1us 7f,4.7us */
> > +     .use_defalut_parameter = false,
> > +};
> > +
> > +static const struct type_c_cfg rtd1619b_type_c_cfg = {
> > +     .parameter_ver = PARAMETER_V1,
> > +     .cc_dfp_mode = CC_MODE_DFP_1_5,
> > +     .cc1_param = { .rp_4p7k_code = 0xf,
> > +                    .rp_36k_code = 0xf,
> > +                    .rp_12k_code = 0xf,
> > +                    .rd_code = 0xf,
> > +                    .ra_code = 0x7,
> > +                    .vref_2p6v = 0x9,
> > +                    .vref_1p23v = 0x7,
> > +                    .vref_0p8v = 0x9,
> > +                    .vref_0p66v = 0x8,
> > +                    .vref_0p4v = 0x7,
> > +                    .vref_0p2v = 0x9,
> > +                    .vref_1_1p6v = 0x7,
> > +                    .vref_0_1p6v = 0x7 },
> > +     .cc2_param = { .rp_4p7k_code = 0xf,
> > +                    .rp_36k_code = 0xf,
> > +                    .rp_12k_code = 0xf,
> > +                    .rd_code = 0xf,
> > +                    .ra_code = 0x7,
> > +                    .vref_1p23v = 0x7,
> > +                    .vref_0p8v = 0x9,
> > +                    .vref_0p66v = 0x8,
> > +                    .vref_0p4v = 0x7,
> > +                    .vref_0p2v = 0x8,
> > +                    .vref_1_1p6v = 0x7,
> > +                    .vref_0_1p6v = 0x7 },
> > +     .debounce_val = 0x7f, /* 1b,1us 7f,4.7us */
> > +     .use_defalut_parameter = false,
> > +};
> > +
> > +static const struct type_c_cfg rtd1319d_type_c_cfg = {
> > +     .parameter_ver = PARAMETER_V1,
> > +     .cc_dfp_mode = CC_MODE_DFP_1_5,
> > +     .cc1_param = { .rp_4p7k_code = 0xe,
> > +                    .rp_36k_code = 0x3,
> > +                    .rp_12k_code = 0xe,
> > +                    .rd_code = 0xf,
> > +                    .ra_code = 0x6,
> > +                    .vref_2p6v = 0x7,
> > +                    .vref_1p23v = 0x7,
> > +                    .vref_0p8v = 0x8,
> > +                    .vref_0p66v = 0x7,
> > +                    .vref_0p4v = 0x7,
> > +                    .vref_0p2v = 0x7,
> > +                    .vref_1_1p6v = 0x7,
> > +                    .vref_0_1p6v = 0x7 },
> > +     .cc2_param = { .rp_4p7k_code = 0xe,
> > +                    .rp_36k_code = 0x3,
> > +                    .rp_12k_code = 0xe,
> > +                    .rd_code = 0xf,
> > +                    .ra_code = 0x6,
> > +                    .vref_2p6v = 0x7,
> > +                    .vref_1p23v = 0x7,
> > +                    .vref_0p8v = 0x8,
> > +                    .vref_0p66v = 0x7,
> > +                    .vref_0p4v = 0x7,
> > +                    .vref_0p2v = 0x8,
> > +                    .vref_1_1p6v = 0x7,
> > +                    .vref_0_1p6v = 0x7 },
> > +     .debounce_val = 0x7f, /* 1b,1us 7f,4.7us */
> > +     .use_defalut_parameter = false,
> > +};
> > +
> > +static const struct type_c_cfg rtd1315e_type_c_cfg = {
> > +     .parameter_ver = PARAMETER_V1,
> > +     .cc_dfp_mode = CC_MODE_DFP_1_5,
> > +     .cc1_param = { .rp_4p7k_code = 0xe,
> > +                    .rp_36k_code = 0x3,
> > +                    .rp_12k_code = 0xe,
> > +                    .rd_code = 0xf,
> > +                    .ra_code = 0x6,
> > +                    .vref_2p6v = 0x7,
> > +                    .vref_1p23v = 0x7,
> > +                    .vref_0p8v = 0x8,
> > +                    .vref_0p66v = 0x7,
> > +                    .vref_0p4v = 0x7,
> > +                    .vref_0p2v = 0x7,
> > +                    .vref_1_1p6v = 0x7,
> > +                    .vref_0_1p6v = 0x7 },
> > +     .cc2_param = { .rp_4p7k_code = 0xe,
> > +                    .rp_36k_code = 0x3,
> > +                    .rp_12k_code = 0xe,
> > +                    .rd_code = 0xf,
> > +                    .ra_code = 0x6,
> > +                    .vref_2p6v = 0x7,
> > +                    .vref_1p23v = 0x7,
> > +                    .vref_0p8v = 0x8,
> > +                    .vref_0p66v = 0x7,
> > +                    .vref_0p4v = 0x7,
> > +                    .vref_0p2v = 0x8,
> > +                    .vref_1_1p6v = 0x7,
> > +                    .vref_0_1p6v = 0x7 },
> > +     .debounce_val = 0x7f, /* 1b,1us 7f,4.7us */
> > +     .use_defalut_parameter = false,
> > +};
> > +
> > +static const struct of_device_id extcon_rtk_type_c_match[] = {
> > +     { .compatible = "realtek,rtd1295-type-c", .data =
> &rtd1295_type_c_cfg },
> > +     { .compatible = "realtek,rtd1312c-type-c", .data =
> &rtd1312c_type_c_cfg },
> > +     { .compatible = "realtek,rtd1315e-type-c", .data =
> &rtd1315e_type_c_cfg },
> > +     { .compatible = "realtek,rtd1319-type-c", .data =
> &rtd1319_type_c_cfg },
> > +     { .compatible = "realtek,rtd1319d-type-c", .data =
> &rtd1319d_type_c_cfg },
> > +     { .compatible = "realtek,rtd1395-type-c", .data =
> &rtd1395_type_c_cfg },
> > +     { .compatible = "realtek,rtd1619-type-c", .data =
> &rtd1619_type_c_cfg },
> > +     { .compatible = "realtek,rtd1619b-type-c", .data =
> &rtd1619b_type_c_cfg },
> > +     {},
> > +};
> > +MODULE_DEVICE_TABLE(of, extcon_rtk_type_c_match);
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > +static int extcon_rtk_type_c_prepare(struct device *dev)
> > +{
> > +     struct type_c_data *type_c = dev_get_drvdata(dev);
> > +     u32 default_ctrl;
> > +     unsigned long flags;
> > +
> > +     cancel_delayed_work_sync(&type_c->delayed_work);
> > +     flush_delayed_work(&type_c->delayed_work);
> > +     WARN_ON_ONCE(delayed_work_pending(&type_c->delayed_work));
> > +
> > +     spin_lock_irqsave(&type_c->lock, flags);
> > +     /* disable interrupt */
> > +     default_ctrl = readl(type_c->reg_base + USB_TYPEC_CTRL) &
> > +                 DEBOUNCE_TIME_MASK;
> > +     writel(default_ctrl, type_c->reg_base + USB_TYPEC_CTRL);
> > +
> > +     /* disable cc detect, rp, rd */
> > +     writel(PLR_EN, type_c->reg_base + USB_TYPEC_CTRL_CC1_0);
> > +     writel(0, type_c->reg_base + USB_TYPEC_CTRL_CC2_0);
> > +
> > +     spin_unlock_irqrestore(&type_c->lock, flags);
> > +
> > +     return 0;
> > +}
> > +
> > +static void extcon_rtk_type_c_complete(struct device *dev)
> > +{
> > +     /* nothing */
> > +}
> > +
> > +static int extcon_rtk_type_c_suspend(struct device *dev)
> > +{
> > +     struct type_c_data *type_c = dev_get_drvdata(dev);
> > +
> > +     if (type_c->rd_ctrl_gpio != -1)
> 
> ditto.
> 
> > +             gpio_free(type_c->rd_ctrl_gpio);
> > +
> > +     return 0;
> > +}
> > +
> > +static int extcon_rtk_type_c_resume(struct device *dev)
> > +{
> > +     struct type_c_data *type_c = dev_get_drvdata(dev);
> > +
> > +     extcon_rtk_type_c_init(type_c);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct dev_pm_ops extcon_rtk_type_c_pm_ops = {
> > +     SET_LATE_SYSTEM_SLEEP_PM_OPS(extcon_rtk_type_c_suspend,
> extcon_rtk_type_c_resume)
> > +     .prepare = extcon_rtk_type_c_prepare,
> > +     .complete = extcon_rtk_type_c_complete,
> > +};
> > +
> > +#define DEV_PM_OPS   (&extcon_rtk_type_c_pm_ops)
> > +#else
> > +#define DEV_PM_OPS   NULL
> > +#endif /* CONFIG_PM_SLEEP */
> > +
> > +static struct platform_driver extcon_rtk_type_c_driver = {
> > +     .probe          = extcon_rtk_type_c_probe,
> > +     .remove_new     = extcon_rtk_type_c_remove,
> > +     .driver         = {
> > +             .name   = "extcon-rtk-type_c",
> > +             .of_match_table = extcon_rtk_type_c_match,
> > +             .pm = DEV_PM_OPS,
> > +     },
> > +};
> > +
> > +module_platform_driver(extcon_rtk_type_c_driver);
> > +
> > +MODULE_DESCRIPTION("Realtek Extcon Type C driver");
> > +MODULE_ALIAS("platform:extcon-rtk-type-c");
> > +MODULE_AUTHOR("Stanley Chang <stanley_chang@realtek.com>");
> > +MODULE_LICENSE("GPL");
> 

Thanks,
Stanley

Hi Stanley,

On 23. 8. 29. 20:15, Stanley Chang[昌育德] wrote:
> Hi Chanwoo,
> 
>>> +static int rtd129x_switch_type_c_plug_config(struct type_c_data *type_c,
>>> +                                          int dr_mode, int cc)
>>> +{
>>> +     void __iomem *reg = type_c->reg_base + USB_TYPEC_CTRL_CC1_0;
>>> +     int val_cc;
>>> +
>>> +#define TYPE_C_EN_SWITCH BIT(29)
>>> +#define TYPE_C_TXRX_SEL (BIT(28) | BIT(27))
>>> +#define TYPE_C_SWITCH_MASK (TYPE_C_EN_SWITCH | TYPE_C_TXRX_SEL)
>>> +#define TYPE_C_ENABLE_CC1 TYPE_C_EN_SWITCH
>>> +#define TYPE_C_ENABLE_CC2 (TYPE_C_EN_SWITCH | TYPE_C_TXRX_SEL)
>>> +#define TYPE_C_DISABLE_CC ~TYPE_C_SWITCH_MASK
>>> +
>>> +     val_cc = readl(reg);
>>
>> I'd like you to use regmap interface to access the register
>> by using regmap_read, regmap_write. You can create the regmap instance
>> via devm_regmap_init_mmio() on probe instead of using 'type_c->reg_base'
>> at the multipe point.
>>
>> For example,
>>         struct regmap_config rtk_regmap_config = {
>>                 .reg_bits = 32,
>>                 .val_bits = 32,
>>         };
>>
>>         void __iomem *base;
>>
>>         base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
>>         if (IS_ERR(base))
>>                 return PTR_ERR(base);
>>
>>         regmap = devm_regmap_init_mmio(dev, base,
>> &rtk_regmap_config);
>>
>>         ---
>>
>>         And then just call regmap_read without any calculation between
>>         base address and specific register.
>>
>>         regmap_read(regmap, USB_TYPEC_CTRL_CC1_0)
>>
> 
> I studied mmio's regmap.
> 
> It only changed one encoding method. And simplifies the calculation between the base address and the specific register.

The regmap provides the consistent interface to access register
regardless of mmio/i2c/spi and so on. It is the advantage of regmap.

And regmap provides the like register dump via debugfs,
is able to specify the kind of register like writable, readable, volatile type.
It is possible to specify the more detailed register information in device driver
in order to improve the readability. It allows you to express more detail of this device
than just working.


> If the register is 32-bit aligned, other operations look the same as readl/writel.
> I think regmap is more simplified if the read registers are not 32-bit aligned, e.g. nvmem read/write.

I'm sorry. I don't understand of what is accurate meaning.
Could you please explain your opinion more detailed with example of this patch?

> So it would be more intuitive for me to keep writel/readl here
> 
>>
>>> +     val_cc &= ~TYPE_C_SWITCH_MASK;
>>> +
>>> +     if (cc == DISABLE_CC) {
>>> +             val_cc &= TYPE_C_DISABLE_CC;
>>> +     } else if (cc == ENABLE_CC1) {
>>> +             val_cc |= TYPE_C_ENABLE_CC1;
>>> +     } else if (cc == ENABLE_CC2) {
>>> +             val_cc |= TYPE_C_ENABLE_CC2;
>>> +     } else {
>>> +             dev_err(type_c->dev, "%s: Error cc setting cc=0x%x\n",
>> __func__, cc);
>>> +             return -EINVAL;
>>> +     }
>>> +     writel(val_cc, reg);
>>> +
> 
> Thanks,
> Stanley

Hi Chanwoo,

> >>> +#define TYPE_C_EN_SWITCH BIT(29)
> >>> +#define TYPE_C_TXRX_SEL (BIT(28) | BIT(27)) #define
> >>> +TYPE_C_SWITCH_MASK (TYPE_C_EN_SWITCH | TYPE_C_TXRX_SEL)
> #define
> >>> +TYPE_C_ENABLE_CC1 TYPE_C_EN_SWITCH #define
> TYPE_C_ENABLE_CC2
> >>> +(TYPE_C_EN_SWITCH | TYPE_C_TXRX_SEL) #define TYPE_C_DISABLE_CC
> >>> +~TYPE_C_SWITCH_MASK
> >>> +
> >>> +     val_cc = readl(reg);
> >>
> >> I'd like you to use regmap interface to access the register by using
> >> regmap_read, regmap_write. You can create the regmap instance via
> >> devm_regmap_init_mmio() on probe instead of using 'type_c->reg_base'
> >> at the multipe point.
> >>
> >> For example,
> >>         struct regmap_config rtk_regmap_config = {
> >>                 .reg_bits = 32,
> >>                 .val_bits = 32,
> >>         };
> >>
> >>         void __iomem *base;
> >>
> >>         base = devm_platform_get_and_ioremap_resource(pdev, 0,
> &res);
> >>         if (IS_ERR(base))
> >>                 return PTR_ERR(base);
> >>
> >>         regmap = devm_regmap_init_mmio(dev, base,
> >> &rtk_regmap_config);
> >>
> >>         ---
> >>
> >>         And then just call regmap_read without any calculation between
> >>         base address and specific register.
> >>
> >>         regmap_read(regmap, USB_TYPEC_CTRL_CC1_0)
> >>
> >
> > I studied mmio's regmap.
> >
> > It only changed one encoding method. And simplifies the calculation
> between the base address and the specific register.
> 
> The regmap provides the consistent interface to access register regardless of
> mmio/i2c/spi and so on. It is the advantage of regmap.
> 
> And regmap provides the like register dump via debugfs, is able to specify the
> kind of register like writable, readable, volatile type.
> It is possible to specify the more detailed register information in device driver
> in order to improve the readability. It allows you to express more detail of this
> device than just working.

These are all advantages of regmap, I agree.
I haven't delved into regmap yet.

> 
> > If the register is 32-bit aligned, other operations look the same as
> readl/writel.
> > I think regmap is more simplified if the read registers are not 32-bit aligned,
> e.g. nvmem read/write.
> 
> I'm sorry. I don't understand of what is accurate meaning.
> Could you please explain your opinion more detailed with example of this
> patch?

Sorry, I misunderstood regmap, I thought he could read register like read otp tables by nvmem regardless of his bitness and size.
So my answer should be incorrect.

Thank,
Stanley