[PATCH v2 0/6] usb: typec: tcpci_rt1711h: Add compatible with rt1715

[PATCH v2 0/6] usb: typec: tcpci_rt1711h: Add compatible with rt1715

[Gene Chen]

This patch series add binding document for rt1711h and compatible driver with
rt1715. Also add different remote rp workaround and initial phy setting.

Gene Chen (2)
 - dt-bindings usb: typec: rt1711h: Add binding for Richtek RT1711H
 - usb: typec: tcpci_rt1711h: Fix vendor setting when set vconn
 - usb: typec: tcpci_rt1711h: Add regulator support when source vbus
 - usb: typec: tcpci_rt1711h: Add initial phy setting
 - usb: typec: tcpci_rt1711h: Add compatible id with rt1715
 - usb: typec: tcpci_rt1711h: Fix CC PHY noise filter of voltage level

 Documentation/devicetree/bindings/usb/richtek,rt1711h.yaml |  100 ++++++++
 drivers/usb/typec/tcpm/tcpci_rt1711h.c                     |  155 ++++++++++++-
 2 files changed, 248 insertions(+), 7 deletions(-)

changelogs between v1 & v2
 - Seperate patch by specific purpose
 - Fix binding document error
 - Set cc change woakaround without using tcpci ops callback

[PATCH v2 1/6] dt-bindings usb: typec: rt1711h: Add binding for Richtek RT1711H

[Gene Chen]

From: Gene Chen <gene_chen@richtek.com>

Add binding for Richtek RT1711H

Signed-off-by: Gene Chen <gene_chen@richtek.com>
---
 .../bindings/usb/richtek,rt1711h.yaml         | 100 ++++++++++++++++++
 1 file changed, 100 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/richtek,rt1711h.yaml

diff --git a/Documentation/devicetree/bindings/usb/richtek,rt1711h.yaml b/Documentation/devicetree/bindings/usb/richtek,rt1711h.yaml
new file mode 100644
index 000000000000..1999f614c89b
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/richtek,rt1711h.yaml
@@ -0,0 +1,100 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/usb/richtek,rt1711h.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Richtek RT1711H Type-C Port Switch and Power Delivery controller
+
+maintainers:
+  - Gene Chen <gene_chen@richtek.com>
+
+description: |
+  The RT1711H is a USB Type-C controller that complies with the latest
+  USB Type-C and PD standards. It does the USB Type-C detection including attach
+  and orientation. It integrates the physical layer of the USB BMC power
+  delivery protocol to allow up to 100W of power. The BMC PD block enables full
+  support for alternative interfaces of the Type-C specification.
+
+properties:
+  compatible:
+    enum:
+      - richtek,rt1711h
+      - richtek,rt1715
+    description:
+      RT1711H support PD20, RT1715 support PD30 except Fast Role Swap.
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  wakeup-source:
+    type: boolean
+
+  connector:
+    type: object
+    $ref: /schemas/connector/usb-connector.yaml#
+    description:
+      Properties for usb c connector.
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - connector
+  - interrupts
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/usb/pd.h>
+    i2c0 {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      rt1711h@4e {
+        compatible = "richtek,rt1711h";
+        reg = <0x4e>;
+        interrupts-extended = <&gpio26 3 IRQ_TYPE_LEVEL_LOW>;
+        wakeup-source;
+
+        connector {
+          compatible = "usb-c-connector";
+          label = "USB-C";
+          data-role = "dual";
+          power-role = "dual";
+          try-power-role = "sink";
+          source-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_DUAL_ROLE | PDO_FIXED_DATA_SWAP)>;
+          sink-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_DUAL_ROLE | PDO_FIXED_DATA_SWAP)>;
+          op-sink-microwatt = <10000000>;
+
+          ports {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            port@0 {
+              reg = <0>;
+              endpoint {
+                remote-endpoint = <&usb_hs>;
+              };
+            };
+            port@1 {
+              reg = <1>;
+              endpoint {
+                remote-endpoint = <&usb_ss>;
+              };
+            };
+            port@2 {
+              reg = <2>;
+              endpoint {
+                remote-endpoint = <&dp_aux>;
+              };
+            };
+          };
+        };
+      };
+    };
+...
-- 
2.25.1

[PATCH v2 2/6] usb: typec: tcpci_rt1711h: Fix vendor setting when set vconn

[Gene Chen]

From: Gene Chen <gene_chen@richtek.com>

replace overwrite whole register with update bits

Signed-off-by: Gene Chen <gene_chen@richtek.com>
---
 drivers/usb/typec/tcpm/tcpci_rt1711h.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpci_rt1711h.c b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
index b56a0880a044..3309ceace2b2 100644
--- a/drivers/usb/typec/tcpm/tcpci_rt1711h.c
+++ b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
@@ -23,6 +23,7 @@
 #define RT1711H_RTCTRL8_SET(ck300, ship_off, auto_idle, tout) \
 			    (((ck300) << 7) | ((ship_off) << 5) | \
 			    ((auto_idle) << 3) | ((tout) & 0x07))
+#define RT1711H_AUTOIDLEEN_MASK	BIT(3)
 
 #define RT1711H_RTCTRL11	0x9E
 
