LinuxLists.cc - [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

[permalink] [raw]

Subject: RE: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

Dear Heikki,

Sorry for bothering.

Just want to check is there anything we need to modify?

Thank you!

Best Regards,
*****************************
Shu-Fan Lee
Richtek Technology Corporation
TEL: +886-3-5526789 #2359
FAX: +886-3-5526612
*****************************

-----Original Message-----
From: ShuFanLee [mailto:[email protected]]
Sent: Wednesday, January 10, 2018 2:59 PM
To: [email protected]
Cc: cy_huang(黃啟原); shufan_lee(李書帆); [email protected]; [email protected]
Subject: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

From: ShuFanLee <[email protected]>

Richtek RT1711H Type-C chip driver that works with
Type-C Port Controller Manager to provide USB PD and
USB Type-C functionalities.

Signed-off-by: ShuFanLee <[email protected]>
---
.../devicetree/bindings/usb/richtek,rt1711h.txt | 38 +
arch/arm64/boot/dts/hisilicon/rt1711h.dtsi | 11 +
drivers/usb/typec/Kconfig | 2 +
drivers/usb/typec/Makefile | 1 +
drivers/usb/typec/rt1711h/Kconfig | 7 +
drivers/usb/typec/rt1711h/Makefile | 2 +
drivers/usb/typec/rt1711h/rt1711h.c | 2241 ++++++++++++++++++++
drivers/usb/typec/rt1711h/rt1711h.h | 300 +++
8 files changed, 2602 insertions(+)
create mode 100644 Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
create mode 100644 arch/arm64/boot/dts/hisilicon/rt1711h.dtsi
create mode 100644 drivers/usb/typec/rt1711h/Kconfig
create mode 100644 drivers/usb/typec/rt1711h/Makefile
create mode 100644 drivers/usb/typec/rt1711h/rt1711h.c
create mode 100644 drivers/usb/typec/rt1711h/rt1711h.h

diff --git a/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt b/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
new file mode 100644
index 0000000..c28299c
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
@@ -0,0 +1,38 @@
+Richtek RT1711H Type-C Port Controller.
+
+Required properties:
+- compatible : Must be "richtek,typec_rt1711h";
+- reg : Must be 0x4e, it's default slave address of RT1711H.
+- rt,intr_gpio : IRQ GPIO pin that's connected to RT1711H interrupt.
+
+Optional node:
+- rt,name : Name used for registering IRQ and creating kthread.
+ If this property is not specified, "default" will be applied.
+- rt,def_role : Default port role (TYPEC_SINK(0) or TYPEC_SOURCE(1)).
+Set to TYPEC_NO_PREFERRED_ROLE(-1) if no default role.
+If this property is not specified, TYPEC_SINK will be applied.
+- rt,port_type : Port type (TYPEC_PORT_DFP(0), TYPEC_PORT_UFP(1),
+ or TYPEC_PORT_DRP(2)). If this property is not specified,
+ TYPEC_PORT_DRP will be applied.
+- rt,max_snk_mv : Maximum acceptable sink voltage in mV.
+ If this property is not specified, 5000mV will be applied.
+- rt,max_snk_ma : Maximum sink current in mA.
+ If this property is not specified, 3000mA will be applied.
+- rt,max_snk_mw : Maximum required sink power in mW.
+ If this property is not specified, 15000mW will be applied.
+- rt,operating_snk_mw : Required operating sink power in mW.
+If this property is not specified,
+2500mW will be applied.
+- rt,try_role_hw : True if try.{Src,Snk} is implemented in hardware.
+ If this property is not specified, False will be applied.
+
+Example:
+rt1711h@4e {
+status = "ok";
+compatible = "richtek,typec_rt1711h";
+reg = <0x4e>;
+rt,intr_gpio = <&gpio26 0 0x0>;
+rt,name = "rt1711h";
+rt,port_type = <2>; /* 0: DFP, 1: UFP, 2: DRP */
+rt,def_role = <0>; /* 0: SNK, 1: SRC, -1: TYPEC_NO_PREFERRED_ROLE */
+};
diff --git a/arch/arm64/boot/dts/hisilicon/rt1711h.dtsi b/arch/arm64/boot/dts/hisilicon/rt1711h.dtsi
new file mode 100644
index 0000000..4196cc0
--- /dev/null
+++ b/arch/arm64/boot/dts/hisilicon/rt1711h.dtsi
@@ -0,0 +1,11 @@
+&i2c7 {
+rt1711h@4e {
+status = "ok";
+compatible = "richtek,typec_rt1711h";
+reg = <0x4e>;
+rt,intr_gpio = <&gpio26 0 0x0>;
+rt,name = "rt1711h";
+rt,port_type = <2>; /* 0: DFP, 1: UFP, 2: DRP */
+rt,def_role = <0>; /* 0: SNK, 1: SRC */
+};
+};
diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
index bcb2744..7bede0b 100644
--- a/drivers/usb/typec/Kconfig
+++ b/drivers/usb/typec/Kconfig
@@ -56,6 +56,8 @@ if TYPEC_TCPM

source "drivers/usb/typec/fusb302/Kconfig"

+source "drivers/usb/typec/rt1711h/Kconfig"
+
config TYPEC_WCOVE
tristate "Intel WhiskeyCove PMIC USB Type-C PHY driver"
depends on ACPI
diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
index bb3138a..e3aaf3c 100644
--- a/drivers/usb/typec/Makefile
+++ b/drivers/usb/typec/Makefile
@@ -2,6 +2,7 @@
obj-$(CONFIG_TYPEC)+= typec.o
obj-$(CONFIG_TYPEC_TCPM)+= tcpm.o
obj-y+= fusb302/
+obj-$(CONFIG_TYPEC_RT1711H)+= rt1711h/
obj-$(CONFIG_TYPEC_WCOVE)+= typec_wcove.o
obj-$(CONFIG_TYPEC_UCSI)+= ucsi/
obj-$(CONFIG_TYPEC_TPS6598X)+= tps6598x.o
diff --git a/drivers/usb/typec/rt1711h/Kconfig b/drivers/usb/typec/rt1711h/Kconfig
new file mode 100644
index 0000000..2fbfff5
--- /dev/null
+++ b/drivers/usb/typec/rt1711h/Kconfig
@@ -0,0 +1,7 @@
+config TYPEC_RT1711H
+tristate "Richtek RT1711H Type-C chip driver"
+depends on I2C && POWER_SUPPLY
+help
+ The Richtek RT1711H Type-C chip driver that works with
+ Type-C Port Controller Manager to provide USB PD and USB
+ Type-C functionalities.
diff --git a/drivers/usb/typec/rt1711h/Makefile b/drivers/usb/typec/rt1711h/Makefile
new file mode 100644
index 0000000..5fab8ae
--- /dev/null
+++ b/drivers/usb/typec/rt1711h/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_TYPEC_RT1711H)+= rt1711h.o
diff --git a/drivers/usb/typec/rt1711h/rt1711h.c b/drivers/usb/typec/rt1711h/rt1711h.c
new file mode 100644
index 0000000..1aef3e8
--- /dev/null
+++ b/drivers/usb/typec/rt1711h/rt1711h.c
@@ -0,0 +1,2241 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2017 Richtek Technologh Corp.
+ *
+ * Richtek RT1711H Type-C Chip Driver
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/version.h>
+#include <linux/err.h>
+#include <linux/debugfs.h>
+#include <linux/pm_runtime.h>
+#include <linux/i2c.h>
+#include <linux/usb/typec.h>
+#include <linux/usb/tcpm.h>
+#include <linux/usb/pd.h>
+#include <linux/of_gpio.h>
+#include <linux/of.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/regulator/consumer.h>
+#include <linux/power_supply.h>
+#include <linux/extcon.h>
+#include <linux/workqueue.h>
+#include <linux/kthread.h>
+#include <linux/cpu.h>
+#include <linux/alarmtimer.h>
+#include <linux/sched/clock.h>
+#include <uapi/linux/sched/types.h>
+
+#include "rt1711h.h"
+
+#define RT1711H_DRV_VERSION"1.0.3"
+
+#define LOG_BUFFER_ENTRIES1024
+#define LOG_BUFFER_ENTRY_SIZE128 /* 128 char per line */
+
+enum {
+RT1711H_DBG_LOG = 0,
+RT1711H_DBG_REGS,
+RT1711H_DBG_REG_ADDR,
+RT1711H_DBG_DATA,
+RT1711H_DBG_MAX,
+};
+
+struct rt1711h_dbg_info {
+struct rt1711h_chip *chip;
+int id;
+};
+
+
+struct rt1711h_chip {
+struct i2c_client *i2c;
+struct device *dev;
+uint16_t did;
+int irq_gpio;
+int irq;
+char *name;
+struct tcpc_dev tcpc_dev;
+struct tcpc_config tcpc_cfg;
+struct tcpm_port *tcpm_port;
+struct regulator *vbus;
+struct extcon_dev *extcon;
+
+/* IRQ */
+struct kthread_worker irq_worker;
+struct kthread_work irq_work;
+struct task_struct *irq_worker_task;
+atomic_t poll_count;
+struct delayed_work poll_work;
+
+/* LPM */
+struct delayed_work wakeup_work;
+struct alarm wakeup_timer;
+struct mutex wakeup_lock;
+enum typec_cc_pull lpm_pull;
+bool wakeup_once;
+bool low_rp_duty_cntdown;
+bool cable_only;
+bool lpm;
+
+/* I2C */
+atomic_t i2c_busy;
+atomic_t pm_suspend;
+
+/* psy + psy status */
+struct power_supply *psy;
+u32 current_limit;
+u32 supply_voltage;
+
+/* lock for sharing chip states */
+struct mutex lock;
+
+/* port status */
+bool vconn_on;
+bool vbus_on;
+bool charge_on;
+bool vbus_present;
+enum typec_cc_polarity polarity;
+enum typec_cc_status cc1;
+enum typec_cc_status cc2;
+enum typec_role pwr_role;
+bool drp_toggling;
+
+#ifdef CONFIG_DEBUG_FS
+struct dentry *dbgdir;
+struct rt1711h_dbg_info dbg_info[RT1711H_DBG_MAX];
+struct dentry *dbg_files[RT1711H_DBG_MAX];
+int dbg_regidx;
+struct mutex dbgops_lock;
+/* lock for log buffer access */
+struct mutex logbuffer_lock;
+int logbuffer_head;
+int logbuffer_tail;
+u8 *logbuffer[LOG_BUFFER_ENTRIES];
+#endif /* CONFIG_DEBUG_FS */
+};
+
+/*
+ * Logging & debugging
+ */
+
+#ifdef CONFIG_DEBUG_FS
+
+static int rt1711h_reg_block_read(struct rt1711h_chip *chip, uint8_t reg,
+int len, uint8_t *data);
+static int rt1711h_reg_block_write(struct rt1711h_chip *chip, uint8_t reg,
+int len, const uint8_t *data);
+
+struct reg_desc {
+uint8_t addr;
+uint8_t size;
+};
+#define DECL_REG(_addr, _size) {.addr = _addr, .size = _size}
+
+static struct reg_desc rt1711h_reg_desc[] = {
+DECL_REG(RT1711H_REG_VID, 2),
+DECL_REG(RT1711H_REG_PID, 2),
+DECL_REG(RT1711H_REG_DID, 2),
+DECL_REG(RT1711H_REG_TYPEC_REV, 2),
+DECL_REG(RT1711H_REG_PD_REV, 2),
+DECL_REG(RT1711H_REG_PDIF_REV, 2),
+DECL_REG(RT1711H_REG_ALERT, 2),
+DECL_REG(RT1711H_REG_ALERT_MASK, 2),
+DECL_REG(RT1711H_REG_POWER_STATUS_MASK, 1),
+DECL_REG(RT1711H_REG_FAULT_STATUS_MASK, 1),
+DECL_REG(RT1711H_REG_TCPC_CTRL, 1),
+DECL_REG(RT1711H_REG_ROLE_CTRL, 1),
+DECL_REG(RT1711H_REG_FAULT_CTRL, 1),
+DECL_REG(RT1711H_REG_POWER_CTRL, 1),
+DECL_REG(RT1711H_REG_CC_STATUS, 1),
+DECL_REG(RT1711H_REG_POWER_STATUS, 1),
+DECL_REG(RT1711H_REG_FAULT_STATUS, 1),
+DECL_REG(RT1711H_REG_COMMAND, 1),
+DECL_REG(RT1711H_REG_MSG_HDR_INFO, 1),
+DECL_REG(RT1711H_REG_RX_DETECT, 1),
+DECL_REG(RT1711H_REG_RX_BYTE_CNT, 1),
+DECL_REG(RT1711H_REG_RX_BUF_FRAME_TYPE, 1),
+DECL_REG(RT1711H_REG_RX_HDR, 2),
+DECL_REG(RT1711H_REG_RX_DATA, 1),
+DECL_REG(RT1711H_REG_TRANSMIT, 1),
+DECL_REG(RT1711H_REG_TX_BYTE_CNT, 1),
+DECL_REG(RT1711H_REG_TX_HDR, 2),
+DECL_REG(RT1711H_REG_TX_DATA, 1),
+DECL_REG(RT1711H_REG_CLK_CTRL2, 1),
+DECL_REG(RT1711H_REG_CLK_CTRL3, 1),
+DECL_REG(RT1711H_REG_BMC_CTRL, 1),
+DECL_REG(RT1711H_REG_BMCIO_RXDZSEL, 1),
+DECL_REG(RT1711H_REG_VCONN_CLIMITEN, 1),
+DECL_REG(RT1711H_REG_RT_STATUS, 1),
+DECL_REG(RT1711H_REG_RT_INT, 1),
+DECL_REG(RT1711H_REG_RT_MASK, 1),
+DECL_REG(RT1711H_REG_IDLE_CTRL, 1),
+DECL_REG(RT1711H_REG_INTRST_CTRL, 1),
+DECL_REG(RT1711H_REG_WATCHDOG_CTRL, 1),
+DECL_REG(RT1711H_REG_I2CRST_CTRL, 1),
+DECL_REG(RT1711H_REG_SWRESET, 1),
+DECL_REG(RT1711H_REG_TTCPC_FILTER, 1),
+DECL_REG(RT1711H_REG_DRP_TOGGLE_CYCLE, 1),
+DECL_REG(RT1711H_REG_DRP_DUTY_CTRL, 1),
+DECL_REG(RT1711H_REG_BMCIO_RXDZEN, 1),
+};
+
+static const char *rt1711h_dbg_filename[RT1711H_DBG_MAX] = {
+"log", "regs", "reg_addr", "data",
+};
+
+static bool rt1711h_log_full(struct rt1711h_chip *chip)
+{
+return chip->logbuffer_tail ==
+(chip->logbuffer_head + 1) % LOG_BUFFER_ENTRIES;
+}
+
+static void _rt1711h_log(struct rt1711h_chip *chip, const char *fmt,
+ va_list args)
+{
+char tmpbuffer[LOG_BUFFER_ENTRY_SIZE];
+u64 ts_nsec = local_clock();
+unsigned long rem_nsec;
+
+if (!chip->logbuffer[chip->logbuffer_head]) {
+chip->logbuffer[chip->logbuffer_head] =
+devm_kzalloc(chip->dev, LOG_BUFFER_ENTRY_SIZE, GFP_KERNEL);
+if (!chip->logbuffer[chip->logbuffer_head])
+return;
+}
+
+vsnprintf(tmpbuffer, sizeof(tmpbuffer), fmt, args);
+
+mutex_lock(&chip->logbuffer_lock);
+
+if (rt1711h_log_full(chip)) {
+chip->logbuffer_head = max(chip->logbuffer_head - 1, 0);
+strlcpy(tmpbuffer, "overflow", sizeof(tmpbuffer));
+}
+
+if (chip->logbuffer_head < 0 ||
+chip->logbuffer_head >= LOG_BUFFER_ENTRIES) {
+dev_warn(chip->dev, "%s bad log buffer index %d\n", __func__,
+chip->logbuffer_head);
+goto abort;
+}
+
+if (!chip->logbuffer[chip->logbuffer_head]) {
+dev_warn(chip->dev, "%s log buffer index %d is NULL\n",
+__func__, chip->logbuffer_head);
+goto abort;
+}
+
+rem_nsec = do_div(ts_nsec, 1000000000);
+scnprintf(chip->logbuffer[chip->logbuffer_head], LOG_BUFFER_ENTRY_SIZE,
+"[%5lu.%06lu] %s", (unsigned long)ts_nsec, rem_nsec / 1000,
+ tmpbuffer);
+chip->logbuffer_head = (chip->logbuffer_head + 1) % LOG_BUFFER_ENTRIES;
+
+abort:
+mutex_unlock(&chip->logbuffer_lock);
+}
+
+static void rt1711h_log(struct rt1711h_chip *chip,
+const char *fmt, ...)
+{
+va_list args;
+
+va_start(args, fmt);
+_rt1711h_log(chip, fmt, args);
+va_end(args);
+}
+
+static int rt1711h_log_show(struct rt1711h_chip *chip, struct seq_file *s)
+{
+int tail;
+
+mutex_lock(&chip->logbuffer_lock);
+tail = chip->logbuffer_tail;
+while (tail != chip->logbuffer_head) {
+seq_printf(s, "%s", chip->logbuffer[tail]);
+tail = (tail + 1) % LOG_BUFFER_ENTRIES;
+}
+if (!seq_has_overflowed(s))
+chip->logbuffer_tail = tail;
+mutex_unlock(&chip->logbuffer_lock);
+
+return 0;
+}
+
+static int rt1711h_regs_show(struct rt1711h_chip *chip, struct seq_file *s)
+{
+int ret = 0;
+int i = 0, j = 0;
+struct reg_desc *desc = NULL;
+uint8_t regval[2] = {0};
+
+for (i = 0; i < ARRAY_SIZE(rt1711h_reg_desc); i++) {
+desc = &rt1711h_reg_desc[i];
+ret = rt1711h_reg_block_read(chip, desc->addr, desc->size,
+regval);
+if (ret < 0) {
+dev_err(chip->dev, "%s read reg0x%02X fail\n",
+__func__, desc->addr);
+continue;
+}
+
+seq_printf(s, "reg0x%02x:0x", desc->addr);
+for (j = 0; j < desc->size; j++)
+seq_printf(s, "%02x,", regval[j]);
+seq_puts(s, "\n");
+}
+
+return 0;
+}
+
+static inline int rt1711h_reg_addr_show(struct rt1711h_chip *chip,
+struct seq_file *s)
+{
+struct reg_desc *desc = &rt1711h_reg_desc[chip->dbg_regidx];
+
+seq_printf(s, "0x%02x\n", desc->addr);
+return 0;
+}
+
+static inline int rt1711h_data_show(struct rt1711h_chip *chip,
+struct seq_file *s)
+{
+int ret = 0, i = 0;
+struct reg_desc *desc = &rt1711h_reg_desc[chip->dbg_regidx];
+uint8_t regval[2] = {0};
+
+ret = rt1711h_reg_block_read(chip, desc->addr, desc->size, regval);
+if (ret < 0)
+return ret;
+
+seq_printf(s, "reg0x%02x=0x", desc->addr);
+for (i = 0; i < desc->size; i++)
+seq_printf(s, "%02x,", regval[i]);
+seq_puts(s, "\n");
+return 0;
+}
+
+static int rt1711h_dbg_show(struct seq_file *s, void *v)
+{
+int ret = 0;
+struct rt1711h_dbg_info *info = (struct rt1711h_dbg_info *)s->private;
+struct rt1711h_chip *chip = info->chip;
+
+mutex_lock(&chip->dbgops_lock);
+switch (info->id) {
+case RT1711H_DBG_LOG:
+ret = rt1711h_log_show(chip, s);
+break;
+case RT1711H_DBG_REGS:
+ret = rt1711h_regs_show(chip, s);
+break;
+case RT1711H_DBG_REG_ADDR:
+ret = rt1711h_reg_addr_show(chip, s);
+break;
+case RT1711H_DBG_DATA:
+ret = rt1711h_data_show(chip, s);
+break;
+default:
+ret = -EINVAL;
+break;
+}
+
+mutex_unlock(&chip->dbgops_lock);
+return ret;
+}
+
+static int rt1711h_dbg_open(struct inode *inode, struct file *file)
+{
+if (file->f_mode & FMODE_READ)
+return single_open(file, rt1711h_dbg_show, inode->i_private);
+file->private_data = inode->i_private;
+return 0;
+}
+
+static int get_parameters(char *buf, long int *param1, int num_of_par)
+{
+char *token;
+int base, cnt;
+
+token = strsep(&buf, " ");
+
+for (cnt = 0; cnt < num_of_par; cnt++) {
+if (token != NULL) {
+if ((token[1] == 'x') || (token[1] == 'X'))
+base = 16;
+else
+base = 10;
+
+if (kstrtoul(token, base, &param1[cnt]) != 0)
+return -EINVAL;
+
+token = strsep(&buf, " ");
+} else
+return -EINVAL;
+}
+return 0;
+}
+
+static int get_datas(const char *buf, const int length,
+unsigned char *data_buffer, unsigned char data_length)
+{
+int i, ptr;
+long int value;
+char token[5];
+
+token[0] = '0';
+token[1] = 'x';
+token[4] = 0;
+if (buf[0] != '0' || buf[1] != 'x')
+return -EINVAL;
+
+ptr = 2;
+for (i = 0; (i < data_length) && (ptr + 2 <= length); i++) {
+token[2] = buf[ptr++];
+token[3] = buf[ptr++];
+ptr++;
+if (kstrtoul(token, 16, &value) != 0)
+return -EINVAL;
+data_buffer[i] = value;
+}
+return 0;
+}
+
+static int rt1711h_regaddr2idx(uint8_t reg_addr)
+{
+int i = 0;
+struct reg_desc *desc = NULL;
+
+for (i = 0; i < ARRAY_SIZE(rt1711h_reg_desc); i++) {
+desc = &rt1711h_reg_desc[i];
+if (desc->addr == reg_addr)
+return i;
+}
+return -EINVAL;
+}
+
+static ssize_t rt1711h_dbg_write(struct file *file, const char __user *ubuf,
+size_t count, loff_t *ppos)
+{
+int ret = 0;
+struct rt1711h_dbg_info *info =
+(struct rt1711h_dbg_info *)file->private_data;
+struct rt1711h_chip *chip = info->chip;
+struct reg_desc *desc = NULL;
+char lbuf[128];
+long int param[5];
+unsigned char reg_data[2] = {0};
+
+if (count > sizeof(lbuf) - 1)
+return -EFAULT;
+
+ret = copy_from_user(lbuf, ubuf, count);
+if (ret)
+return -EFAULT;
+lbuf[count] = '\0';
+
+mutex_lock(&chip->dbgops_lock);
+switch (info->id) {
+case RT1711H_DBG_REG_ADDR:
+ret = get_parameters(lbuf, param, 1);
+if (ret < 0) {
+dev_err(chip->dev, "%s get param fail\n", __func__);
+ret = -EINVAL;
+goto out;
+}
+ret = rt1711h_regaddr2idx(param[0]);
+if (ret < 0) {
+dev_err(chip->dev, "%s addr2idx fail\n", __func__);
+ret = -EINVAL;
+goto out;
+}
+chip->dbg_regidx = ret;
+break;
+case RT1711H_DBG_DATA:
+desc = &rt1711h_reg_desc[chip->dbg_regidx];
+if ((desc->size - 1) * 3 + 5 != count) {
+dev_err(chip->dev, "%s incorrect input length\n",
+__func__);
+ret = -EINVAL;
+goto out;
+}
+ret = get_datas((char *)ubuf, count, reg_data, desc->size);
+if (ret < 0) {
+dev_err(chip->dev, "%s get data fail\n", __func__);
+ret = -EINVAL;
+goto out;
+}
+ret = rt1711h_reg_block_write(chip, desc->addr, desc->size,
+reg_data);
+break;
+default:
+ret = -EINVAL;
+break;
+}
+
+out:
+mutex_unlock(&chip->dbgops_lock);
+return ret < 0 ? ret : count;
+}
+
+static int rt1711h_dbg_release(struct inode *inode, struct file *file)
+{
+if (file->f_mode & FMODE_READ)
+return single_release(inode, file);
+return 0;
+}
+
+static const struct file_operations rt1711h_dbg_ops = {
+.open= rt1711h_dbg_open,
+.llseek= seq_lseek,
+.read= seq_read,
+.write= rt1711h_dbg_write,
+.release= rt1711h_dbg_release,
+};
+
+
+static int rt1711h_debugfs_init(struct rt1711h_chip *chip)
+{
+int ret = 0, i = 0;
+struct rt1711h_dbg_info *info = NULL;
+int len = 0;
+char *dirname = NULL;
+
+mutex_init(&chip->logbuffer_lock);
+mutex_init(&chip->dbgops_lock);
+len = strlen(dev_name(chip->dev));
+dirname = devm_kzalloc(chip->dev, len + 9, GFP_KERNEL);
+if (!dirname)
+return -ENOMEM;
+snprintf(dirname, len + 9, "rt1711h-%s", dev_name(chip->dev));
+if (!chip->dbgdir) {
+chip->dbgdir = debugfs_create_dir(dirname, NULL);
+if (!chip->dbgdir)
+return -ENOMEM;
+}
+
+for (i = 0; i < RT1711H_DBG_MAX; i++) {
+info = &chip->dbg_info[i];
+info->chip = chip;
+info->id = i;
+chip->dbg_files[i] = debugfs_create_file(
+rt1711h_dbg_filename[i], S_IFREG | 0444,
+chip->dbgdir, info, &rt1711h_dbg_ops);
+if (!chip->dbg_files[i]) {
+ret = -EINVAL;
+goto err;
+}
+}
+
+return 0;
+err:
+debugfs_remove_recursive(chip->dbgdir);
+return ret;
+}
+
+static void rt1711h_debugfs_exit(struct rt1711h_chip *chip)
+{
+debugfs_remove_recursive(chip->dbgdir);
+}
+
+#else
+
+static void rt1711h_log(const struct rt1711h_chip *chip, const char *fmt, ...)
+{
+}
+
+static int rt1711h_debugfs_init(const struct rt1711h_chip *chip)
+{
+return 0;
+}
+
+static void rt1711h_debugfs_exit(const struct rt1711h_chip *chip)
+{
+}
+
+#endif /* CONFIG_DEBUG_FS */
+
+static const char * const typec_cc_status_name[] = {
+[TYPEC_CC_OPEN]= "Open",
+[TYPEC_CC_RA]= "Ra",
+[TYPEC_CC_RD]= "Rd",
+[TYPEC_CC_RP_DEF]= "Rp-def",
+[TYPEC_CC_RP_1_5]= "Rp-1.5",
+[TYPEC_CC_RP_3_0]= "Rp-3.0",
+};
+
+static const char * const cc_polarity_name[] = {
+[TYPEC_POLARITY_CC1]= "Polarity_CC1",
+[TYPEC_POLARITY_CC2]= "Polarity_CC2",
+};
+
+static const char * const transmit_type_name[] = {
+[TCPC_TX_SOP]= "SOP",
+[TCPC_TX_SOP_PRIME]= "SOP'",
+[TCPC_TX_SOP_PRIME_PRIME]= "SOP''",
+[TCPC_TX_SOP_DEBUG_PRIME]= "DEBUG'",
+[TCPC_TX_SOP_DEBUG_PRIME_PRIME]= "DEBUG''",
+[TCPC_TX_HARD_RESET]= "HARD_RESET",
+[TCPC_TX_CABLE_RESET]= "CABLE_RESET",
+[TCPC_TX_BIST_MODE_2]= "BIST_MODE_2",
+};
+
+static const char * const typec_role_name[] = {
+[TYPEC_SINK]= "Sink",
+[TYPEC_SOURCE]= "Source",
+};
+
+static const char * const typec_data_role_name[] = {
+[TYPEC_DEVICE]= "Device",
+[TYPEC_HOST]= "Host",
+};
+
+static const enum typec_cc_pull typec_cc_status_pull_mapping[] = {
+[TYPEC_CC_OPEN] = TYPEC_CC_PULL_OPEN,
+[TYPEC_CC_RA] = TYPEC_CC_PULL_RA,
+[TYPEC_CC_RD] = TYPEC_CC_PULL_RD,
+[TYPEC_CC_RP_DEF] = TYPEC_CC_PULL_RP_DEF,
+[TYPEC_CC_RP_1_5] = TYPEC_CC_PULL_RP_1_5,
+[TYPEC_CC_RP_3_0] = TYPEC_CC_PULL_RP_3_0,
+};
+
+static inline enum typec_cc_pull rt1711h_cc_status2pull(enum typec_cc_status cc)
+{
+return typec_cc_status_pull_mapping[cc];
+}
+
+#define PDO_FIXED_FLAGS \
+(PDO_FIXED_DUAL_ROLE | PDO_FIXED_DATA_SWAP | PDO_FIXED_USB_COMM)
+
+static const u32 src_pdo[] = {
+PDO_FIXED(5000, 500, PDO_FIXED_FLAGS),
+};
+
+static const u32 snk_pdo[] = {
+PDO_FIXED(5000, 500, PDO_FIXED_FLAGS),
+};
+
+static const struct tcpc_config rt1711h_tcpc_config = {
+.src_pdo = src_pdo,
+.nr_src_pdo = ARRAY_SIZE(src_pdo),
+.snk_pdo = snk_pdo,
+.nr_snk_pdo = ARRAY_SIZE(snk_pdo),
+.max_snk_mv = 5000,
+.max_snk_ma = 3000,
+.max_snk_mw = 15000,
+.operating_snk_mw = 2500,
+.type = TYPEC_PORT_DRP,
+.default_role = TYPEC_SINK,
+.alt_modes = NULL,
+};
+
+#define RT1711H_RESUME_RETRY 10
+#define RT1711H_RESUME_RETRY_SLEEP 50
+
+static inline bool rt1711h_is_suspended(struct rt1711h_chip *chip)
+{
+int retry_cnt = 0;
+
+for (retry_cnt = 0; retry_cnt < RT1711H_RESUME_RETRY; retry_cnt++) {
+if (atomic_read(&chip->pm_suspend)) {
+rt1711h_log(chip, "%s retry %d/%d\n", __func__,
+retry_cnt + 1, RT1711H_RESUME_RETRY);
+msleep(RT1711H_RESUME_RETRY_SLEEP);
+} else
+return false;
+}
+
+return true;
+}
+
+static int rt1711h_reg_read(struct rt1711h_chip *chip, uint8_t reg,
+uint8_t *data)
+{
+int ret = 0;
+
+atomic_set(&chip->i2c_busy, 1);
+if (rt1711h_is_suspended(chip)) {
+atomic_set(&chip->i2c_busy, 0);
+return -ETIMEDOUT;
+}
+
+ret = i2c_smbus_read_i2c_block_data(chip->i2c, reg, 1, data);
+if (ret < 0)
+rt1711h_log(chip, "%s reg%02X fail(%d)\n", __func__, reg, ret);
+atomic_set(&chip->i2c_busy, 0);
+
+return ret;
+}
+
+static int rt1711h_reg_write(struct rt1711h_chip *chip, uint8_t reg,
+uint8_t data)
+{
+int ret = 0;
+
+atomic_set(&chip->i2c_busy, 1);
+if (rt1711h_is_suspended(chip)) {
+atomic_set(&chip->i2c_busy, 0);
+return -ETIMEDOUT;
+}
+
+ret = i2c_smbus_write_i2c_block_data(chip->i2c, reg, 1, &data);
+if (ret < 0)
+rt1711h_log(chip, "%s reg%02X = %02X fail(%d)\n", __func__, reg,
+data, ret);
+atomic_set(&chip->i2c_busy, 0);
+
+return ret;
+}
+
+static int rt1711h_reg_block_write(struct rt1711h_chip *chip, uint8_t reg,
+int len, const uint8_t *data)
+{
+int ret = 0;
+
+atomic_set(&chip->i2c_busy, 1);
+if (rt1711h_is_suspended(chip)) {
+atomic_set(&chip->i2c_busy, 0);
+return -ETIMEDOUT;
+}
+
+ret = i2c_smbus_write_i2c_block_data(chip->i2c, reg, len, data);
+if (ret < 0)
+rt1711h_log(chip, "%s reg%02X, len = %d fail(%d)\n", __func__,
+reg, len, ret);
+atomic_set(&chip->i2c_busy, 0);
+
+return ret;
+}
+
+static int rt1711h_reg_block_read(struct rt1711h_chip *chip, uint8_t reg,
+int len, uint8_t *data)
+{
+int ret = 0;
+
+atomic_set(&chip->i2c_busy, 1);
+if (rt1711h_is_suspended(chip)) {
+atomic_set(&chip->i2c_busy, 0);
+return -ETIMEDOUT;
+}
+
+ret = i2c_smbus_read_i2c_block_data(chip->i2c, reg, len, data);
+if (ret < 0)
+rt1711h_log(chip, "%s reg%02X, len = %d fail(%d)\n", __func__,
+reg, len, ret);
+atomic_set(&chip->i2c_busy, 0);
+
+return ret;
+}
+
+static inline int rt1711h_reg_write_word(struct rt1711h_chip *chip, uint8_t reg,
+uint16_t data)
+{
+data = cpu_to_le16(data);
+return rt1711h_reg_block_write(chip, reg, 2, (uint8_t *)&data);
+}
+
+static inline int rt1711h_reg_read_word(struct rt1711h_chip *chip, uint8_t reg,
+uint16_t *data)
+{
+int ret = 0;
+
+ret = rt1711h_reg_block_read(chip, reg, 2, (uint8_t *)data);
+if (ret < 0)
+return ret;
+*data = le16_to_cpu(*data);
+return 0;
+}
+
+static int rt1711h_psy_get_property(struct power_supply *psy,
+enum power_supply_property psp, union power_supply_propval *val)
+{
+struct rt1711h_chip *chip = power_supply_get_drvdata(psy);
+
+switch (psp) {
+case POWER_SUPPLY_PROP_ONLINE:
+val->intval = chip->charge_on;
+break;
+case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+val->intval = chip->supply_voltage * 1000; /* mV -> µV */
+break;
+case POWER_SUPPLY_PROP_CURRENT_MAX:
+val->intval = chip->current_limit * 1000; /* mA -> µA */
+break;
+default:
+return -ENODATA;
+}
+
+return 0;
+}
+
+static enum power_supply_property rt1711h_psy_properties[] = {
+POWER_SUPPLY_PROP_ONLINE,
+POWER_SUPPLY_PROP_VOLTAGE_NOW,
+POWER_SUPPLY_PROP_CURRENT_MAX,
+};
+
+static const struct power_supply_desc rt1711h_psy_desc = {
+.name= "rt1711h-typec-source",
+.type= POWER_SUPPLY_TYPE_USB_TYPE_C,
+.properties= rt1711h_psy_properties,
+.num_properties= ARRAY_SIZE(rt1711h_psy_properties),
+.get_property= rt1711h_psy_get_property,
+};
+
+static inline int rt1711h_software_reset(struct rt1711h_chip *chip)
+{
+int ret = 0;
+
+ret = rt1711h_reg_write(chip, RT1711H_REG_SWRESET, 0x01);
+if (ret < 0)
+return ret;
+
+usleep_range(1000, 2000);
+return 0;
+}
+
+static inline int rt1711h_command(struct rt1711h_chip *chip, uint8_t cmd)
+{
+return rt1711h_reg_write(chip, RT1711H_REG_COMMAND, cmd);
+}
+
+static inline int rt1711h_init_cc_params(struct rt1711h_chip *chip,
+const enum typec_cc_status *cc)
+{
+int ret = 0;
+uint8_t en = 0, sel = 0;
+
+if (*cc == TYPEC_CC_RP_DEF) { /* 0.55 */
+en = 0;
+sel = 0x81;
+} else if (chip->did >= RT1711H_DID_D) { /* 0.35 & 0.75 */
+en = 1;
+sel = 0x81;
+} else { /* 0.4 & 0.7 */
+en = 1;
+sel = 0x80;
+}
+
+ret = rt1711h_reg_write(chip, RT1711H_REG_BMCIO_RXDZEN, en);
+if (ret < 0)
+return ret;
+
+return rt1711h_reg_write(chip, RT1711H_REG_BMCIO_RXDZSEL, sel);
+}
+
+static int rt1711h_alert_status_clear(struct rt1711h_chip *chip, uint32_t mask)
+{
+int ret = 0;
+uint16_t mask_t1 = 0;
+uint8_t mask_t2 = 0;
+
+/* Write 1 clear */
+mask_t1 = (uint16_t)mask;
+ret = rt1711h_reg_write_word(chip, RT1711H_REG_ALERT, mask_t1);
+if (ret < 0)
+return ret;
+
+mask_t2 = mask >> 16;
+if (mask_t2) {
+ret = rt1711h_reg_write(chip, RT1711H_REG_RT_INT, mask_t2);
+if (ret < 0)
+return ret;
+}
+
+return 0;
+}
+
+static int rt1711h_init_alert_mask(struct rt1711h_chip *chip)
+{
+uint16_t mask = 0;
+
+mask = RT1711H_REG_ALERT_CC_STATUS | RT1711H_REG_ALERT_POWER_STATUS;
+
+mask |= RT1711H_REG_ALERT_TX_SUCCESS | RT1711H_REG_ALERT_TX_DISCARDED
+| RT1711H_REG_ALERT_TX_FAILED | RT1711H_REG_ALERT_RX_HARD_RST
+| RT1711H_REG_ALERT_RX_STATUS | RT1711H_REG_ALERT_RX_BUF_OVF;
+
+mask |= RT1711H_REG_ALERT_FAULT;
+
+return rt1711h_reg_write_word(chip, RT1711H_REG_ALERT_MASK, mask);
+}
+
+static int rt1711h_init_power_status_mask(struct rt1711h_chip *chip)
+{
+uint8_t mask = RT1711H_REG_POWER_STATUS_VBUS_PRES;
+
+return rt1711h_reg_write(chip, RT1711H_REG_POWER_STATUS_MASK, mask);
+}
+
+static int rt1711h_init_fault_mask(struct rt1711h_chip *chip)
+{
+const uint8_t mask = RT1711H_REG_FAULT_STATUS_VCONN_OV
+| RT1711H_REG_FAULT_STATUS_VCONN_OC;
+
+return rt1711h_reg_write(chip, RT1711H_REG_FAULT_STATUS_MASK, mask);
+}
+
+static int rt1711h_init_rt_mask(struct rt1711h_chip *chip)
+{
+uint8_t rt_mask = RT1711H_REG_M_VBUS_80;
+
+if (chip->did < RT1711H_DID_D)
+rt_mask |= (RT1711H_REG_M_WAKEUP | RT1711H_REG_M_RA_DETACH);
+
+return rt1711h_reg_write(chip, RT1711H_REG_RT_MASK, rt_mask);
+}
+
+#define RT1711H_WAKEUP_WORK_TIME(1000)
+static enum alarmtimer_restart
+rt1711h_alarm_wakeup_handler(struct alarm *alarm, ktime_t now)
+{
+struct rt1711h_chip *chip =
+container_of(alarm, struct rt1711h_chip, wakeup_timer);
+
+rt1711h_log(chip, "%s\n", __func__);
+pm_wakeup_event(chip->dev, RT1711H_WAKEUP_WORK_TIME);
+schedule_delayed_work(&chip->wakeup_work, 0);
+return ALARMTIMER_NORESTART;
+}
+
+static void rt1711h_enable_wakeup_timer(struct rt1711h_chip *chip, bool en)
+{
+int tout = 300; /* s */
+
+rt1711h_log(chip, "%s %d\n", __func__, en);
+if (en) {
+if (!chip->wakeup_once)
+tout = (chip->low_rp_duty_cntdown) ? 5 : 20;
+alarm_start_relative(&chip->wakeup_timer, ktime_set(tout, 0));
+} else
+alarm_cancel(&chip->wakeup_timer);
+}
+
+static inline bool rt1711h_is_cc_open(struct rt1711h_chip *chip)
+{
+if (!chip->drp_toggling && chip->cc1 == TYPEC_CC_OPEN &&
+chip->cc2 == TYPEC_CC_OPEN)
+return true;
+return false;
+}
+
+static int rt1711h_set_low_rp_duty(struct rt1711h_chip *chip, bool low_rp)
+{
+uint16_t duty = low_rp ? RT1711H_LOW_RP_DUTY : RT1711H_NORMAL_RP_DUTY;
+
+return rt1711h_reg_write_word(chip, RT1711H_REG_DRP_DUTY_CTRL, duty);
+}
+
+/*
+ * rt1711h_check_false_ra_detach
+ *
+ * Check single Ra resistance (eMark) exists or not when
+ *1) ra detach int triggered
+ *2) wakeup timer triggered
+ *
+ * If reentering low-power mode and eMark still exists,
+ * it may cause an infinite loop.
+ *
+ * If cc status is both open, return true; otherwise return false
+ */
+static int __tcpm_get_cc(struct rt1711h_chip *chip);
+static int __tcpm_set_cc(struct rt1711h_chip *chip, enum typec_cc_status cc);
+static inline int __tcpm_start_drp_toggling(struct rt1711h_chip *chip);
+static inline bool rt1711h_check_false_ra_detach(struct rt1711h_chip *chip)
+{
+bool drp = (chip->tcpc_cfg.type == TYPEC_PORT_DRP) ? true : false;
+
+rt1711h_log(chip, "%s\n", __func__);
+
+/*
+ * If the DUT is DRP and current CC status has stopped toggling,
+ * let cc_handler to handle it later.
+ *
+ * If CC is toggling, force CC to present Rp
+ */
+if (drp) {
+__tcpm_get_cc(chip);
+
+if (!chip->drp_toggling) {
+rt1711h_log(chip, "%s 1(%s, %s)\n", __func__,
+typec_cc_status_name[chip->cc1],
+typec_cc_status_name[chip->cc2]);
+return true;
+}
+__tcpm_set_cc(chip, TYPEC_CC_RP_DEF);
+usleep_range(1000, 2000);
+}
+
+/*
+ * Check CC status
+ * Rd (device) -> let cc_handler to handle it later
+ * eMark only -> Reschedule wakeup timer
+ * Open -> (true condition)
+ * Read to reenter low-power mode.
+ * If we repeatedly enter this situation,
+ * it will trigger low rp duty protection
+ */
+__tcpm_get_cc(chip);
+if (rt1711h_is_cc_open(chip))
+chip->cable_only = false;
+else if ((chip->cc1 + chip->cc2) == TYPEC_CC_RA) {
+chip->cable_only = true;
+rt1711h_log(chip, "%s 2(emark)\n", __func__);
+} else {
+chip->cable_only = false;
+rt1711h_log(chip, "%s 3(%s %s)\n", __func__,
+typec_cc_status_name[chip->cc1],
+typec_cc_status_name[chip->cc2]);
+return true;
+}
+
+if (chip->cable_only)
+rt1711h_enable_wakeup_timer(chip, true);
+else {
+if (chip->low_rp_duty_cntdown)
+rt1711h_set_low_rp_duty(chip, true);
+else {
+chip->wakeup_once = false;
+chip->low_rp_duty_cntdown = true;
+}
+}
+
+/* If DUP is DRP, force CC to toggle again */
+if (drp) {
+__tcpm_start_drp_toggling(chip);
+rt1711h_alert_status_clear(chip,
+RT1711H_REG_ALERT_EXT_RA_DETACH);
+}
+
+return chip->cable_only;
+}
+
+static int rt1711h_set_low_power_mode(struct rt1711h_chip *chip, bool en,
+enum typec_cc_pull pull)
+{
+uint8_t data = 0;
+
+rt1711h_log(chip, "%s %d\n", __func__, en);
+
+if (en) {
+data = RT1711H_REG_BMCIO_LPEN;
+
+if (pull & TYPEC_CC_PULL_RP)
+data |= RT1711H_REG_BMCIO_LPRPRD;
+} else
+data = RT1711H_REG_BMCIO_BG_EN |
+RT1711H_REG_VBUS_DET_EN | RT1711H_REG_BMCIO_OSC_EN;
+
+return rt1711h_reg_write(chip, RT1711H_REG_BMC_CTRL, data);
+}
+
+static int rt1711h_enter_lpm_again(struct rt1711h_chip *chip)
+{
+bool check_ra = (chip->lpm) || (chip->cable_only);
+
+if (check_ra && rt1711h_check_false_ra_detach(chip))
+return 0;
+
+rt1711h_log(chip, "%s retry lpm\n", __func__);
+chip->lpm = true;
+
+rt1711h_set_low_power_mode(chip, true,
+(chip->pwr_role != TYPEC_SOURCE) ?
+TYPEC_CC_PULL_DRP : TYPEC_CC_PULL_RP);
+
+return 0;
+}
+
+static void rt1711h_wakeup_work(struct work_struct *work)
+{
+struct rt1711h_chip *chip =
+container_of(work, struct rt1711h_chip, wakeup_work.work);
+
+mutex_lock(&chip->wakeup_lock);
+mutex_lock(&chip->lock);
+rt1711h_log(chip, "%s\n", __func__);
+chip->wakeup_once = true;
+rt1711h_enter_lpm_again(chip);
+mutex_unlock(&chip->lock);
+mutex_unlock(&chip->wakeup_lock);
+pm_relax(chip->dev);
+}
+
+static inline int rt1711h_try_low_power_mode(struct rt1711h_chip *chip)
+{
+return rt1711h_set_low_power_mode(chip, true, chip->lpm_pull);
+}
+
+static inline int rt1711h_enter_low_power_mode(struct rt1711h_chip *chip)
+{
+return rt1711h_try_low_power_mode(chip);
+}
+
+static inline int rt1711h_enable_low_power_mode(struct rt1711h_chip *chip,
+enum typec_cc_pull pull)
+{
+if (chip->cable_only) {
+rt1711h_log(chip, "%s ra only\n", __func__);
+rt1711h_enable_wakeup_timer(chip, true);
+return 0;
+}
+
+if (chip->lpm != true) {
+chip->lpm = true;
+chip->lpm_pull = pull;
+return rt1711h_enter_low_power_mode(chip);
+}
+
+return 0;
+}
+
+static inline int rt1711h_disable_low_power_mode(struct rt1711h_chip *chip)
+{
+int ret = 0;
+
+if (chip->lpm != false) {
+chip->lpm = false;
+rt1711h_set_low_rp_duty(chip, false);
+ret = rt1711h_set_low_power_mode(chip, false,
+TYPEC_CC_PULL_DRP);
+}
+
+chip->wakeup_once = false;
+chip->low_rp_duty_cntdown = false;
+return ret;
+}
+
+static int tcpm_init(struct tcpc_dev *dev)
+{
+int ret = 0;
+struct rt1711h_chip *chip = container_of(dev,
+struct rt1711h_chip, tcpc_dev);
+
+rt1711h_log(chip, "%s\n", __func__);
+
+/* CK 300K from 320K, shipping off, auto_idle enable, tout = 32ms */
+ret = rt1711h_reg_write(chip, RT1711H_REG_IDLE_CTRL,
+RT1711H_REG_IDLE_SET(0, 1, 1, 2));
+if (ret < 0) {
+rt1711h_log(chip, "%s set idle ctrl fail(%d)\n", __func__, ret);
+return ret;
+}
+
+ret = rt1711h_reg_write(chip, RT1711H_REG_I2CRST_CTRL,
+RT1711H_REG_I2CRST_SET(true, 0x0F));
+if (ret < 0) {
+rt1711h_log(chip, "%s set i2crst fail(%d)\n", __func__, ret);
+return ret;
+}
+
+/* UFP Both RD setting */
+/* DRP = 0, RpVal = 0 (Default), Rd, Rd */
+ret = rt1711h_reg_write(chip, RT1711H_REG_ROLE_CTRL,
+RT1711H_REG_ROLE_CTRL_RES_SET(0, 0, TYPEC_CC_PULL_RD,
+TYPEC_CC_PULL_RD));
+if (ret < 0) {
+rt1711h_log(chip, "%s set role ctrl fail(%d)\n", __func__, ret);
+return ret;
+}
+
+/*
+ * CC Detect Debounce : (26.7 * val) us
+ * Transition window count : spec 12~20us, based on 2.4MHz
+ */
+ret = rt1711h_reg_write(chip, RT1711H_REG_TTCPC_FILTER, 0x0F);
+if (ret < 0) {
+rt1711h_log(chip, "%s set cc deb fail(%d)\n", __func__, ret);
+return ret;
+}
+
+/* DRP Toggle Cycle : (51.2 + 6.4 * val) ms */
+rt1711h_reg_write(chip, RT1711H_REG_DRP_TOGGLE_CYCLE, 4);
+if (ret < 0) {
+rt1711h_log(chip, "%s set tog cyc fail(%d)\n", __func__, ret);
+return ret;
+}
+
+/* DRP Duty Ctrl: 33% */
+ret = rt1711h_reg_write_word(chip, RT1711H_REG_DRP_DUTY_CTRL,
+RT1711H_NORMAL_RP_DUTY);
+if (ret < 0) {
+rt1711h_log(chip, "%s set drp duty fail(%d)\n", __func__, ret);
+return ret;
+}
+
+/* Vconn OC */
+ret = rt1711h_reg_write(chip, RT1711H_REG_VCONN_CLIMITEN, 1);
+if (ret < 0) {
+rt1711h_log(chip, "%s en vconn oc fail(%d)\n", __func__, ret);
+return ret;
+}
+
+/* Alert & Mask */
+ret = rt1711h_alert_status_clear(chip, 0xffffffff);
+if (ret < 0) {
+rt1711h_log(chip, "%s clear alert fail(%d)\n", __func__, ret);
+return ret;
+}
+ret = rt1711h_init_power_status_mask(chip);
+if (ret < 0) {
+rt1711h_log(chip, "%s init pwr mask fail(%d)\n", __func__, ret);
+return ret;
+}
+ret = rt1711h_init_alert_mask(chip);
+if (ret < 0) {
+rt1711h_log(chip, "%s init alert mask fail(%d)\n", __func__,
+ret);
+return ret;
+}
+ret = rt1711h_init_fault_mask(chip);
+if (ret < 0) {
+rt1711h_log(chip, "%s init fault mask fail(%d)\n", __func__,
+ret);
+return ret;
+}
+ret = rt1711h_init_rt_mask(chip);
+if (ret < 0) {
+rt1711h_log(chip, "%s init rt mask fail(%d)\n", __func__, ret);
+return ret;
+}
+
+return 0;
+}
+
+static int tcpm_get_vbus(struct tcpc_dev *dev)
+{
+int ret = 0;
+struct rt1711h_chip *chip = container_of(dev, struct rt1711h_chip,
+tcpc_dev);
+
+rt1711h_log(chip, "%s\n", __func__);
+mutex_lock(&chip->lock);
+ret = chip->vbus_present ? 1 : 0;
+mutex_unlock(&chip->lock);
+
+return ret;
+}
+
+static int tcpm_get_current_limit(struct tcpc_dev *dev)
+{
+struct rt1711h_chip *chip = container_of(dev, struct rt1711h_chip,
+tcpc_dev);
+int current_limit = 0;
+unsigned long timeout;
+
+rt1711h_log(chip, "%s\n", __func__);
+if (!chip->extcon)
+return 0;
+
+/*
+ * USB2 Charger detection may still be in progress when we get here,
+ * this can take upto 600ms, wait 800ms max.
+ */
+timeout = jiffies + msecs_to_jiffies(800);
+do {
+if (extcon_get_state(chip->extcon, EXTCON_CHG_USB_SDP) == 1)
+current_limit = 500;
+
+if (extcon_get_state(chip->extcon, EXTCON_CHG_USB_CDP) == 1 ||
+ extcon_get_state(chip->extcon, EXTCON_CHG_USB_ACA) == 1)
+current_limit = 1500;
+
+if (extcon_get_state(chip->extcon, EXTCON_CHG_USB_DCP) == 1)
+current_limit = 2000;
+
+msleep(50);
+} while (current_limit == 0 && time_before(jiffies, timeout));
+
+return current_limit;
+}
+
+static int __tcpm_set_cc(struct rt1711h_chip *chip, enum typec_cc_status cc)
+{
+uint8_t data = 0, pull = 0, rp_lvl = 0;
+
+rt1711h_log(chip, "%s %s\n", __func__, typec_cc_status_name[cc]);
+switch (cc) {
+case TYPEC_CC_OPEN:
+case TYPEC_CC_RD:
+case TYPEC_CC_RP_DEF:
+case TYPEC_CC_RP_1_5:
+case TYPEC_CC_RP_3_0:
+pull = rt1711h_cc_status2pull(cc);
+rp_lvl = RT1711H_TYPEC_CC_PULL_GET_RP_LVL(pull);
+pull = RT1711H_TYPEC_CC_PULL_GET_RES(pull);
+data = RT1711H_REG_ROLE_CTRL_RES_SET(0, rp_lvl, pull, pull);
+break;
+default:
+rt1711h_log(chip, "%s unsupported cc value %s\n", __func__,
+typec_cc_status_name[cc]);
+return -EINVAL;
+}
+
+return rt1711h_reg_write(chip, RT1711H_REG_ROLE_CTRL, data);
+}
+
+static int tcpm_set_cc(struct tcpc_dev *dev, enum typec_cc_status cc)
+{
+int ret = 0;
+struct rt1711h_chip *chip = container_of(dev, struct rt1711h_chip,
+tcpc_dev);
+
+mutex_lock(&chip->lock);
+ret = __tcpm_set_cc(chip, cc);
+mutex_unlock(&chip->lock);
+
+return ret;
+}
+
+static inline enum typec_cc_status rt1711h_cc2enum(enum typec_cc_status cc,
+bool act_as_sink)
+{
+return act_as_sink ? cc + 2 : cc;
+}
+
+static int __tcpm_get_cc(struct rt1711h_chip *chip)
+{
+int ret = 0;
+uint8_t status = 0, role_ctrl = 0, cc_role = 0;
+bool act_as_sink, act_as_drp;
+
+ret = rt1711h_reg_read(chip, RT1711H_REG_CC_STATUS, &status);
+if (ret < 0)
+return ret;
+
+ret = rt1711h_reg_read(chip, RT1711H_REG_ROLE_CTRL, &role_ctrl);
+if (ret < 0)
+return ret;
+
+if (status & RT1711H_REG_CC_STATUS_DRP_TOGGLING) {
+/* during toggling, consider cc as Open */
+chip->cc1 = TYPEC_CC_OPEN;
+chip->cc2 = TYPEC_CC_OPEN;
+rt1711h_log(chip, "%s drp toggling\n", __func__);
+return 0;
+}
+chip->drp_toggling = false;
+
+act_as_drp = RT1711H_REG_ROLE_CTRL_DRP & role_ctrl;
+
+if (act_as_drp)
+act_as_sink = RT1711H_REG_CC_STATUS_DRP_RESULT(status);
+else {
+cc_role = RT1711H_REG_ROLE_CTRL_CC1(role_ctrl);
+act_as_sink = (cc_role == TYPEC_CC_PULL_RP) ? false : true;
+}
+
+chip->cc1 = RT1711H_REG_CC_STATUS_CC1(status);
+chip->cc2 = RT1711H_REG_CC_STATUS_CC2(status);
+if (chip->cc1 != TYPEC_CC_OPEN)
+chip->cc1 = rt1711h_cc2enum(chip->cc1, act_as_sink);
+if (chip->cc2 != TYPEC_CC_OPEN)
+chip->cc2 = rt1711h_cc2enum(chip->cc2, act_as_sink);
+
+ret = rt1711h_init_cc_params(chip, chip->polarity ?
+&chip->cc2 : &chip->cc1);
+if (ret < 0)
+rt1711h_log(chip, "%s init cc param fail(%d)\n", __func__, ret);
+
+rt1711h_log(chip, "%s cc1 = %s, cc2 = %s\n", __func__,
+typec_cc_status_name[chip->cc1],
+typec_cc_status_name[chip->cc2]);
+
+return 0;
+}
+
+static int tcpm_get_cc(struct tcpc_dev *dev,
+enum typec_cc_status *cc1, enum typec_cc_status *cc2)
+{
+int ret = 0;
+struct rt1711h_chip *chip = container_of(dev, struct rt1711h_chip,
+tcpc_dev);
+
+mutex_lock(&chip->lock);
+ret = __tcpm_get_cc(chip);
+if (ret < 0)
+goto out;
+*cc1 = chip->cc1;
+*cc2 = chip->cc2;
+out:
+mutex_unlock(&chip->lock);
+return ret;
+}
+
+static int tcpm_set_polarity(struct tcpc_dev *dev,
+enum typec_cc_polarity polarity)
+{
+int ret = 0;
+uint8_t data = 0;
+struct rt1711h_chip *chip = container_of(dev, struct rt1711h_chip,
+tcpc_dev);
+
+mutex_lock(&chip->lock);
+rt1711h_log(chip, "%s %s\n", __func__, cc_polarity_name[polarity]);
+ret = rt1711h_init_cc_params(chip, polarity ? &chip->cc2 : &chip->cc1);
+if (ret < 0)
+goto out;
+
+ret = rt1711h_reg_read(chip, RT1711H_REG_TCPC_CTRL, &data);
+if (ret < 0)
+goto out;
+
+data &= ~RT1711H_REG_TCPC_CTRL_PLUG_ORIENT;
+data |= polarity ? RT1711H_REG_TCPC_CTRL_PLUG_ORIENT : 0;
+
+ret = rt1711h_reg_write(chip, RT1711H_REG_TCPC_CTRL, data);
+out:
+mutex_unlock(&chip->lock);
+return ret;
+}
+
+static int tcpm_set_vconn(struct tcpc_dev *dev, bool on)
+{
+int ret = 0;
+uint8_t data = 0;
+struct rt1711h_chip *chip = container_of(dev, struct rt1711h_chip,
+tcpc_dev);
+
+mutex_lock(&chip->lock);
+if (chip->vconn_on == on) {
+rt1711h_log(chip, "%s vconn is already %d\n", __func__, on);
+goto out;
+}
+ret = rt1711h_reg_read(chip, RT1711H_REG_POWER_CTRL, &data);
+if (ret < 0)
+goto out;
+
+data &= ~RT1711H_REG_POWER_CTRL_VCONN;
+data |= on ? RT1711H_REG_POWER_CTRL_VCONN : 0;
+
+ret = rt1711h_reg_write(chip, RT1711H_REG_POWER_CTRL, data);
+if (ret < 0)
+goto out;
+
+ret = rt1711h_reg_write(chip, RT1711H_REG_IDLE_CTRL,
+RT1711H_REG_IDLE_SET(0, 1, on ? 0 : 1, 2));
+if (ret < 0)
+goto out;
+
+chip->vconn_on = on;
+rt1711h_log(chip, "%s vconn = %d\n", __func__, on);
+
+out:
+mutex_unlock(&chip->lock);
+return ret;
+}
+
+static int tcpm_set_vbus(struct tcpc_dev *dev, bool on, bool charge)
+{
+int ret = 0;
+struct rt1711h_chip *chip = container_of(dev, struct rt1711h_chip,
+tcpc_dev);
+
+mutex_lock(&chip->lock);
+if (chip->vbus_on == on)
+rt1711h_log(chip, "%s vbus is already %d\n", __func__, on);
+else {
+ret = (on ? regulator_enable : regulator_disable)(chip->vbus);
+if (ret < 0) {
+rt1711h_log(chip, "%s cannot %s vbus regulator(%d)\n",
+__func__, on ? "enable" : "disable", ret);
+goto out;
+}
+chip->vbus_on = on;
+rt1711h_log(chip, "%s vbus = %d\n", __func__, on);
+}
+if (chip->charge_on == charge)
+rt1711h_log(chip, "%s chg is already %d\n", __func__, charge);
+else {
+chip->charge_on = charge;
+power_supply_changed(chip->psy);
+}
+
+out:
+mutex_unlock(&chip->lock);
+return 0;
+}
+
+static int tcpm_set_current_limit(struct tcpc_dev *dev, u32 max_ma,
+u32 mv)
+{
+struct rt1711h_chip *chip = container_of(dev, struct rt1711h_chip,
+tcpc_dev);
+
+rt1711h_log(chip, "%s %d ma, %d mv (not implemented)\n", __func__,
+max_ma, mv);
+
+mutex_lock(&chip->lock);
+chip->supply_voltage = mv;
+chip->current_limit = max_ma;
+mutex_unlock(&chip->lock);
+
+power_supply_changed(chip->psy);
+return 0;
+}
+
+static int tcpm_set_pd_rx(struct tcpc_dev *dev, bool on)
+{
+int ret = 0;
+uint8_t rx_en = 0x00;
+struct rt1711h_chip *chip = container_of(dev, struct rt1711h_chip,
+tcpc_dev);
+
+mutex_lock(&chip->lock);
+rt1711h_log(chip, "%s %d\n", __func__, on);
+if (on)
+rx_en = BIT(TCPC_TX_SOP) | BIT(TCPC_TX_HARD_RESET);
+
+ret = rt1711h_reg_write(chip, RT1711H_REG_RX_DETECT, rx_en);
+mutex_unlock(&chip->lock);
+return ret;
+}
+
+static int tcpm_set_roles(struct tcpc_dev *dev, bool attached,
+enum typec_role pwr, enum typec_data_role data)
+{
+int ret = 0;
+uint8_t msg_hdr = RT1711H_REG_MSG_HDR_INFO_SET(data, pwr);
+struct rt1711h_chip *chip = container_of(dev, struct rt1711h_chip,
+tcpc_dev);
+
+mutex_lock(&chip->lock);
+rt1711h_log(chip, "%s %s %s\n", __func__, typec_role_name[pwr],
+typec_data_role_name[data]);
+ret = rt1711h_reg_write(chip, RT1711H_REG_MSG_HDR_INFO, msg_hdr);
+if (ret < 0)
+goto out;
+chip->pwr_role = pwr;
+out:
+mutex_unlock(&chip->lock);
+return ret;
+}
+
+static inline int __tcpm_start_drp_toggling(struct rt1711h_chip *chip)
+{
+int ret = 0;
+uint8_t data = 0;
+uint8_t rp_def = RT1711H_TYPEC_CC_PULL_GET_RP_LVL(TYPEC_CC_PULL_RP_DEF);
+uint8_t cc_role = TYPEC_CC_PULL_RD;
+
+data = RT1711H_REG_ROLE_CTRL_RES_SET(0, rp_def, cc_role, cc_role);
+ret = rt1711h_reg_write(chip, RT1711H_REG_ROLE_CTRL, data);
+if (ret < 0)
+return ret;
+mdelay(1);
+data = RT1711H_REG_ROLE_CTRL_RES_SET(1, rp_def, cc_role, cc_role);
+ret = rt1711h_reg_write(chip, RT1711H_REG_ROLE_CTRL, data);
+if (ret < 0)
+return ret;
+ret = rt1711h_command(chip, RT1711H_CMD_LOOK_CONNECTION);
+if (ret < 0)
+return ret;
+chip->drp_toggling = true;
+
+return 0;
+}
+
+static int tcpm_start_drp_toggling(struct tcpc_dev *dev,
+enum typec_cc_status cc)
+{
+int ret = 0;
+struct rt1711h_chip *chip = container_of(dev, struct rt1711h_chip,
+tcpc_dev);
+
+mutex_lock(&chip->lock);
+rt1711h_log(chip, "%s\n", __func__);
+ret = __tcpm_start_drp_toggling(chip);
+if (ret < 0)
+goto out;
+if (chip->did < RT1711H_DID_D)
+ret = rt1711h_enable_low_power_mode(chip, TYPEC_CC_PULL_DRP);
+
+out:
+mutex_unlock(&chip->lock);
+return ret;
+}
+
+#pragma pack(push, 1)
+struct tcpc_transmit_packet {
+uint8_t cnt;
+uint16_t msg_header;
+uint8_t data[sizeof(uint32_t) * PD_MAX_PAYLOAD];
+};
+#pragma pack(pop)
+
+static int tcpm_pd_transmit(struct tcpc_dev *dev,
+enum tcpm_transmit_type type, const struct pd_message *msg)
+{
+int ret = 0;
+int data_cnt = 0;
+struct rt1711h_chip *chip = container_of(dev, struct rt1711h_chip,
+tcpc_dev);
+struct tcpc_transmit_packet packet;
+
+rt1711h_log(chip, "%s %s\n", __func__, transmit_type_name[type]);
+mutex_lock(&chip->lock);
+switch (type) {
+case TCPC_TX_SOP:
+data_cnt = sizeof(uint32_t) * pd_header_cnt_le(msg->header);
+packet.cnt = data_cnt + sizeof(uint16_t);
+packet.msg_header = msg->header;
+if (data_cnt > 0)
+memcpy(packet.data, (uint8_t *)msg->payload, data_cnt);
+
+ret = rt1711h_reg_block_write(chip, RT1711H_REG_TX_BYTE_CNT,
+packet.cnt + 1, (uint8_t *)&packet);
+if (ret < 0) {
+rt1711h_log(chip, "%s fail (%d)\n", __func__, ret);
+goto out;
+}
+break;
+case TCPC_TX_HARD_RESET:
+break;
+default:
+rt1711h_log(chip, "type %s not supported\n",
+transmit_type_name[type]);
+ret = -EINVAL;
+goto out;
+}
+
+ret = rt1711h_reg_write(chip, RT1711H_REG_TRANSMIT,
+RT1711H_REG_TRANSMIT_SET(3, type));
+out:
+mutex_unlock(&chip->lock);
+return ret;
+}
+
+static int rt1711h_parse_dt(struct rt1711h_chip *chip)
+{
+int ret = 0, len = 0;
+uint32_t val = 0;
+struct device_node *np = chip->dev->of_node;
+struct tcpc_config *cfg = &chip->tcpc_cfg;
+const char *name = "default";
+
+if (!np)
+return -EINVAL;
+
+dev_info(chip->dev, "%s\n", __func__);
+
+memcpy(cfg, &rt1711h_tcpc_config, sizeof(struct tcpc_config));
+
+ret = of_get_named_gpio(np, "rt,intr_gpio", 0);
+if (ret < 0) {
+dev_err(chip->dev, "%s get int gpio fail(%d)\n",
+__func__, ret);
+return ret;
+}
+chip->irq_gpio = ret;
+dev_info(chip->dev, "%s irq_gpio = %d\n", __func__, chip->irq_gpio);
+
+of_property_read_string(np, "rt,name", &name);
+
+len = strlen(name);
+chip->name = devm_kzalloc(chip->dev, len + 1, GFP_KERNEL);
+if (!chip->name)
+return -ENOMEM;
+strlcpy(chip->name, name, strlen(name) + 1);
+
+if (of_property_read_u32(np, "rt,def_role", &val) == 0)
+cfg->default_role = val;
+
+if (of_property_read_u32(np, "rt,port_type", &val) == 0)
+cfg->type = val;
+
+if (of_property_read_u32(np, "rt,max_snk_mv", &val) == 0)
+cfg->max_snk_mv = val;
+
+if (of_property_read_u32(np, "rt,max_snk_ma", &val) == 0)
+cfg->max_snk_ma = val;
+
+if (of_property_read_u32(np, "rt,max_snk_mw", &val) == 0)
+cfg->max_snk_mw = val;
+
+if (of_property_read_u32(np, "rt,operating_snk_mw", &val) == 0)
+cfg->operating_snk_mw = val;
+
+cfg->try_role_hw = of_property_read_bool(np, "rt,try_role_hw");
+
+return 0;
+}
+
+static void rt1711h_init_tcpc_dev(struct rt1711h_chip *chip)
+{
+chip->tcpc_dev.config = &chip->tcpc_cfg;
+chip->tcpc_dev.init = tcpm_init;
+chip->tcpc_dev.get_vbus = tcpm_get_vbus;
+chip->tcpc_dev.get_current_limit = tcpm_get_current_limit;
+chip->tcpc_dev.set_cc = tcpm_set_cc;
+chip->tcpc_dev.get_cc = tcpm_get_cc;
+chip->tcpc_dev.set_polarity = tcpm_set_polarity;
+chip->tcpc_dev.set_vconn = tcpm_set_vconn;
+chip->tcpc_dev.set_vbus = tcpm_set_vbus;
+chip->tcpc_dev.set_current_limit = tcpm_set_current_limit;
+chip->tcpc_dev.set_pd_rx = tcpm_set_pd_rx;
+chip->tcpc_dev.set_roles = tcpm_set_roles;
+chip->tcpc_dev.start_drp_toggling = tcpm_start_drp_toggling;
+chip->tcpc_dev.pd_transmit = tcpm_pd_transmit;
+chip->tcpc_dev.mux = NULL;
+}
+
+static int rt1711h_get_alert_status(struct rt1711h_chip *chip,
+uint32_t *alert)
+{
+int ret = 0;
+uint16_t data = 0;
+uint8_t rt_int = 0;
+
+ret = rt1711h_reg_read_word(chip, RT1711H_REG_ALERT, &data);
+if (ret < 0)
+return ret;
+*alert = data;
+
+ret = rt1711h_reg_read(chip, RT1711H_REG_RT_INT, &rt_int);
+if (ret < 0)
+return ret;
+*alert |= rt_int << 16;
+
+return 0;
+}
+
+static int rt1711h_get_fault_status(struct rt1711h_chip *chip, uint8_t *status)
+{
+return rt1711h_reg_read(chip, RT1711H_REG_FAULT_STATUS, status);
+}
+
+static inline int rt1711h_fault_status_vconn_ov(struct rt1711h_chip *chip)
+{
+int ret = 0;
+uint8_t data = 0;
+
+ret = rt1711h_reg_read(chip, RT1711H_REG_BMC_CTRL, &data);
+if (ret < 0)
+return ret;
+
+data &= ~RT1711H_REG_DISCHARGE_EN;
+return rt1711h_reg_write(chip, RT1711H_REG_BMC_CTRL, data);
+}
+
+static int rt1711h_fault_status_clear(struct rt1711h_chip *chip, uint8_t status)
+{
+if (status & RT1711H_REG_FAULT_STATUS_VCONN_OV)
+rt1711h_fault_status_vconn_ov(chip);
+
+return rt1711h_reg_write(chip, RT1711H_REG_FAULT_STATUS, status);
+}
+
+/* Alert handlers */
+
+static int rt1711h_alert_cc_changed(struct rt1711h_chip *chip)
+{
+int ret = 0;
+
+ret = __tcpm_get_cc(chip);
+if (ret < 0)
+return ret;
+
+if (chip->drp_toggling) {
+rt1711h_log(chip, "%s DRP toggling\n", __func__);
+if (chip->did < RT1711H_DID_D && chip->lpm && !chip->cable_only)
+rt1711h_enter_low_power_mode(chip);
+return 0;
+}
+if (chip->did < RT1711H_DID_D)
+rt1711h_disable_low_power_mode(chip);
+
+tcpm_cc_change(chip->tcpm_port);
+return 0;
+}
+
+static int rt1711h_alert_power_status_changed(struct rt1711h_chip *chip)
+{
+int ret = 0;
+bool vbus_pres = false;
+uint8_t data = 0;
+
+ret = rt1711h_reg_read(chip, RT1711H_REG_POWER_STATUS, &data);
+if (ret < 0)
+goto out;
+
+vbus_pres = (data & RT1711H_REG_POWER_STATUS_VBUS_PRES) ? true : false;
+if (vbus_pres != chip->vbus_present) {
+chip->vbus_present = vbus_pres;
+rt1711h_log(chip, "%s vbus = %d\n", __func__, vbus_pres);
+tcpm_vbus_change(chip->tcpm_port);
+}
+
+out:
+return ret;
+}
+
+static int rt1711h_alert_recv_msg(struct rt1711h_chip *chip)
+{
+int ret = 0, len = 0;
+uint8_t buf[2];
+struct pd_message msg;
+const uint32_t alert_rx =
+RT1711H_REG_ALERT_RX_STATUS | RT1711H_REG_ALERT_RX_BUF_OVF;
+
+rt1711h_log(chip, "%s\n", __func__);
+ret = rt1711h_reg_block_read(chip, RT1711H_REG_RX_HDR, 2, buf);
+if (ret < 0)
+return ret;
+
+memcpy(&(msg.header), buf, 2);
+
+len = pd_header_cnt_le(msg.header) * 4;
+if (len > PD_MAX_PAYLOAD * 4) {
+rt1711h_log(chip, "%s PD message too long %d\n", __func__, len);
+return -EINVAL;
+}
+if (len > 0)
+ret = rt1711h_reg_block_read(chip, RT1711H_REG_RX_DATA, len,
+(uint8_t *)msg.payload);
+
+/* Read complete, clear RX status alert bit */
+rt1711h_alert_status_clear(chip, alert_rx);
+
+tcpm_pd_receive(chip->tcpm_port, &msg);
+return ret;
+}
+
+static int rt1711h_alert_recv_hard_reset(struct rt1711h_chip *chip)
+{
+tcpm_pd_hard_reset(chip->tcpm_port);
+return 0;
+}
+
+static int rt1711h_alert_tx_failed(struct rt1711h_chip *chip)
+{
+tcpm_pd_transmit_complete(chip->tcpm_port, TCPC_TX_FAILED);
+return 0;
+}
+
+static int rt1711h_alert_tx_discard(struct rt1711h_chip *chip)
+{
+tcpm_pd_transmit_complete(chip->tcpm_port, TCPC_TX_DISCARDED);
+return 0;
+}
+
+static int rt1711h_alert_tx_success(struct rt1711h_chip *chip)
+{
+tcpm_pd_transmit_complete(chip->tcpm_port, TCPC_TX_SUCCESS);
+return 0;
+}
+
+static int rt1711h_alert_fault(struct rt1711h_chip *chip)
+{
+int ret = 0;
+uint8_t status = 0;
+
+ret = rt1711h_get_fault_status(chip, &status);
+if (ret < 0)
+return ret;
+
+rt1711h_log(chip, "%s 0x%02X\n", __func__, status);
+rt1711h_fault_status_clear(chip, status);
+return 0;
+}
+
+static int rt1711h_alert_rx_overflow(struct rt1711h_chip *chip)
+{
+int ret = 0;
+uint32_t alert_status = 0;
+
+rt1711h_log(chip, "%s\n", __func__);
+
+ret = rt1711h_get_alert_status(chip, &alert_status);
+if (ret < 0)
+return ret;
+
+if (alert_status & RT1711H_REG_ALERT_RX_STATUS)
+return rt1711h_alert_recv_msg(chip);
+
+return 0;
+}
+
+static int rt1711h_alert_wakeup(struct rt1711h_chip *chip)
+{
+rt1711h_log(chip, "%s\n", __func__);
+if (chip->drp_toggling)
+rt1711h_enable_wakeup_timer(chip, true);
+return 0;
+}
+
+static int rt1711h_alert_ra_detach(struct rt1711h_chip *chip)
+{
+rt1711h_log(chip, "%s\n", __func__);
+if (chip->drp_toggling)
+rt1711h_enter_lpm_again(chip);
+
+return 0;
+}
+
+struct rt1711h_alert_handler {
+uint32_t bit_mask;
+int (*handler)(struct rt1711h_chip *chip);
+};
+
+#define RT1711H_DECL_ALERT_HANDLER(xbit, xhandler) { \
+.bit_mask = 1 << xbit, \
+.handler = xhandler, \
+}
+
+static const struct rt1711h_alert_handler rt1711h_alert_handlers[] = {
+RT1711H_DECL_ALERT_HANDLER(4, rt1711h_alert_tx_failed),
+RT1711H_DECL_ALERT_HANDLER(5, rt1711h_alert_tx_discard),
+RT1711H_DECL_ALERT_HANDLER(6, rt1711h_alert_tx_success),
+RT1711H_DECL_ALERT_HANDLER(2, rt1711h_alert_recv_msg),
+RT1711H_DECL_ALERT_HANDLER(7, NULL),
+RT1711H_DECL_ALERT_HANDLER(8, NULL),
+RT1711H_DECL_ALERT_HANDLER(3, rt1711h_alert_recv_hard_reset),
+RT1711H_DECL_ALERT_HANDLER(10, rt1711h_alert_rx_overflow),
+RT1711H_DECL_ALERT_HANDLER(16, rt1711h_alert_wakeup),
+RT1711H_DECL_ALERT_HANDLER(21, rt1711h_alert_ra_detach),
+RT1711H_DECL_ALERT_HANDLER(9, rt1711h_alert_fault),
+RT1711H_DECL_ALERT_HANDLER(0, rt1711h_alert_cc_changed),
+RT1711H_DECL_ALERT_HANDLER(1, rt1711h_alert_power_status_changed),
+};
+
+static int __rt1711h_irq_handler(struct rt1711h_chip *chip)
+{
+int i = 0, ret = 0;
+uint32_t alert_status = 0;
+
+ret = rt1711h_get_alert_status(chip, &alert_status);
+if (ret < 0) {
+rt1711h_log(chip, "%s get alert status fail(%d)\n",
+__func__, ret);
+goto out;
+}
+
+rt1711h_alert_status_clear(chip,
+alert_status & (~RT1711H_REG_ALERT_RX_MASK));
+
+if (alert_status)
+rt1711h_log(chip, "%s 0x%04X\n", __func__, alert_status);
+
+if (alert_status & RT1711H_REG_ALERT_EXT_VBUS_80)
+alert_status |= RT1711H_REG_ALERT_POWER_STATUS;
+
+for (i = 0; i < ARRAY_SIZE(rt1711h_alert_handlers); i++) {
+if (rt1711h_alert_handlers[i].bit_mask & alert_status) {
+if (rt1711h_alert_handlers[i].handler != 0)
+rt1711h_alert_handlers[i].handler(chip);
+}
+}
+
+out:
+return ret;
+}
+
+static inline void rt1711h_poll_ctrl(struct rt1711h_chip *chip)
+{
+cancel_delayed_work_sync(&chip->poll_work);
+
+if (atomic_read(&chip->poll_count) == 0) {
+atomic_inc(&chip->poll_count);
+cpu_idle_poll_ctrl(true);
+}
+
+schedule_delayed_work(&chip->poll_work, msecs_to_jiffies(40));
+}
+
+static void rt1711h_irq_work_handler(struct kthread_work *work)
+{
+struct rt1711h_chip *chip =
+container_of(work, struct rt1711h_chip, irq_work);
+int ret = 0, gpio_val = 0;
+
+rt1711h_poll_ctrl(chip);
+mutex_lock(&chip->wakeup_lock);
+mutex_lock(&chip->lock);
+do {
+ret = __rt1711h_irq_handler(chip);
+if (ret < 0)
+break;
+gpio_val = gpio_get_value(chip->irq_gpio);
+} while (gpio_val == 0);
+mutex_unlock(&chip->lock);
+mutex_unlock(&chip->wakeup_lock);
+}
+
+static void rt1711h_poll_work(struct work_struct *work)
+{
+struct rt1711h_chip *chip = container_of(work, struct rt1711h_chip,
+poll_work.work);
+
+if (atomic_dec_and_test(&chip->poll_count))
+cpu_idle_poll_ctrl(false);
+}
+#define RT1711H_IRQ_WAKE_TIME(500) /* ms */
+
+static irqreturn_t rt1711h_intr_handler(int irq, void *data)
+{
+struct rt1711h_chip *chip = data;
+
+pm_wakeup_event(chip->dev, RT1711H_IRQ_WAKE_TIME);
+kthread_queue_work(&chip->irq_worker, &chip->irq_work);
+
+return IRQ_HANDLED;
+}
+
+static int rt1711h_init_alert(struct rt1711h_chip *chip)
+{
+int ret = 0, len = 0;
+char *name = NULL;
+struct sched_param param = {.sched_priority = MAX_RT_PRIO - 1};
+
+rt1711h_reg_write_word(chip, RT1711H_REG_ALERT_MASK, 0);
+rt1711h_reg_write_word(chip, RT1711H_REG_ALERT, 0xffff);
+
+len = strlen(chip->name);
+name = devm_kzalloc(chip->dev, len + 5, GFP_KERNEL);
+if (!name)
+return -ENOMEM;
+
+snprintf(name, len, "%s-IRQ", chip->name);
+
+dev_info(chip->dev, "%s name = %s, gpio = %d\n", __func__, chip->name,
+chip->irq_gpio);
+
+ret = devm_gpio_request_one(chip->dev, chip->irq_gpio,
+GPIOF_IN, name);
+if (ret < 0) {
+dev_err(chip->dev, "%s request gpio fail(%d)\n",
+__func__, ret);
+goto err_init_alert;
+}
+
+chip->irq = gpio_to_irq(chip->irq_gpio);
+if (chip->irq <= 0) {
+dev_err(chip->dev, "%s gpio2irq fail(%d)\n",
+__func__, chip->irq);
+ret = -EINVAL;
+goto err_init_alert;
+}
+dev_info(chip->dev, "%s irq = %d\n", __func__, chip->irq);
+
+kthread_init_worker(&chip->irq_worker);
+chip->irq_worker_task = kthread_run(kthread_worker_fn,
+&chip->irq_worker, chip->name);
+if (IS_ERR(chip->irq_worker_task)) {
+dev_err(chip->dev, "%s could not create tcpc task\n", __func__);
+goto err_init_alert;
+}
+
+sched_setscheduler(chip->irq_worker_task, SCHED_FIFO, &param);
+kthread_init_work(&chip->irq_work, rt1711h_irq_work_handler);
+
+ret = devm_request_irq(chip->dev, chip->irq, rt1711h_intr_handler,
+IRQF_TRIGGER_FALLING | IRQF_NO_THREAD | IRQF_NO_SUSPEND, name,
+chip);
+if (ret < 0) {
+dev_err(chip->dev, "%s request irq%d fail(%d)\n",
+__func__, chip->irq, ret);
+goto err_init_alert;
+}
+enable_irq_wake(chip->irq);
+return 0;
+
+err_init_alert:
+devm_kfree(chip->dev, name);
+return ret;
+}
+
+static int rt1711h_check_revision(struct i2c_client *i2c)
+{
+int ret = 0;
+
+ret = i2c_smbus_read_word_data(i2c, 0x00);
+if (ret < 0)
+return ret;
+if (ret != 0x29cf) {
+dev_err(&i2c->dev, "vid is not correct, 0x%04x\n", ret);
+return -ENODEV;
+}
+ret = i2c_smbus_read_word_data(i2c, 0x02);
+if (ret < 0)
+return ret;
+if (ret != 0x1711) {
+dev_err(&i2c->dev, "pid is not correct, 0x%04x\n", ret);
+return -ENODEV;
+}
+ret = i2c_smbus_read_word_data(i2c, 0x04);
+if (ret < 0)
+return ret;
+/* return did */
+return ret;
+}
+
+static int rt1711h_i2c_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+int ret = 0;
+uint16_t did = 0;
+struct rt1711h_chip *chip = NULL;
+struct power_supply_config cfg = {};
+
+pr_info("%s %s\n", __func__, RT1711H_DRV_VERSION);
+
+if (!i2c_check_functionality(client->adapter,
+I2C_FUNC_SMBUS_I2C_BLOCK)) {
+dev_err(&client->dev,
+"I2C/SMBusyy block functionality not supported!\n");
+return -ENODEV;
+}
+ret = rt1711h_check_revision(client);
+if (ret < 0) {
+dev_err(&client->dev, "check vid/pid/did fail\n");
+return ret;
+}
+did = (uint16_t)ret;
+dev_info(&client->dev, "did = 0x%04x\n", did);
+chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
+if (!chip)
+return -ENOMEM;
+chip->i2c = client;
+chip->dev = &client->dev;
+chip->did = did;
+mutex_init(&chip->lock);
+mutex_init(&chip->wakeup_lock);
+INIT_DELAYED_WORK(&chip->poll_work, rt1711h_poll_work);
+INIT_DELAYED_WORK(&chip->wakeup_work, rt1711h_wakeup_work);
+alarm_init(&chip->wakeup_timer, ALARM_REALTIME,
+rt1711h_alarm_wakeup_handler);
+i2c_set_clientdata(client, chip);
+
+ret = rt1711h_parse_dt(chip);
+if (ret < 0)
+goto out_parse_dt;
+
+cfg.drv_data = chip;
+chip->psy = devm_power_supply_register(chip->dev, &rt1711h_psy_desc,
+&cfg);
+if (IS_ERR(chip->psy)) {
+ret = PTR_ERR(chip->psy);
+dev_err(chip->dev, "%s register psy fail(%d)\n", __func__, ret);
+goto out_psy_reg;
+}
+
+chip->vbus = devm_regulator_get(chip->dev, "vbus");
+if (IS_ERR(chip->vbus)) {
+ret = PTR_ERR(chip->vbus);
+goto out_reg_get;
+}
+
+ret = rt1711h_debugfs_init(chip);
+if (ret < 0)
+goto out_dbgfs_init;
+
+ret = rt1711h_software_reset(chip);
+if (ret < 0)
+goto out_sw_reset;
+
+ret = rt1711h_init_alert(chip);
+if (ret < 0)
+goto out_init_alert;
+
+rt1711h_init_tcpc_dev(chip);
+
+chip->tcpm_port = tcpm_register_port(chip->dev,
+&chip->tcpc_dev);
+if (IS_ERR(chip->tcpm_port)) {
+ret = PTR_ERR(chip->tcpm_port);
+dev_err(chip->dev, "%s register tcpm port fail(%d)",
+__func__, ret);
+goto out_tcpm_reg;
+}
+
+pm_runtime_set_active(&client->dev);
+pm_runtime_enable(&client->dev);
+dev_info(chip->dev, "%s: successfully\n", __func__);
+return 0;
+
+out_tcpm_reg:
+out_init_alert:
+out_sw_reset:
+rt1711h_debugfs_exit(chip);
+out_dbgfs_init:
+out_reg_get:
+out_psy_reg:
+out_parse_dt:
+mutex_destroy(&chip->lock);
+devm_kfree(&client->dev, chip);
+return 0;
+}
+
+static int rt1711h_i2c_remove(struct i2c_client *client)
+{
+struct rt1711h_chip *chip = i2c_get_clientdata(client);
+
+pm_runtime_disable(&client->dev);
+pm_runtime_set_suspended(&client->dev);
+if (chip) {
+rt1711h_debugfs_exit(chip);
+mutex_destroy(&chip->lock);
+}
+dev_info(chip->dev, "%s: successfully\n", __func__);
+return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int rt1711h_i2c_pm_suspend(struct device *dev)
+{
+struct rt1711h_chip *chip = dev_get_drvdata(dev);
+
+if (chip) {
+if (atomic_read(&chip->i2c_busy))
+return -EBUSY;
+atomic_set(&chip->pm_suspend, 1);
+}
+return 0;
+}
+
+static int rt1711h_i2c_pm_resume(struct device *dev)
+{
+struct rt1711h_chip *chip = dev_get_drvdata(dev);
+
+if (chip)
+atomic_set(&chip->pm_suspend, 0);
+return 0;
+}
+#endif /* CONFIG_PM_SLEEP */
+
+static const struct dev_pm_ops rt1711h_i2c_pm_ops = {
+SET_SYSTEM_SLEEP_PM_OPS(rt1711h_i2c_pm_suspend, rt1711h_i2c_pm_resume)
+};
+
+static const struct of_device_id rt1711h_of_device_id[] = {
+{ .compatible = "richtek,typec_rt1711h",},
+{ },
+};
+MODULE_DEVICE_TABLE(of, rt1711h_of_device_id);
+
+static const struct i2c_device_id rt1711h_i2c_device_id[] = {
+{ "typec_rt1711h", 0},
+{ },
+};
+MODULE_DEVICE_TABLE(i2c, rt1711h_i2c_device_id);
+
+static struct i2c_driver rt1711h_i2c_driver = {
+.driver = {
+.name = "typec_rt1711h",
+.owner = THIS_MODULE,
+.of_match_table = of_match_ptr(rt1711h_of_device_id),
+.pm = &rt1711h_i2c_pm_ops,
+},
+.probe = rt1711h_i2c_probe,
+.remove = rt1711h_i2c_remove,
+.id_table = rt1711h_i2c_device_id,
+};
+module_i2c_driver(rt1711h_i2c_driver);
+
+MODULE_AUTHOR("cy_huang <[email protected]>");
+MODULE_DESCRIPTION("rt1711h typec driver");
+MODULE_VERSION(RT1711H_DRV_VERSION);
+MODULE_LICENSE("GPL");
diff --git a/drivers/usb/typec/rt1711h/rt1711h.h b/drivers/usb/typec/rt1711h/rt1711h.h
new file mode 100644
index 0000000..8b67464
--- /dev/null
+++ b/drivers/usb/typec/rt1711h/rt1711h.h
@@ -0,0 +1,300 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2017 Richtek Technologh Corp.
+ *
+ * Richtek RT1711H Type-C Chip Driver
+ */
+
+#ifndef __LINUX_RT1711H_H
+#define __LINUX_RT1711H_H
+
+/* Device ID */
+#define RT1711H_DID_A0x2170
+#define RT1711H_DID_B0x2171
+#define RT1711H_DID_C0x2172
+#define RT1711H_DID_D0x2173
+
+/* Registers */
+#define RT1711H_REG_VID(0x00)
+#define RT1711H_REG_PID(0x02)
+#define RT1711H_REG_DID(0x04)
+#define RT1711H_REG_TYPEC_REV(0x06)
+#define RT1711H_REG_PD_REV(0x08)
+#define RT1711H_REG_PDIF_REV(0x0A)
+#define RT1711H_REG_ALERT(0x10)
+#define RT1711H_REG_ALERT_MASK(0x12)
+#define RT1711H_REG_POWER_STATUS_MASK(0x14)
+#define RT1711H_REG_FAULT_STATUS_MASK(0x15)
+#define RT1711H_REG_TCPC_CTRL(0x19)
+#define RT1711H_REG_ROLE_CTRL(0x1A)
+#define RT1711H_REG_FAULT_CTRL(0x1B)
+#define RT1711H_REG_POWER_CTRL(0x1C)
+#define RT1711H_REG_CC_STATUS(0x1D)
+#define RT1711H_REG_POWER_STATUS(0x1E)
+#define RT1711H_REG_FAULT_STATUS(0x1F)
+#define RT1711H_REG_COMMAND(0x23)
+#define RT1711H_REG_MSG_HDR_INFO(0x2e)
+#define RT1711H_REG_RX_DETECT(0x2f)
+#define RT1711H_REG_RX_BYTE_CNT(0x30)
+#define RT1711H_REG_RX_BUF_FRAME_TYPE(0x31)
+#define RT1711H_REG_RX_HDR(0x32)
+#define RT1711H_REG_RX_DATA(0x34)
+#define RT1711H_REG_TRANSMIT(0x50)
+#define RT1711H_REG_TX_BYTE_CNT(0x51)
+#define RT1711H_REG_TX_HDR(0x52)
+#define RT1711H_REG_TX_DATA(0x54)
+
+#define RT1711H_REG_CLK_CTRL2(0x87)
+#define RT1711H_REG_CLK_CTRL3(0x88)
+#define RT1711H_REG_BMC_CTRL(0x90)
+#define RT1711H_REG_BMCIO_RXDZSEL(0x93)
+#define RT1711H_REG_VCONN_CLIMITEN(0x95)
+#define RT1711H_REG_RT_STATUS(0x97)
+#define RT1711H_REG_RT_INT(0x98)
+#define RT1711H_REG_RT_MASK(0x99)
+#define RT1711H_REG_IDLE_CTRL(0x9B)
+#define RT1711H_REG_INTRST_CTRL(0x9C)
+#define RT1711H_REG_WATCHDOG_CTRL(0x9D)
+#define RT1711H_REG_I2CRST_CTRL(0X9E)
+#define RT1711H_REG_SWRESET(0xA0)
+#define RT1711H_REG_TTCPC_FILTER(0xA1)
+#define RT1711H_REG_DRP_TOGGLE_CYCLE(0xA2)
+#define RT1711H_REG_DRP_DUTY_CTRL(0xA3)
+#define RT1711H_REG_BMCIO_RXDZEN(0xAF)
+
+
+#ifndef BIT
+#define BIT(x)(1 << (x))
+#endif
+
+/*
+ * RT1711H_REG_ALERT(0x10)
+ * RT1711H_REG_ALERT_MASK(0x12)
+ */
+#define RT1711H_REG_VBUS_SINK_DISCONNECTBIT(11)
+#define RT1711H_REG_ALERT_RX_BUF_OVFBIT(10)
+#define RT1711H_REG_ALERT_FAULTBIT(9)
+#define RT1711H_REG_ALERT_LO_VOLTBIT(8)
+#define RT1711H_REG_ALERT_HI_VOLTBIT(7)
+#define RT1711H_REG_ALERT_TX_SUCCESSBIT(6)
+#define RT1711H_REG_ALERT_TX_DISCARDEDBIT(5)
+#define RT1711H_REG_ALERT_TX_FAILEDBIT(4)
+#define RT1711H_REG_ALERT_RX_HARD_RSTBIT(3)
+#define RT1711H_REG_ALERT_RX_STATUSBIT(2)
+#define RT1711H_REG_ALERT_POWER_STATUSBIT(1)
+#define RT1711H_REG_ALERT_CC_STATUSBIT(0)
+#define RT1711H_REG_ALERT_RX_MASK \
+(RT1711H_REG_ALERT_RX_STATUS | RT1711H_REG_ALERT_RX_BUF_OVF)
+
+/*
+ * RT1711H_REG_POWER_STATUS_MASK(0x14)
+ * RT1711H_REG_POWER_STATUS(0x1E)
+ */
+#define RT1711H_REG_POWER_STATUS_TCPC_INITIALBIT(6)
+#define RT1711H_REG_POWER_STATUS_SRC_HVBIT(5)
+#define RT1711H_REG_POWER_STATUS_SRC_VBUSBIT(4)
+#define RT1711H_REG_POWER_STATUS_VBUS_PRES_DETBIT(3)
+#define RT1711H_REG_POWER_STATUS_VBUS_PRESBIT(2)
+#define RT1711H_REG_POWER_STATUS_VCONN_PRESBIT(1)
+#define RT1711H_REG_POWER_STATUS_SINK_VBUSBIT(0)
+
+/*
+ * RT1711H_REG_FAULT_STATUS_MASK(0x15)
+ * RT1711H_REG_FAULT_STATUS(0x1F)
+ */
+#define RT1711H_REG_FAULT_STATUS_VCONN_OVBIT(7)
+#define RT1711H_REG_FAULT_STATUS_FORCE_OFF_VBUSBIT(6)
+#define RT1711H_REG_FAULT_STATUS_AUTO_DISC_FAILBIT(5)
+#define RT1711H_REG_FAULT_STATUS_FORCE_DISC_FAILBIT(4)
+#define RT1711H_REG_FAULT_STATUS_VBUS_OCBIT(3)
+#define RT1711H_REG_FAULT_STATUS_VBUS_OVBIT(2)
+#define RT1711H_REG_FAULT_STATUS_VCONN_OCBIT(1)
+#define RT1711H_REG_FAULT_STATUS_I2C_ERRORBIT(0)
+
+/*
+ * RT1711H_REG_ROLE_CTRL(0x1A)
+ */
+#define RT1711H_REG_ROLE_CTRL_DRPBIT(6)
+#define RT1711H_REG_ROLE_CTRL_RES_SET(drp, rp, cc1, cc2) \
+((drp) << 6 | (rp) << 4 | (cc2) << 2 | (cc1))
+#define RT1711H_REG_ROLE_CTRL_CC2(reg)(((reg) & 0x0C) >> 2)
+#define RT1711H_REG_ROLE_CTRL_CC1(reg)((reg) & 0x03)
+#define RT1711H_TYPEC_CC_PULL_GET_RP_LVL(pull)((pull & 0x18) >> 3)
+#define RT1711H_TYPEC_CC_PULL_GET_RES(pull)(pull & 0x07)
+
+enum typec_cc_pull {
+TYPEC_CC_PULL_RA = 0,
+TYPEC_CC_PULL_RP,
+TYPEC_CC_PULL_RD,
+TYPEC_CC_PULL_OPEN,
+TYPEC_CC_PULL_DRP,/* from rd */
+
+TYPEC_CC_PULL_RP_DEF = 1,/* 0x00 + 1 */
+TYPEC_CC_PULL_RP_1_5 = 9,/* 0x08 + 1 */
+TYPEC_CC_PULL_RP_3_0 = 17,/* 0x10 + 1 */
+
+TYPEC_CC_PULL_DRP_DEF = 4,/* 0x00 + 4 */
+TYPEC_CC_PULL_DRP_1_5 = 12,/* 0x08 + 4 */
+TYPEC_CC_PULL_DRP_3_0 = 20,/* 0x10 + 4 */
+};
+
+/*
+ * RT1711H_REG_TCPC_CTRL(0x19)
+ */
+#define RT1711H_REG_TCPC_CTRL_BIST_TEST_MODEBIT(1)
+#define RT1711H_REG_TCPC_CTRL_PLUG_ORIENTBIT(0)
+
+/*
+ * RT1711H_REG_FAULT_CTRL(0x1B)
+ */
+#define RT1711H_REG_FAULT_CTRL_DIS_VCONN_OVBIT(7)
+#define RT1711H_REG_FAULT_CTRL_DIS_SNK_VBUS_OCBIT(2)
+#define RT1711H_REG_FAULT_CTRL_DIS_VCONN_OCBIT(0)
+
+/*
+ * RT1711H_REG_POWER_CTRL(0x1C)
+ */
+#define RT1711H_REG_POWER_CTRL_VCONNBIT(0)
+
+/*
+ * RT1711H_REG_CC_STATUS(0x1D)
+ */
+#define RT1711H_REG_CC_STATUS_DRP_TOGGLINGBIT(5)
+#define RT1711H_REG_CC_STATUS_DRP_RESULT(reg)(((reg) & 0x10) >> 4)
+#define RT1711H_REG_CC_STATUS_CC2(reg)(((reg) & 0x0C) >> 2)
+#define RT1711H_REG_CC_STATUS_CC1(reg)((reg) & 0x03)
+#define RT1711H_REG_CC_STATUS_RD2ENUM(cc)((cc) + 2)
+
+/*
+ * RT1711H_REG_COMMAND(0x23)
+ */
+enum rt1711h_command {
+RT1711H_CMD_WAKE_I2C = 0x11,
+RT1711H_CMD_DISABLE_VBUS_DETECT = 0x22,
+RT1711H_CMD_ENABLE_VBUS_DETECT = 0x33,
+RT1711H_CMD_DISABLE_SINK_VBUS = 0x44,
+RT1711H_CMD_ENABLE_SINK_VBUS = 0x55,
+RT1711H_CMD_DISABLE_SOURCE_VBUS = 0x66,
+RT1711H_CMD_ENABLE_SOURCE_VBUS = 0x77,
+RT1711H_CMD_SOURCE_VBUS_HV = 0x88,
+RT1711H_CMD_LOOK_CONNECTION = 0x99,
+RT1711H_CMD_RX_ONE_MODE = 0xAA,
+RT1711H_CMD_I2C_IDLE = 0xFF,
+};
+
+
+/*
+ * RT1711H_REG_MSG_HDR_INFO(0x2E)
+ */
+#define RT1711H_REG_MSG_HDR_INFO_SET(drole, prole) \
+((drole) << 3 | (PD_REV20 << 1) | (prole))
+#define RT1711H_REG_MSG_HDR_INFO_DROLE(reg)(((reg) & 0x08) >> 3)
+#define RT1711H_REG_MSG_HDR_INFO_PROLE(reg)((reg) & 0x01)
+
+/*
+ * RT1711H_REG_TRANSMIT(0x50)
+ */
+#define RT1711H_REG_TRANSMIT_SET(retry, type)((retry) << 4 | (type))
+
+
+/*
+ * RT1711H_REG_CLK_CTRL2(0x87)
+ */
+#define RT1711H_REG_CLK_DIV_600K_ENBIT(7)
+#define RT1711H_REG_CLK_BCLK2_ENBIT(6)
+#define RT1711H_REG_CLK_BCLK2_TG_ENBIT(5)
+#define RT1711H_REG_CLK_DIV_300K_ENBIT(3)
+#define RT1711H_REG_CLK_CK_300K_ENBIT(2)
+#define RT1711H_REG_CLK_BCLK_ENBIT(1)
+#define RT1711H_REG_CLK_BCLK_TH_ENBIT(0)
+
+/*
+ * RT1711H_REG_CLK_CTRL3(0x88)
+ */
+#define RT1711H_REG_CLK_OSCMUX_RG_ENBIT(7)
+#define RT1711H_REG_CLK_CK_24M_ENBIT(6)
+#define RT1711H_REG_CLK_OSC_RG_ENBIT(5)
+#define RT1711H_REG_CLK_DIV_2P4M_ENBIT(4)
+#define RT1711H_REG_CLK_CK_2P4M_ENBIT(3)
+#define RT1711H_REG_CLK_PCLK_ENBIT(2)
+#define RT1711H_REG_CLK_PCLK_RG_ENBIT(1)
+#define RT1711H_REG_CLK_PCLK_TG_ENBIT(0)
+
+/*
+ * RT1711H_REG_BMC_CTRL(0x90)
+ */
+#define RT1711H_REG_IDLE_ENBIT(6)
+#define RT1711H_REG_DISCHARGE_ENBIT(5)
+#define RT1711H_REG_BMCIO_LPRPRDBIT(4)
+#define RT1711H_REG_BMCIO_LPENBIT(3)
+#define RT1711H_REG_BMCIO_BG_ENBIT(2)
+#define RT1711H_REG_VBUS_DET_ENBIT(1)
+#define RT1711H_REG_BMCIO_OSC_ENBIT(0)
+
+/*
+ * RT1711H_REG_RT_STATUS(0x97)
+ */
+#define RT1711H_REG_RA_DETACHBIT(5)
+#define RT1711H_REG_VBUS_80BIT(1)
+
+/*
+ * RT1711H_REG_RT_INT(0x98)
+ */
+#define RT1711H_REG_INT_RA_DETACHBIT(5)
+#define RT1711H_REG_INT_WATCHDOGBIT(2)
+#define RT1711H_REG_INT_VBUS_80BIT(1)
+#define RT1711H_REG_INT_WAKEUPBIT(0)
+
+/*
+ * RT1711H_REG_RT_MASK(0x99)
+ */
+#define RT1711H_REG_M_RA_DETACHBIT(5)
+#define RT1711H_REG_M_WATCHDOGBIT(2)
+#define RT1711H_REG_M_VBUS_80BIT(1)
+#define RT1711H_REG_M_WAKEUPBIT(0)
+#define RT1711H_REG_ALERT_EXT_RA_DETACH(1 << (16 + 5))
+#define RT1711H_REG_ALERT_EXT_VBUS_80(1 << (16 + 1))
+
+/*
+ * RT1711H_REG_IDLE_CTRL(0x9B)
+ */
+#define RT1711H_REG_CK_300K_SELBIT(7)
+#define RT1711H_REG_SHIPPING_OFFBIT(5)
+#define RT1711H_REG_AUTOIDLE_ENBIT(3)
+
+/* timeout = (tout*2+1) * 6.4ms */
+#define RT1711H_REG_IDLE_SET(ck300, ship_dis, auto_idle, tout) \
+(((ck300) << 7) | ((ship_dis) << 5) | \
+ ((auto_idle) << 3) | ((tout) & 0x07))
+
+/*
+ * RT1711H_REG_INTRST_CTRL(0x9C)
+ */
+#define RT1711H_REG_INTRST_ENBIT(7)
+
+/* timeout = (tout+1) * 0.2sec */
+#define RT1711H_REG_INTRST_SET(en, tout)(((en) << 7) | ((tout) & 0x03))
+
+/*
+ * RT1711H_REG_WATCHDOG_CTRL(0x9D)
+ */
+#define RT1711H_REG_WATCHDOG_ENBIT(7)
+
+/* timeout = (tout+1) * 0.4sec */
+#define RT1711H_REG_WATCHDOG_CTRL_SET(en, tout)(((en) << 7) | ((tout) & 0x07))
+
+/*
+ * RT1711H_REG_I2CRST_CTRL(0x9E)
+ */
+#define RT1711H_REG_I2CRST_ENBIT(7)
+
+/* timeout = (tout+1) * 12.5ms */
+#define RT1711H_REG_I2CRST_SET(en, tout)((en << 7) | (tout & 0x0F))
+
+/*
+ * RT1711H_REG_DRP_DUTY_CTRL(0xA3)
+ */
+#define RT1711H_LOW_RP_DUTY(100)/* 10% */
+#define RT1711H_NORMAL_RP_DUTY(330)/* 33% */
+
+#endif /* __LINUX_RT1711H_H */
--
1.9.1

************* Email Confidentiality Notice ********************

The information contained in this e-mail message (including any attachments) may be confidential, proprietary, privileged, or otherwise exempt from disclosure under applicable laws. It is intended to be conveyed only to the designated recipient(s). Any use, dissemination, distribution, printing, retaining or copying of this e-mail (including its attachments) by unintended recipient(s) is strictly prohibited and may be unlawful. If you are not an intended recipient of this e-mail, or believe that you have received this e-mail in error, please notify the sender immediately (by replying to this e-mail), delete any and all copies of this e-mail (including any attachments) from your system, and do not disclose the content of this e-mail to any other person. Thank you!

2018-01-17 11:09:05

[permalink] [raw]

Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

Hi,

On Wed, Jan 17, 2018 at 09:30:45AM +0000, shufan_lee(?????????) wrote:
> Dear Heikki,
>
> Sorry for bothering.
>
> Just want to check is there anything we need to modify?

I'll check the patch this week, but please note that we are -rc8, so
nothing is going to happen before -rc1 is out.

Br,

--
heikki

2018-01-17 11:14:22

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

On Wed, Jan 17, 2018 at 01:08:58PM +0200, Heikki Krogerus wrote:
> Hi,
>
> On Wed, Jan 17, 2018 at 09:30:45AM +0000, shufan_lee(?????????) wrote:
> > Dear Heikki,
> >
> > Sorry for bothering.
> >
> > Just want to check is there anything we need to modify?
>
> I'll check the patch this week, but please note that we are -rc8, so
> nothing is going to happen before -rc1 is out.

If you ack it today, I could queue it up for -rc1 as it is a stand-alone
driver...

2018-01-17 12:00:35

[permalink] [raw]

Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

Hi Greg,

On Wed, Jan 17, 2018 at 12:14:02PM +0100, Greg KH wrote:
> On Wed, Jan 17, 2018 at 01:08:58PM +0200, Heikki Krogerus wrote:
> > Hi,
> >
> > On Wed, Jan 17, 2018 at 09:30:45AM +0000, shufan_lee(?????????) wrote:
> > > Dear Heikki,
> > >
> > > Sorry for bothering.
> > >
> > > Just want to check is there anything we need to modify?
> >
> > I'll check the patch this week, but please note that we are -rc8, so
> > nothing is going to happen before -rc1 is out.
>
> If you ack it today, I could queue it up for -rc1 as it is a stand-alone
> driver...

The driver does not compile as module:

ERROR: "cpu_idle_poll_ctrl" [drivers/usb/typec/rt1711h/rt1711h.ko] undefined!

Br,

--
heikki

2018-01-17 13:31:13

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

On Wed, Jan 17, 2018 at 02:00:28PM +0200, Heikki Krogerus wrote:
> Hi Greg,
>
> On Wed, Jan 17, 2018 at 12:14:02PM +0100, Greg KH wrote:
> > On Wed, Jan 17, 2018 at 01:08:58PM +0200, Heikki Krogerus wrote:
> > > Hi,
> > >
> > > On Wed, Jan 17, 2018 at 09:30:45AM +0000, shufan_lee(?????????) wrote:
> > > > Dear Heikki,
> > > >
> > > > Sorry for bothering.
> > > >
> > > > Just want to check is there anything we need to modify?
> > >
> > > I'll check the patch this week, but please note that we are -rc8, so
> > > nothing is going to happen before -rc1 is out.
> >
> > If you ack it today, I could queue it up for -rc1 as it is a stand-alone
> > driver...
>
> The driver does not compile as module:
>
> ERROR: "cpu_idle_poll_ctrl" [drivers/usb/typec/rt1711h/rt1711h.ko] undefined!

Well of course it has to pass review :)

