[PATCH v7] staging: typec: handle vendor defined part and modify drp toggling flow

[PATCH v7] staging: typec: handle vendor defined part and modify drp toggling flow

[ShuFan Lee]

From: ShuFan Lee <shufan_lee@richtek.com>

Handle vendor defined behavior in tcpci_init, tcpci_set_vconn, tcpci_start_drp_toggling
and export tcpci_irq. More operations can be extended in tcpci_data if needed.
According to TCPCI specification, 4.4.5.2 ROLE_CONTROL,
TCPC shall not start DRP toggling until subsequently the TCPM
writes to the COMMAND register to start DRP toggling.
DRP toggling flow is changed as following:
  - Write DRP = 1, Rp level and RC.CCx to Rd/Rd or Rp/Rp
  - Set LOOK4CONNECTION command

Signed-off-by: ShuFan Lee <shufan_lee@richtek.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/staging/typec/tcpci.c | 127 +++++++++++++++++++++++++++++++++---------
 drivers/staging/typec/tcpci.h |  15 +++++
 2 files changed, 116 insertions(+), 26 deletions(-)

 patch changelogs between v1 & v2
 - Remove unnecessary i2c_client in the structure of tcpci.
 - Rename structure of tcpci_vendor_data to tcpci_data.
 - Not exporting tcpci read/write wrappers but register i2c regmap in glue driver.
 - Add set_vconn ops in tcpci_data.
   (It is necessary for RT1711H to enable/disable idle mode before disabling/enabling vconn)
 - Export tcpci_irq so that vendor can call it in their own IRQ handler.

 patch changelogs between v2 & v3
 - Change the return type of tcpci_irq from int to irqreturn_t.

 patch changelogs between v3 & v4
 - Directly return regmap_write at the end of drp_toggling function.
 - Because tcpci_irq is called in _tcpci_irq, move _tcpci_irq to the place after tcpci_irq.

 patch changelogs between v4 & v5
 - Add a space between my first & last name, from ShuFanLee to ShuFan Lee.

 patch changelogs between v5 & v6
 - Add start_drp_toggling in tcpci_data and call it at the beginning of tcpci_start_drp_toggling.
 - Modify the flow of tcpci_start_drp_toggling as following:
    - Set Rp level, RC.CCx to Rd/Rd or Rp/Rp and DRP = 1
    - Set LOOK4CONNECTION command

 patch changelogs between v6 & v7
 - Remove checking whether tcpci->data is NULL because it is guaranteed to be available.

diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
index 9bd4412..8043740 100644
--- a/drivers/staging/typec/tcpci.c
+++ b/drivers/staging/typec/tcpci.c
@@ -21,7 +21,6 @@
 
 struct tcpci {
 	struct device *dev;
-	struct i2c_client *client;
 
 	struct tcpm_port *port;
 
@@ -30,6 +29,12 @@ struct tcpci {
 	bool controls_vbus;
 
 	struct tcpc_dev tcpc;
+	struct tcpci_data *data;
+};
+
+struct tcpci_chip {
+	struct tcpci *tcpci;
+	struct tcpci_data data;
 };
 
 static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev *tcpc)
@@ -37,8 +42,7 @@ static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev *tcpc)
 	return container_of(tcpc, struct tcpci, tcpc);
 }
 
-static int tcpci_read16(struct tcpci *tcpci, unsigned int reg,
-			u16 *val)
+static int tcpci_read16(struct tcpci *tcpci, unsigned int reg, u16 *val)
 {
 	return regmap_raw_read(tcpci->regmap, reg, val, sizeof(u16));
 }
@@ -98,9 +102,17 @@ static int tcpci_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status cc)
 static int tcpci_start_drp_toggling(struct tcpc_dev *tcpc,
 				    enum typec_cc_status cc)
 {
+	int ret;
 	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
 	unsigned int reg = TCPC_ROLE_CTRL_DRP;
 
+	/* Handle vendor drp toggling */
+	if (tcpci->data->start_drp_toggling) {
+		ret = tcpci->data->start_drp_toggling(tcpci, tcpci->data, cc);
+		if (ret < 0)
+			return ret;
+	}
+
 	switch (cc) {
 	default:
 	case TYPEC_CC_RP_DEF:
@@ -117,7 +129,17 @@ static int tcpci_start_drp_toggling(struct tcpc_dev *tcpc,
 		break;
 	}
 
-	return regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
+	if (cc == TYPEC_CC_RD)
+		reg |= (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC1_SHIFT) |
+			   (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC2_SHIFT);
+	else
+		reg |= (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC1_SHIFT) |
+			   (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC2_SHIFT);
+	ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
+	if (ret < 0)
+		return ret;
+	return regmap_write(tcpci->regmap, TCPC_COMMAND,
+			    TCPC_CMD_LOOK4CONNECTION);
 }
 
 static enum typec_cc_status tcpci_to_typec_cc(unsigned int cc, bool sink)
@@ -178,6 +200,13 @@ static int tcpci_set_vconn(struct tcpc_dev *tcpc, bool enable)
 	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
 	int ret;
 
+	/* Handle vendor set vconn */
+	if (tcpci->data->set_vconn) {
+		ret = tcpci->data->set_vconn(tcpci, tcpci->data, enable);
+		if (ret < 0)
+			return ret;
+	}
+
 	ret = regmap_write(tcpci->regmap, TCPC_POWER_CTRL,
 			   enable ? TCPC_POWER_CTRL_VCONN_ENABLE : 0);
 	if (ret < 0)
@@ -323,6 +352,13 @@ static int tcpci_init(struct tcpc_dev *tcpc)
 	if (time_after(jiffies, timeout))
 		return -ETIMEDOUT;
 
+	/* Handle vendor init */
+	if (tcpci->data->init) {
+		ret = tcpci->data->init(tcpci, tcpci->data);
+		if (ret < 0)
+			return ret;
+	}
+
 	/* Clear all events */
 	ret = tcpci_write16(tcpci, TCPC_ALERT, 0xffff);
 	if (ret < 0)
@@ -344,9 +380,8 @@ static int tcpci_init(struct tcpc_dev *tcpc)
 	return tcpci_write16(tcpci, TCPC_ALERT_MASK, reg);
 }
 