@@ -109,8 +110,8 @@ static int rt1711h_set_vconn(struct tcpci *tcpci, struct tcpci_data *tdata,
 {
 	struct rt1711h_chip *chip = tdata_to_rt1711h(tdata);
 
-	return rt1711h_write8(chip, RT1711H_RTCTRL8,
-			      RT1711H_RTCTRL8_SET(0, 1, !enable, 2));
+	return regmap_update_bits(chip->data.regmap, RT1711H_RTCTRL8,
+				  RT1711H_AUTOIDLEEN_MASK, enable ? 0 : 0xFF);
 }
 
 static int rt1711h_start_drp_toggling(struct tcpci *tcpci,
-- 
2.25.1

[PATCH v2 3/6] usb: typec: tcpci_rt1711h: Add regulator support when source vbus

[Gene Chen]

From: Gene Chen <gene_chen@richtek.com>

Add regulator support when source vbus

Signed-off-by: Gene Chen <gene_chen@richtek.com>
---
 drivers/usb/typec/tcpm/tcpci_rt1711h.c | 28 ++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/usb/typec/tcpm/tcpci_rt1711h.c b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
index 3309ceace2b2..52c9594e531d 100644
--- a/drivers/usb/typec/tcpm/tcpci_rt1711h.c
+++ b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
@@ -10,6 +10,7 @@
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/gpio/consumer.h>
+#include <linux/regulator/consumer.h>
 #include <linux/usb/tcpm.h>
 #include <linux/regmap.h>
 #include "tcpci.h"
@@ -40,6 +41,8 @@ struct rt1711h_chip {
 	struct tcpci_data data;
 	struct tcpci *tcpci;
 	struct device *dev;
+	struct regulator *vbus;
+	bool src_en;
 };
 
 static int rt1711h_read16(struct rt1711h_chip *chip, unsigned int reg, u16 *val)
@@ -103,6 +106,26 @@ static int rt1711h_init(struct tcpci *tcpci, struct tcpci_data *tdata)
 
 	/* dcSRC.DRP : 33% */
 	return rt1711h_write16(chip, RT1711H_RTCTRL16, 330);
+
+}
+
+static int rt1711h_set_vbus(struct tcpci *tcpci, struct tcpci_data *tdata,
+			    bool src, bool snk)
+{
+	struct rt1711h_chip *chip = tdata_to_rt1711h(tdata);
+	int ret;
+
+	if (chip->src_en == src)
+		return 1;
+
+	if (src)
+		ret = regulator_enable(chip->vbus);
+	else
+		ret = regulator_disable(chip->vbus);
+
+	if (!ret)
+		chip->src_en = src;
+	return ret ? ret : 1;
 }
 
 static int rt1711h_set_vconn(struct tcpci *tcpci, struct tcpci_data *tdata,
@@ -246,7 +269,12 @@ static int rt1711h_probe(struct i2c_client *client,
 	if (ret < 0)
 		return ret;
 
+	chip->vbus = devm_regulator_get(&client->dev, "vbus");
+	if (IS_ERR(chip->vbus))
+		return PTR_ERR(chip->vbus);
+
 	chip->data.init = rt1711h_init;
+	chip->data.set_vbus = rt1711h_set_vbus;
 	chip->data.set_vconn = rt1711h_set_vconn;
 	chip->data.start_drp_toggling = rt1711h_start_drp_toggling;
 	chip->tcpci = tcpci_register_port(chip->dev, &chip->data);
-- 
2.25.1

[PATCH v2 4/6] usb: typec: tcpci_rt1711h: Add initial phy setting

[Gene Chen]

From: Gene Chen <gene_chen@richtek.com>

Add initial phy setting about phy dicard retry,
rx filter deglitech time and BMC-encoded wait time

Signed-off-by: Gene Chen <gene_chen@richtek.com>
---
 drivers/usb/typec/tcpm/tcpci_rt1711h.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/tcpm/tcpci_rt1711h.c b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
index 52c9594e531d..e65b568959e9 100644
--- a/drivers/usb/typec/tcpm/tcpci_rt1711h.c
+++ b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
@@ -18,6 +18,9 @@
 #define RT1711H_VID		0x29CF
 #define RT1711H_PID		0x1711
 
+#define RT1711H_PHYCTRL1	0x80
+#define RT1711H_PHYCTRL2	0x81
+
 #define RT1711H_RTCTRL8		0x9B
 
 /* Autoidle timeout = (tout * 2 + 1) * 6.4ms */
@@ -105,8 +108,18 @@ static int rt1711h_init(struct tcpci *tcpci, struct tcpci_data *tdata)
 		return ret;
 
 	/* dcSRC.DRP : 33% */
-	return rt1711h_write16(chip, RT1711H_RTCTRL16, 330);
+	ret = rt1711h_write16(chip, RT1711H_RTCTRL16, 330);
+	if (ret < 0)
+		return ret;
+
+	/* Enable phy discard retry, retry count 7, rx filter deglitech 100 us */
+	ret = rt1711h_write8(chip, RT1711H_PHYCTRL1, 0xF1);
+	if (ret < 0)
+		return ret;
 
+	/* Decrease wait time of BMC-encoded 1 bit from 2.67us to 2.55us */
+	/* wait time : (val * .4167) us */
+	return rt1711h_write8(chip, RT1711H_PHYCTRL2, 62);
 }
 
 static int rt1711h_set_vbus(struct tcpci *tcpci, struct tcpci_data *tdata,
-- 
2.25.1

[PATCH v2 5/6] usb: typec: tcpci_rt1711h: Add compatible id with rt1715

[Gene Chen]

From: Gene Chen <gene_chen@richtek.com>

Add compatible id with rt1715, and add initial setting for
specific support PD30 command.

Signed-off-by: Gene Chen <gene_chen@richtek.com>
---
 drivers/usb/typec/tcpm/tcpci_rt1711h.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpci_rt1711h.c b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
index e65b568959e9..3316dfaeee0d 100644
--- a/drivers/usb/typec/tcpm/tcpci_rt1711h.c
+++ b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
@@ -17,6 +17,7 @@
 
 #define RT1711H_VID		0x29CF
 #define RT1711H_PID		0x1711
+#define RT1715_DID		0x2173
 
 #define RT1711H_PHYCTRL1	0x80
 #define RT1711H_PHYCTRL2	0x81
@@ -28,6 +29,7 @@
 			    (((ck300) << 7) | ((ship_off) << 5) | \
 			    ((auto_idle) << 3) | ((tout) & 0x07))
 #define RT1711H_AUTOIDLEEN_MASK	BIT(3)
+#define RT1711H_ENEXTMSG_MASK	BIT(4)
 
 #define RT1711H_RTCTRL11	0x9E
 
@@ -46,6 +48,7 @@ struct rt1711h_chip {
 	struct device *dev;
 	struct regulator *vbus;
 	bool src_en;
+	u16 did;
 };
 
 static int rt1711h_read16(struct rt1711h_chip *chip, unsigned int reg, u16 *val)
@@ -82,8 +85,9 @@ static struct rt1711h_chip *tdata_to_rt1711h(struct tcpci_data *tdata)
 
 static int rt1711h_init(struct tcpci *tcpci, struct tcpci_data *tdata)
 {
-	int ret;
 	struct rt1711h_chip *chip = tdata_to_rt1711h(tdata);
+	struct regmap *regmap = chip->data.regmap;
+	int ret;
 
 	/* CK 300K from 320K, shipping off, auto_idle enable, tout = 32ms */
 	ret = rt1711h_write8(chip, RT1711H_RTCTRL8,
@@ -91,6 +95,14 @@ static int rt1711h_init(struct tcpci *tcpci, struct tcpci_data *tdata)
 	if (ret < 0)
 		return ret;
 
+	/* Enable PD30 extended message for RT1715 */
+	if (chip->did == RT1715_DID) {
+		ret = regmap_update_bits(regmap, RT1711H_RTCTRL8,
+					 RT1711H_ENEXTMSG_MASK, 0xFF);
+		if (ret < 0)
+			return ret;
+	}
+
 	/* I2C reset : (val + 1) * 12.5ms */
 	ret = rt1711h_write8(chip, RT1711H_RTCTRL11,
 			     RT1711H_RTCTRL11_SET(1, 0x0F));
@@ -246,7 +258,11 @@ static int rt1711h_check_revision(struct i2c_client *i2c)
 		dev_err(&i2c->dev, "pid is not correct, 0x%04x\n", ret);
 		return -ENODEV;
 	}
-	return 0;
+	ret = i2c_smbus_read_word_data(i2c, TCPC_BCD_DEV);
+	if (ret < 0)
+		return ret;
+	dev_info(&i2c->dev, "did is 0x%04x\n", ret);
+	return ret;
 }
 
 static int rt1711h_probe(struct i2c_client *client,
@@ -265,6 +281,8 @@ static int rt1711h_probe(struct i2c_client *client,
 	if (!chip)
 		return -ENOMEM;
 
+	chip->did = ret;
+
 	chip->data.regmap = devm_regmap_init_i2c(client,
 						 &rt1711h_regmap_config);
 	if (IS_ERR(chip->data.regmap))
@@ -315,6 +333,7 @@ static int rt1711h_remove(struct i2c_client *client)
 
 static const struct i2c_device_id rt1711h_id[] = {
 	{ "rt1711h", 0 },
+	{ "rt1715", 0 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, rt1711h_id);
@@ -322,6 +341,7 @@ MODULE_DEVICE_TABLE(i2c, rt1711h_id);
 #ifdef CONFIG_OF
 static const struct of_device_id rt1711h_of_match[] = {
 	{ .compatible = "richtek,rt1711h", },
+	{ .compatible = "richtek,rt1715", },
 	{},
 };
 MODULE_DEVICE_TABLE(of, rt1711h_of_match);
-- 
2.25.1

[PATCH v2 6/6] usb: typec: tcpci_rt1711h: Fix CC PHY noise filter of voltage level

[Gene Chen]

From: Gene Chen <gene_chen@richtek.com>

Fix CC PHY noise filter of voltage level according to
current cc voltage level

Signed-off-by: Gene Chen <gene_chen@richtek.com>
---
 drivers/usb/typec/tcpm/tcpci_rt1711h.c | 83 +++++++++++++++++++++++++-
 1 file changed, 81 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpci_rt1711h.c b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
index 3316dfaeee0d..f0c46bf7f00b 100644
--- a/drivers/usb/typec/tcpm/tcpci_rt1711h.c
+++ b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
@@ -22,8 +22,11 @@
 #define RT1711H_PHYCTRL1	0x80
 #define RT1711H_PHYCTRL2	0x81
 
-#define RT1711H_RTCTRL8		0x9B
+#define RT1711H_RTCTRL4		0x93
+/* rx threshold of rd/rp: 1b0 for level 0.4V/0.7V, 1b1 for 0.35V/0.75V */
+#define RT1711H_BMCIO_RXDZSEL	BIT(0)
 
+#define RT1711H_RTCTRL8		0x9B
 /* Autoidle timeout = (tout * 2 + 1) * 6.4ms */
 #define RT1711H_RTCTRL8_SET(ck300, ship_off, auto_idle, tout) \
 			    (((ck300) << 7) | ((ship_off) << 5) | \
@@ -32,7 +35,6 @@
 #define RT1711H_ENEXTMSG_MASK	BIT(4)
 
 #define RT1711H_RTCTRL11	0x9E
-
 /* I2C timeout = (tout + 1) * 12.5ms */
 #define RT1711H_RTCTRL11_SET(en, tout) \
 			     (((en) << 7) | ((tout) & 0x0F))
@@ -42,6 +44,10 @@
 #define RT1711H_RTCTRL15	0xA2
 #define RT1711H_RTCTRL16	0xA3
 
+#define RT1711H_RTCTRL18	0xAF
+/* 1b0 as fixed rx threshold of rd/rp 0.55V, 1b1 depends on RTCRTL4[0] */
+#define BMCIO_RXDZEN_MASK	BIT(0)
+
 struct rt1711h_chip {
 	struct tcpci_data data;
 	struct tcpci *tcpci;
@@ -162,6 +168,77 @@ static int rt1711h_set_vconn(struct tcpci *tcpci, struct tcpci_data *tdata,
 				  RT1711H_AUTOIDLEEN_MASK, enable ? 0 : 0xFF);
 }
 
+#define tcpc_presenting_rd(reg, cc) \
+	(!(TCPC_ROLE_CTRL_DRP & (reg)) && \
+	 (((reg) & (TCPC_ROLE_CTRL_## cc ##_MASK << TCPC_ROLE_CTRL_## cc ##_SHIFT)) == \
+	  (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_## cc ##_SHIFT)))
+
+static enum typec_cc_status tcpci_to_typec_cc(unsigned int cc, bool sink)
+{
+	switch (cc) {
+	case 0x1:
+		return sink ? TYPEC_CC_RP_DEF : TYPEC_CC_RA;
+	case 0x2:
+		return sink ? TYPEC_CC_RP_1_5 : TYPEC_CC_RD;
+	case 0x3:
+		if (sink)
+			return TYPEC_CC_RP_3_0;
+		fallthrough;
+	case 0x0:
+	default:
+		return TYPEC_CC_OPEN;
+	}
+}
+
+/*
+ * Selects the CC PHY noise filter voltage level according to the current
+ * CC voltage level.
+ *
+ * @param cc_level The CC voltage level for the port's current role
+ * @return 0 if writes succeed; failure code otherwise
+ */
+static inline int rt1711h_init_cc_params(struct rt1711h_chip *chip, u8 status)
+{
+	int ret, cc1, cc2;
+	u8 role = 0;
+	u32 rxdz_en = 0, rxdz_sel = 0;
+
+	ret = rt1711h_read8(chip, TCPC_ROLE_CTRL, &role);
+	if (ret < 0)
+		return ret;
+
+	cc1 = tcpci_to_typec_cc((status >> TCPC_CC_STATUS_CC1_SHIFT) &
+				TCPC_CC_STATUS_CC1_MASK,
+				status & TCPC_CC_STATUS_TERM ||
+				tcpc_presenting_rd(role, CC1));
+	cc2 = tcpci_to_typec_cc((status >> TCPC_CC_STATUS_CC2_SHIFT) &
+				TCPC_CC_STATUS_CC2_MASK,
+				status & TCPC_CC_STATUS_TERM ||
+				tcpc_presenting_rd(role, CC2));
+
+	if ((cc1 >= TYPEC_CC_RP_1_5 && cc2 < TYPEC_CC_RP_DEF) ||
+	    (cc2 >= TYPEC_CC_RP_1_5 && cc1 < TYPEC_CC_RP_DEF)) {
+		if (chip->did == RT1715_DID) {
+			rxdz_en = 1;
+			rxdz_sel = 1;
+		} else {
+			rxdz_en = 1;
+			rxdz_sel = 0;
+		}
+	} else {
+		rxdz_en = 0;
+		rxdz_sel = 1;
+	}
+
+	ret = regmap_update_bits(chip->data.regmap, RT1711H_RTCTRL18,
+				 BMCIO_RXDZEN_MASK, rxdz_en);
+	if (ret < 0)
+		return ret;
+
+	return regmap_update_bits(chip->data.regmap, RT1711H_RTCTRL4,
+				  RT1711H_BMCIO_RXDZSEL, rxdz_sel);
+}
+
 static int rt1711h_start_drp_toggling(struct tcpci *tcpci,
 				      struct tcpci_data *tdata,
 				      enum typec_cc_status cc)
@@ -222,6 +299,8 @@ static irqreturn_t rt1711h_irq(int irq, void *dev_id)
 		/* Clear cc change event triggered by starting toggling */
 		if (status & TCPC_CC_STATUS_TOGGLING)
 			rt1711h_write8(chip, TCPC_ALERT, TCPC_ALERT_CC_STATUS);
+		else
+			rt1711h_init_cc_params(chip, status);
 	}
 
 out:
-- 
2.25.1

Re: [PATCH v2 3/6] usb: typec: tcpci_rt1711h: Add regulator support when source vbus

[Guenter Roeck]

On 7/20/22 23:11, Gene Chen wrote:
> From: Gene Chen <gene_chen@richtek.com>
> 
> Add regulator support when source vbus
> 
> Signed-off-by: Gene Chen <gene_chen@richtek.com>
> ---
>   drivers/usb/typec/tcpm/tcpci_rt1711h.c | 28 ++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpci_rt1711h.c b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> index 3309ceace2b2..52c9594e531d 100644
> --- a/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> +++ b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> @@ -10,6 +10,7 @@
>   #include <linux/i2c.h>
>   #include <linux/interrupt.h>
>   #include <linux/gpio/consumer.h>
> +#include <linux/regulator/consumer.h>
>   #include <linux/usb/tcpm.h>
>   #include <linux/regmap.h>
>   #include "tcpci.h"
> @@ -40,6 +41,8 @@ struct rt1711h_chip {
>   	struct tcpci_data data;
>   	struct tcpci *tcpci;
>   	struct device *dev;
> +	struct regulator *vbus;
> +	bool src_en;
>   };
>   
>   static int rt1711h_read16(struct rt1711h_chip *chip, unsigned int reg, u16 *val)
> @@ -103,6 +106,26 @@ static int rt1711h_init(struct tcpci *tcpci, struct tcpci_data *tdata)
>   
>   	/* dcSRC.DRP : 33% */
>   	return rt1711h_write16(chip, RT1711H_RTCTRL16, 330);
> +
> +}
> +
> +static int rt1711h_set_vbus(struct tcpci *tcpci, struct tcpci_data *tdata,
> +			    bool src, bool snk)
> +{
> +	struct rt1711h_chip *chip = tdata_to_rt1711h(tdata);
> +	int ret;
> +
> +	if (chip->src_en == src)
> +		return 1;
> +
> +	if (src)
> +		ret = regulator_enable(chip->vbus);
> +	else
> +		ret = regulator_disable(chip->vbus);
> +
> +	if (!ret)
> +		chip->src_en = src;
> +	return ret ? ret : 1;

Are you sure this is what you want ? Returning 1 bypasses the code setting
the vbus registers in tcpci.c. If that is on purpose it might make sense
to explain it.

>   }
>   
>   static int rt1711h_set_vconn(struct tcpci *tcpci, struct tcpci_data *tdata,
> @@ -246,7 +269,12 @@ static int rt1711h_probe(struct i2c_client *client,
>   	if (ret < 0)
>   		return ret;
>   
> +	chip->vbus = devm_regulator_get(&client->dev, "vbus");
> +	if (IS_ERR(chip->vbus))
> +		return PTR_ERR(chip->vbus);
> +

This makes regulator support mandatory, which so far was not the case.
That warrants an explanation why it is not a problem for existing users.

Thanks,
Guenter

>   	chip->data.init = rt1711h_init;
> +	chip->data.set_vbus = rt1711h_set_vbus;
>   	chip->data.set_vconn = rt1711h_set_vconn;
>   	chip->data.start_drp_toggling = rt1711h_start_drp_toggling;
>   	chip->tcpci = tcpci_register_port(chip->dev, &chip->data);

Re: [PATCH v2 2/6] usb: typec: tcpci_rt1711h: Fix vendor setting when set vconn

[Guenter Roeck]

On 7/20/22 23:11, Gene Chen wrote:
> From: Gene Chen <gene_chen@richtek.com>
> 
> replace overwrite whole register with update bits
> 
> Signed-off-by: Gene Chen <gene_chen@richtek.com>
> ---
>   drivers/usb/typec/tcpm/tcpci_rt1711h.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpci_rt1711h.c b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> index b56a0880a044..3309ceace2b2 100644
> --- a/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> +++ b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> @@ -23,6 +23,7 @@
>   #define RT1711H_RTCTRL8_SET(ck300, ship_off, auto_idle, tout) \
>   			    (((ck300) << 7) | ((ship_off) << 5) | \
>   			    ((auto_idle) << 3) | ((tout) & 0x07))
> +#define RT1711H_AUTOIDLEEN_MASK	BIT(3)

Simple RT1711H_AUTOIDLEEN would be sufficient. Also, when using BIT(),
include the necessary include file.

>   
>   #define RT1711H_RTCTRL11	0x9E
>   
> @@ -109,8 +110,8 @@ static int rt1711h_set_vconn(struct tcpci *tcpci, struct tcpci_data *tdata,
>   {
>   	struct rt1711h_chip *chip = tdata_to_rt1711h(tdata);
>   
> -	return rt1711h_write8(chip, RT1711H_RTCTRL8,
> -			      RT1711H_RTCTRL8_SET(0, 1, !enable, 2));
> +	return regmap_update_bits(chip->data.regmap, RT1711H_RTCTRL8,
> +				  RT1711H_AUTOIDLEEN_MASK, enable ? 0 : 0xFF);

I would suggest
	return regmap_update_bits(chip->data.regmap, RT1711H_RTCTRL8,
				  RT1711H_AUTOIDLEEN, enable ? 0 : RT1711H_AUTOIDLEEN);

to avoid confusion why 0xFF is used.

Thanks,
Guenter

>   }
>   
>   static int rt1711h_start_drp_toggling(struct tcpci *tcpci,

Re: [PATCH v2 4/6] usb: typec: tcpci_rt1711h: Add initial phy setting

[Guenter Roeck]

On 7/20/22 23:11, Gene Chen wrote:
> From: Gene Chen <gene_chen@richtek.com>
> 
> Add initial phy setting about phy dicard retry,
> rx filter deglitech time and BMC-encoded wait time
> 
> Signed-off-by: Gene Chen <gene_chen@richtek.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/usb/typec/tcpm/tcpci_rt1711h.c | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpci_rt1711h.c b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> index 52c9594e531d..e65b568959e9 100644
> --- a/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> +++ b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> @@ -18,6 +18,9 @@
>   #define RT1711H_VID		0x29CF
>   #define RT1711H_PID		0x1711
>   
> +#define RT1711H_PHYCTRL1	0x80
> +#define RT1711H_PHYCTRL2	0x81
> +
>   #define RT1711H_RTCTRL8		0x9B
>   
>   /* Autoidle timeout = (tout * 2 + 1) * 6.4ms */
> @@ -105,8 +108,18 @@ static int rt1711h_init(struct tcpci *tcpci, struct tcpci_data *tdata)
>   		return ret;
>   
>   	/* dcSRC.DRP : 33% */
> -	return rt1711h_write16(chip, RT1711H_RTCTRL16, 330);
> +	ret = rt1711h_write16(chip, RT1711H_RTCTRL16, 330);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Enable phy discard retry, retry count 7, rx filter deglitech 100 us */
> +	ret = rt1711h_write8(chip, RT1711H_PHYCTRL1, 0xF1);
> +	if (ret < 0)
> +		return ret;
>   
> +	/* Decrease wait time of BMC-encoded 1 bit from 2.67us to 2.55us */
> +	/* wait time : (val * .4167) us */
> +	return rt1711h_write8(chip, RT1711H_PHYCTRL2, 62);
>   }
>   
>   static int rt1711h_set_vbus(struct tcpci *tcpci, struct tcpci_data *tdata,

Re: [PATCH v2 5/6] usb: typec: tcpci_rt1711h: Add compatible id with rt1715

[Guenter Roeck]

On 7/20/22 23:11, Gene Chen wrote:
> From: Gene Chen <gene_chen@richtek.com>
> 
> Add compatible id with rt1715, and add initial setting for
> specific support PD30 command.
> 
> Signed-off-by: Gene Chen <gene_chen@richtek.com>
> ---
>   drivers/usb/typec/tcpm/tcpci_rt1711h.c | 24 ++++++++++++++++++++++--
>   1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpci_rt1711h.c b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> index e65b568959e9..3316dfaeee0d 100644
> --- a/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> +++ b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> @@ -17,6 +17,7 @@
>   
>   #define RT1711H_VID		0x29CF
>   #define RT1711H_PID		0x1711
> +#define RT1715_DID		0x2173
>   
>   #define RT1711H_PHYCTRL1	0x80
>   #define RT1711H_PHYCTRL2	0x81
> @@ -28,6 +29,7 @@
>   			    (((ck300) << 7) | ((ship_off) << 5) | \
>   			    ((auto_idle) << 3) | ((tout) & 0x07))
>   #define RT1711H_AUTOIDLEEN_MASK	BIT(3)
> +#define RT1711H_ENEXTMSG_MASK	BIT(4)

I would suggest to drop _MASK.

>   
>   #define RT1711H_RTCTRL11	0x9E
>   
> @@ -46,6 +48,7 @@ struct rt1711h_chip {
>   	struct device *dev;
>   	struct regulator *vbus;
>   	bool src_en;
> +	u16 did;
>   };
>   
>   static int rt1711h_read16(struct rt1711h_chip *chip, unsigned int reg, u16 *val)
> @@ -82,8 +85,9 @@ static struct rt1711h_chip *tdata_to_rt1711h(struct tcpci_data *tdata)
>   
>   static int rt1711h_init(struct tcpci *tcpci, struct tcpci_data *tdata)
>   {
> -	int ret;
>   	struct rt1711h_chip *chip = tdata_to_rt1711h(tdata);
> +	struct regmap *regmap = chip->data.regmap;
> +	int ret;
>   
>   	/* CK 300K from 320K, shipping off, auto_idle enable, tout = 32ms */
>   	ret = rt1711h_write8(chip, RT1711H_RTCTRL8,
> @@ -91,6 +95,14 @@ static int rt1711h_init(struct tcpci *tcpci, struct tcpci_data *tdata)
>   	if (ret < 0)
>   		return ret;
>   
> +	/* Enable PD30 extended message for RT1715 */
> +	if (chip->did == RT1715_DID) {
> +		ret = regmap_update_bits(regmap, RT1711H_RTCTRL8,
> +					 RT1711H_ENEXTMSG_MASK, 0xFF);

0xFF -> RT1711H_ENEXTMSG

> +		if (ret < 0)
> +			return ret;
> +	}
> +
>   	/* I2C reset : (val + 1) * 12.5ms */
>   	ret = rt1711h_write8(chip, RT1711H_RTCTRL11,
>   			     RT1711H_RTCTRL11_SET(1, 0x0F));
> @@ -246,7 +258,11 @@ static int rt1711h_check_revision(struct i2c_client *i2c)
>   		dev_err(&i2c->dev, "pid is not correct, 0x%04x\n", ret);
>   		return -ENODEV;
>   	}
> -	return 0;
> +	ret = i2c_smbus_read_word_data(i2c, TCPC_BCD_DEV);
> +	if (ret < 0)
> +		return ret;
> +	dev_info(&i2c->dev, "did is 0x%04x\n", ret);

Unnecessary noise. If needed for testing, please make it dev_dbg.

> +	return ret;

I think it would make sense to pass chip as parameter and set chip->did here.

Also, validation is missing. This function is supposed to check/validate
revision data, but it just accepts all DIDs. Then later DID values are used
to distinguish functionality. At the same time, the new device ID and OF
compatible strings are not used for that purpose and thus have no real value.

Since there can be chips with different DIDs which require different functionality,
DID values should be validated, and only chips with supported DIDs should be
accepted. Also, since there are separate device IDs and devicetree compatible
properties, the DIDs associated with supported chips should be referenced there.

Thanks,
Guenter

>   }
>   
>   static int rt1711h_probe(struct i2c_client *client,
> @@ -265,6 +281,8 @@ static int rt1711h_probe(struct i2c_client *client,
>   	if (!chip)
>   		return -ENOMEM;
>   
> +	chip->did = ret;
> +
>   	chip->data.regmap = devm_regmap_init_i2c(client,
>   						 &rt1711h_regmap_config);
>   	if (IS_ERR(chip->data.regmap))
> @@ -315,6 +333,7 @@ static int rt1711h_remove(struct i2c_client *client)
>   
>   static const struct i2c_device_id rt1711h_id[] = {
>   	{ "rt1711h", 0 },
> +	{ "rt1715", 0 },
>   	{ }
>   };
>   MODULE_DEVICE_TABLE(i2c, rt1711h_id);
> @@ -322,6 +341,7 @@ MODULE_DEVICE_TABLE(i2c, rt1711h_id);
>   #ifdef CONFIG_OF
>   static const struct of_device_id rt1711h_of_match[] = {
>   	{ .compatible = "richtek,rt1711h", },
> +	{ .compatible = "richtek,rt1715", },
>   	{},
>   };
>   MODULE_DEVICE_TABLE(of, rt1711h_of_match);

Re: [PATCH v2 6/6] usb: typec: tcpci_rt1711h: Fix CC PHY noise filter of voltage level

[Guenter Roeck]

On 7/20/22 23:11, Gene Chen wrote:
> From: Gene Chen <gene_chen@richtek.com>
> 
> Fix CC PHY noise filter of voltage level according to
> current cc voltage level
> 
> Signed-off-by: Gene Chen <gene_chen@richtek.com>
> ---
>   drivers/usb/typec/tcpm/tcpci_rt1711h.c | 83 +++++++++++++++++++++++++-
>   1 file changed, 81 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpci_rt1711h.c b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> index 3316dfaeee0d..f0c46bf7f00b 100644
> --- a/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> +++ b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> @@ -22,8 +22,11 @@
>   #define RT1711H_PHYCTRL1	0x80
>   #define RT1711H_PHYCTRL2	0x81
>   
> -#define RT1711H_RTCTRL8		0x9B
> +#define RT1711H_RTCTRL4		0x93
> +/* rx threshold of rd/rp: 1b0 for level 0.4V/0.7V, 1b1 for 0.35V/0.75V */
> +#define RT1711H_BMCIO_RXDZSEL	BIT(0)
>   
> +#define RT1711H_RTCTRL8		0x9B
>   /* Autoidle timeout = (tout * 2 + 1) * 6.4ms */
>   #define RT1711H_RTCTRL8_SET(ck300, ship_off, auto_idle, tout) \
>   			    (((ck300) << 7) | ((ship_off) << 5) | \
> @@ -32,7 +35,6 @@
>   #define RT1711H_ENEXTMSG_MASK	BIT(4)
>   
>   #define RT1711H_RTCTRL11	0x9E
> -
>   /* I2C timeout = (tout + 1) * 12.5ms */
>   #define RT1711H_RTCTRL11_SET(en, tout) \
>   			     (((en) << 7) | ((tout) & 0x0F))
> @@ -42,6 +44,10 @@
>   #define RT1711H_RTCTRL15	0xA2
>   #define RT1711H_RTCTRL16	0xA3
>   
> +#define RT1711H_RTCTRL18	0xAF
> +/* 1b0 as fixed rx threshold of rd/rp 0.55V, 1b1 depends on RTCRTL4[0] */
> +#define BMCIO_RXDZEN_MASK	BIT(0)

I really dislike the use of _MASK for register bit values.

> +
>   struct rt1711h_chip {
>   	struct tcpci_data data;
>   	struct tcpci *tcpci;
> @@ -162,6 +168,77 @@ static int rt1711h_set_vconn(struct tcpci *tcpci, struct tcpci_data *tdata,
>   				  RT1711H_AUTOIDLEEN_MASK, enable ? 0 : 0xFF);
>   }
>   
> +#define tcpc_presenting_rd(reg, cc) \
> +	(!(TCPC_ROLE_CTRL_DRP & (reg)) && \
> +	 (((reg) & (TCPC_ROLE_CTRL_## cc ##_MASK << TCPC_ROLE_CTRL_## cc ##_SHIFT)) == \
> +	  (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_## cc ##_SHIFT)))
> +
> +static enum typec_cc_status tcpci_to_typec_cc(unsigned int cc, bool sink)
> +{
> +	switch (cc) {
> +	case 0x1:
> +		return sink ? TYPEC_CC_RP_DEF : TYPEC_CC_RA;
> +	case 0x2:
> +		return sink ? TYPEC_CC_RP_1_5 : TYPEC_CC_RD;
> +	case 0x3:
> +		if (sink)
> +			return TYPEC_CC_RP_3_0;
> +		fallthrough;
> +	case 0x0:
> +	default:
> +		return TYPEC_CC_OPEN;
> +	}
> +}
> +
The above is a straight copy from tcpci.c. Can it be moved to
include/linux/usb/tcpci.h ?

> +/*
> + * Selects the CC PHY noise filter voltage level according to the current
> + * CC voltage level.
> + *
> + * @param cc_level The CC voltage level for the port's current role

I seem to be missing that parameter.

> + * @return 0 if writes succeed; failure code otherwise
> + */
> +static inline int rt1711h_init_cc_params(struct rt1711h_chip *chip, u8 status)
> +{
> +	int ret, cc1, cc2;
> +	u8 role = 0;
> +	u32 rxdz_en = 0, rxdz_sel = 0;

Those variables are always set and thus do not need to be initialized.
> +
> +	ret = rt1711h_read8(chip, TCPC_ROLE_CTRL, &role);
> +	if (ret < 0)
> +		return ret;
> +
> +	cc1 = tcpci_to_typec_cc((status >> TCPC_CC_STATUS_CC1_SHIFT) &
> +				TCPC_CC_STATUS_CC1_MASK,
> +				status & TCPC_CC_STATUS_TERM ||
> +				tcpc_presenting_rd(role, CC1));
> +	cc2 = tcpci_to_typec_cc((status >> TCPC_CC_STATUS_CC2_SHIFT) &
> +				TCPC_CC_STATUS_CC2_MASK,
> +				status & TCPC_CC_STATUS_TERM ||
> +				tcpc_presenting_rd(role, CC2));
> +
> +	if ((cc1 >= TYPEC_CC_RP_1_5 && cc2 < TYPEC_CC_RP_DEF) ||
> +	    (cc2 >= TYPEC_CC_RP_1_5 && cc1 < TYPEC_CC_RP_DEF)) {
> +		if (chip->did == RT1715_DID) {
> +			rxdz_en = 1;

This should be
			rxdz_en = BMCIO_RXDZEN;

> +			rxdz_sel = 1;

This should be
			rxdz_sel = RT1711H_BMCIO_RXDZSEL;

> +		} else {
> +			rxdz_en = 1;
> +			rxdz_sel = 0;
> +		}

The assignment of rxdz_en can be moved outside the if/else block.

> +	} else {
> +		rxdz_en = 0;
> +		rxdz_sel = 1;
> +	}
> +
> +	ret = regmap_update_bits(chip->data.regmap, RT1711H_RTCTRL18,
> +				 BMCIO_RXDZEN_MASK, rxdz_en);
> +	if (ret < 0)
> +		return ret;
> +
> +	return regmap_update_bits(chip->data.regmap, RT1711H_RTCTRL4,
> +				  RT1711H_BMCIO_RXDZSEL, rxdz_sel);
> +}
> +
>   static int rt1711h_start_drp_toggling(struct tcpci *tcpci,
>   				      struct tcpci_data *tdata,
>   				      enum typec_cc_status cc)
> @@ -222,6 +299,8 @@ static irqreturn_t rt1711h_irq(int irq, void *dev_id)
>   		/* Clear cc change event triggered by starting toggling */
>   		if (status & TCPC_CC_STATUS_TOGGLING)
>   			rt1711h_write8(chip, TCPC_ALERT, TCPC_ALERT_CC_STATUS);
> +		else
> +			rt1711h_init_cc_params(chip, status);
>   	}
>   
>   out:

Re: [PATCH v2 3/6] usb: typec: tcpci_rt1711h: Add regulator support when source vbus

[Gene Chen]

Guenter Roeck <linux@roeck-us.net> 於 2022年7月21日 週四 晚上10:28寫道：
>
> On 7/20/22 23:11, Gene Chen wrote:
> > From: Gene Chen <gene_chen@richtek.com>
> >
> > Add regulator support when source vbus
> >
> > Signed-off-by: Gene Chen <gene_chen@richtek.com>
> > ---
> >   drivers/usb/typec/tcpm/tcpci_rt1711h.c | 28 ++++++++++++++++++++++++++
> >   1 file changed, 28 insertions(+)
> >
> > diff --git a/drivers/usb/typec/tcpm/tcpci_rt1711h.c b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> > index 3309ceace2b2..52c9594e531d 100644
> > --- a/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> > +++ b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> > @@ -10,6 +10,7 @@
> >   #include <linux/i2c.h>
> >   #include <linux/interrupt.h>
> >   #include <linux/gpio/consumer.h>
> > +#include <linux/regulator/consumer.h>
> >   #include <linux/usb/tcpm.h>
> >   #include <linux/regmap.h>
> >   #include "tcpci.h"
> > @@ -40,6 +41,8 @@ struct rt1711h_chip {
> >       struct tcpci_data data;
> >       struct tcpci *tcpci;
> >       struct device *dev;
> > +     struct regulator *vbus;
> > +     bool src_en;
> >   };
> >
> >   static int rt1711h_read16(struct rt1711h_chip *chip, unsigned int reg, u16 *val)
> > @@ -103,6 +106,26 @@ static int rt1711h_init(struct tcpci *tcpci, struct tcpci_data *tdata)
> >
> >       /* dcSRC.DRP : 33% */
> >       return rt1711h_write16(chip, RT1711H_RTCTRL16, 330);
> > +
> > +}
> > +
> > +static int rt1711h_set_vbus(struct tcpci *tcpci, struct tcpci_data *tdata,
> > +                         bool src, bool snk)
> > +{
> > +     struct rt1711h_chip *chip = tdata_to_rt1711h(tdata);
> > +     int ret;
> > +
> > +     if (chip->src_en == src)
> > +             return 1;
> > +
> > +     if (src)
> > +             ret = regulator_enable(chip->vbus);
> > +     else
> > +             ret = regulator_disable(chip->vbus);
> > +
> > +     if (!ret)
> > +             chip->src_en = src;
> > +     return ret ? ret : 1;
>
> Are you sure this is what you want ? Returning 1 bypasses the code setting
> the vbus registers in tcpci.c. If that is on purpose it might make sense
> to explain it.
>

ACK, return 0 is more compatible with next generation chip,
and writing tcpci vbus command won't affect to ic if not supported.

> >   }
> >
> >   static int rt1711h_set_vconn(struct tcpci *tcpci, struct tcpci_data *tdata,
> > @@ -246,7 +269,12 @@ static int rt1711h_probe(struct i2c_client *client,
> >       if (ret < 0)
> >               return ret;
> >
> > +     chip->vbus = devm_regulator_get(&client->dev, "vbus");
> > +     if (IS_ERR(chip->vbus))
> > +             return PTR_ERR(chip->vbus);
> > +
>
> This makes regulator support mandatory, which so far was not the case.
> That warrants an explanation why it is not a problem for existing users.
>

We verified ic behavior as SNK only, because we couldn't add tcpci set
vbus callback and external boost otg vbus.
And we use our own type-c state machine and pd policy engine for mass
production to user.

> Thanks,
> Guenter
>
> >       chip->data.init = rt1711h_init;
> > +     chip->data.set_vbus = rt1711h_set_vbus;
> >       chip->data.set_vconn = rt1711h_set_vconn;
> >       chip->data.start_drp_toggling = rt1711h_start_drp_toggling;
> >       chip->tcpci = tcpci_register_port(chip->dev, &chip->data);
>

Re: [PATCH v2 5/6] usb: typec: tcpci_rt1711h: Add compatible id with rt1715

[Gene Chen]

Guenter Roeck <linux@roeck-us.net> 於 2022年7月21日 週四 晚上10:44寫道：
>
> On 7/20/22 23:11, Gene Chen wrote:
> > From: Gene Chen <gene_chen@richtek.com>
> >
> > Add compatible id with rt1715, and add initial setting for
> > specific support PD30 command.
> >
> > Signed-off-by: Gene Chen <gene_chen@richtek.com>
> > ---
> >   drivers/usb/typec/tcpm/tcpci_rt1711h.c | 24 ++++++++++++++++++++++--
> >   1 file changed, 22 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/typec/tcpm/tcpci_rt1711h.c b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> > index e65b568959e9..3316dfaeee0d 100644
> > --- a/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> > +++ b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
> > @@ -17,6 +17,7 @@
> >
> >   #define RT1711H_VID         0x29CF
> >   #define RT1711H_PID         0x1711
> > +#define RT1715_DID           0x2173
> >
> >   #define RT1711H_PHYCTRL1    0x80
> >   #define RT1711H_PHYCTRL2    0x81
> > @@ -28,6 +29,7 @@
> >                           (((ck300) << 7) | ((ship_off) << 5) | \
> >                           ((auto_idle) << 3) | ((tout) & 0x07))
> >   #define RT1711H_AUTOIDLEEN_MASK     BIT(3)
> > +#define RT1711H_ENEXTMSG_MASK        BIT(4)
>
> I would suggest to drop _MASK.
>
> >
> >   #define RT1711H_RTCTRL11    0x9E
> >
> > @@ -46,6 +48,7 @@ struct rt1711h_chip {
> >       struct device *dev;
> >       struct regulator *vbus;
> >       bool src_en;
> > +     u16 did;
> >   };
> >
> >   static int rt1711h_read16(struct rt1711h_chip *chip, unsigned int reg, u16 *val)
> > @@ -82,8 +85,9 @@ static struct rt1711h_chip *tdata_to_rt1711h(struct tcpci_data *tdata)
> >
> >   static int rt1711h_init(struct tcpci *tcpci, struct tcpci_data *tdata)
> >   {
> > -     int ret;
> >       struct rt1711h_chip *chip = tdata_to_rt1711h(tdata);
> > +     struct regmap *regmap = chip->data.regmap;
> > +     int ret;
> >
> >       /* CK 300K from 320K, shipping off, auto_idle enable, tout = 32ms */
> >       ret = rt1711h_write8(chip, RT1711H_RTCTRL8,
> > @@ -91,6 +95,14 @@ static int rt1711h_init(struct tcpci *tcpci, struct tcpci_data *tdata)
> >       if (ret < 0)
> >               return ret;
> >
> > +     /* Enable PD30 extended message for RT1715 */
> > +     if (chip->did == RT1715_DID) {
> > +             ret = regmap_update_bits(regmap, RT1711H_RTCTRL8,
> > +                                      RT1711H_ENEXTMSG_MASK, 0xFF);
>
> 0xFF -> RT1711H_ENEXTMSG
>
> > +             if (ret < 0)
> > +                     return ret;
> > +     }
> > +
> >       /* I2C reset : (val + 1) * 12.5ms */
> >       ret = rt1711h_write8(chip, RT1711H_RTCTRL11,
> >                            RT1711H_RTCTRL11_SET(1, 0x0F));
> > @@ -246,7 +258,11 @@ static int rt1711h_check_revision(struct i2c_client *i2c)
> >               dev_err(&i2c->dev, "pid is not correct, 0x%04x\n", ret);
> >               return -ENODEV;
> >       }
> > -     return 0;
> > +     ret = i2c_smbus_read_word_data(i2c, TCPC_BCD_DEV);
> > +     if (ret < 0)
> > +             return ret;
> > +     dev_info(&i2c->dev, "did is 0x%04x\n", ret);
>
> Unnecessary noise. If needed for testing, please make it dev_dbg.
>
> > +     return ret;
>
> I think it would make sense to pass chip as parameter and set chip->did here.
>
> Also, validation is missing. This function is supposed to check/validate
> revision data, but it just accepts all DIDs. Then later DID values are used
> to distinguish functionality. At the same time, the new device ID and OF
> compatible strings are not used for that purpose and thus have no real value.
>
> Since there can be chips with different DIDs which require different functionality,
> DID values should be validated, and only chips with supported DIDs should be
> accepted. Also, since there are separate device IDs and devicetree compatible
> properties, the DIDs associated with supported chips should be referenced there.
>
> Thanks,
> Guenter
>

ACK, I will add RT1711H_DID, and check did in order to only support
for rt1711h and rt1715.

The difference between RT1711H and RT1715 is that whether PD3.0 is
supported and rx dead zone workaround setting.
PD3.0 is set in pd_version of typec_caps which is fixed while calling
tcpm_register_port.
Therefore, the data of struct of_device_id isn't needed. Should I just
remove the compatible name "rt1715"?

> >   }
> >
> >   static int rt1711h_probe(struct i2c_client *client,
> > @@ -265,6 +281,8 @@ static int rt1711h_probe(struct i2c_client *client,
> >       if (!chip)
> >               return -ENOMEM;
> >
> > +     chip->did = ret;
> > +
> >       chip->data.regmap = devm_regmap_init_i2c(client,
> >                                                &rt1711h_regmap_config);
> >       if (IS_ERR(chip->data.regmap))
> > @@ -315,6 +333,7 @@ static int rt1711h_remove(struct i2c_client *client)
> >
> >   static const struct i2c_device_id rt1711h_id[] = {
> >       { "rt1711h", 0 },
> > +     { "rt1715", 0 },
> >       { }
> >   };
> >   MODULE_DEVICE_TABLE(i2c, rt1711h_id);
> > @@ -322,6 +341,7 @@ MODULE_DEVICE_TABLE(i2c, rt1711h_id);
> >   #ifdef CONFIG_OF
> >   static const struct of_device_id rt1711h_of_match[] = {
> >       { .compatible = "richtek,rt1711h", },
> > +     { .compatible = "richtek,rt1715", },
> >       {},
> >   };
> >   MODULE_DEVICE_TABLE(of, rt1711h_of_match);
>

Re: [PATCH v2 1/6] dt-bindings usb: typec: rt1711h: Add binding for Richtek RT1711H

[Rob Herring]

On Thu, 21 Jul 2022 14:11:39 +0800, Gene Chen wrote:
> From: Gene Chen <gene_chen@richtek.com>
> 
> Add binding for Richtek RT1711H
> 
> Signed-off-by: Gene Chen <gene_chen@richtek.com>
> ---
>  .../bindings/usb/richtek,rt1711h.yaml         | 100 ++++++++++++++++++
>  1 file changed, 100 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/richtek,rt1711h.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>