Why wouuld a driver be calling that function? That's not right at all,
ick...

Shufan, what are you doing there?

thanks,

greg k-h

2018-01-17 13:33:15

[permalink] [raw]

Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

On Wed, Jan 10, 2018 at 02:59:12PM +0800, ShuFanLee wrote:
> +static inline void rt1711h_poll_ctrl(struct rt1711h_chip *chip)
> +{
> + cancel_delayed_work_sync(&chip->poll_work);
> +
> + if (atomic_read(&chip->poll_count) == 0) {
> + atomic_inc(&chip->poll_count);
> + cpu_idle_poll_ctrl(true);
> + }
> +
> + schedule_delayed_work(&chip->poll_work, msecs_to_jiffies(40));
> +}

This is very odd, and not good. What are you trying to do here? And
why are you thinking that poll_count should be an atomic variable?

This feels really strange, and not something you should be doing in an
irq handler, right?

thanks,

greg k-h

2018-01-17 13:34:10

[permalink] [raw]

Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

On Wed, Jan 10, 2018 at 02:59:12PM +0800, ShuFanLee wrote:
> +static inline int rt1711h_reg_write_word(struct rt1711h_chip *chip, uint8_t reg,
> + uint16_t data)
> +{
> + data = cpu_to_le16(data);
> + return rt1711h_reg_block_write(chip, reg, 2, (uint8_t *)&data);
> +}