-static irqreturn_t tcpci_irq(int irq, void *dev_id)
+irqreturn_t tcpci_irq(struct tcpci *tcpci)
 {
-	struct tcpci *tcpci = dev_id;
 	u16 status;
 
 	tcpci_read16(tcpci, TCPC_ALERT, &status);
@@ -412,6 +447,14 @@ static irqreturn_t tcpci_irq(int irq, void *dev_id)
 
 	return IRQ_HANDLED;
 }
+EXPORT_SYMBOL_GPL(tcpci_irq);
+
+static irqreturn_t _tcpci_irq(int irq, void *dev_id)
+{
+	struct tcpci *tcpci = dev_id;
+
+	return tcpci_irq(tcpci);
+}
 
 static const struct regmap_config tcpci_regmap_config = {
 	.reg_bits = 8,
@@ -435,22 +478,18 @@ static int tcpci_parse_config(struct tcpci *tcpci)
 	return 0;
 }
 
-static int tcpci_probe(struct i2c_client *client,
-		       const struct i2c_device_id *i2c_id)
+struct tcpci *tcpci_register_port(struct device *dev, struct tcpci_data *data)
 {
 	struct tcpci *tcpci;
 	int err;
 
-	tcpci = devm_kzalloc(&client->dev, sizeof(*tcpci), GFP_KERNEL);
+	tcpci = devm_kzalloc(dev, sizeof(*tcpci), GFP_KERNEL);
 	if (!tcpci)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
-	tcpci->client = client;
-	tcpci->dev = &client->dev;
-	i2c_set_clientdata(client, tcpci);
-	tcpci->regmap = devm_regmap_init_i2c(client, &tcpci_regmap_config);
-	if (IS_ERR(tcpci->regmap))
-		return PTR_ERR(tcpci->regmap);
+	tcpci->dev = dev;
+	tcpci->data = data;
+	tcpci->regmap = data->regmap;
 
 	tcpci->tcpc.init = tcpci_init;
 	tcpci->tcpc.get_vbus = tcpci_get_vbus;
@@ -467,27 +506,63 @@ static int tcpci_probe(struct i2c_client *client,
 
 	err = tcpci_parse_config(tcpci);
 	if (err < 0)
-		return err;
+		return ERR_PTR(err);
+
+	tcpci->port = tcpm_register_port(tcpci->dev, &tcpci->tcpc);
+	if (PTR_ERR_OR_ZERO(tcpci->port))
+		return ERR_CAST(tcpci->port);
+
+	return tcpci;
+}
+EXPORT_SYMBOL_GPL(tcpci_register_port);
+
+void tcpci_unregister_port(struct tcpci *tcpci)
+{
+	tcpm_unregister_port(tcpci->port);
+}
+EXPORT_SYMBOL_GPL(tcpci_unregister_port);
+
+static int tcpci_probe(struct i2c_client *client,
+		       const struct i2c_device_id *i2c_id)
+{
+	struct tcpci_chip *chip;
+	int err;
+	u16 val = 0;
 
-	/* Disable chip interrupts */
-	tcpci_write16(tcpci, TCPC_ALERT_MASK, 0);
+	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	chip->data.regmap = devm_regmap_init_i2c(client, &tcpci_regmap_config);
+	if (IS_ERR(chip->data.regmap))
+		return PTR_ERR(chip->data.regmap);
 
-	err = devm_request_threaded_irq(tcpci->dev, client->irq, NULL,
-					tcpci_irq,
+	/* Disable chip interrupts before requesting irq */
+	err = regmap_raw_write(chip->data.regmap, TCPC_ALERT_MASK, &val,
+			       sizeof(u16));
+	if (err < 0)
+		return err;
+
+	err = devm_request_threaded_irq(&client->dev, client->irq, NULL,
+					_tcpci_irq,
 					IRQF_ONESHOT | IRQF_TRIGGER_LOW,
-					dev_name(tcpci->dev), tcpci);
+					dev_name(&client->dev), chip);
 	if (err < 0)
 		return err;
 
-	tcpci->port = tcpm_register_port(tcpci->dev, &tcpci->tcpc);
-	return PTR_ERR_OR_ZERO(tcpci->port);
+	chip->tcpci = tcpci_register_port(&client->dev, &chip->data);
+	if (PTR_ERR_OR_ZERO(chip->tcpci))
+		return PTR_ERR(chip->tcpci);
+
+	i2c_set_clientdata(client, chip);
+	return 0;
 }
 
 static int tcpci_remove(struct i2c_client *client)
 {
-	struct tcpci *tcpci = i2c_get_clientdata(client);
+	struct tcpci_chip *chip = i2c_get_clientdata(client);
 
-	tcpm_unregister_port(tcpci->port);
+	tcpci_unregister_port(chip->tcpci);
 
 	return 0;
 }
diff --git a/drivers/staging/typec/tcpci.h b/drivers/staging/typec/tcpci.h
index fdfb06c..a2c1754 100644
--- a/drivers/staging/typec/tcpci.h
+++ b/drivers/staging/typec/tcpci.h
@@ -59,6 +59,7 @@
 #define TCPC_POWER_CTRL_VCONN_ENABLE	BIT(0)
 
 #define TCPC_CC_STATUS			0x1d
+#define TCPC_CC_STATUS_DRPRST		BIT(5)
 #define TCPC_CC_STATUS_TERM		BIT(4)
 #define TCPC_CC_STATUS_CC2_SHIFT	2
 #define TCPC_CC_STATUS_CC2_MASK		0x3
@@ -121,4 +122,18 @@
 #define TCPC_VBUS_VOLTAGE_ALARM_HI_CFG		0x76
 #define TCPC_VBUS_VOLTAGE_ALARM_LO_CFG		0x78
 
+struct tcpci;
+struct tcpci_data {
+	struct regmap *regmap;
+	int (*init)(struct tcpci *tcpci, struct tcpci_data *data);
+	int (*set_vconn)(struct tcpci *tcpci, struct tcpci_data *data,
+			 bool enable);
+	int (*start_drp_toggling)(struct tcpci *tcpci, struct tcpci_data *data,
+				  enum typec_cc_status cc);
+};
+
+struct tcpci *tcpci_register_port(struct device *dev, struct tcpci_data *data);
+void tcpci_unregister_port(struct tcpci *tcpci);
+irqreturn_t tcpci_irq(struct tcpci *tcpci);
+
 #endif /* __LINUX_USB_TCPCI_H */
-- 
1.9.1

Re: [PATCH v7] staging: typec: handle vendor defined part and modify drp toggling flow

[Guenter Roeck]

On Tue, Mar 06, 2018 at 09:50:32AM +0800, ShuFan Lee wrote:
> From: ShuFan Lee <shufan_lee@richtek.com>
> 
> Handle vendor defined behavior in tcpci_init, tcpci_set_vconn, tcpci_start_drp_toggling
> and export tcpci_irq. More operations can be extended in tcpci_data if needed.
> According to TCPCI specification, 4.4.5.2 ROLE_CONTROL,
> TCPC shall not start DRP toggling until subsequently the TCPM
> writes to the COMMAND register to start DRP toggling.
> DRP toggling flow is changed as following:
>   - Write DRP = 1, Rp level and RC.CCx to Rd/Rd or Rp/Rp
>   - Set LOOK4CONNECTION command
> 
> Signed-off-by: ShuFan Lee <shufan_lee@richtek.com>

I am fine with this version.

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

> ---
>  drivers/staging/typec/tcpci.c | 127 +++++++++++++++++++++++++++++++++---------
>  drivers/staging/typec/tcpci.h |  15 +++++
>  2 files changed, 116 insertions(+), 26 deletions(-)
> 
>  patch changelogs between v1 & v2
>  - Remove unnecessary i2c_client in the structure of tcpci.
>  - Rename structure of tcpci_vendor_data to tcpci_data.
>  - Not exporting tcpci read/write wrappers but register i2c regmap in glue driver.
>  - Add set_vconn ops in tcpci_data.
>    (It is necessary for RT1711H to enable/disable idle mode before disabling/enabling vconn)
>  - Export tcpci_irq so that vendor can call it in their own IRQ handler.
> 
>  patch changelogs between v2 & v3
>  - Change the return type of tcpci_irq from int to irqreturn_t.
> 
>  patch changelogs between v3 & v4
>  - Directly return regmap_write at the end of drp_toggling function.
>  - Because tcpci_irq is called in _tcpci_irq, move _tcpci_irq to the place after tcpci_irq.
> 
>  patch changelogs between v4 & v5
>  - Add a space between my first & last name, from ShuFanLee to ShuFan Lee.
> 
>  patch changelogs between v5 & v6
>  - Add start_drp_toggling in tcpci_data and call it at the beginning of tcpci_start_drp_toggling.
>  - Modify the flow of tcpci_start_drp_toggling as following:
>     - Set Rp level, RC.CCx to Rd/Rd or Rp/Rp and DRP = 1
>     - Set LOOK4CONNECTION command
> 
>  patch changelogs between v6 & v7
>  - Remove checking whether tcpci->data is NULL because it is guaranteed to be available.
> 
> diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
> index 9bd4412..8043740 100644
> --- a/drivers/staging/typec/tcpci.c
> +++ b/drivers/staging/typec/tcpci.c
> @@ -21,7 +21,6 @@
>  
>  struct tcpci {
>  	struct device *dev;
> -	struct i2c_client *client;
>  
>  	struct tcpm_port *port;
>  
> @@ -30,6 +29,12 @@ struct tcpci {
>  	bool controls_vbus;
>  
>  	struct tcpc_dev tcpc;
> +	struct tcpci_data *data;
> +};
> +
> +struct tcpci_chip {
> +	struct tcpci *tcpci;
> +	struct tcpci_data data;
>  };
>  
>  static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev *tcpc)
> @@ -37,8 +42,7 @@ static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev *tcpc)
>  	return container_of(tcpc, struct tcpci, tcpc);
>  }
>  
> -static int tcpci_read16(struct tcpci *tcpci, unsigned int reg,
> -			u16 *val)
> +static int tcpci_read16(struct tcpci *tcpci, unsigned int reg, u16 *val)
>  {
>  	return regmap_raw_read(tcpci->regmap, reg, val, sizeof(u16));
>  }
> @@ -98,9 +102,17 @@ static int tcpci_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status cc)
>  static int tcpci_start_drp_toggling(struct tcpc_dev *tcpc,
>  				    enum typec_cc_status cc)
>  {
> +	int ret;
>  	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
>  	unsigned int reg = TCPC_ROLE_CTRL_DRP;
>  
> +	/* Handle vendor drp toggling */
> +	if (tcpci->data->start_drp_toggling) {
> +		ret = tcpci->data->start_drp_toggling(tcpci, tcpci->data, cc);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	switch (cc) {
>  	default:
>  	case TYPEC_CC_RP_DEF:
> @@ -117,7 +129,17 @@ static int tcpci_start_drp_toggling(struct tcpc_dev *tcpc,
>  		break;
>  	}
>  
> -	return regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
> +	if (cc == TYPEC_CC_RD)
> +		reg |= (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC1_SHIFT) |
> +			   (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC2_SHIFT);
> +	else
> +		reg |= (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC1_SHIFT) |
> +			   (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC2_SHIFT);
> +	ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
> +	if (ret < 0)
> +		return ret;
> +	return regmap_write(tcpci->regmap, TCPC_COMMAND,
> +			    TCPC_CMD_LOOK4CONNECTION);
>  }
>  
>  static enum typec_cc_status tcpci_to_typec_cc(unsigned int cc, bool sink)
> @@ -178,6 +200,13 @@ static int tcpci_set_vconn(struct tcpc_dev *tcpc, bool enable)
>  	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
>  	int ret;
>  
> +	/* Handle vendor set vconn */
> +	if (tcpci->data->set_vconn) {
> +		ret = tcpci->data->set_vconn(tcpci, tcpci->data, enable);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	ret = regmap_write(tcpci->regmap, TCPC_POWER_CTRL,
>  			   enable ? TCPC_POWER_CTRL_VCONN_ENABLE : 0);
>  	if (ret < 0)
> @@ -323,6 +352,13 @@ static int tcpci_init(struct tcpc_dev *tcpc)
>  	if (time_after(jiffies, timeout))
>  		return -ETIMEDOUT;
>  
> +	/* Handle vendor init */
> +	if (tcpci->data->init) {
> +		ret = tcpci->data->init(tcpci, tcpci->data);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	/* Clear all events */
>  	ret = tcpci_write16(tcpci, TCPC_ALERT, 0xffff);
>  	if (ret < 0)
> @@ -344,9 +380,8 @@ static int tcpci_init(struct tcpc_dev *tcpc)
>  	return tcpci_write16(tcpci, TCPC_ALERT_MASK, reg);
>  }
>  
> -static irqreturn_t tcpci_irq(int irq, void *dev_id)
> +irqreturn_t tcpci_irq(struct tcpci *tcpci)
>  {
> -	struct tcpci *tcpci = dev_id;
>  	u16 status;
>  
>  	tcpci_read16(tcpci, TCPC_ALERT, &status);
> @@ -412,6 +447,14 @@ static irqreturn_t tcpci_irq(int irq, void *dev_id)
>  
>  	return IRQ_HANDLED;
>  }
> +EXPORT_SYMBOL_GPL(tcpci_irq);
> +
> +static irqreturn_t _tcpci_irq(int irq, void *dev_id)
> +{
> +	struct tcpci *tcpci = dev_id;
> +
> +	return tcpci_irq(tcpci);
> +}
>  
>  static const struct regmap_config tcpci_regmap_config = {
>  	.reg_bits = 8,
> @@ -435,22 +478,18 @@ static int tcpci_parse_config(struct tcpci *tcpci)
>  	return 0;
>  }
>  
> -static int tcpci_probe(struct i2c_client *client,
> -		       const struct i2c_device_id *i2c_id)
> +struct tcpci *tcpci_register_port(struct device *dev, struct tcpci_data *data)
>  {
>  	struct tcpci *tcpci;
>  	int err;
>  
> -	tcpci = devm_kzalloc(&client->dev, sizeof(*tcpci), GFP_KERNEL);
> +	tcpci = devm_kzalloc(dev, sizeof(*tcpci), GFP_KERNEL);
>  	if (!tcpci)
> -		return -ENOMEM;
> +		return ERR_PTR(-ENOMEM);
>  
> -	tcpci->client = client;
> -	tcpci->dev = &client->dev;
> -	i2c_set_clientdata(client, tcpci);
> -	tcpci->regmap = devm_regmap_init_i2c(client, &tcpci_regmap_config);
> -	if (IS_ERR(tcpci->regmap))
> -		return PTR_ERR(tcpci->regmap);
> +	tcpci->dev = dev;
> +	tcpci->data = data;
> +	tcpci->regmap = data->regmap;
>  
>  	tcpci->tcpc.init = tcpci_init;
>  	tcpci->tcpc.get_vbus = tcpci_get_vbus;
> @@ -467,27 +506,63 @@ static int tcpci_probe(struct i2c_client *client,
>  
>  	err = tcpci_parse_config(tcpci);
>  	if (err < 0)
> -		return err;
> +		return ERR_PTR(err);
> +
> +	tcpci->port = tcpm_register_port(tcpci->dev, &tcpci->tcpc);
> +	if (PTR_ERR_OR_ZERO(tcpci->port))
> +		return ERR_CAST(tcpci->port);
> +
> +	return tcpci;
> +}
> +EXPORT_SYMBOL_GPL(tcpci_register_port);
> +
> +void tcpci_unregister_port(struct tcpci *tcpci)
> +{
> +	tcpm_unregister_port(tcpci->port);
> +}
> +EXPORT_SYMBOL_GPL(tcpci_unregister_port);
> +
> +static int tcpci_probe(struct i2c_client *client,
> +		       const struct i2c_device_id *i2c_id)
> +{
> +	struct tcpci_chip *chip;
> +	int err;
> +	u16 val = 0;
>  
> -	/* Disable chip interrupts */
> -	tcpci_write16(tcpci, TCPC_ALERT_MASK, 0);
> +	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return -ENOMEM;
> +
> +	chip->data.regmap = devm_regmap_init_i2c(client, &tcpci_regmap_config);
> +	if (IS_ERR(chip->data.regmap))
> +		return PTR_ERR(chip->data.regmap);
>  
> -	err = devm_request_threaded_irq(tcpci->dev, client->irq, NULL,
> -					tcpci_irq,
> +	/* Disable chip interrupts before requesting irq */
> +	err = regmap_raw_write(chip->data.regmap, TCPC_ALERT_MASK, &val,
> +			       sizeof(u16));
> +	if (err < 0)
> +		return err;
> +
> +	err = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> +					_tcpci_irq,
>  					IRQF_ONESHOT | IRQF_TRIGGER_LOW,
> -					dev_name(tcpci->dev), tcpci);
> +					dev_name(&client->dev), chip);
>  	if (err < 0)
>  		return err;
>  
> -	tcpci->port = tcpm_register_port(tcpci->dev, &tcpci->tcpc);
> -	return PTR_ERR_OR_ZERO(tcpci->port);
> +	chip->tcpci = tcpci_register_port(&client->dev, &chip->data);
> +	if (PTR_ERR_OR_ZERO(chip->tcpci))
> +		return PTR_ERR(chip->tcpci);
> +
> +	i2c_set_clientdata(client, chip);
> +	return 0;
>  }
>  
>  static int tcpci_remove(struct i2c_client *client)
>  {
> -	struct tcpci *tcpci = i2c_get_clientdata(client);
> +	struct tcpci_chip *chip = i2c_get_clientdata(client);
>  
> -	tcpm_unregister_port(tcpci->port);
> +	tcpci_unregister_port(chip->tcpci);
>  
>  	return 0;
>  }
> diff --git a/drivers/staging/typec/tcpci.h b/drivers/staging/typec/tcpci.h
> index fdfb06c..a2c1754 100644
> --- a/drivers/staging/typec/tcpci.h
> +++ b/drivers/staging/typec/tcpci.h
> @@ -59,6 +59,7 @@
>  #define TCPC_POWER_CTRL_VCONN_ENABLE	BIT(0)
>  
>  #define TCPC_CC_STATUS			0x1d
> +#define TCPC_CC_STATUS_DRPRST		BIT(5)
>  #define TCPC_CC_STATUS_TERM		BIT(4)
>  #define TCPC_CC_STATUS_CC2_SHIFT	2
>  #define TCPC_CC_STATUS_CC2_MASK		0x3
> @@ -121,4 +122,18 @@
>  #define TCPC_VBUS_VOLTAGE_ALARM_HI_CFG		0x76
>  #define TCPC_VBUS_VOLTAGE_ALARM_LO_CFG		0x78
>  
> +struct tcpci;
> +struct tcpci_data {
> +	struct regmap *regmap;
> +	int (*init)(struct tcpci *tcpci, struct tcpci_data *data);
> +	int (*set_vconn)(struct tcpci *tcpci, struct tcpci_data *data,
> +			 bool enable);
> +	int (*start_drp_toggling)(struct tcpci *tcpci, struct tcpci_data *data,
> +				  enum typec_cc_status cc);
> +};
> +
> +struct tcpci *tcpci_register_port(struct device *dev, struct tcpci_data *data);
> +void tcpci_unregister_port(struct tcpci *tcpci);
> +irqreturn_t tcpci_irq(struct tcpci *tcpci);
> +
>  #endif /* __LINUX_USB_TCPCI_H */
> -- 
> 1.9.1
> 

RE: [PATCH v7] staging: typec: handle vendor defined part and modify drp toggling flow

[Jun Li]

Hi,

> +static irqreturn_t _tcpci_irq(int irq, void *dev_id) {
> +	struct tcpci *tcpci = dev_id;
> +
> +	return tcpci_irq(tcpci);
> +}
> 
...

> +	err = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> +					_tcpci_irq,
>  					IRQF_ONESHOT | IRQF_TRIGGER_LOW,
> -					dev_name(tcpci->dev), tcpci);
> +					dev_name(&client->dev), chip);

- dev_name(&client->dev), chip);
+ dev_name(&client->dev), chip->tcpci); 