Did you run sparse on this code? What are you doing casting the types
all over the place for data? That does not seem correct at all.

thanks,

greg k-h

2018-01-17 13:42:32

[permalink] [raw]

Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

On Wed, Jan 10, 2018 at 02:59:12PM +0800, ShuFanLee wrote:
> From: ShuFanLee <[email protected]>
>
> Richtek RT1711H Type-C chip driver that works with
> Type-C Port Controller Manager to provide USB PD and
> USB Type-C functionalities.
>
> Signed-off-by: ShuFanLee <[email protected]>

Minor review of your main structure and your debugfs code and other
stuff, all of which need work:

> ---
> .../devicetree/bindings/usb/richtek,rt1711h.txt | 38 +
> arch/arm64/boot/dts/hisilicon/rt1711h.dtsi | 11 +
> drivers/usb/typec/Kconfig | 2 +
> drivers/usb/typec/Makefile | 1 +
> drivers/usb/typec/rt1711h/Kconfig | 7 +
> drivers/usb/typec/rt1711h/Makefile | 2 +
> drivers/usb/typec/rt1711h/rt1711h.c | 2241 ++++++++++++++++++++
> drivers/usb/typec/rt1711h/rt1711h.h | 300 +++
> 8 files changed, 2602 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
> create mode 100644 arch/arm64/boot/dts/hisilicon/rt1711h.dtsi
> create mode 100644 drivers/usb/typec/rt1711h/Kconfig
> create mode 100644 drivers/usb/typec/rt1711h/Makefile
> create mode 100644 drivers/usb/typec/rt1711h/rt1711h.c
> create mode 100644 drivers/usb/typec/rt1711h/rt1711h.h
>
> diff --git a/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt b/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
> new file mode 100644
> index 0000000..c28299c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
> @@ -0,0 +1,38 @@
> +Richtek RT1711H Type-C Port Controller.
> +
> +Required properties:
> +- compatible : Must be "richtek,typec_rt1711h";
> +- reg : Must be 0x4e, it's default slave address of RT1711H.
> +- rt,intr_gpio : IRQ GPIO pin that's connected to RT1711H interrupt.
> +
> +Optional node:
> +- rt,name : Name used for registering IRQ and creating kthread.
> + If this property is not specified, "default" will be applied.
> +- rt,def_role : Default port role (TYPEC_SINK(0) or TYPEC_SOURCE(1)).
> + Set to TYPEC_NO_PREFERRED_ROLE(-1) if no default role.
> + If this property is not specified, TYPEC_SINK will be applied.
> +- rt,port_type : Port type (TYPEC_PORT_DFP(0), TYPEC_PORT_UFP(1),
> + or TYPEC_PORT_DRP(2)). If this property is not specified,
> + TYPEC_PORT_DRP will be applied.
> +- rt,max_snk_mv : Maximum acceptable sink voltage in mV.
> + If this property is not specified, 5000mV will be applied.
> +- rt,max_snk_ma : Maximum sink current in mA.
> + If this property is not specified, 3000mA will be applied.
> +- rt,max_snk_mw : Maximum required sink power in mW.
> + If this property is not specified, 15000mW will be applied.
> +- rt,operating_snk_mw : Required operating sink power in mW.
> + If this property is not specified,
> + 2500mW will be applied.
> +- rt,try_role_hw : True if try.{Src,Snk} is implemented in hardware.
> + If this property is not specified, False will be applied.
> +
> +Example:
> +rt1711h@4e {
> + status = "ok";
> + compatible = "richtek,typec_rt1711h";
> + reg = <0x4e>;
> + rt,intr_gpio = <&gpio26 0 0x0>;
> + rt,name = "rt1711h";
> + rt,port_type = <2>; /* 0: DFP, 1: UFP, 2: DRP */
> + rt,def_role = <0>; /* 0: SNK, 1: SRC, -1: TYPEC_NO_PREFERRED_ROLE */
> +};