Did you ever test this patch?

I noticed Greg already picked this patch[1]:
[1] https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/?h=usb-testing&id=8f94390226487bcb86c554ddc12eef0d27bdec3b

One more minor comment below.

Jun Li

> diff --git a/drivers/staging/typec/tcpci.h b/drivers/staging/typec/tcpci.h index
> fdfb06c..a2c1754 100644
> --- a/drivers/staging/typec/tcpci.h
> +++ b/drivers/staging/typec/tcpci.h
> @@ -59,6 +59,7 @@
>  #define TCPC_POWER_CTRL_VCONN_ENABLE	BIT(0)
> 
>  #define TCPC_CC_STATUS			0x1d
> +#define TCPC_CC_STATUS_DRPRST		BIT(5)

Defined but not used.

>  #define TCPC_CC_STATUS_TERM		BIT(4)
>  #define TCPC_CC_STATUS_CC2_SHIFT	2
>  #define TCPC_CC_STATUS_CC2_MASK		0x3
> @@ -121,4 +122,18 @@
>  #define TCPC_VBUS_VOLTAGE_ALARM_HI_CFG		0x76
>  #define TCPC_VBUS_VOLTAGE_ALARM_LO_CFG		0x78
> 
> +struct tcpci;
> +struct tcpci_data {
> +	struct regmap *regmap;
> +	int (*init)(struct tcpci *tcpci, struct tcpci_data *data);
> +	int (*set_vconn)(struct tcpci *tcpci, struct tcpci_data *data,
> +			 bool enable);
> +	int (*start_drp_toggling)(struct tcpci *tcpci, struct tcpci_data *data,
> +				  enum typec_cc_status cc);
> +};
> +
> +struct tcpci *tcpci_register_port(struct device *dev, struct tcpci_data
> +*data); void tcpci_unregister_port(struct tcpci *tcpci); irqreturn_t
> +tcpci_irq(struct tcpci *tcpci);
> +
>  #endif /* __LINUX_USB_TCPCI_H */
> --
> 1.9.1