dts stuff needs to always be in a separate file so the DT maintainers
can review/ack it. Split this patch up into smaller pieces please.

> diff --git a/arch/arm64/boot/dts/hisilicon/rt1711h.dtsi b/arch/arm64/boot/dts/hisilicon/rt1711h.dtsi
> new file mode 100644
> index 0000000..4196cc0
> --- /dev/null
> +++ b/arch/arm64/boot/dts/hisilicon/rt1711h.dtsi
> @@ -0,0 +1,11 @@
> +&i2c7 {
> + rt1711h@4e {
> + status = "ok";
> + compatible = "richtek,typec_rt1711h";
> + reg = <0x4e>;
> + rt,intr_gpio = <&gpio26 0 0x0>;
> + rt,name = "rt1711h";
> + rt,port_type = <2>; /* 0: DFP, 1: UFP, 2: DRP */
> + rt,def_role = <0>; /* 0: SNK, 1: SRC */
> + };
> +};
> diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
> index bcb2744..7bede0b 100644
> --- a/drivers/usb/typec/Kconfig
> +++ b/drivers/usb/typec/Kconfig
> @@ -56,6 +56,8 @@ if TYPEC_TCPM
>
> source "drivers/usb/typec/fusb302/Kconfig"
>
> +source "drivers/usb/typec/rt1711h/Kconfig"
> +
> config TYPEC_WCOVE
> tristate "Intel WhiskeyCove PMIC USB Type-C PHY driver"
> depends on ACPI
> diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
> index bb3138a..e3aaf3c 100644
> --- a/drivers/usb/typec/Makefile
> +++ b/drivers/usb/typec/Makefile
> @@ -2,6 +2,7 @@
> obj-$(CONFIG_TYPEC) += typec.o
> obj-$(CONFIG_TYPEC_TCPM) += tcpm.o
> obj-y += fusb302/
> +obj-$(CONFIG_TYPEC_RT1711H) += rt1711h/