Re: [PATCH v7] staging: typec: handle vendor defined part and modify drp toggling flow

[李書帆]

Hi Jun,

  Thank you.

2018-03-12 12:33 GMT+08:00 Jun Li <jun.li@nxp.com>:
> Hi,
>
>> +static irqreturn_t _tcpci_irq(int irq, void *dev_id) {
>> +     struct tcpci *tcpci = dev_id;
>> +
>> +     return tcpci_irq(tcpci);
>> +}
>>
> ...
>
>> +     err = devm_request_threaded_irq(&client->dev, client->irq, NULL,
>> +                                     _tcpci_irq,
>>                                       IRQF_ONESHOT | IRQF_TRIGGER_LOW,
>> -                                     dev_name(tcpci->dev), tcpci);
>> +                                     dev_name(&client->dev), chip);
>
> - dev_name(&client->dev), chip);
> + dev_name(&client->dev), chip->tcpci);
>
> Did you ever test this patch?
I've tested this patch with tcpci_rt1711h.c that will be sent out for
reviewing in the next patch after tcpci's modification is passed.
Because interrupt handler is registered in tcpci_rt1711h.c, here is
the place I didn't notice.
The interrupt handler for tcpci.c should be modified as following:
 static irqreturn_t _tcpci_irq(int irq, void *dev_id)
 {
-       struct tcpci *tcpci = dev_id;
+       struct tcpci_chip *chip = dev_id;

-       return tcpci_irq(tcpci);
+       return tcpci_irq(chip->tcpci);
 }