Why do you need a whole directory for one file?

> obj-$(CONFIG_TYPEC_WCOVE) += typec_wcove.o
> obj-$(CONFIG_TYPEC_UCSI) += ucsi/
> obj-$(CONFIG_TYPEC_TPS6598X) += tps6598x.o
> diff --git a/drivers/usb/typec/rt1711h/Kconfig b/drivers/usb/typec/rt1711h/Kconfig
> new file mode 100644
> index 0000000..2fbfff5
> --- /dev/null
> +++ b/drivers/usb/typec/rt1711h/Kconfig
> @@ -0,0 +1,7 @@
> +config TYPEC_RT1711H
> + tristate "Richtek RT1711H Type-C chip driver"
> + depends on I2C && POWER_SUPPLY
> + help
> + The Richtek RT1711H Type-C chip driver that works with
> + Type-C Port Controller Manager to provide USB PD and USB
> + Type-C functionalities.
> diff --git a/drivers/usb/typec/rt1711h/Makefile b/drivers/usb/typec/rt1711h/Makefile
> new file mode 100644
> index 0000000..5fab8ae
> --- /dev/null
> +++ b/drivers/usb/typec/rt1711h/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0
> +obj-$(CONFIG_TYPEC_RT1711H) += rt1711h.o
> diff --git a/drivers/usb/typec/rt1711h/rt1711h.c b/drivers/usb/typec/rt1711h/rt1711h.c
> new file mode 100644
> index 0000000..1aef3e8
> --- /dev/null
> +++ b/drivers/usb/typec/rt1711h/rt1711h.c
> @@ -0,0 +1,2241 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2017 Richtek Technologh Corp.
> + *
> + * Richtek RT1711H Type-C Chip Driver
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/version.h>
> +#include <linux/err.h>
> +#include <linux/debugfs.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/i2c.h>
> +#include <linux/usb/typec.h>
> +#include <linux/usb/tcpm.h>
> +#include <linux/usb/pd.h>
> +#include <linux/of_gpio.h>
> +#include <linux/of.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/power_supply.h>
> +#include <linux/extcon.h>
> +#include <linux/workqueue.h>
> +#include <linux/kthread.h>
> +#include <linux/cpu.h>
> +#include <linux/alarmtimer.h>
> +#include <linux/sched/clock.h>
> +#include <uapi/linux/sched/types.h>

This last #include should not be needed. If it does, you are doing
something really wrong...

> +
> +#include "rt1711h.h"

Why a .h file for a single .c file?

> +
> +#define RT1711H_DRV_VERSION "1.0.3"

When code is in the kernel tree, versions mean nothing, you will note
that no other USB driver has them, right? Please remove.