>
> I noticed Greg already picked this patch[1]:
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/?h=usb-testing&id=8f94390226487bcb86c554ddc12eef0d27bdec3b
>
> One more minor comment below.
>
> Jun Li
>
>> diff --git a/drivers/staging/typec/tcpci.h b/drivers/staging/typec/tcpci.h index
>> fdfb06c..a2c1754 100644
>> --- a/drivers/staging/typec/tcpci.h
>> +++ b/drivers/staging/typec/tcpci.h
>> @@ -59,6 +59,7 @@
>>  #define TCPC_POWER_CTRL_VCONN_ENABLE BIT(0)
>>
>>  #define TCPC_CC_STATUS                       0x1d
>> +#define TCPC_CC_STATUS_DRPRST                BIT(5)
>
> Defined but not used.
This definition can be removed for now.
>
>>  #define TCPC_CC_STATUS_TERM          BIT(4)
>>  #define TCPC_CC_STATUS_CC2_SHIFT     2
>>  #define TCPC_CC_STATUS_CC2_MASK              0x3
>> @@ -121,4 +122,18 @@
>>  #define TCPC_VBUS_VOLTAGE_ALARM_HI_CFG               0x76
>>  #define TCPC_VBUS_VOLTAGE_ALARM_LO_CFG               0x78
>>
>> +struct tcpci;
>> +struct tcpci_data {
>> +     struct regmap *regmap;
>> +     int (*init)(struct tcpci *tcpci, struct tcpci_data *data);
>> +     int (*set_vconn)(struct tcpci *tcpci, struct tcpci_data *data,
>> +                      bool enable);
>> +     int (*start_drp_toggling)(struct tcpci *tcpci, struct tcpci_data *data,
>> +                               enum typec_cc_status cc);
>> +};
>> +
>> +struct tcpci *tcpci_register_port(struct device *dev, struct tcpci_data
>> +*data); void tcpci_unregister_port(struct tcpci *tcpci); irqreturn_t
>> +tcpci_irq(struct tcpci *tcpci);
>> +
>>  #endif /* __LINUX_USB_TCPCI_H */
>> --
>> 1.9.1
>



-- 
Best Regards,
書帆

RE: [PATCH v7] staging: typec: handle vendor defined part and modify drp toggling flow

[Jun Li]

Hi
> -----Original Message-----
> From: 李書帆 [mailto:leechu729@gmail.com]
> Sent: 2018年3月12日 13:22
> To: Jun Li <jun.li@nxp.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>;
> heikki.krogerus@linux.intel.com; linux@roeck-us.net; greg@kroah.com;
> shufan_lee@richtek.com; cy_huang@richtek.com;
> linux-kernel@vger.kernel.org; linux-usb@vger.kernel.org
> Subject: Re: [PATCH v7] staging: typec: handle vendor defined part and modify
> drp toggling flow
> 
> Hi Jun,
> 
>   Thank you.
> 
> 2018-03-12 12:33 GMT+08:00 Jun Li <jun.li@nxp.com>:
> > Hi,
> >
> >> +static irqreturn_t _tcpci_irq(int irq, void *dev_id) {
> >> +     struct tcpci *tcpci = dev_id;
> >> +
> >> +     return tcpci_irq(tcpci);
> >> +}
> >>
> > ...
> >
> >> +     err = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> >> +                                     _tcpci_irq,
> >>                                       IRQF_ONESHOT |
> IRQF_TRIGGER_LOW,
> >> -                                     dev_name(tcpci->dev), tcpci);
> >> +                                     dev_name(&client->dev), chip);
> >
> > - dev_name(&client->dev), chip);
> > + dev_name(&client->dev), chip->tcpci);
> >
> > Did you ever test this patch?
> I've tested this patch with tcpci_rt1711h.c that will be sent out for reviewing in
> the next patch after tcpci's modification is passed.
> Because interrupt handler is registered in tcpci_rt1711h.c, here is the place I
> didn't notice.