> +
> +#define LOG_BUFFER_ENTRIES 1024
> +#define LOG_BUFFER_ENTRY_SIZE 128 /* 128 char per line */
> +
> +enum {
> + RT1711H_DBG_LOG = 0,
> + RT1711H_DBG_REGS,
> + RT1711H_DBG_REG_ADDR,
> + RT1711H_DBG_DATA,
> + RT1711H_DBG_MAX,
> +};
> +
> +struct rt1711h_dbg_info {
> + struct rt1711h_chip *chip;
> + int id;
> +};
> +
> +
> +struct rt1711h_chip {
> + struct i2c_client *i2c;
> + struct device *dev;
> + uint16_t did;

kernel types are u16, u32, u8, and the like, not uint16_t, those are for
userspace code only.

Yeah, other drivers do it, but you shouldn't :)

> + int irq_gpio;
> + int irq;
> + char *name;
> + struct tcpc_dev tcpc_dev;
> + struct tcpc_config tcpc_cfg;
> + struct tcpm_port *tcpm_port;
> + struct regulator *vbus;
> + struct extcon_dev *extcon;
> +
> + /* IRQ */
> + struct kthread_worker irq_worker;
> + struct kthread_work irq_work;
> + struct task_struct *irq_worker_task;

3 things for an irq handler? That feels wrong.

> + atomic_t poll_count;

Like I said before, why is this an atomic?

> + struct delayed_work poll_work;
> +
> + /* LPM */
> + struct delayed_work wakeup_work;
> + struct alarm wakeup_timer;
> + struct mutex wakeup_lock;
> + enum typec_cc_pull lpm_pull;
> + bool wakeup_once;
> + bool low_rp_duty_cntdown;
> + bool cable_only;
> + bool lpm;
> +
> + /* I2C */
> + atomic_t i2c_busy;
> + atomic_t pm_suspend;

Why are these atomic? You know that doesn't mean they do not need
locking, right?

> +
> + /* psy + psy status */
> + struct power_supply *psy;
> + u32 current_limit;
> + u32 supply_voltage;
> +
> + /* lock for sharing chip states */
> + struct mutex lock;

How many locks do you have in this structure? You should only need 1.

> +
> + /* port status */
> + bool vconn_on;
> + bool vbus_on;
> + bool charge_on;
> + bool vbus_present;
> + enum typec_cc_polarity polarity;
> + enum typec_cc_status cc1;
> + enum typec_cc_status cc2;
> + enum typec_role pwr_role;
> + bool drp_toggling;
> +
> +#ifdef CONFIG_DEBUG_FS
> + struct dentry *dbgdir;
> + struct rt1711h_dbg_info dbg_info[RT1711H_DBG_MAX];
> + struct dentry *dbg_files[RT1711H_DBG_MAX];
> + int dbg_regidx;
> + struct mutex dbgops_lock;
> + /* lock for log buffer access */
> + struct mutex logbuffer_lock;
> + int logbuffer_head;
> + int logbuffer_tail;
> + u8 *logbuffer[LOG_BUFFER_ENTRIES];
> +#endif /* CONFIG_DEBUG_FS */

That's a lot of stuff jsut for debugfs. Why do you care about #define
at all? The code should not.

And another 2 locks? Ick, no.

> +};
> +
> +/*
> + * Logging & debugging
> + */
> +
> +#ifdef CONFIG_DEBUG_FS
> +
> +static int rt1711h_reg_block_read(struct rt1711h_chip *chip, uint8_t reg,
> + int len, uint8_t *data);
> +static int rt1711h_reg_block_write(struct rt1711h_chip *chip, uint8_t reg,
> + int len, const uint8_t *data);
> +
> +struct reg_desc {
> + uint8_t addr;
> + uint8_t size;
> +};
> +#define DECL_REG(_addr, _size) {.addr = _addr, .size = _size}
> +
> +static struct reg_desc rt1711h_reg_desc[] = {
> + DECL_REG(RT1711H_REG_VID, 2),
> + DECL_REG(RT1711H_REG_PID, 2),
> + DECL_REG(RT1711H_REG_DID, 2),
> + DECL_REG(RT1711H_REG_TYPEC_REV, 2),
> + DECL_REG(RT1711H_REG_PD_REV, 2),
> + DECL_REG(RT1711H_REG_PDIF_REV, 2),
> + DECL_REG(RT1711H_REG_ALERT, 2),
> + DECL_REG(RT1711H_REG_ALERT_MASK, 2),
> + DECL_REG(RT1711H_REG_POWER_STATUS_MASK, 1),
> + DECL_REG(RT1711H_REG_FAULT_STATUS_MASK, 1),
> + DECL_REG(RT1711H_REG_TCPC_CTRL, 1),
> + DECL_REG(RT1711H_REG_ROLE_CTRL, 1),
> + DECL_REG(RT1711H_REG_FAULT_CTRL, 1),
> + DECL_REG(RT1711H_REG_POWER_CTRL, 1),
> + DECL_REG(RT1711H_REG_CC_STATUS, 1),
> + DECL_REG(RT1711H_REG_POWER_STATUS, 1),
> + DECL_REG(RT1711H_REG_FAULT_STATUS, 1),
> + DECL_REG(RT1711H_REG_COMMAND, 1),
> + DECL_REG(RT1711H_REG_MSG_HDR_INFO, 1),
> + DECL_REG(RT1711H_REG_RX_DETECT, 1),
> + DECL_REG(RT1711H_REG_RX_BYTE_CNT, 1),
> + DECL_REG(RT1711H_REG_RX_BUF_FRAME_TYPE, 1),
> + DECL_REG(RT1711H_REG_RX_HDR, 2),
> + DECL_REG(RT1711H_REG_RX_DATA, 1),
> + DECL_REG(RT1711H_REG_TRANSMIT, 1),
> + DECL_REG(RT1711H_REG_TX_BYTE_CNT, 1),
> + DECL_REG(RT1711H_REG_TX_HDR, 2),
> + DECL_REG(RT1711H_REG_TX_DATA, 1),
> + DECL_REG(RT1711H_REG_CLK_CTRL2, 1),
> + DECL_REG(RT1711H_REG_CLK_CTRL3, 1),
> + DECL_REG(RT1711H_REG_BMC_CTRL, 1),
> + DECL_REG(RT1711H_REG_BMCIO_RXDZSEL, 1),
> + DECL_REG(RT1711H_REG_VCONN_CLIMITEN, 1),
> + DECL_REG(RT1711H_REG_RT_STATUS, 1),
> + DECL_REG(RT1711H_REG_RT_INT, 1),
> + DECL_REG(RT1711H_REG_RT_MASK, 1),
> + DECL_REG(RT1711H_REG_IDLE_CTRL, 1),
> + DECL_REG(RT1711H_REG_INTRST_CTRL, 1),
> + DECL_REG(RT1711H_REG_WATCHDOG_CTRL, 1),
> + DECL_REG(RT1711H_REG_I2CRST_CTRL, 1),
> + DECL_REG(RT1711H_REG_SWRESET, 1),
> + DECL_REG(RT1711H_REG_TTCPC_FILTER, 1),
> + DECL_REG(RT1711H_REG_DRP_TOGGLE_CYCLE, 1),
> + DECL_REG(RT1711H_REG_DRP_DUTY_CTRL, 1),
> + DECL_REG(RT1711H_REG_BMCIO_RXDZEN, 1),
> +};
> +
> +static const char *rt1711h_dbg_filename[RT1711H_DBG_MAX] = {
> + "log", "regs", "reg_addr", "data",
> +};
> +
> +static bool rt1711h_log_full(struct rt1711h_chip *chip)
> +{
> + return chip->logbuffer_tail ==
> + (chip->logbuffer_head + 1) % LOG_BUFFER_ENTRIES;
> +}
> +
> +static void _rt1711h_log(struct rt1711h_chip *chip, const char *fmt,
> + va_list args)
> +{
> + char tmpbuffer[LOG_BUFFER_ENTRY_SIZE];
> + u64 ts_nsec = local_clock();
> + unsigned long rem_nsec;
> +
> + if (!chip->logbuffer[chip->logbuffer_head]) {
> + chip->logbuffer[chip->logbuffer_head] =
> + devm_kzalloc(chip->dev, LOG_BUFFER_ENTRY_SIZE, GFP_KERNEL);
> + if (!chip->logbuffer[chip->logbuffer_head])
> + return;
> + }
> +
> + vsnprintf(tmpbuffer, sizeof(tmpbuffer), fmt, args);
> +
> + mutex_lock(&chip->logbuffer_lock);
> +
> + if (rt1711h_log_full(chip)) {
> + chip->logbuffer_head = max(chip->logbuffer_head - 1, 0);
> + strlcpy(tmpbuffer, "overflow", sizeof(tmpbuffer));
> + }
> +
> + if (chip->logbuffer_head < 0 ||
> + chip->logbuffer_head >= LOG_BUFFER_ENTRIES) {
> + dev_warn(chip->dev, "%s bad log buffer index %d\n", __func__,
> + chip->logbuffer_head);
> + goto abort;
> + }
> +
> + if (!chip->logbuffer[chip->logbuffer_head]) {
> + dev_warn(chip->dev, "%s log buffer index %d is NULL\n",
> + __func__, chip->logbuffer_head);
> + goto abort;
> + }
> +
> + rem_nsec = do_div(ts_nsec, 1000000000);
> + scnprintf(chip->logbuffer[chip->logbuffer_head], LOG_BUFFER_ENTRY_SIZE,
> + "[%5lu.%06lu] %s", (unsigned long)ts_nsec, rem_nsec / 1000,
> + tmpbuffer);
> + chip->logbuffer_head = (chip->logbuffer_head + 1) % LOG_BUFFER_ENTRIES;
> +
> +abort:
> + mutex_unlock(&chip->logbuffer_lock);
> +}
> +
> +static void rt1711h_log(struct rt1711h_chip *chip,
> + const char *fmt, ...)
> +{
> + va_list args;
> +
> + va_start(args, fmt);
> + _rt1711h_log(chip, fmt, args);
> + va_end(args);
> +}
> +
> +static int rt1711h_log_show(struct rt1711h_chip *chip, struct seq_file *s)
> +{
> + int tail;
> +
> + mutex_lock(&chip->logbuffer_lock);
> + tail = chip->logbuffer_tail;
> + while (tail != chip->logbuffer_head) {
> + seq_printf(s, "%s", chip->logbuffer[tail]);
> + tail = (tail + 1) % LOG_BUFFER_ENTRIES;
> + }
> + if (!seq_has_overflowed(s))
> + chip->logbuffer_tail = tail;
> + mutex_unlock(&chip->logbuffer_lock);
> +
> + return 0;
> +}
> +
> +static int rt1711h_regs_show(struct rt1711h_chip *chip, struct seq_file *s)
> +{
> + int ret = 0;
> + int i = 0, j = 0;
> + struct reg_desc *desc = NULL;
> + uint8_t regval[2] = {0};
> +
> + for (i = 0; i < ARRAY_SIZE(rt1711h_reg_desc); i++) {
> + desc = &rt1711h_reg_desc[i];
> + ret = rt1711h_reg_block_read(chip, desc->addr, desc->size,
> + regval);
> + if (ret < 0) {
> + dev_err(chip->dev, "%s read reg0x%02X fail\n",
> + __func__, desc->addr);
> + continue;
> + }
> +
> + seq_printf(s, "reg0x%02x:0x", desc->addr);
> + for (j = 0; j < desc->size; j++)
> + seq_printf(s, "%02x,", regval[j]);
> + seq_puts(s, "\n");
> + }
> +
> + return 0;
> +}
> +
> +static inline int rt1711h_reg_addr_show(struct rt1711h_chip *chip,
> + struct seq_file *s)
> +{
> + struct reg_desc *desc = &rt1711h_reg_desc[chip->dbg_regidx];
> +
> + seq_printf(s, "0x%02x\n", desc->addr);
> + return 0;
> +}
> +
> +static inline int rt1711h_data_show(struct rt1711h_chip *chip,
> + struct seq_file *s)
> +{
> + int ret = 0, i = 0;
> + struct reg_desc *desc = &rt1711h_reg_desc[chip->dbg_regidx];
> + uint8_t regval[2] = {0};
> +
> + ret = rt1711h_reg_block_read(chip, desc->addr, desc->size, regval);
> + if (ret < 0)
> + return ret;
> +
> + seq_printf(s, "reg0x%02x=0x", desc->addr);
> + for (i = 0; i < desc->size; i++)
> + seq_printf(s, "%02x,", regval[i]);
> + seq_puts(s, "\n");
> + return 0;
> +}
> +
> +static int rt1711h_dbg_show(struct seq_file *s, void *v)
> +{
> + int ret = 0;
> + struct rt1711h_dbg_info *info = (struct rt1711h_dbg_info *)s->private;
> + struct rt1711h_chip *chip = info->chip;
> +
> + mutex_lock(&chip->dbgops_lock);
> + switch (info->id) {
> + case RT1711H_DBG_LOG:
> + ret = rt1711h_log_show(chip, s);
> + break;
> + case RT1711H_DBG_REGS:
> + ret = rt1711h_regs_show(chip, s);
> + break;
> + case RT1711H_DBG_REG_ADDR:
> + ret = rt1711h_reg_addr_show(chip, s);
> + break;
> + case RT1711H_DBG_DATA:
> + ret = rt1711h_data_show(chip, s);
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + mutex_unlock(&chip->dbgops_lock);
> + return ret;
> +}
> +
> +static int rt1711h_dbg_open(struct inode *inode, struct file *file)
> +{
> + if (file->f_mode & FMODE_READ)
> + return single_open(file, rt1711h_dbg_show, inode->i_private);
> + file->private_data = inode->i_private;
> + return 0;
> +}
> +
> +static int get_parameters(char *buf, long int *param1, int num_of_par)
> +{
> + char *token;
> + int base, cnt;
> +
> + token = strsep(&buf, " ");
> +
> + for (cnt = 0; cnt < num_of_par; cnt++) {
> + if (token != NULL) {
> + if ((token[1] == 'x') || (token[1] == 'X'))
> + base = 16;
> + else
> + base = 10;
> +
> + if (kstrtoul(token, base, &param1[cnt]) != 0)
> + return -EINVAL;
> +
> + token = strsep(&buf, " ");
> + } else
> + return -EINVAL;
> + }
> + return 0;
> +}

What is this function doing? What is your debugfs files for?

> +
> +static int get_datas(const char *buf, const int length,
> + unsigned char *data_buffer, unsigned char data_length)
> +{
> + int i, ptr;
> + long int value;
> + char token[5];
> +
> + token[0] = '0';
> + token[1] = 'x';
> + token[4] = 0;
> + if (buf[0] != '0' || buf[1] != 'x')
> + return -EINVAL;
> +
> + ptr = 2;
> + for (i = 0; (i < data_length) && (ptr + 2 <= length); i++) {
> + token[2] = buf[ptr++];
> + token[3] = buf[ptr++];
> + ptr++;
> + if (kstrtoul(token, 16, &value) != 0)
> + return -EINVAL;
> + data_buffer[i] = value;
> + }
> + return 0;
> +}
> +
> +static int rt1711h_regaddr2idx(uint8_t reg_addr)
> +{
> + int i = 0;
> + struct reg_desc *desc = NULL;
> +
> + for (i = 0; i < ARRAY_SIZE(rt1711h_reg_desc); i++) {
> + desc = &rt1711h_reg_desc[i];
> + if (desc->addr == reg_addr)
> + return i;
> + }
> + return -EINVAL;
> +}
> +
> +static ssize_t rt1711h_dbg_write(struct file *file, const char __user *ubuf,
> + size_t count, loff_t *ppos)
> +{
> + int ret = 0;
> + struct rt1711h_dbg_info *info =
> + (struct rt1711h_dbg_info *)file->private_data;
> + struct rt1711h_chip *chip = info->chip;
> + struct reg_desc *desc = NULL;
> + char lbuf[128];
> + long int param[5];
> + unsigned char reg_data[2] = {0};
> +
> + if (count > sizeof(lbuf) - 1)
> + return -EFAULT;
> +
> + ret = copy_from_user(lbuf, ubuf, count);
> + if (ret)
> + return -EFAULT;
> + lbuf[count] = '\0';
> +
> + mutex_lock(&chip->dbgops_lock);
> + switch (info->id) {
> + case RT1711H_DBG_REG_ADDR:
> + ret = get_parameters(lbuf, param, 1);
> + if (ret < 0) {
> + dev_err(chip->dev, "%s get param fail\n", __func__);
> + ret = -EINVAL;
> + goto out;
> + }
> + ret = rt1711h_regaddr2idx(param[0]);
> + if (ret < 0) {
> + dev_err(chip->dev, "%s addr2idx fail\n", __func__);
> + ret = -EINVAL;
> + goto out;
> + }
> + chip->dbg_regidx = ret;
> + break;
> + case RT1711H_DBG_DATA:
> + desc = &rt1711h_reg_desc[chip->dbg_regidx];
> + if ((desc->size - 1) * 3 + 5 != count) {
> + dev_err(chip->dev, "%s incorrect input length\n",
> + __func__);
> + ret = -EINVAL;
> + goto out;
> + }
> + ret = get_datas((char *)ubuf, count, reg_data, desc->size);
> + if (ret < 0) {
> + dev_err(chip->dev, "%s get data fail\n", __func__);
> + ret = -EINVAL;
> + goto out;
> + }
> + ret = rt1711h_reg_block_write(chip, desc->addr, desc->size,
> + reg_data);
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> +out:
> + mutex_unlock(&chip->dbgops_lock);
> + return ret < 0 ? ret : count;
> +}
> +
> +static int rt1711h_dbg_release(struct inode *inode, struct file *file)
> +{
> + if (file->f_mode & FMODE_READ)
> + return single_release(inode, file);
> + return 0;
> +}
> +
> +static const struct file_operations rt1711h_dbg_ops = {
> + .open = rt1711h_dbg_open,
> + .llseek = seq_lseek,
> + .read = seq_read,
> + .write = rt1711h_dbg_write,
> + .release = rt1711h_dbg_release,
> +};
> +
> +
> +static int rt1711h_debugfs_init(struct rt1711h_chip *chip)
> +{
> + int ret = 0, i = 0;
> + struct rt1711h_dbg_info *info = NULL;
> + int len = 0;
> + char *dirname = NULL;
> +
> + mutex_init(&chip->logbuffer_lock);
> + mutex_init(&chip->dbgops_lock);
> + len = strlen(dev_name(chip->dev));
> + dirname = devm_kzalloc(chip->dev, len + 9, GFP_KERNEL);
> + if (!dirname)
> + return -ENOMEM;
> + snprintf(dirname, len + 9, "rt1711h-%s", dev_name(chip->dev));
> + if (!chip->dbgdir) {
> + chip->dbgdir = debugfs_create_dir(dirname, NULL);
> + if (!chip->dbgdir)
> + return -ENOMEM;

No need to ever check the return value of debugfs_ calls, you should not
care and can always use the value to any future debugfs calls, if you
really need it.

> + }
> +
> + for (i = 0; i < RT1711H_DBG_MAX; i++) {
> + info = &chip->dbg_info[i];

static array of debug info? That feels odd.

> + info->chip = chip;
> + info->id = i;
> + chip->dbg_files[i] = debugfs_create_file(
> + rt1711h_dbg_filename[i], S_IFREG | 0444,
> + chip->dbgdir, info, &rt1711h_dbg_ops);
> + if (!chip->dbg_files[i]) {
> + ret = -EINVAL;

Like here, you don't need this, and you don't need to care about the
return value.

> + goto err;
> + }
> + }
> +
> + return 0;
> +err:
> + debugfs_remove_recursive(chip->dbgdir);
> + return ret;

Why do you care about an error here? Your code should not do anything
different if debugfs stuff does not work or if it does. It's debugging
only.

> +}
> +
> +static void rt1711h_debugfs_exit(struct rt1711h_chip *chip)
> +{
> + debugfs_remove_recursive(chip->dbgdir);

See, you didn't need those file handles :)

thanks,

greg k-h

2018-01-18 14:59:52

[permalink] [raw]

Subject: RE: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

Dear Gerg,

Many thanks to your comment.
I've checked all of them and here are some questions need your help.

> +Example:
> +rt1711h@4e {
> +status = "ok";
> +compatible = "richtek,typec_rt1711h";
> +reg = <0x4e>;
> +rt,intr_gpio = <&gpio26 0 0x0>;
> +rt,name = "rt1711h";
> +rt,port_type = <2>; /* 0: DFP, 1: UFP, 2: DRP */
> +rt,def_role = <0>; /* 0: SNK, 1: SRC, -1: TYPEC_NO_PREFERRED_ROLE */
> +};

dts stuff needs to always be in a separate file so the DT maintainers can review/ack it. Split this patch up into smaller pieces please.

Ok, I'll split it into two patches, one for source code and once for dts related files.
========================================================================

> diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
> index bb3138a..e3aaf3c 100644
> --- a/drivers/usb/typec/Makefile
> +++ b/drivers/usb/typec/Makefile
> @@ -2,6 +2,7 @@
> obj-$(CONFIG_TYPEC)+= typec.o
> obj-$(CONFIG_TYPEC_TCPM)+= tcpm.o
> obj-y+= fusb302/
> +obj-$(CONFIG_TYPEC_RT1711H)+= rt1711h/

Why do you need a whole directory for one file?

Is the suggestion to move rt1711h.c to the same directory level as tcpm? i.e. drivers/usb/typec/rt1711h.c
========================================================================

> +<uapi/linux/sched/types.h>

This last #include should not be needed. If it does, you are doing something really wrong...

Ok, this #include will be removed
========================================================================

> +
> +#include "rt1711h.h"

Why a .h file for a single .c file?

Is the suggestion to move all content in rt1711h.h into rt1711h.c?
========================================================================

> +
> +#define RT1711H_DRV_VERSION"1.0.3"

When code is in the kernel tree, versions mean nothing, you will note that no other USB driver has them, right? Please remove.

Ok, this will be removed.
========================================================================

kernel types are u16, u32, u8, and the like, not uint16_t, those are for userspace code only.
Yeah, other drivers do it, but you shouldn't :)

Ok, I'll use u16, u32 and u8 instead of uint16_t, uint32_t and uint8_t
========================================================================

> +/* IRQ */
> +struct kthread_worker irq_worker;
> +struct kthread_work irq_work;
> +struct task_struct *irq_worker_task;

3 things for an irq handler? That feels wrong.

> +atomic_t poll_count;

Like I said before, why is this an atomic?

I'll use threaded_irq instead, the above things will be removed.
========================================================================

> +/* I2C */
> +atomic_t i2c_busy;
> +atomic_t pm_suspend;

Why are these atomic? You know that doesn't mean they do not need locking, right?

For my understanding, a single operation on atomic_t doesn't need lock, like a single atomic_set.
But two consecutive operations doesn't guarantee anything. Like atomic_set followed by an atomic_read.
This part is referenced from fusb302 used to make sure I2C is idle before system suspends.
It only needs to guarantee a single read/write on these variable is atomic operation, so atomic is used.
========================================================================

> +};
> +
> +/*
> + * Logging & debugging
> + */
> +
> +#ifdef CONFIG_DEBUG_FS
> +
> +static int rt1711h_reg_block_read(struct rt1711h_chip *chip, uint8_t reg,
> +int len, uint8_t *data);
> +static int rt1711h_reg_block_write(struct rt1711h_chip *chip, uint8_t reg,
> +int len, const uint8_t *data);
> +
> +struct reg_desc {
> +uint8_t addr;
> +uint8_t size;
> +};
> +#define DECL_REG(_addr, _size) {.addr = _addr, .size = _size}
> +
> +static struct reg_desc rt1711h_reg_desc[] = {
> +DECL_REG(RT1711H_REG_VID, 2),
> +DECL_REG(RT1711H_REG_PID, 2),
> +DECL_REG(RT1711H_REG_DID, 2),
> +DECL_REG(RT1711H_REG_TYPEC_REV, 2),
> +DECL_REG(RT1711H_REG_PD_REV, 2),
> +DECL_REG(RT1711H_REG_PDIF_REV, 2),
> +DECL_REG(RT1711H_REG_ALERT, 2),
> +DECL_REG(RT1711H_REG_ALERT_MASK, 2),
> +DECL_REG(RT1711H_REG_POWER_STATUS_MASK, 1),
> +DECL_REG(RT1711H_REG_FAULT_STATUS_MASK, 1),
> +DECL_REG(RT1711H_REG_TCPC_CTRL, 1),
> +DECL_REG(RT1711H_REG_ROLE_CTRL, 1),
> +DECL_REG(RT1711H_REG_FAULT_CTRL, 1),
> +DECL_REG(RT1711H_REG_POWER_CTRL, 1),
> +DECL_REG(RT1711H_REG_CC_STATUS, 1),
> +DECL_REG(RT1711H_REG_POWER_STATUS, 1),
> +DECL_REG(RT1711H_REG_FAULT_STATUS, 1),
> +DECL_REG(RT1711H_REG_COMMAND, 1),
> +DECL_REG(RT1711H_REG_MSG_HDR_INFO, 1),
> +DECL_REG(RT1711H_REG_RX_DETECT, 1),
> +DECL_REG(RT1711H_REG_RX_BYTE_CNT, 1),
> +DECL_REG(RT1711H_REG_RX_BUF_FRAME_TYPE, 1),
> +DECL_REG(RT1711H_REG_RX_HDR, 2),
> +DECL_REG(RT1711H_REG_RX_DATA, 1),
> +DECL_REG(RT1711H_REG_TRANSMIT, 1),
> +DECL_REG(RT1711H_REG_TX_BYTE_CNT, 1),
> +DECL_REG(RT1711H_REG_TX_HDR, 2),
> +DECL_REG(RT1711H_REG_TX_DATA, 1),
> +DECL_REG(RT1711H_REG_CLK_CTRL2, 1),
> +DECL_REG(RT1711H_REG_CLK_CTRL3, 1),
> +DECL_REG(RT1711H_REG_BMC_CTRL, 1),
> +DECL_REG(RT1711H_REG_BMCIO_RXDZSEL, 1),
> +DECL_REG(RT1711H_REG_VCONN_CLIMITEN, 1),
> +DECL_REG(RT1711H_REG_RT_STATUS, 1),
> +DECL_REG(RT1711H_REG_RT_INT, 1),
> +DECL_REG(RT1711H_REG_RT_MASK, 1),
> +DECL_REG(RT1711H_REG_IDLE_CTRL, 1),
> +DECL_REG(RT1711H_REG_INTRST_CTRL, 1),
> +DECL_REG(RT1711H_REG_WATCHDOG_CTRL, 1),
> +DECL_REG(RT1711H_REG_I2CRST_CTRL, 1),
> +DECL_REG(RT1711H_REG_SWRESET, 1),
> +DECL_REG(RT1711H_REG_TTCPC_FILTER, 1),
> +DECL_REG(RT1711H_REG_DRP_TOGGLE_CYCLE, 1),
> +DECL_REG(RT1711H_REG_DRP_DUTY_CTRL, 1),
> +DECL_REG(RT1711H_REG_BMCIO_RXDZEN, 1), };
> +
> +static const char *rt1711h_dbg_filename[RT1711H_DBG_MAX] = {
> +"log", "regs", "reg_addr", "data",
> +};
> +
> +static bool rt1711h_log_full(struct rt1711h_chip *chip) {
> +return chip->logbuffer_tail ==
> +(chip->logbuffer_head + 1) % LOG_BUFFER_ENTRIES; }
> +
> +static void _rt1711h_log(struct rt1711h_chip *chip, const char *fmt,
> + va_list args)
> +{
> +char tmpbuffer[LOG_BUFFER_ENTRY_SIZE];
> +u64 ts_nsec = local_clock();
> +unsigned long rem_nsec;
> +
> +if (!chip->logbuffer[chip->logbuffer_head]) {
> +chip->logbuffer[chip->logbuffer_head] =
> +devm_kzalloc(chip->dev, LOG_BUFFER_ENTRY_SIZE, GFP_KERNEL);
> +if (!chip->logbuffer[chip->logbuffer_head])
> +return;
> +}
> +
> +vsnprintf(tmpbuffer, sizeof(tmpbuffer), fmt, args);
> +
> +mutex_lock(&chip->logbuffer_lock);
> +
> +if (rt1711h_log_full(chip)) {
> +chip->logbuffer_head = max(chip->logbuffer_head - 1, 0);
> +strlcpy(tmpbuffer, "overflow", sizeof(tmpbuffer));
> +}
> +
> +if (chip->logbuffer_head < 0 ||
> +chip->logbuffer_head >= LOG_BUFFER_ENTRIES) {
> +dev_warn(chip->dev, "%s bad log buffer index %d\n", __func__,
> +chip->logbuffer_head);
> +goto abort;
> +}
> +
> +if (!chip->logbuffer[chip->logbuffer_head]) {
> +dev_warn(chip->dev, "%s log buffer index %d is NULL\n",
> +__func__, chip->logbuffer_head);
> +goto abort;
> +}
> +
> +rem_nsec = do_div(ts_nsec, 1000000000);
> +scnprintf(chip->logbuffer[chip->logbuffer_head], LOG_BUFFER_ENTRY_SIZE,
> +"[%5lu.%06lu] %s", (unsigned long)ts_nsec, rem_nsec / 1000,
> + tmpbuffer);
> +chip->logbuffer_head = (chip->logbuffer_head + 1) %
> +LOG_BUFFER_ENTRIES;
> +
> +abort:
> +mutex_unlock(&chip->logbuffer_lock);
> +}
> +
> +static void rt1711h_log(struct rt1711h_chip *chip,
> +const char *fmt, ...)
> +{
> +va_list args;
> +
> +va_start(args, fmt);
> +_rt1711h_log(chip, fmt, args);
> +va_end(args);
> +}
> +
> +static int rt1711h_log_show(struct rt1711h_chip *chip, struct
> +seq_file *s) {
> +int tail;
> +
> +mutex_lock(&chip->logbuffer_lock);
> +tail = chip->logbuffer_tail;
> +while (tail != chip->logbuffer_head) {
> +seq_printf(s, "%s", chip->logbuffer[tail]);
> +tail = (tail + 1) % LOG_BUFFER_ENTRIES;
> +}
> +if (!seq_has_overflowed(s))
> +chip->logbuffer_tail = tail;
> +mutex_unlock(&chip->logbuffer_lock);
> +
> +return 0;
> +}
> +
> +static int rt1711h_regs_show(struct rt1711h_chip *chip, struct
> +seq_file *s) {
> +int ret = 0;
> +int i = 0, j = 0;
> +struct reg_desc *desc = NULL;
> +uint8_t regval[2] = {0};
> +
> +for (i = 0; i < ARRAY_SIZE(rt1711h_reg_desc); i++) {
> +desc = &rt1711h_reg_desc[i];
> +ret = rt1711h_reg_block_read(chip, desc->addr, desc->size,
> +regval);
> +if (ret < 0) {
> +dev_err(chip->dev, "%s read reg0x%02X fail\n",
> +__func__, desc->addr);
> +continue;
> +}
> +
> +seq_printf(s, "reg0x%02x:0x", desc->addr);
> +for (j = 0; j < desc->size; j++)
> +seq_printf(s, "%02x,", regval[j]);
> +seq_puts(s, "\n");
> +}
> +
> +return 0;
> +}
> +
> +static inline int rt1711h_reg_addr_show(struct rt1711h_chip *chip,
> +struct seq_file *s)
> +{
> +struct reg_desc *desc = &rt1711h_reg_desc[chip->dbg_regidx];
> +
> +seq_printf(s, "0x%02x\n", desc->addr);
> +return 0;
> +}
> +
> +static inline int rt1711h_data_show(struct rt1711h_chip *chip,
> +struct seq_file *s)
> +{
> +int ret = 0, i = 0;
> +struct reg_desc *desc = &rt1711h_reg_desc[chip->dbg_regidx];
> +uint8_t regval[2] = {0};
> +
> +ret = rt1711h_reg_block_read(chip, desc->addr, desc->size, regval);
> +if (ret < 0)
> +return ret;
> +
> +seq_printf(s, "reg0x%02x=0x", desc->addr);
> +for (i = 0; i < desc->size; i++)
> +seq_printf(s, "%02x,", regval[i]);
> +seq_puts(s, "\n");
> +return 0;
> +}
> +
> +static int rt1711h_dbg_show(struct seq_file *s, void *v) {
> +int ret = 0;
> +struct rt1711h_dbg_info *info = (struct rt1711h_dbg_info *)s->private;
> +struct rt1711h_chip *chip = info->chip;
> +
> +mutex_lock(&chip->dbgops_lock);
> +switch (info->id) {
> +case RT1711H_DBG_LOG:
> +ret = rt1711h_log_show(chip, s);
> +break;
> +case RT1711H_DBG_REGS:
> +ret = rt1711h_regs_show(chip, s);
> +break;
> +case RT1711H_DBG_REG_ADDR:
> +ret = rt1711h_reg_addr_show(chip, s);
> +break;
> +case RT1711H_DBG_DATA:
> +ret = rt1711h_data_show(chip, s);
> +break;
> +default:
> +ret = -EINVAL;
> +break;
> +}
> +
> +mutex_unlock(&chip->dbgops_lock);
> +return ret;
> +}
> +
> +static int rt1711h_dbg_open(struct inode *inode, struct file *file) {
> +if (file->f_mode & FMODE_READ)
> +return single_open(file, rt1711h_dbg_show, inode->i_private);
> +file->private_data = inode->i_private;
> +return 0;
> +}
> +
> +static int get_parameters(char *buf, long int *param1, int
> +num_of_par) {
> +char *token;
> +int base, cnt;
> +
> +token = strsep(&buf, " ");
> +
> +for (cnt = 0; cnt < num_of_par; cnt++) {
> +if (token != NULL) {
> +if ((token[1] == 'x') || (token[1] == 'X'))
> +base = 16;
> +else
> +base = 10;
> +
> +if (kstrtoul(token, base, &param1[cnt]) != 0)
> +return -EINVAL;
> +
> +token = strsep(&buf, " ");
> +} else
> +return -EINVAL;
> +}
> +return 0;
> +}

What is this function doing? What is your debugfs files for?

There are 4 debug files.
First(log) is for logging which needs a lock for log buffer. The way to log is referenced from fusb302 and tcpm.
Second(regs) is used to dump all register of rt1711h.
Third(reg_addr)&Forth(data) are used to write/read a register specified in reg_addr.
The reason to create these debug files is to make issue support easier.
========================================================================

> +#ifdef CONFIG_DEBUG_FS
> +struct dentry *dbgdir;
> +struct rt1711h_dbg_info dbg_info[RT1711H_DBG_MAX];
> +struct dentry *dbg_files[RT1711H_DBG_MAX];
> +int dbg_regidx;
> +struct mutex dbgops_lock;
> +/* lock for log buffer access */
> +struct mutex logbuffer_lock;
> +int logbuffer_head;
> +int logbuffer_tail;
> +u8 *logbuffer[LOG_BUFFER_ENTRIES];
> +#endif /* CONFIG_DEBUG_FS */

That's a lot of stuff jsut for debugfs. Why do you care about #define at all? The code should not.

Is the suggestion to remove #ifdef CONFIG_DEBUG_FS?

And another 2 locks? Ick, no.

dbgops_lock is used to prevent user from accessing different debug files simultaneously.
Is the suggestion to use the lock of the following one?
> +/* lock for sharing chip states */
> +struct mutex lock;
========================================================================

> +snprintf(dirname, len + 9, "rt1711h-%s", dev_name(chip->dev));
> +if (!chip->dbgdir) {
> +chip->dbgdir = debugfs_create_dir(dirname, NULL);
> +if (!chip->dbgdir)
> +return -ENOMEM;

No need to ever check the return value of debugfs_ calls, you should not care and can always use the value to any future debugfs calls, if you really need it.

If it is NULL without checking and we use it in debugfs_create_file, all the debug files will be created in the root of the debugfs filesystem.
Is this correct?
========================================================================

> +for (i = 0; i < RT1711H_DBG_MAX; i++) {
> +info = &chip->dbg_info[i];

static array of debug info? That feels odd.

Is the suggestion to use pointer of array and dynamically allocated it?
========================================================================

Like here, you don't need this, and you don't need to care about the return value.

> +goto err;
> +}
> +}
> +
> +return 0;
> +err:
> +debugfs_remove_recursive(chip->dbgdir);
> +return ret;

Why do you care about an error here? Your code should not do anything different if debugfs stuff does not work or if it does. It's debugging only.
Ok, this will be removed.
========================================================================

Best Regards,
*****************************
Shu-Fan Lee
Richtek Technology Corporation
TEL: +886-3-5526789 #2359
FAX: +886-3-5526612
*****************************

-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Wednesday, January 17, 2018 9:42 PM
To: ShuFanLee
Cc: [email protected]; cy_huang(???ҭ?); shufan_lee(???Ѧ|); [email protected]; [email protected]
Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

On Wed, Jan 10, 2018 at 02:59:12PM +0800, ShuFanLee wrote:
> From: ShuFanLee <[email protected]>
>
> Richtek RT1711H Type-C chip driver that works with Type-C Port
> Controller Manager to provide USB PD and USB Type-C functionalities.
>
> Signed-off-by: ShuFanLee <[email protected]>

Minor review of your main structure and your debugfs code and other stuff, all of which need work:

> ---
> .../devicetree/bindings/usb/richtek,rt1711h.txt | 38 +
> arch/arm64/boot/dts/hisilicon/rt1711h.dtsi | 11 +
> drivers/usb/typec/Kconfig | 2 +
> drivers/usb/typec/Makefile | 1 +
> drivers/usb/typec/rt1711h/Kconfig | 7 +
> drivers/usb/typec/rt1711h/Makefile | 2 +
> drivers/usb/typec/rt1711h/rt1711h.c | 2241 ++++++++++++++++++++
> drivers/usb/typec/rt1711h/rt1711h.h | 300 +++
> 8 files changed, 2602 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
> create mode 100644 arch/arm64/boot/dts/hisilicon/rt1711h.dtsi
> create mode 100644 drivers/usb/typec/rt1711h/Kconfig create mode
> 100644 drivers/usb/typec/rt1711h/Makefile
> create mode 100644 drivers/usb/typec/rt1711h/rt1711h.c
> create mode 100644 drivers/usb/typec/rt1711h/rt1711h.h
>
> diff --git a/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
> b/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
> new file mode 100644
> index 0000000..c28299c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
> @@ -0,0 +1,38 @@
> +Richtek RT1711H Type-C Port Controller.
> +
> +Required properties:
> +- compatible : Must be "richtek,typec_rt1711h";
> +- reg : Must be 0x4e, it's default slave address of RT1711H.
> +- rt,intr_gpio : IRQ GPIO pin that's connected to RT1711H interrupt.
> +
> +Optional node:
> +- rt,name : Name used for registering IRQ and creating kthread.
> + If this property is not specified, "default" will be applied.
> +- rt,def_role : Default port role (TYPEC_SINK(0) or TYPEC_SOURCE(1)).
> +Set to TYPEC_NO_PREFERRED_ROLE(-1) if no default role.
> +If this property is not specified, TYPEC_SINK will be applied.
> +- rt,port_type : Port type (TYPEC_PORT_DFP(0), TYPEC_PORT_UFP(1),
> + or TYPEC_PORT_DRP(2)). If this property is not specified,
> + TYPEC_PORT_DRP will be applied.
> +- rt,max_snk_mv : Maximum acceptable sink voltage in mV.
> + If this property is not specified, 5000mV will be applied.
> +- rt,max_snk_ma : Maximum sink current in mA.
> + If this property is not specified, 3000mA will be applied.
> +- rt,max_snk_mw : Maximum required sink power in mW.
> + If this property is not specified, 15000mW will be applied.
> +- rt,operating_snk_mw : Required operating sink power in mW.
> +If this property is not specified,
> +2500mW will be applied.
> +- rt,try_role_hw : True if try.{Src,Snk} is implemented in hardware.
> + If this property is not specified, False will be applied.
> +
> +Example:
> +rt1711h@4e {
> +status = "ok";
> +compatible = "richtek,typec_rt1711h";
> +reg = <0x4e>;
> +rt,intr_gpio = <&gpio26 0 0x0>;
> +rt,name = "rt1711h";
> +rt,port_type = <2>; /* 0: DFP, 1: UFP, 2: DRP */
> +rt,def_role = <0>; /* 0: SNK, 1: SRC, -1: TYPEC_NO_PREFERRED_ROLE */
> +};

dts stuff needs to always be in a separate file so the DT maintainers can review/ack it. Split this patch up into smaller pieces please.

> diff --git a/arch/arm64/boot/dts/hisilicon/rt1711h.dtsi
> b/arch/arm64/boot/dts/hisilicon/rt1711h.dtsi
> new file mode 100644
> index 0000000..4196cc0
> --- /dev/null
> +++ b/arch/arm64/boot/dts/hisilicon/rt1711h.dtsi
> @@ -0,0 +1,11 @@
> +&i2c7 {
> +rt1711h@4e {
> +status = "ok";
> +compatible = "richtek,typec_rt1711h";
> +reg = <0x4e>;
> +rt,intr_gpio = <&gpio26 0 0x0>;
> +rt,name = "rt1711h";
> +rt,port_type = <2>; /* 0: DFP, 1: UFP, 2: DRP */
> +rt,def_role = <0>; /* 0: SNK, 1: SRC */
> +};
> +};
> diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
> index bcb2744..7bede0b 100644
> --- a/drivers/usb/typec/Kconfig
> +++ b/drivers/usb/typec/Kconfig
> @@ -56,6 +56,8 @@ if TYPEC_TCPM
>
> source "drivers/usb/typec/fusb302/Kconfig"
>
> +source "drivers/usb/typec/rt1711h/Kconfig"
> +
> config TYPEC_WCOVE
> tristate "Intel WhiskeyCove PMIC USB Type-C PHY driver"
> depends on ACPI
> diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
> index bb3138a..e3aaf3c 100644
> --- a/drivers/usb/typec/Makefile
> +++ b/drivers/usb/typec/Makefile
> @@ -2,6 +2,7 @@
> obj-$(CONFIG_TYPEC)+= typec.o
> obj-$(CONFIG_TYPEC_TCPM)+= tcpm.o
> obj-y+= fusb302/
> +obj-$(CONFIG_TYPEC_RT1711H)+= rt1711h/

Why do you need a whole directory for one file?

> obj-$(CONFIG_TYPEC_WCOVE)+= typec_wcove.o
> obj-$(CONFIG_TYPEC_UCSI)+= ucsi/
> obj-$(CONFIG_TYPEC_TPS6598X)+= tps6598x.o
> diff --git a/drivers/usb/typec/rt1711h/Kconfig
> b/drivers/usb/typec/rt1711h/Kconfig
> new file mode 100644
> index 0000000..2fbfff5
> --- /dev/null
> +++ b/drivers/usb/typec/rt1711h/Kconfig
> @@ -0,0 +1,7 @@
> +config TYPEC_RT1711H
> +tristate "Richtek RT1711H Type-C chip driver"
> +depends on I2C && POWER_SUPPLY
> +help
> + The Richtek RT1711H Type-C chip driver that works with
> + Type-C Port Controller Manager to provide USB PD and USB
> + Type-C functionalities.
> diff --git a/drivers/usb/typec/rt1711h/Makefile
> b/drivers/usb/typec/rt1711h/Makefile
> new file mode 100644
> index 0000000..5fab8ae
> --- /dev/null
> +++ b/drivers/usb/typec/rt1711h/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0
> +obj-$(CONFIG_TYPEC_RT1711H)+= rt1711h.o
> diff --git a/drivers/usb/typec/rt1711h/rt1711h.c
> b/drivers/usb/typec/rt1711h/rt1711h.c
> new file mode 100644
> index 0000000..1aef3e8
> --- /dev/null
> +++ b/drivers/usb/typec/rt1711h/rt1711h.c
> @@ -0,0 +1,2241 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2017 Richtek Technologh Corp.
> + *
> + * Richtek RT1711H Type-C Chip Driver */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/version.h>
> +#include <linux/err.h>
> +#include <linux/debugfs.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/i2c.h>
> +#include <linux/usb/typec.h>
> +#include <linux/usb/tcpm.h>
> +#include <linux/usb/pd.h>
> +#include <linux/of_gpio.h>
> +#include <linux/of.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/regulator/consumer.h> #include <linux/power_supply.h>
> +#include <linux/extcon.h> #include <linux/workqueue.h> #include
> +<linux/kthread.h> #include <linux/cpu.h> #include
> +<linux/alarmtimer.h> #include <linux/sched/clock.h> #include
> +<uapi/linux/sched/types.h>

This last #include should not be needed. If it does, you are doing something really wrong...

> +
> +#include "rt1711h.h"

Why a .h file for a single .c file?

> +
> +#define RT1711H_DRV_VERSION"1.0.3"

When code is in the kernel tree, versions mean nothing, you will note that no other USB driver has them, right? Please remove.

> +
> +#define LOG_BUFFER_ENTRIES1024
> +#define LOG_BUFFER_ENTRY_SIZE128 /* 128 char per line */
> +
> +enum {
> +RT1711H_DBG_LOG = 0,
> +RT1711H_DBG_REGS,
> +RT1711H_DBG_REG_ADDR,
> +RT1711H_DBG_DATA,
> +RT1711H_DBG_MAX,
> +};
> +
> +struct rt1711h_dbg_info {
> +struct rt1711h_chip *chip;
> +int id;
> +};
> +
> +
> +struct rt1711h_chip {
> +struct i2c_client *i2c;
> +struct device *dev;
> +uint16_t did;

kernel types are u16, u32, u8, and the like, not uint16_t, those are for userspace code only.

Yeah, other drivers do it, but you shouldn't :)

> +int irq_gpio;
> +int irq;
> +char *name;
> +struct tcpc_dev tcpc_dev;
> +struct tcpc_config tcpc_cfg;
> +struct tcpm_port *tcpm_port;
> +struct regulator *vbus;
> +struct extcon_dev *extcon;
> +
> +/* IRQ */
> +struct kthread_worker irq_worker;
> +struct kthread_work irq_work;
> +struct task_struct *irq_worker_task;

3 things for an irq handler? That feels wrong.

> +atomic_t poll_count;

Like I said before, why is this an atomic?

> +struct delayed_work poll_work;
> +
> +/* LPM */
> +struct delayed_work wakeup_work;
> +struct alarm wakeup_timer;
> +struct mutex wakeup_lock;
> +enum typec_cc_pull lpm_pull;
> +bool wakeup_once;
> +bool low_rp_duty_cntdown;
> +bool cable_only;
> +bool lpm;
> +
> +/* I2C */
> +atomic_t i2c_busy;
> +atomic_t pm_suspend;

Why are these atomic? You know that doesn't mean they do not need locking, right?

> +
> +/* psy + psy status */
> +struct power_supply *psy;
> +u32 current_limit;
> +u32 supply_voltage;
> +
> +/* lock for sharing chip states */
> +struct mutex lock;

How many locks do you have in this structure? You should only need 1.

> +
> +/* port status */
> +bool vconn_on;
> +bool vbus_on;
> +bool charge_on;
> +bool vbus_present;
> +enum typec_cc_polarity polarity;
> +enum typec_cc_status cc1;
> +enum typec_cc_status cc2;
> +enum typec_role pwr_role;
> +bool drp_toggling;
> +
> +#ifdef CONFIG_DEBUG_FS
> +struct dentry *dbgdir;
> +struct rt1711h_dbg_info dbg_info[RT1711H_DBG_MAX];
> +struct dentry *dbg_files[RT1711H_DBG_MAX];
> +int dbg_regidx;
> +struct mutex dbgops_lock;
> +/* lock for log buffer access */
> +struct mutex logbuffer_lock;
> +int logbuffer_head;
> +int logbuffer_tail;
> +u8 *logbuffer[LOG_BUFFER_ENTRIES];
> +#endif /* CONFIG_DEBUG_FS */

That's a lot of stuff jsut for debugfs. Why do you care about #define at all? The code should not.

And another 2 locks? Ick, no.

> +};
> +
> +/*
> + * Logging & debugging
> + */
> +
> +#ifdef CONFIG_DEBUG_FS
> +
> +static int rt1711h_reg_block_read(struct rt1711h_chip *chip, uint8_t reg,
> +int len, uint8_t *data);
> +static int rt1711h_reg_block_write(struct rt1711h_chip *chip, uint8_t reg,
> +int len, const uint8_t *data);
> +
> +struct reg_desc {
> +uint8_t addr;
> +uint8_t size;
> +};
> +#define DECL_REG(_addr, _size) {.addr = _addr, .size = _size}
> +
> +static struct reg_desc rt1711h_reg_desc[] = {
> +DECL_REG(RT1711H_REG_VID, 2),
> +DECL_REG(RT1711H_REG_PID, 2),
> +DECL_REG(RT1711H_REG_DID, 2),
> +DECL_REG(RT1711H_REG_TYPEC_REV, 2),
> +DECL_REG(RT1711H_REG_PD_REV, 2),
> +DECL_REG(RT1711H_REG_PDIF_REV, 2),
> +DECL_REG(RT1711H_REG_ALERT, 2),
> +DECL_REG(RT1711H_REG_ALERT_MASK, 2),
> +DECL_REG(RT1711H_REG_POWER_STATUS_MASK, 1),
> +DECL_REG(RT1711H_REG_FAULT_STATUS_MASK, 1),
> +DECL_REG(RT1711H_REG_TCPC_CTRL, 1),
> +DECL_REG(RT1711H_REG_ROLE_CTRL, 1),
> +DECL_REG(RT1711H_REG_FAULT_CTRL, 1),
> +DECL_REG(RT1711H_REG_POWER_CTRL, 1),
> +DECL_REG(RT1711H_REG_CC_STATUS, 1),
> +DECL_REG(RT1711H_REG_POWER_STATUS, 1),
> +DECL_REG(RT1711H_REG_FAULT_STATUS, 1),
> +DECL_REG(RT1711H_REG_COMMAND, 1),
> +DECL_REG(RT1711H_REG_MSG_HDR_INFO, 1),
> +DECL_REG(RT1711H_REG_RX_DETECT, 1),
> +DECL_REG(RT1711H_REG_RX_BYTE_CNT, 1),
> +DECL_REG(RT1711H_REG_RX_BUF_FRAME_TYPE, 1),
> +DECL_REG(RT1711H_REG_RX_HDR, 2),
> +DECL_REG(RT1711H_REG_RX_DATA, 1),
> +DECL_REG(RT1711H_REG_TRANSMIT, 1),
> +DECL_REG(RT1711H_REG_TX_BYTE_CNT, 1),
> +DECL_REG(RT1711H_REG_TX_HDR, 2),
> +DECL_REG(RT1711H_REG_TX_DATA, 1),
> +DECL_REG(RT1711H_REG_CLK_CTRL2, 1),
> +DECL_REG(RT1711H_REG_CLK_CTRL3, 1),
> +DECL_REG(RT1711H_REG_BMC_CTRL, 1),
> +DECL_REG(RT1711H_REG_BMCIO_RXDZSEL, 1),
> +DECL_REG(RT1711H_REG_VCONN_CLIMITEN, 1),
> +DECL_REG(RT1711H_REG_RT_STATUS, 1),
> +DECL_REG(RT1711H_REG_RT_INT, 1),
> +DECL_REG(RT1711H_REG_RT_MASK, 1),
> +DECL_REG(RT1711H_REG_IDLE_CTRL, 1),
> +DECL_REG(RT1711H_REG_INTRST_CTRL, 1),
> +DECL_REG(RT1711H_REG_WATCHDOG_CTRL, 1),
> +DECL_REG(RT1711H_REG_I2CRST_CTRL, 1),
> +DECL_REG(RT1711H_REG_SWRESET, 1),
> +DECL_REG(RT1711H_REG_TTCPC_FILTER, 1),
> +DECL_REG(RT1711H_REG_DRP_TOGGLE_CYCLE, 1),
> +DECL_REG(RT1711H_REG_DRP_DUTY_CTRL, 1),
> +DECL_REG(RT1711H_REG_BMCIO_RXDZEN, 1), };
> +
> +static const char *rt1711h_dbg_filename[RT1711H_DBG_MAX] = {
> +"log", "regs", "reg_addr", "data",
> +};
> +
> +static bool rt1711h_log_full(struct rt1711h_chip *chip) {
> +return chip->logbuffer_tail ==
> +(chip->logbuffer_head + 1) % LOG_BUFFER_ENTRIES; }
> +
> +static void _rt1711h_log(struct rt1711h_chip *chip, const char *fmt,
> + va_list args)
> +{
> +char tmpbuffer[LOG_BUFFER_ENTRY_SIZE];
> +u64 ts_nsec = local_clock();
> +unsigned long rem_nsec;
> +
> +if (!chip->logbuffer[chip->logbuffer_head]) {
> +chip->logbuffer[chip->logbuffer_head] =
> +devm_kzalloc(chip->dev, LOG_BUFFER_ENTRY_SIZE, GFP_KERNEL);
> +if (!chip->logbuffer[chip->logbuffer_head])
> +return;
> +}
> +
> +vsnprintf(tmpbuffer, sizeof(tmpbuffer), fmt, args);
> +
> +mutex_lock(&chip->logbuffer_lock);
> +
> +if (rt1711h_log_full(chip)) {
> +chip->logbuffer_head = max(chip->logbuffer_head - 1, 0);
> +strlcpy(tmpbuffer, "overflow", sizeof(tmpbuffer));
> +}
> +
> +if (chip->logbuffer_head < 0 ||
> +chip->logbuffer_head >= LOG_BUFFER_ENTRIES) {
> +dev_warn(chip->dev, "%s bad log buffer index %d\n", __func__,
> +chip->logbuffer_head);
> +goto abort;
> +}
> +
> +if (!chip->logbuffer[chip->logbuffer_head]) {
> +dev_warn(chip->dev, "%s log buffer index %d is NULL\n",
> +__func__, chip->logbuffer_head);
> +goto abort;
> +}
> +
> +rem_nsec = do_div(ts_nsec, 1000000000);
> +scnprintf(chip->logbuffer[chip->logbuffer_head], LOG_BUFFER_ENTRY_SIZE,
> +"[%5lu.%06lu] %s", (unsigned long)ts_nsec, rem_nsec / 1000,
> + tmpbuffer);
> +chip->logbuffer_head = (chip->logbuffer_head + 1) %
> +LOG_BUFFER_ENTRIES;
> +
> +abort:
> +mutex_unlock(&chip->logbuffer_lock);
> +}
> +
> +static void rt1711h_log(struct rt1711h_chip *chip,
> +const char *fmt, ...)
> +{
> +va_list args;
> +
> +va_start(args, fmt);
> +_rt1711h_log(chip, fmt, args);
> +va_end(args);
> +}
> +
> +static int rt1711h_log_show(struct rt1711h_chip *chip, struct
> +seq_file *s) {
> +int tail;
> +
> +mutex_lock(&chip->logbuffer_lock);
> +tail = chip->logbuffer_tail;
> +while (tail != chip->logbuffer_head) {
> +seq_printf(s, "%s", chip->logbuffer[tail]);
> +tail = (tail + 1) % LOG_BUFFER_ENTRIES;
> +}
> +if (!seq_has_overflowed(s))
> +chip->logbuffer_tail = tail;
> +mutex_unlock(&chip->logbuffer_lock);
> +
> +return 0;
> +}
> +
> +static int rt1711h_regs_show(struct rt1711h_chip *chip, struct
> +seq_file *s) {
> +int ret = 0;
> +int i = 0, j = 0;
> +struct reg_desc *desc = NULL;
> +uint8_t regval[2] = {0};
> +
> +for (i = 0; i < ARRAY_SIZE(rt1711h_reg_desc); i++) {
> +desc = &rt1711h_reg_desc[i];
> +ret = rt1711h_reg_block_read(chip, desc->addr, desc->size,
> +regval);
> +if (ret < 0) {
> +dev_err(chip->dev, "%s read reg0x%02X fail\n",
> +__func__, desc->addr);
> +continue;
> +}
> +
> +seq_printf(s, "reg0x%02x:0x", desc->addr);
> +for (j = 0; j < desc->size; j++)
> +seq_printf(s, "%02x,", regval[j]);
> +seq_puts(s, "\n");
> +}
> +
> +return 0;
> +}
> +
> +static inline int rt1711h_reg_addr_show(struct rt1711h_chip *chip,
> +struct seq_file *s)
> +{
> +struct reg_desc *desc = &rt1711h_reg_desc[chip->dbg_regidx];
> +
> +seq_printf(s, "0x%02x\n", desc->addr);
> +return 0;
> +}
> +
> +static inline int rt1711h_data_show(struct rt1711h_chip *chip,
> +struct seq_file *s)
> +{
> +int ret = 0, i = 0;
> +struct reg_desc *desc = &rt1711h_reg_desc[chip->dbg_regidx];
> +uint8_t regval[2] = {0};
> +
> +ret = rt1711h_reg_block_read(chip, desc->addr, desc->size, regval);
> +if (ret < 0)
> +return ret;
> +
> +seq_printf(s, "reg0x%02x=0x", desc->addr);
> +for (i = 0; i < desc->size; i++)
> +seq_printf(s, "%02x,", regval[i]);
> +seq_puts(s, "\n");
> +return 0;
> +}
> +
> +static int rt1711h_dbg_show(struct seq_file *s, void *v) {
> +int ret = 0;
> +struct rt1711h_dbg_info *info = (struct rt1711h_dbg_info *)s->private;
> +struct rt1711h_chip *chip = info->chip;
> +
> +mutex_lock(&chip->dbgops_lock);
> +switch (info->id) {
> +case RT1711H_DBG_LOG:
> +ret = rt1711h_log_show(chip, s);
> +break;
> +case RT1711H_DBG_REGS:
> +ret = rt1711h_regs_show(chip, s);
> +break;
> +case RT1711H_DBG_REG_ADDR:
> +ret = rt1711h_reg_addr_show(chip, s);
> +break;
> +case RT1711H_DBG_DATA:
> +ret = rt1711h_data_show(chip, s);
> +break;
> +default:
> +ret = -EINVAL;
> +break;
> +}
> +
> +mutex_unlock(&chip->dbgops_lock);
> +return ret;
> +}
> +
> +static int rt1711h_dbg_open(struct inode *inode, struct file *file) {
> +if (file->f_mode & FMODE_READ)
> +return single_open(file, rt1711h_dbg_show, inode->i_private);
> +file->private_data = inode->i_private;
> +return 0;
> +}
> +
> +static int get_parameters(char *buf, long int *param1, int
> +num_of_par) {
> +char *token;
> +int base, cnt;
> +
> +token = strsep(&buf, " ");
> +
> +for (cnt = 0; cnt < num_of_par; cnt++) {
> +if (token != NULL) {
> +if ((token[1] == 'x') || (token[1] == 'X'))
> +base = 16;
> +else
> +base = 10;
> +
> +if (kstrtoul(token, base, &param1[cnt]) != 0)
> +return -EINVAL;
> +
> +token = strsep(&buf, " ");
> +} else
> +return -EINVAL;
> +}
> +return 0;
> +}

What is this function doing? What is your debugfs files for?

> +
> +static int get_datas(const char *buf, const int length,
> +unsigned char *data_buffer, unsigned char data_length) {
> +int i, ptr;
> +long int value;
> +char token[5];
> +
> +token[0] = '0';
> +token[1] = 'x';
> +token[4] = 0;
> +if (buf[0] != '0' || buf[1] != 'x')
> +return -EINVAL;
> +
> +ptr = 2;
> +for (i = 0; (i < data_length) && (ptr + 2 <= length); i++) {
> +token[2] = buf[ptr++];
> +token[3] = buf[ptr++];
> +ptr++;
> +if (kstrtoul(token, 16, &value) != 0)
> +return -EINVAL;
> +data_buffer[i] = value;
> +}
> +return 0;
> +}
> +
> +static int rt1711h_regaddr2idx(uint8_t reg_addr) {
> +int i = 0;
> +struct reg_desc *desc = NULL;
> +
> +for (i = 0; i < ARRAY_SIZE(rt1711h_reg_desc); i++) {
> +desc = &rt1711h_reg_desc[i];
> +if (desc->addr == reg_addr)
> +return i;
> +}
> +return -EINVAL;
> +}
> +
> +static ssize_t rt1711h_dbg_write(struct file *file, const char __user *ubuf,
> +size_t count, loff_t *ppos)
> +{
> +int ret = 0;
> +struct rt1711h_dbg_info *info =
> +(struct rt1711h_dbg_info *)file->private_data;
> +struct rt1711h_chip *chip = info->chip;
> +struct reg_desc *desc = NULL;
> +char lbuf[128];
> +long int param[5];
> +unsigned char reg_data[2] = {0};
> +
> +if (count > sizeof(lbuf) - 1)
> +return -EFAULT;
> +
> +ret = copy_from_user(lbuf, ubuf, count);
> +if (ret)
> +return -EFAULT;
> +lbuf[count] = '\0';
> +
> +mutex_lock(&chip->dbgops_lock);
> +switch (info->id) {
> +case RT1711H_DBG_REG_ADDR:
> +ret = get_parameters(lbuf, param, 1);
> +if (ret < 0) {
> +dev_err(chip->dev, "%s get param fail\n", __func__);
> +ret = -EINVAL;
> +goto out;
> +}
> +ret = rt1711h_regaddr2idx(param[0]);
> +if (ret < 0) {
> +dev_err(chip->dev, "%s addr2idx fail\n", __func__);
> +ret = -EINVAL;
> +goto out;
> +}
> +chip->dbg_regidx = ret;
> +break;
> +case RT1711H_DBG_DATA:
> +desc = &rt1711h_reg_desc[chip->dbg_regidx];
> +if ((desc->size - 1) * 3 + 5 != count) {
> +dev_err(chip->dev, "%s incorrect input length\n",
> +__func__);
> +ret = -EINVAL;
> +goto out;
> +}
> +ret = get_datas((char *)ubuf, count, reg_data, desc->size);
> +if (ret < 0) {
> +dev_err(chip->dev, "%s get data fail\n", __func__);
> +ret = -EINVAL;
> +goto out;
> +}
> +ret = rt1711h_reg_block_write(chip, desc->addr, desc->size,
> +reg_data);
> +break;
> +default:
> +ret = -EINVAL;
> +break;
> +}
> +
> +out:
> +mutex_unlock(&chip->dbgops_lock);
> +return ret < 0 ? ret : count;
> +}
> +
> +static int rt1711h_dbg_release(struct inode *inode, struct file
> +*file) {
> +if (file->f_mode & FMODE_READ)
> +return single_release(inode, file);
> +return 0;
> +}
> +
> +static const struct file_operations rt1711h_dbg_ops = {
> +.open= rt1711h_dbg_open,
> +.llseek= seq_lseek,
> +.read= seq_read,
> +.write= rt1711h_dbg_write,
> +.release= rt1711h_dbg_release,
> +};
> +
> +
> +static int rt1711h_debugfs_init(struct rt1711h_chip *chip) {
> +int ret = 0, i = 0;
> +struct rt1711h_dbg_info *info = NULL;
> +int len = 0;
> +char *dirname = NULL;
> +
> +mutex_init(&chip->logbuffer_lock);
> +mutex_init(&chip->dbgops_lock);
> +len = strlen(dev_name(chip->dev));
> +dirname = devm_kzalloc(chip->dev, len + 9, GFP_KERNEL);
> +if (!dirname)
> +return -ENOMEM;
> +snprintf(dirname, len + 9, "rt1711h-%s", dev_name(chip->dev));
> +if (!chip->dbgdir) {
> +chip->dbgdir = debugfs_create_dir(dirname, NULL);
> +if (!chip->dbgdir)
> +return -ENOMEM;

No need to ever check the return value of debugfs_ calls, you should not care and can always use the value to any future debugfs calls, if you really need it.

> +}
> +
> +for (i = 0; i < RT1711H_DBG_MAX; i++) {
> +info = &chip->dbg_info[i];

static array of debug info? That feels odd.

> +info->chip = chip;
> +info->id = i;
> +chip->dbg_files[i] = debugfs_create_file(
> +rt1711h_dbg_filename[i], S_IFREG | 0444,
> +chip->dbgdir, info, &rt1711h_dbg_ops);
> +if (!chip->dbg_files[i]) {
> +ret = -EINVAL;

Like here, you don't need this, and you don't need to care about the return value.

> +goto err;
> +}
> +}
> +
> +return 0;
> +err:
> +debugfs_remove_recursive(chip->dbgdir);
> +return ret;

Why do you care about an error here? Your code should not do anything different if debugfs stuff does not work or if it does. It's debugging only.

> +}
> +
> +static void rt1711h_debugfs_exit(struct rt1711h_chip *chip) {
> +debugfs_remove_recursive(chip->dbgdir);

See, you didn't need those file handles :)

thanks,

greg k-h
************* Email Confidentiality Notice ********************

The information contained in this e-mail message (including any attachments) may be confidential, proprietary, privileged, or otherwise exempt from disclosure under applicable laws. It is intended to be conveyed only to the designated recipient(s). Any use, dissemination, distribution, printing, retaining or copying of this e-mail (including its attachments) by unintended recipient(s) is strictly prohibited and may be unlawful. If you are not an intended recipient of this e-mail, or believe that you have received this e-mail in error, please notify the sender immediately (by replying to this e-mail), delete any and all copies of this e-mail (including any attachments) from your system, and do not disclose the content of this e-mail to any other person. Thank you!

2018-01-19 03:10:46

by Jun Li

[permalink] [raw]

Subject: RE: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

Hi
> -----Original Message-----
> From: [email protected] [mailto:linux-usb-
> [email protected]] On Behalf Of ShuFanLee
> Sent: Wednesday, January 10, 2018 2:59 PM
> To: [email protected]
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]
> Subject: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver
>
> From: ShuFanLee <[email protected]>
>
> Richtek RT1711H Type-C chip driver that works with
> Type-C Port Controller Manager to provide USB PD and
> USB Type-C functionalities.

A general question, is this Rt1711h type-c chip compatible with TCPCI
(Universal Serial Bus Type-C Port Controller Interface Specification)?
looks like it has the same register map and has some extension, can
the existing ./drivers/staging/typec/tcpic.c basically work for you?

+Guenter

Li Jun

>
> Signed-off-by: ShuFanLee <[email protected]>
> ---
> .../devicetree/bindings/usb/richtek,rt1711h.txt | 38 +
> arch/arm64/boot/dts/hisilicon/rt1711h.dtsi | 11 +
> drivers/usb/typec/Kconfig | 2 +
> drivers/usb/typec/Makefile | 1 +
> drivers/usb/typec/rt1711h/Kconfig | 7 +
> drivers/usb/typec/rt1711h/Makefile | 2 +
> drivers/usb/typec/rt1711h/rt1711h.c | 2241 ++++++++++++++++++++
> drivers/usb/typec/rt1711h/rt1711h.h | 300 +++
> 8 files changed, 2602 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
> create mode 100644 arch/arm64/boot/dts/hisilicon/rt1711h.dtsi
> create mode 100644 drivers/usb/typec/rt1711h/Kconfig
> create mode 100644 drivers/usb/typec/rt1711h/Makefile
> create mode 100644 drivers/usb/typec/rt1711h/rt1711h.c
> create mode 100644 drivers/usb/typec/rt1711h/rt1711h.h
>

2018-01-19 05:49:54

[permalink] [raw]

Subject: RE: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

Hi Jun,

For now, RT1711H is not fully compatible with TCPCI. So the existing tcpci.c may not work for it.

Best Regards,
*****************************
Shu-Fan Lee
Richtek Technology Corporation
TEL: +886-3-5526789 #2359
FAX: +886-3-5526612
*****************************

-----Original Message-----
From: Jun Li [mailto:[email protected]]
Sent: Friday, January 19, 2018 11:10 AM
To: ShuFanLee; [email protected]
Cc: cy_huang(黃啟原); shufan_lee(李書帆); [email protected]; [email protected]; Guenter Roeck
Subject: RE: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

Hi
> -----Original Message-----
> From: [email protected] [mailto:linux-usb-
> [email protected]] On Behalf Of ShuFanLee
> Sent: Wednesday, January 10, 2018 2:59 PM
> To: [email protected]
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]
> Subject: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver
>
> From: ShuFanLee <[email protected]>
>
> Richtek RT1711H Type-C chip driver that works with Type-C Port
> Controller Manager to provide USB PD and USB Type-C functionalities.

A general question, is this Rt1711h type-c chip compatible with TCPCI (Universal Serial Bus Type-C Port Controller Interface Specification)?
looks like it has the same register map and has some extension, can the existing ./drivers/staging/typec/tcpic.c basically work for you?

+Guenter

Li Jun

>
> Signed-off-by: ShuFanLee <[email protected]>
> ---
> .../devicetree/bindings/usb/richtek,rt1711h.txt | 38 +
> arch/arm64/boot/dts/hisilicon/rt1711h.dtsi | 11 +
> drivers/usb/typec/Kconfig | 2 +
> drivers/usb/typec/Makefile | 1 +
> drivers/usb/typec/rt1711h/Kconfig | 7 +
> drivers/usb/typec/rt1711h/Makefile | 2 +
> drivers/usb/typec/rt1711h/rt1711h.c | 2241 ++++++++++++++++++++
> drivers/usb/typec/rt1711h/rt1711h.h | 300 +++
> 8 files changed, 2602 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
> create mode 100644 arch/arm64/boot/dts/hisilicon/rt1711h.dtsi
> create mode 100644 drivers/usb/typec/rt1711h/Kconfig create mode
> 100644 drivers/usb/typec/rt1711h/Makefile
> create mode 100644 drivers/usb/typec/rt1711h/rt1711h.c
> create mode 100644 drivers/usb/typec/rt1711h/rt1711h.h
>
************* Email Confidentiality Notice ********************

The information contained in this e-mail message (including any attachments) may be confidential, proprietary, privileged, or otherwise exempt from disclosure under applicable laws. It is intended to be conveyed only to the designated recipient(s). Any use, dissemination, distribution, printing, retaining or copying of this e-mail (including its attachments) by unintended recipient(s) is strictly prohibited and may be unlawful. If you are not an intended recipient of this e-mail, or believe that you have received this e-mail in error, please notify the sender immediately (by replying to this e-mail), delete any and all copies of this e-mail (including any attachments) from your system, and do not disclose the content of this e-mail to any other person. Thank you!

2018-01-19 08:05:02

[permalink] [raw]

Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

On Thu, Jan 18, 2018 at 01:13:15PM +0000, shufan_lee(李書帆) wrote:
> > +
> > +#include "rt1711h.h"
>
> Why a .h file for a single .c file?
>
> Is the suggestion to move all content in rt1711h.h into rt1711h.c?

If it can be, sure, you only need a .h file for things that are shared
among other .c files.

> > +/* I2C */
> > +atomic_t i2c_busy;
> > +atomic_t pm_suspend;
>
> Why are these atomic? You know that doesn't mean they do not need locking, right?
>
> For my understanding, a single operation on atomic_t doesn't need lock, like a single atomic_set.
> But two consecutive operations doesn't guarantee anything. Like atomic_set followed by an atomic_read.
> This part is referenced from fusb302 used to make sure I2C is idle before system suspends.
> It only needs to guarantee a single read/write on these variable is atomic operation, so atomic is used.

It's atomic for read/write, yes, but that does not mean it can not be
instantly changed after the value is read, right? So you might need to
look and ensure you are not doing something wrong that can race. A
single lock should be simpler than this type of thing, and will be
correct.

> > +#ifdef CONFIG_DEBUG_FS
> > +struct dentry *dbgdir;
> > +struct rt1711h_dbg_info dbg_info[RT1711H_DBG_MAX];
> > +struct dentry *dbg_files[RT1711H_DBG_MAX];
> > +int dbg_regidx;
> > +struct mutex dbgops_lock;
> > +/* lock for log buffer access */
> > +struct mutex logbuffer_lock;
> > +int logbuffer_head;
> > +int logbuffer_tail;
> > +u8 *logbuffer[LOG_BUFFER_ENTRIES];
> > +#endif /* CONFIG_DEBUG_FS */
>
> That's a lot of stuff jsut for debugfs. Why do you care about #define at all? The code should not.
>
> Is the suggestion to remove #ifdef CONFIG_DEBUG_FS?

Yes. Or just move it all to another structure that you can dynamically
add to this one if needed.

> And another 2 locks? Ick, no.
>
> dbgops_lock is used to prevent user from accessing different debug files simultaneously.
> Is the suggestion to use the lock of the following one?
> > +/* lock for sharing chip states */
> > +struct mutex lock;

Sure, why not?

> ========================================================================
>
> > +snprintf(dirname, len + 9, "rt1711h-%s", dev_name(chip->dev));
> > +if (!chip->dbgdir) {
> > +chip->dbgdir = debugfs_create_dir(dirname, NULL);
> > +if (!chip->dbgdir)
> > +return -ENOMEM;
>
> No need to ever check the return value of debugfs_ calls, you should not care and can always use the value to any future debugfs calls, if you really need it.
>
> If it is NULL without checking and we use it in debugfs_create_file, all the debug files will be created in the root of the debugfs filesystem.
> Is this correct?

If it returns NULL then any future calls to debugfs will also not be
working, so all will be fine. So there is no need to check this.

> ========================================================================
>
> > +for (i = 0; i < RT1711H_DBG_MAX; i++) {
> > +info = &chip->dbg_info[i];
>
> static array of debug info? That feels odd.
>
> Is the suggestion to use pointer of array and dynamically allocated it?

If that makes more sense, it's up to you. Just a suggestion.

thanks,

greg k-h

2018-01-19 08:23:17

[permalink] [raw]

Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

Hi Shu-Fan,

On Fri, Jan 19, 2018 at 05:48:02AM +0000, shufan_lee(?????????) wrote:
> Hi Jun,
>
> For now, RT1711H is not fully compatible with TCPCI. So the existing tcpci.c
> may not work for it.

The datasheet for RT1711H does talk about TCPCi and TCPM+TCPC [1].
What are the differences that justify a separate driver?

[1] http://www.richtek.com/assets/product_file/RT1711H/DS1711H-02.pdf

Br,

--
heikki

2018-01-19 09:03:12

[permalink] [raw]

Subject: RE: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

Hi Heikki,

For example, the flow of tcpci_init is a little bit different.
In tcpci_init, there are more parameters need to be set for RT1711H.

Best Regards,
*****************************
Shu-Fan Lee
Richtek Technology Corporation
TEL: +886-3-5526789 #2359
FAX: +886-3-5526612
*****************************

-----Original Message-----
From: Heikki Krogerus [mailto:[email protected]]
Sent: Friday, January 19, 2018 4:22 PM
To: shufan_lee(???Ѧ|)
Cc: 'Jun Li'; ShuFanLee; cy_huang(???ҭ?); [email protected]; [email protected]; Guenter Roeck
Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

Hi Shu-Fan,

On Fri, Jan 19, 2018 at 05:48:02AM +0000, shufan_lee(?????????) wrote:
> Hi Jun,
>
> For now, RT1711H is not fully compatible with TCPCI. So the existing tcpci.c
> may not work for it.

The datasheet for RT1711H does talk about TCPCi and TCPM+TCPC [1].
What are the differences that justify a separate driver?

[1] http://www.richtek.com/assets/product_file/RT1711H/DS1711H-02.pdf

Br,

--
heikki
************* Email Confidentiality Notice ********************

The information contained in this e-mail message (including any attachments) may be confidential, proprietary, privileged, or otherwise exempt from disclosure under applicable laws. It is intended to be conveyed only to the designated recipient(s). Any use, dissemination, distribution, printing, retaining or copying of this e-mail (including its attachments) by unintended recipient(s) is strictly prohibited and may be unlawful. If you are not an intended recipient of this e-mail, or believe that you have received this e-mail in error, please notify the sender immediately (by replying to this e-mail), delete any and all copies of this e-mail (including any attachments) from your system, and do not disclose the content of this e-mail to any other person. Thank you!

2018-01-19 09:25:32

[permalink] [raw]

Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

Hi,

On Fri, Jan 19, 2018 at 09:01:24AM +0000, shufan_lee(?????|) wrote:
> Hi Heikki,
>
> For example, the flow of tcpci_init is a little bit different.
> In tcpci_init, there are more parameters need to be set for RT1711H.

Different init parameters is really not a reason for a fork of the
driver. The configuration of the TCPC will depend on the platform and
TCPC vendor most cases.

Thanks,

--
heikki

2018-01-19 16:06:09

[permalink] [raw]

Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

On Fri, Jan 19, 2018 at 11:24:13AM +0200, Heikki Krogerus wrote:
> Hi,
>
> On Fri, Jan 19, 2018 at 09:01:24AM +0000, shufan_lee(?????|) wrote:
> > Hi Heikki,
> >
> > For example, the flow of tcpci_init is a little bit different.
> > In tcpci_init, there are more parameters need to be set for RT1711H.
>
> Different init parameters is really not a reason for a fork of the
> driver. The configuration of the TCPC will depend on the platform and
> TCPC vendor most cases.
>
Agreed. dwc3 usb support is an excellent example on how to handle this
kind of variation.

Guenter

2018-01-22 02:16:13

[permalink] [raw]

Subject: RE: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

Dear Heikki and Guenter,

Because there are still other controls of RT1711H that are different from standard TCPCI, e.x. flow of drp toggling.

Is the suggestion to customize the difference based on tcpci.c for RT1711H?

Best Regards,
*****************************
Shu-Fan Lee
Richtek Technology Corporation
TEL: +886-3-5526789 #2359
FAX: +886-3-5526612
*****************************

-----Original Message-----
From: Guenter Roeck [mailto:[email protected]] On Behalf Of Guenter Roeck
Sent: Saturday, January 20, 2018 12:03 AM
To: Heikki Krogerus
Cc: shufan_lee(???Ѧ|); 'Jun Li'; ShuFanLee; cy_huang(???ҭ?); [email protected]; [email protected]
Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

On Fri, Jan 19, 2018 at 11:24:13AM +0200, Heikki Krogerus wrote:
> Hi,
>
> On Fri, Jan 19, 2018 at 09:01:24AM +0000, shufan_lee(?????|) wrote:
> > Hi Heikki,
> >
> > For example, the flow of tcpci_init is a little bit different.
> > In tcpci_init, there are more parameters need to be set for RT1711H.
>
> Different init parameters is really not a reason for a fork of the
> driver. The configuration of the TCPC will depend on the platform and
> TCPC vendor most cases.
>
Agreed. dwc3 usb support is an excellent example on how to handle this kind of variation.

Guenter
************* Email Confidentiality Notice ********************

The information contained in this e-mail message (including any attachments) may be confidential, proprietary, privileged, or otherwise exempt from disclosure under applicable laws. It is intended to be conveyed only to the designated recipient(s). Any use, dissemination, distribution, printing, retaining or copying of this e-mail (including its attachments) by unintended recipient(s) is strictly prohibited and may be unlawful. If you are not an intended recipient of this e-mail, or believe that you have received this e-mail in error, please notify the sender immediately (by replying to this e-mail), delete any and all copies of this e-mail (including any attachments) from your system, and do not disclose the content of this e-mail to any other person. Thank you!

2018-01-22 18:51:39

[permalink] [raw]

Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

On Mon, Jan 22, 2018 at 02:01:13AM +0000, shufan_lee(李書帆) wrote:
> Dear Heikki and Guenter,
>
> Because there are still other controls of RT1711H that are different from standard TCPCI, e.x. flow of drp toggling.
>
> Is the suggestion to customize the difference based on tcpci.c for RT1711H?
>

In general, I would say yes. However, I won't have ime to review
the differences between tcpci and the RT1711H. On a high level,
if RT1711H claims to suport TCPCI, it should use (or, rather, extend)
the TCPCI driver.

Note that the TCPCI driver does not claim to be complete; there is
a reason why it is still in staging. However, I would prefer if new
devices claiming to support TCPCI would use it instead of going
their own way. I don't have problems extending it with chip specific
details if needed. Such extensions may be implemented in tcpci.c,
or maybe better in a chip specific file.

Even if you don't use the existing driver, I don't really see why
it would make sense to redeclare all its defines.

Either case, you might want to run checkpatch --strict on your
driver. Most of that it reports is really unnecessary.
Also, some of the code, such as

+#ifndef BIT
+#define BIT(x) (1 << (x))
+#endif

is _really_ odd and, at least in this case, simply wrong.

Guenter

> Best Regards,
> *****************************
> Shu-Fan Lee
> Richtek Technology Corporation
> TEL: +886-3-5526789 #2359
> FAX: +886-3-5526612
> *****************************
>
> -----Original Message-----
> From: Guenter Roeck [mailto:[email protected]] On Behalf Of Guenter Roeck
> Sent: Saturday, January 20, 2018 12:03 AM
> To: Heikki Krogerus
> Cc: shufan_lee(李書帆); 'Jun Li'; ShuFanLee; cy_huang(黃啟原); [email protected]; [email protected]
> Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver
>
> On Fri, Jan 19, 2018 at 11:24:13AM +0200, Heikki Krogerus wrote:
> > Hi,
> >
> > On Fri, Jan 19, 2018 at 09:01:24AM +0000, shufan_lee(?????|) wrote:
> > > Hi Heikki,
> > >
> > > For example, the flow of tcpci_init is a little bit different.
> > > In tcpci_init, there are more parameters need to be set for RT1711H.
> >
> > Different init parameters is really not a reason for a fork of the
> > driver. The configuration of the TCPC will depend on the platform and
> > TCPC vendor most cases.
> >
> Agreed. dwc3 usb support is an excellent example on how to handle this kind of variation.
>
> Guenter
> ************* Email Confidentiality Notice ********************
>
> The information contained in this e-mail message (including any attachments) may be confidential, proprietary, privileged, or otherwise exempt from disclosure under applicable laws. It is intended to be conveyed only to the designated recipient(s). Any use, dissemination, distribution, printing, retaining or copying of this e-mail (including its attachments) by unintended recipient(s) is strictly prohibited and may be unlawful. If you are not an intended recipient of this e-mail, or believe that you have received this e-mail in error, please notify the sender immediately (by replying to this e-mail), delete any and all copies of this e-mail (including any attachments) from your system, and do not disclose the content of this e-mail to any other person. Thank you!

2018-01-29 07:26:09

[permalink] [raw]

Subject: RE: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

Hi Guenter,

We try to use the TCPCI driver on RT1711H and here are some questions.

Q1. Is current TCPCI driver written according to TypeC Port Controller Interface Specification Revision 1.0 & Version 1.2?
Q2. Because 0x80~0xFF are vendor defined registers. Some of them are needed to be initialized in tcpci_init for RT1711H (or other chips also).
In the future TCPCI driver, will an initial interface that is called in tcpci_init be released for different vendors to implement.
Or, we should directly copy tcpci.c to tcpci_rt1711h.c and add the different parts?
Q3. If there are IRQs defined in vendor defined registers, will an interface that is called in tcpci_irq be released for different vendors to implement.
So that they can handle their own IRQs first?
If the suggestion of Q2 is to copy tcpci.c to tcpci_rt1711h.c, then Q3 will not be a problem.
Q4. According to TCPCI Specification Revision 1.0, we should set DRP = 1 and role to Rp/Rp or Rd/Rd and set LOOK4CONNECTION command to start toggle.
So we modify the tcpci_start_drp_toggling in TCPCI driver as following. Here we write Rd/Rd and DRP = 0 simultaneously so that Rd/Rd takes effect.
Then we write DRP = 1 and set LOOK4CONNECTION command to start toggling.

static int tcpci_start_drp_toggling(struct tcpc_dev *tcpc,
enum typec_cc_status cc)
{
+int ret = 0;
struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
-unsigned int reg = TCPC_ROLE_CTRL_DRP;
+u32 reg = (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC1_SHIFT) |
+(TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC2_SHIFT);

switch (cc) {
default:
@@ -125,8 +672,19 @@ static int tcpci_start_drp_toggling(stru
TCPC_ROLE_CTRL_RP_VAL_SHIFT);
break;
}
-
-return regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
+ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
+if (ret < 0)
+return ret;
+mdelay(1);
+reg |= TCPC_ROLE_CTRL_DRP;
+ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
+if (ret < 0)
+return ret;
+ret = regmap_write(tcpci->regmap, TCPC_COMMAND,
+TCPC_CMD_LOOK4CONNECTION);
+if (ret < 0)
+return ret;
+return 0;
}

Q5. The tcpci_set_vbus in TCPCI driver uses command to control Sink/Source VBUS.
If our chip does not support power path, i.e. Sink & Source are controlled by other charger IC. Our chip will do nothing while setting these commands.
In the future TCPCI driver, will a framework be applied to notify these events. i.g. power_supply or notifier.

Best Regards,
*****************************
Shu-Fan Lee
Richtek Technology Corporation
TEL: +886-3-5526789 #2359
FAX: +886-3-5526612
*****************************

-----Original Message-----
From: Guenter Roeck [mailto:[email protected]] On Behalf Of Guenter Roeck
Sent: Tuesday, January 23, 2018 2:51 AM
To: shufan_lee(李書帆)
Cc: Heikki Krogerus; 'Jun Li'; ShuFanLee; cy_huang(黃啟原); [email protected]; [email protected]
Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

On Mon, Jan 22, 2018 at 02:01:13AM +0000, shufan_lee(李書帆) wrote:
> Dear Heikki and Guenter,
>
> Because there are still other controls of RT1711H that are different from standard TCPCI, e.x. flow of drp toggling.
>
> Is the suggestion to customize the difference based on tcpci.c for RT1711H?
>

In general, I would say yes. However, I won't have ime to review the differences between tcpci and the RT1711H. On a high level, if RT1711H claims to suport TCPCI, it should use (or, rather, extend) the TCPCI driver.

Note that the TCPCI driver does not claim to be complete; there is a reason why it is still in staging. However, I would prefer if new devices claiming to support TCPCI would use it instead of going their own way. I don't have problems extending it with chip specific details if needed. Such extensions may be implemented in tcpci.c, or maybe better in a chip specific file.

Even if you don't use the existing driver, I don't really see why it would make sense to redeclare all its defines.

Either case, you might want to run checkpatch --strict on your driver. Most of that it reports is really unnecessary.
Also, some of the code, such as

+#ifndef BIT
+#define BIT(x) (1 << (x))
+#endif

is _really_ odd and, at least in this case, simply wrong.

Guenter

> Best Regards,
> *****************************
> Shu-Fan Lee
> Richtek Technology Corporation
> TEL: +886-3-5526789 #2359
> FAX: +886-3-5526612
> *****************************
>
> -----Original Message-----
> From: Guenter Roeck [mailto:[email protected]] On Behalf Of Guenter
> Roeck
> Sent: Saturday, January 20, 2018 12:03 AM
> To: Heikki Krogerus
> Cc: shufan_lee(李書帆); 'Jun Li'; ShuFanLee; cy_huang(黃啟原);
> [email protected]; [email protected]
> Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver
>
> On Fri, Jan 19, 2018 at 11:24:13AM +0200, Heikki Krogerus wrote:
> > Hi,
> >
> > On Fri, Jan 19, 2018 at 09:01:24AM +0000, shufan_lee(?????|) wrote:
> > > Hi Heikki,
> > >
> > > For example, the flow of tcpci_init is a little bit different.
> > > In tcpci_init, there are more parameters need to be set for RT1711H.
> >
> > Different init parameters is really not a reason for a fork of the
> > driver. The configuration of the TCPC will depend on the platform
> > and TCPC vendor most cases.
> >
> Agreed. dwc3 usb support is an excellent example on how to handle this kind of variation.
>
> Guenter
> ************* Email Confidentiality Notice ********************
>
> The information contained in this e-mail message (including any attachments) may be confidential, proprietary, privileged, or otherwise exempt from disclosure under applicable laws. It is intended to be conveyed only to the designated recipient(s). Any use, dissemination, distribution, printing, retaining or copying of this e-mail (including its attachments) by unintended recipient(s) is strictly prohibited and may be unlawful. If you are not an intended recipient of this e-mail, or believe that you have received this e-mail in error, please notify the sender immediately (by replying to this e-mail), delete any and all copies of this e-mail (including any attachments) from your system, and do not disclose the content of this e-mail to any other person. Thank you!
************* Email Confidentiality Notice ********************

The information contained in this e-mail message (including any attachments) may be confidential, proprietary, privileged, or otherwise exempt from disclosure under applicable laws. It is intended to be conveyed only to the designated recipient(s). Any use, dissemination, distribution, printing, retaining or copying of this e-mail (including its attachments) by unintended recipient(s) is strictly prohibited and may be unlawful. If you are not an intended recipient of this e-mail, or believe that you have received this e-mail in error, please notify the sender immediately (by replying to this e-mail), delete any and all copies of this e-mail (including any attachments) from your system, and do not disclose the content of this e-mail to any other person. Thank you!

2018-01-29 19:58:35

[permalink] [raw]

Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

On Mon, Jan 29, 2018 at 07:19:06AM +0000, shufan_lee(李書帆) wrote:
> Hi Guenter,
>
> We try to use the TCPCI driver on RT1711H and here are some questions.
>
> Q1. Is current TCPCI driver written according to TypeC Port Controller Interface Specification Revision 1.0 & Version 1.2?

Revision 1.0. Note that I did not find revision 1.2, only
Revision 1.0 and Revision 2.0 version 1.0.

> Q2. Because 0x80~0xFF are vendor defined registers. Some of them are needed to be initialized in tcpci_init for RT1711H (or other chips also).
> In the future TCPCI driver, will an initial interface that is called in tcpci_init be released for different vendors to implement.

My suggestion would be to provide an API for vendor specific code.

> Or, we should directly copy tcpci.c to tcpci_rt1711h.c and add the different parts?

That would defeat the purpose. It would be better to implement vendor
specific code in tcpci_rt1711h.c and call it from tcpci.c

Possible example:

struct tcpci_vendor_data {
int (*init)(...);
int (*irq_handler)(...);
...
}

static irqreturn_t tcpci_irq(...)
{
struct tcpci *tcpci = dev_id;

...
if (tcpci->vendor_data->irq_handler) {
ret = (*tcpci->vendor_data->irq_handler)(...);
...
}
...
}

tcpci_init()
{
struct tcpci_vendor_data *vendor_data = &tcpci_rt1711h_data;
// eg from devicetree compatible property

...
if (vendor_data->init) {
ret = (*vendor_data->init)(...);
if (ret)
return ret;
}
...
}

> Q3. If there are IRQs defined in vendor defined registers, will an interface that is called in tcpci_irq be released for different vendors to implement.
> So that they can handle their own IRQs first?

If there are vendor specific interrupts, I would assume that vendor specific
code will have to be called. Either the generic interrupt handler can call
vendor specific code, or the vendor specific code handles the interrupt and
calls the generic handler. I don't know at this point which one is better.

> If the suggestion of Q2 is to copy tcpci.c to tcpci_rt1711h.c, then Q3 will not be a problem.
> Q4. According to TCPCI Specification Revision 1.0, we should set DRP = 1 and role to Rp/Rp or Rd/Rd and set LOOK4CONNECTION command to start toggle.
> So we modify the tcpci_start_drp_toggling in TCPCI driver as following. Here we write Rd/Rd and DRP = 0 simultaneously so that Rd/Rd takes effect.
> Then we write DRP = 1 and set LOOK4CONNECTION command to start toggling.
>
SGTM.

> static int tcpci_start_drp_toggling(struct tcpc_dev *tcpc,
> enum typec_cc_status cc)
> {
> +int ret = 0;
> struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> -unsigned int reg = TCPC_ROLE_CTRL_DRP;
> +u32 reg = (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC1_SHIFT) |
> +(TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC2_SHIFT);
>
> switch (cc) {
> default:
> @@ -125,8 +672,19 @@ static int tcpci_start_drp_toggling(stru
> TCPC_ROLE_CTRL_RP_VAL_SHIFT);
> break;
> }
> -
> -return regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
> +ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
> +if (ret < 0)
> +return ret;
> +mdelay(1);

That is bad; you don't want to hold up teh system for that much.
Try to use usleep_range().

> +reg |= TCPC_ROLE_CTRL_DRP;
> +ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
> +if (ret < 0)
> +return ret;
> +ret = regmap_write(tcpci->regmap, TCPC_COMMAND,
> +TCPC_CMD_LOOK4CONNECTION);
> +if (ret < 0)
> +return ret;
> +return 0;
> }
>
> Q5. The tcpci_set_vbus in TCPCI driver uses command to control Sink/Source VBUS.
> If our chip does not support power path, i.e. Sink & Source are controlled by other charger IC. Our chip will do nothing while setting these commands.
> In the future TCPCI driver, will a framework be applied to notify these events. i.g. power_supply or notifier.
>
I would think so.

Note that the driver is, at this point, fair game to change to
make it work with your chip. The only condition is that a standard
chip should still work, ie you should not make any changes which
would cause the driver to _only_ work with your chip. Everything else
is fair game.

Thanks,
Guenter

2018-01-30 13:43:06

[permalink] [raw]

Subject: RE: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

Hi Guenter,

For now, it looks like there are two ways to implement vendor data. It would be nice to hear your suggestion.

1. Set vendor data in the data field of of_device_id.
If I understand correctly, this would be the one more like you mentioned before.
In this case, tcpci_rt1711h_data needs to be defined inside tcpci.c or defined by other file(tcpci_rt1711h.c) but extern in tcpci.c.

For example:
static struct tcpci_vendor_data tcpci_rt1711h_data = {
.init = rt1711h_init;
.irq_handler = rt1711h_irq_handler
};
OR
extern struct tcpci_vendor_data tcpci_rt1711h_data;

Then, put this structure here
static const struct of_device_id tcpci_of_match[] = {
{ .compatible = "usb,tcpci", },
{ .compatible = "richtek,rt1711h", .data = (void *)&tcpci_rt1711h_data },
{},
};

For other vendors who want to handle vendor data also need to add these code inside tcpci.c.
We are not sure that's what you expect or not.

2. In tcpci.c, create a static list_head used to maintain vendor data.
TCPCI driver provides an API for those vendors to add their vendor data in the list.
Then, we could find vendor data in the list according to compatible string.

For example:
In tcpci.h
struct tcpci_vendor_data {
const char *compatible;
int (*init)(...);
int (*irq_handler)(...);
struct list_head list;
};
/* This function adds tcpci_vendor_data to the list */
extern int tcpci_register_vendor_data(struct tcpci_vendor_data *data);

In tcpci.c
static LIST_HEAD(tcpci_vendor_data_list);
int tcpci_register_vendor_data(...)
{
...
list_add(...);
...
}

tcpci_init()
{
...
/* Find correct vendor data */
list_for_each(...)
...
}

In this case, vendor needs to guarantee that vendor data is added to the list before tcpci starts to work.

Best Regards,
*****************************
Shu-Fan Lee
Richtek Technology Corporation
TEL: +886-3-5526789 #2359
FAX: +886-3-5526612
*****************************

-----Original Message-----
From: Guenter Roeck [mailto:[email protected]] On Behalf Of Guenter Roeck
Sent: Tuesday, January 30, 2018 3:58 AM
To: shufan_lee(李書帆)
Cc: Heikki Krogerus; 'Jun Li'; ShuFanLee; cy_huang(黃啟原); [email protected]; [email protected]
Subject: Re: [PATCH] USB TYPEC: RT1711H Type-C Chip Driver

On Mon, Jan 29, 2018 at 07:19:06AM +0000, shufan_lee(李書帆) wrote:
> Hi Guenter,
>
> We try to use the TCPCI driver on RT1711H and here are some questions.
>
> Q1. Is current TCPCI driver written according to TypeC Port Controller Interface Specification Revision 1.0 & Version 1.2?

Revision 1.0. Note that I did not find revision 1.2, only Revision 1.0 and Revision 2.0 version 1.0.

> Q2. Because 0x80~0xFF are vendor defined registers. Some of them are needed to be initialized in tcpci_init for RT1711H (or other chips also).
> In the future TCPCI driver, will an initial interface that is called in tcpci_init be released for different vendors to implement.

My suggestion would be to provide an API for vendor specific code.

> Or, we should directly copy tcpci.c to tcpci_rt1711h.c and add the different parts?

That would defeat the purpose. It would be better to implement vendor specific code in tcpci_rt1711h.c and call it from tcpci.c

Possible example:

struct tcpci_vendor_data {
int (*init)(...);
int (*irq_handler)(...);
...
}

static irqreturn_t tcpci_irq(...)
{
struct tcpci *tcpci = dev_id;

...
if (tcpci->vendor_data->irq_handler) {
ret = (*tcpci->vendor_data->irq_handler)(...);
...
}
...
}

tcpci_init()
{
struct tcpci_vendor_data *vendor_data = &tcpci_rt1711h_data;
// eg from devicetree compatible property

...
if (vendor_data->init) {
ret = (*vendor_data->init)(...);
if (ret)
return ret;
}
...
}

> Q3. If there are IRQs defined in vendor defined registers, will an interface that is called in tcpci_irq be released for different vendors to implement.
> So that they can handle their own IRQs first?

If there are vendor specific interrupts, I would assume that vendor specific code will have to be called. Either the generic interrupt handler can call vendor specific code, or the vendor specific code handles the interrupt and calls the generic handler. I don't know at this point which one is better.

> If the suggestion of Q2 is to copy tcpci.c to tcpci_rt1711h.c, then Q3 will not be a problem.
> Q4. According to TCPCI Specification Revision 1.0, we should set DRP = 1 and role to Rp/Rp or Rd/Rd and set LOOK4CONNECTION command to start toggle.
> So we modify the tcpci_start_drp_toggling in TCPCI driver as following. Here we write Rd/Rd and DRP = 0 simultaneously so that Rd/Rd takes effect.
> Then we write DRP = 1 and set LOOK4CONNECTION command to start toggling.
>
SGTM.

> static int tcpci_start_drp_toggling(struct tcpc_dev *tcpc,
> enum typec_cc_status cc)
> {
> +int ret = 0;
> struct tcpci *tcpci = tcpc_to_tcpci(tcpc); -unsigned int reg =
> TCPC_ROLE_CTRL_DRP;
> +u32 reg = (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC1_SHIFT) |
> +(TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC2_SHIFT);
>
> switch (cc) {
> default:
> @@ -125,8 +672,19 @@ static int tcpci_start_drp_toggling(stru
> TCPC_ROLE_CTRL_RP_VAL_SHIFT); break; }
> -
> -return regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
> +ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg); if (ret < 0)
> +return ret; mdelay(1);

That is bad; you don't want to hold up teh system for that much.
Try to use usleep_range().

> +reg |= TCPC_ROLE_CTRL_DRP;
> +ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg); if (ret < 0)
> +return ret; ret = regmap_write(tcpci->regmap, TCPC_COMMAND,
> +TCPC_CMD_LOOK4CONNECTION); if (ret < 0) return ret; return 0;
> }
>
> Q5. The tcpci_set_vbus in TCPCI driver uses command to control Sink/Source VBUS.
> If our chip does not support power path, i.e. Sink & Source are controlled by other charger IC. Our chip will do nothing while setting these commands.
> In the future TCPCI driver, will a framework be applied to notify these events. i.g. power_supply or notifier.
>
I would think so.

Note that the driver is, at this point, fair game to change to make it work with your chip. The only condition is that a standard chip should still work, ie you should not make any changes which would cause the driver to _only_ work with your chip. Everything else is fair game.

Thanks,
Guenter
************* Email Confidentiality Notice ********************

The information contained in this e-mail message (including any attachments) may be confidential, proprietary, privileged, or otherwise exempt from disclosure under applicable laws. It is intended to be conveyed only to the designated recipient(s). Any use, dissemination, distribution, printing, retaining or copying of this e-mail (including its attachments) by unintended recipient(s) is strictly prohibited and may be unlawful. If you are not an intended recipient of this e-mail, or believe that you have received this e-mail in error, please notify the sender immediately (by replying to this e-mail), delete any and all copies of this e-mail (including any attachments) from your system, and do not disclose the content of this e-mail to any other person. Thank you!

2018-01-31 00:31:54