Understood.

> The interrupt handler for tcpci.c should be modified as following:
>  static irqreturn_t _tcpci_irq(int irq, void *dev_id)  {
> -       struct tcpci *tcpci = dev_id;
> +       struct tcpci_chip *chip = dev_id;
> 
> -       return tcpci_irq(tcpci);
> +       return tcpci_irq(chip->tcpci);
>  }
> 

Either way is OK to fix it.
You may send out your v8 and notify Greg to drop your v7 version.

Jun Li

Re: [PATCH v7] staging: typec: handle vendor defined part and modify drp toggling flow

[李書帆]

Hi Jun,

2018-03-12 13:58 GMT+08:00 Jun Li <jun.li@nxp.com>:
> Hi
>> -----Original Message-----
>> From: 李書帆 [mailto:leechu729@gmail.com]
>> Sent: 2018年3月12日 13:22
>> To: Jun Li <jun.li@nxp.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>;
>> heikki.krogerus@linux.intel.com; linux@roeck-us.net; greg@kroah.com;
>> shufan_lee@richtek.com; cy_huang@richtek.com;
>> linux-kernel@vger.kernel.org; linux-usb@vger.kernel.org
>> Subject: Re: [PATCH v7] staging: typec: handle vendor defined part and modify
>> drp toggling flow
>>
>> Hi Jun,
>>
>>   Thank you.
>>
>> 2018-03-12 12:33 GMT+08:00 Jun Li <jun.li@nxp.com>:
>> > Hi,
>> >
>> >> +static irqreturn_t _tcpci_irq(int irq, void *dev_id) {
>> >> +     struct tcpci *tcpci = dev_id;
>> >> +
>> >> +     return tcpci_irq(tcpci);
>> >> +}
>> >>
>> > ...
>> >
>> >> +     err = devm_request_threaded_irq(&client->dev, client->irq, NULL,
>> >> +                                     _tcpci_irq,
>> >>                                       IRQF_ONESHOT |
>> IRQF_TRIGGER_LOW,
>> >> -                                     dev_name(tcpci->dev), tcpci);
>> >> +                                     dev_name(&client->dev), chip);
>> >
>> > - dev_name(&client->dev), chip);
>> > + dev_name(&client->dev), chip->tcpci);
>> >
>> > Did you ever test this patch?
>> I've tested this patch with tcpci_rt1711h.c that will be sent out for reviewing in
>> the next patch after tcpci's modification is passed.
>> Because interrupt handler is registered in tcpci_rt1711h.c, here is the place I
>> didn't notice.
>
> Understood.
>
>> The interrupt handler for tcpci.c should be modified as following:
>>  static irqreturn_t _tcpci_irq(int irq, void *dev_id)  {
>> -       struct tcpci *tcpci = dev_id;
>> +       struct tcpci_chip *chip = dev_id;
>>
>> -       return tcpci_irq(tcpci);
>> +       return tcpci_irq(chip->tcpci);
>>  }
>>
>
> Either way is OK to fix it.
> You may send out your v8 and notify Greg to drop your v7 version.
>
> Jun Li

May I add you in the Reported-by list?

-- 
Best Regards,
書帆

RE: [PATCH v7] staging: typec: handle vendor defined part and modify drp toggling flow

[Jun Li]

> -----Original Message-----
> From: 李書帆 [mailto:leechu729@gmail.com]
> Sent: 2018年3月12日 14:57
> To: Jun Li <jun.li@nxp.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>;
> heikki.krogerus@linux.intel.com; linux@roeck-us.net; greg@kroah.com;
> shufan_lee@richtek.com; cy_huang@richtek.com;
> linux-kernel@vger.kernel.org; linux-usb@vger.kernel.org
> Subject: Re: [PATCH v7] staging: typec: handle vendor defined part and modify
> drp toggling flow
> 
> Hi Jun,
> 
> 2018-03-12 13:58 GMT+08:00 Jun Li <jun.li@nxp.com>:
> > Hi
> >> -----Original Message-----
> >> From: 李書帆 [mailto:leechu729@gmail.com]
> >> Sent: 2018年3月12日 13:22
> >> To: Jun Li <jun.li@nxp.com>
> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>;
> >> heikki.krogerus@linux.intel.com; linux@roeck-us.net; greg@kroah.com;
> >> shufan_lee@richtek.com; cy_huang@richtek.com;
> >> linux-kernel@vger.kernel.org; linux-usb@vger.kernel.org
> >> Subject: Re: [PATCH v7] staging: typec: handle vendor defined part
> >> and modify drp toggling flow
> >>
> >> Hi Jun,
> >>
> >>   Thank you.
> >>
> >> 2018-03-12 12:33 GMT+08:00 Jun Li <jun.li@nxp.com>:
> >> > Hi,
> >> >
> >> >> +static irqreturn_t _tcpci_irq(int irq, void *dev_id) {
> >> >> +     struct tcpci *tcpci = dev_id;
> >> >> +
> >> >> +     return tcpci_irq(tcpci);
> >> >> +}
> >> >>
> >> > ...
> >> >
> >> >> +     err = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> >> >> +                                     _tcpci_irq,
> >> >>                                       IRQF_ONESHOT |
> >> IRQF_TRIGGER_LOW,
> >> >> -                                     dev_name(tcpci->dev),
> tcpci);
> >> >> +                                     dev_name(&client->dev),
> >> >> + chip);
> >> >
> >> > - dev_name(&client->dev), chip);
> >> > + dev_name(&client->dev), chip->tcpci);
> >> >
> >> > Did you ever test this patch?
> >> I've tested this patch with tcpci_rt1711h.c that will be sent out for
> >> reviewing in the next patch after tcpci's modification is passed.
> >> Because interrupt handler is registered in tcpci_rt1711h.c, here is
> >> the place I didn't notice.
> >
> > Understood.
> >
> >> The interrupt handler for tcpci.c should be modified as following:
> >>  static irqreturn_t _tcpci_irq(int irq, void *dev_id)  {
> >> -       struct tcpci *tcpci = dev_id;
> >> +       struct tcpci_chip *chip = dev_id;
> >>
> >> -       return tcpci_irq(tcpci);
> >> +       return tcpci_irq(chip->tcpci);
> >>  }
> >>
> >
> > Either way is OK to fix it.
> > You may send out your v8 and notify Greg to drop your v7 version.
> >
> > Jun Li
> 
> May I add you in the Reported-by list?

I just gave a run with your patch on my HW, so for your new version,
you can directly add:

Reviewed-by: Li Jun <jun.li@nxp.com>
Tested-by: Li Jun <jun.li@nxp.com>

> 
> --
> Best Regards,
> 書帆