All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] staging: typec: handle vendor defined part and modify drp toggling flow
@ 2018-02-14 12:45 ` ShuFanLee
  0 siblings, 0 replies; 11+ messages in thread
From: ShuFanLee @ 2018-02-14 12:45 UTC (permalink / raw)
  To: heikki.krogerus, linux; +Cc: linux-kernel, linux-usb, cy_huang, shufan_lee

From: ShuFanLee <shufan_lee@richtek.com>

Handle vendor defined behavior in tcpci_init, tcpci_set_vconn 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 chagned as following:
  - Write DRP = 0 & Rd/Rd
  - Write DRP = 1
  - Set LOOK4CONNECTION command

Signed-off-by: ShuFanLee <shufan_lee@richtek.com>
---
 drivers/staging/typec/tcpci.c | 111 ++++++++++++++++++++++++++++++++++--------
 drivers/staging/typec/tcpci.h |  13 +++++
 2 files changed, 103 insertions(+), 21 deletions(-)

diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
index 9bd4412..461fbd48 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,7 @@ struct tcpci {
 	bool controls_vbus;
 
 	struct tcpc_dev tcpc;
+	struct tcpci_data *data;
 };
 
 static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev *tcpc)
@@ -98,8 +98,10 @@ 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;
+	unsigned int reg = (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC1_SHIFT) |
+			   (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC2_SHIFT);
 
 	switch (cc) {
 	default:
@@ -116,8 +118,19 @@ static int tcpci_start_drp_toggling(struct tcpc_dev *tcpc,
 			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;
+	usleep_range(500, 1000);
+	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;
 }
 
 static enum typec_cc_status tcpci_to_typec_cc(unsigned int cc, bool sink)
@@ -178,6 +191,16 @@ 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) {
+		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 +346,15 @@ static int tcpci_init(struct tcpc_dev *tcpc)
 	if (time_after(jiffies, timeout))
 		return -ETIMEDOUT;
 
+	/* Handle vendor init */
+	if (tcpci->data) {
+		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 +376,16 @@ 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)
+static irqreturn_t _tcpci_irq(int irq, void *dev_id)
 {
 	struct tcpci *tcpci = dev_id;
+
+	tcpci_irq(tcpci);
+	return IRQ_HANDLED;
+}
+
+int tcpci_irq(struct tcpci *tcpci)
+{
 	u16 status;
 
 	tcpci_read16(tcpci, TCPC_ALERT, &status);
@@ -412,6 +451,7 @@ static irqreturn_t tcpci_irq(int irq, void *dev_id)
 
 	return IRQ_HANDLED;
 }
+EXPORT_SYMBOL_GPL(tcpci_irq);
 
 static const struct regmap_config tcpci_regmap_config = {
 	.reg_bits = 8,
@@ -435,22 +475,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 +503,60 @@ static int tcpci_probe(struct i2c_client *client,
 
 	err = tcpci_parse_config(tcpci);
 	if (err < 0)
-		return err;
+		return ERR_PTR(err);
 
 	/* Disable chip interrupts */
 	tcpci_write16(tcpci, TCPC_ALERT_MASK, 0);
 
-	err = devm_request_threaded_irq(tcpci->dev, client->irq, NULL,
-					tcpci_irq,
+	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 *tcpci;
+	struct tcpci_data *data;
+	int err;
+
+	data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->regmap = devm_regmap_init_i2c(client, &tcpci_regmap_config);
+	if (IS_ERR(data->regmap))
+		return PTR_ERR(data->regmap);
+
+	tcpci = tcpci_register_port(&client->dev, data);
+	if (PTR_ERR_OR_ZERO(tcpci))
+		return PTR_ERR(tcpci);
+
+	err = devm_request_threaded_irq(&client->dev, client->irq, NULL,
+					_tcpci_irq,
 					IRQF_ONESHOT | IRQF_TRIGGER_LOW,
 					dev_name(tcpci->dev), tcpci);
 	if (err < 0)
 		return err;
 
-	tcpci->port = tcpm_register_port(tcpci->dev, &tcpci->tcpc);
-	return PTR_ERR_OR_ZERO(tcpci->port);
+	i2c_set_clientdata(client, tcpci);
+	return 0;
 }
 
 static int tcpci_remove(struct i2c_client *client)
 {
 	struct tcpci *tcpci = i2c_get_clientdata(client);
 
-	tcpm_unregister_port(tcpci->port);
+	tcpci_unregister_port(tcpci);
 
 	return 0;
 }
diff --git a/drivers/staging/typec/tcpci.h b/drivers/staging/typec/tcpci.h
index fdfb06c..c838723 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,16 @@
 #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);
+};
+
+struct tcpci *tcpci_register_port(struct device *dev, struct tcpci_data *data);
+void tcpci_unregister_port(struct tcpci *tcpci);
+int tcpci_irq(struct tcpci *tcpci);
+
 #endif /* __LINUX_USB_TCPCI_H */
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [v2] staging: typec: handle vendor defined part and modify drp toggling flow
@ 2018-02-14 12:45 ` ShuFanLee
  0 siblings, 0 replies; 11+ messages in thread
From: ShuFanLee @ 2018-02-14 12:45 UTC (permalink / raw)
  To: heikki.krogerus, linux; +Cc: linux-kernel, linux-usb, cy_huang, shufan_lee

From: ShuFanLee <shufan_lee@richtek.com>

Handle vendor defined behavior in tcpci_init, tcpci_set_vconn 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 chagned as following:
  - Write DRP = 0 & Rd/Rd
  - Write DRP = 1
  - Set LOOK4CONNECTION command

Signed-off-by: ShuFanLee <shufan_lee@richtek.com>
---
 drivers/staging/typec/tcpci.c | 111 ++++++++++++++++++++++++++++++++++--------
 drivers/staging/typec/tcpci.h |  13 +++++
 2 files changed, 103 insertions(+), 21 deletions(-)

diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
index 9bd4412..461fbd48 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,7 @@ struct tcpci {
 	bool controls_vbus;
 
 	struct tcpc_dev tcpc;
+	struct tcpci_data *data;
 };
 
 static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev *tcpc)
@@ -98,8 +98,10 @@ 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;
+	unsigned int reg = (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC1_SHIFT) |
+			   (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC2_SHIFT);
 
 	switch (cc) {
 	default:
@@ -116,8 +118,19 @@ static int tcpci_start_drp_toggling(struct tcpc_dev *tcpc,
 			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;
+	usleep_range(500, 1000);
+	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;
 }
 
 static enum typec_cc_status tcpci_to_typec_cc(unsigned int cc, bool sink)
@@ -178,6 +191,16 @@ 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) {
+		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 +346,15 @@ static int tcpci_init(struct tcpc_dev *tcpc)
 	if (time_after(jiffies, timeout))
 		return -ETIMEDOUT;
 
+	/* Handle vendor init */
+	if (tcpci->data) {
+		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 +376,16 @@ 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)
+static irqreturn_t _tcpci_irq(int irq, void *dev_id)
 {
 	struct tcpci *tcpci = dev_id;
+
+	tcpci_irq(tcpci);
+	return IRQ_HANDLED;
+}
+
+int tcpci_irq(struct tcpci *tcpci)
+{
 	u16 status;
 
 	tcpci_read16(tcpci, TCPC_ALERT, &status);
@@ -412,6 +451,7 @@ static irqreturn_t tcpci_irq(int irq, void *dev_id)
 
 	return IRQ_HANDLED;
 }
+EXPORT_SYMBOL_GPL(tcpci_irq);
 
 static const struct regmap_config tcpci_regmap_config = {
 	.reg_bits = 8,
@@ -435,22 +475,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 +503,60 @@ static int tcpci_probe(struct i2c_client *client,
 
 	err = tcpci_parse_config(tcpci);
 	if (err < 0)
-		return err;
+		return ERR_PTR(err);
 
 	/* Disable chip interrupts */
 	tcpci_write16(tcpci, TCPC_ALERT_MASK, 0);
 
-	err = devm_request_threaded_irq(tcpci->dev, client->irq, NULL,
-					tcpci_irq,
+	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 *tcpci;
+	struct tcpci_data *data;
+	int err;
+
+	data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->regmap = devm_regmap_init_i2c(client, &tcpci_regmap_config);
+	if (IS_ERR(data->regmap))
+		return PTR_ERR(data->regmap);
+
+	tcpci = tcpci_register_port(&client->dev, data);
+	if (PTR_ERR_OR_ZERO(tcpci))
+		return PTR_ERR(tcpci);
+
+	err = devm_request_threaded_irq(&client->dev, client->irq, NULL,
+					_tcpci_irq,
 					IRQF_ONESHOT | IRQF_TRIGGER_LOW,
 					dev_name(tcpci->dev), tcpci);
 	if (err < 0)
 		return err;
 
-	tcpci->port = tcpm_register_port(tcpci->dev, &tcpci->tcpc);
-	return PTR_ERR_OR_ZERO(tcpci->port);
+	i2c_set_clientdata(client, tcpci);
+	return 0;
 }
 
 static int tcpci_remove(struct i2c_client *client)
 {
 	struct tcpci *tcpci = i2c_get_clientdata(client);
 
-	tcpm_unregister_port(tcpci->port);
+	tcpci_unregister_port(tcpci);
 
 	return 0;
 }
diff --git a/drivers/staging/typec/tcpci.h b/drivers/staging/typec/tcpci.h
index fdfb06c..c838723 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,16 @@
 #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);
+};
+
+struct tcpci *tcpci_register_port(struct device *dev, struct tcpci_data *data);
+void tcpci_unregister_port(struct tcpci *tcpci);
+int tcpci_irq(struct tcpci *tcpci);
+
 #endif /* __LINUX_USB_TCPCI_H */

^ permalink raw reply related	[flat|nested] 11+ messages in thread
* Re: [PATCH v2] staging: typec: handle vendor defined part and modify drp toggling flow
@ 2018-02-14 12:57   ` Greg KH
  0 siblings, 0 replies; 11+ messages in thread
From: Greg KH @ 2018-02-14 12:57 UTC (permalink / raw)
  To: ShuFanLee
  Cc: heikki.krogerus, linux, linux-kernel, linux-usb, cy_huang, shufan_lee

On Wed, Feb 14, 2018 at 08:45:34PM +0800, ShuFanLee wrote:
> From: ShuFanLee <shufan_lee@richtek.com>
> 
> Handle vendor defined behavior in tcpci_init, tcpci_set_vconn 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 chagned as following:
>   - Write DRP = 0 & Rd/Rd
>   - Write DRP = 1
>   - Set LOOK4CONNECTION command
> 
> Signed-off-by: ShuFanLee <shufan_lee@richtek.com>
> ---
>  drivers/staging/typec/tcpci.c | 111 ++++++++++++++++++++++++++++++++++--------
>  drivers/staging/typec/tcpci.h |  13 +++++
>  2 files changed, 103 insertions(+), 21 deletions(-)

What changed from v1?

And I have to ask, are there no ' ' characters in your name?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 11+ messages in thread
* [v2] staging: typec: handle vendor defined part and modify drp toggling flow
@ 2018-02-14 12:57   ` Greg KH
  0 siblings, 0 replies; 11+ messages in thread
From: Greg KH @ 2018-02-14 12:57 UTC (permalink / raw)
  To: ShuFanLee
  Cc: heikki.krogerus, linux, linux-kernel, linux-usb, cy_huang, shufan_lee

On Wed, Feb 14, 2018 at 08:45:34PM +0800, ShuFanLee wrote:
> From: ShuFanLee <shufan_lee@richtek.com>
> 
> Handle vendor defined behavior in tcpci_init, tcpci_set_vconn 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 chagned as following:
>   - Write DRP = 0 & Rd/Rd
>   - Write DRP = 1
>   - Set LOOK4CONNECTION command
> 
> Signed-off-by: ShuFanLee <shufan_lee@richtek.com>
> ---
>  drivers/staging/typec/tcpci.c | 111 ++++++++++++++++++++++++++++++++++--------
>  drivers/staging/typec/tcpci.h |  13 +++++
>  2 files changed, 103 insertions(+), 21 deletions(-)

What changed from v1?

And I have to ask, are there no ' ' characters in your name?

thanks,

greg k-h
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH v2] staging: typec: handle vendor defined part and modify drp toggling flow
  2018-02-14 12:57   ` [v2] " Greg KH
  (?)
@ 2018-02-14 14:43   ` 李書帆
  2018-02-14 15:37       ` [v2] " Greg KH
  -1 siblings, 1 reply; 11+ messages in thread
From: 李書帆 @ 2018-02-14 14:43 UTC (permalink / raw)
  To: Greg KH
  Cc: heikki.krogerus, Guenter Roeck, linux-kernel, linux-usb,
	cy_huang, shufan_lee

[-- Attachment #1: Type: text/plain, Size: 9731 bytes --]

Hi Gerg,


What changed from v1?

The diff between v1 and v2 is as following:

diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c

index 305a75d..8bccaa8 100644

--- a/drivers/staging/typec/tcpci.c

+++ b/drivers/staging/typec/tcpci.c

@@ -30,7 +30,7 @@ struct tcpci {

        bool controls_vbus;



        struct tcpc_dev tcpc;

-       struct tcpci_vendor_data *vdata;

+       struct tcpci_data *data;

 };



 static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev *tcpc)

@@ -38,29 +38,16 @@ static inline struct tcpci *tcpc_to_tcpci(struct
tcpc_dev *tcpc)
        return container_of(tcpc, struct tcpci, tcpc);


 }



-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));

 }

-EXPORT_SYMBOL_GPL(tcpci_read16);



-int tcpci_write16(struct tcpci *tcpci, unsigned int reg, u16 val)

+static int tcpci_write16(struct tcpci *tcpci, unsigned int reg, u16 val)

 {

        return regmap_raw_write(tcpci->regmap, reg, &val, sizeof(u16));

 }

-EXPORT_SYMBOL_GPL(tcpci_write16);

-

-int tcpci_read8(struct tcpci *tcpci, unsigned int reg, u8 *val)

-{

-       return regmap_raw_read(tcpci->regmap, reg, val, sizeof(u8));

-}

-EXPORT_SYMBOL_GPL(tcpci_read8);

-

-int tcpci_write8(struct tcpci *tcpci, unsigned int reg, u8 val)

-{

-       return regmap_raw_write(tcpci->regmap, reg, &val, sizeof(u8));

-}

-EXPORT_SYMBOL_GPL(tcpci_write8);



 static int tcpci_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status cc)

 {

@@ -206,6 +193,16 @@ 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) {

+               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)

@@ -352,9 +349,9 @@ static int tcpci_init(struct tcpc_dev *tcpc)

                return -ETIMEDOUT;



        /* Handle vendor init */

-       if (tcpci->vdata) {

-               if (tcpci->vdata->init) {

-                       ret = (*tcpci->vdata->init)(tcpci, tcpci->vdata);

+       if (tcpci->data) {

+               if (tcpci->data->init) {

+                       ret = tcpci->data->init(tcpci, tcpci->data);

                        if (ret < 0)

                                return ret;

                }

@@ -381,20 +378,20 @@ 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)

+static irqreturn_t _tcpci_irq(int irq, void *dev_id)

 {

        struct tcpci *tcpci = dev_id;

+

+       tcpci_irq(tcpci);

+       return IRQ_HANDLED;

+}

+

+int tcpci_irq(struct tcpci *tcpci)

+{

        u16 status;



        tcpci_read16(tcpci, TCPC_ALERT, &status);



-       /* Handle vendor defined interrupt */

-       if (tcpci->vdata) {

-               if (tcpci->vdata->irq_handler)

-                       (*tcpci->vdata->irq_handler)(tcpci, tcpci->vdata,

-                                       &status);

-       }

-

        /*

         * Clear alert status for everything except RX_STATUS, which
shouldn't
         * be cleared until we have successfully retrieved message.

@@ -456,6 +453,7 @@ static irqreturn_t tcpci_irq(int irq, void *dev_id)



        return IRQ_HANDLED;

 }


+EXPORT_SYMBOL_GPL(tcpci_irq);



 static const struct regmap_config tcpci_regmap_config = {

        .reg_bits = 8,

@@ -479,22 +477,18 @@ static int tcpci_parse_config(struct tcpci *tcpci)

        return 0;

 }



-struct tcpci *tcpci_register_port(struct i2c_client *client,

-                                 struct tcpci_vendor_data *vdata)

+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 ERR_PTR(-ENOMEM);



-       tcpci->client = client;

-       tcpci->dev = &client->dev;

-       tcpci->vdata = vdata;

-       tcpci->regmap = devm_regmap_init_i2c(client, &tcpci_regmap_config);

-       if (IS_ERR(tcpci->regmap))

-               return ERR_CAST(tcpci->regmap);

+       tcpci->dev = dev;

+       tcpci->data = data;

+       tcpci->regmap = data->regmap;



        tcpci->tcpc.init = tcpci_init;

        tcpci->tcpc.get_vbus = tcpci_get_vbus;

@@ -516,13 +510,6 @@ struct tcpci *tcpci_register_port(struct i2c_client
*client,
        /* Disable chip interrupts */

        tcpci_write16(tcpci, TCPC_ALERT_MASK, 0);



-       err = devm_request_threaded_irq(tcpci->dev, client->irq, NULL,

-                                       tcpci_irq,

-                                       IRQF_ONESHOT | IRQF_TRIGGER_LOW,

-                                       dev_name(tcpci->dev), tcpci);

-       if (err < 0)

-               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);

@@ -541,11 +528,28 @@ static int tcpci_probe(struct i2c_client *client,

                       const struct i2c_device_id *i2c_id)

 {

        struct tcpci *tcpci;

+       struct tcpci_data *data;

+       int err;

+

+       data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);

+       if (!data)

+               return -ENOMEM;



-       tcpci = tcpci_register_port(client, NULL);

+       data->regmap = devm_regmap_init_i2c(client, &tcpci_regmap_config);

+       if (IS_ERR(data->regmap))

+               return PTR_ERR(data->regmap);

+

+       tcpci = tcpci_register_port(&client->dev, data);

        if (PTR_ERR_OR_ZERO(tcpci))

                return PTR_ERR(tcpci);



+       err = devm_request_threaded_irq(&client->dev, client->irq, NULL,

+                                       _tcpci_irq,

+                                       IRQF_ONESHOT | IRQF_TRIGGER_LOW,

+                                       dev_name(tcpci->dev), tcpci);

+       if (err < 0)

+               return err;

+

        i2c_set_clientdata(client, tcpci);

        return 0;

 }

diff --git a/drivers/staging/typec/tcpci.h b/drivers/staging/typec/tcpci.h

index b1fcb4a..c838723 100644

--- a/drivers/staging/typec/tcpci.h

+++ b/drivers/staging/typec/tcpci.h

@@ -123,17 +123,15 @@

 #define TCPC_VBUS_VOLTAGE_ALARM_LO_CFG         0x78




 struct tcpci;

-struct tcpci_vendor_data {

-       int (*init)(struct tcpci *tcpci, struct tcpci_vendor_data *data);

-       int (*irq_handler)(struct tcpci *tcpci, struct tcpci_vendor_data
*data,
-                          u16 *status);

+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 tcpci_read16(struct tcpci *tcpci, unsigned int reg, u16 *val);

-int tcpci_write16(struct tcpci *tcpci, unsigned int reg, u16 val);

-int tcpci_read8(struct tcpci *tcpci, unsigned int reg, u8 *val);

-int tcpci_write8(struct tcpci *tcpci, unsigned int reg, u8 val);

-struct tcpci *tcpci_register_port(struct i2c_client *client,

-                                 struct tcpci_vendor_data *data);

+struct tcpci *tcpci_register_port(struct device *dev, struct tcpci_data
*data);
 void tcpci_unregister_port(struct tcpci *tcpci);

+int tcpci_irq(struct tcpci *tcpci);

+

 #endif /* __LINUX_USB_TCPCI_H */



And I have to ask, are there no ' ' characters in your name?
Yes, there should be a ' '. So the name is ShuFan Lee.

2018-02-14 20:57 GMT+08:00 Greg KH <greg@kroah.com>:

> On Wed, Feb 14, 2018 at 08:45:34PM +0800, ShuFanLee wrote:
> > From: ShuFanLee <shufan_lee@richtek.com>
> >
> > Handle vendor defined behavior in tcpci_init, tcpci_set_vconn 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 chagned as following:
> >   - Write DRP = 0 & Rd/Rd
> >   - Write DRP = 1
> >   - Set LOOK4CONNECTION command
> >
> > Signed-off-by: ShuFanLee <shufan_lee@richtek.com>
> > ---
> >  drivers/staging/typec/tcpci.c | 111 ++++++++++++++++++++++++++++++
> ++++--------
> >  drivers/staging/typec/tcpci.h |  13 +++++
> >  2 files changed, 103 insertions(+), 21 deletions(-)
>
> What changed from v1?
>
> And I have to ask, are there no ' ' characters in your name?
>
> thanks,
>
> greg k-h
>



-- 
Best Regards,
書帆

[-- Attachment #2: Type: text/html, Size: 33455 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH v2] staging: typec: handle vendor defined part and modify drp toggling flow
@ 2018-02-14 14:46   ` Guenter Roeck
  0 siblings, 0 replies; 11+ messages in thread
From: Guenter Roeck @ 2018-02-14 14:46 UTC (permalink / raw)
  To: ShuFanLee, heikki.krogerus; +Cc: linux-kernel, linux-usb, cy_huang, shufan_lee

On 02/14/2018 04:45 AM, ShuFanLee wrote:
> From: ShuFanLee <shufan_lee@richtek.com>
> 
> Handle vendor defined behavior in tcpci_init, tcpci_set_vconn 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 chagned as following:
>    - Write DRP = 0 & Rd/Rd
>    - Write DRP = 1
>    - Set LOOK4CONNECTION command
> 
> Signed-off-by: ShuFanLee <shufan_lee@richtek.com>
> ---
>   drivers/staging/typec/tcpci.c | 111 ++++++++++++++++++++++++++++++++++--------
>   drivers/staging/typec/tcpci.h |  13 +++++
>   2 files changed, 103 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
> index 9bd4412..461fbd48 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,7 @@ struct tcpci {
>   	bool controls_vbus;
>   
>   	struct tcpc_dev tcpc;
> +	struct tcpci_data *data;
>   };
>   
>   static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev *tcpc)
> @@ -98,8 +98,10 @@ 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;
> +	unsigned int reg = (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC1_SHIFT) |
> +			   (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC2_SHIFT);
>   
>   	switch (cc) {
>   	default:
> @@ -116,8 +118,19 @@ static int tcpci_start_drp_toggling(struct tcpc_dev *tcpc,
>   			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;
> +	usleep_range(500, 1000);
> +	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;
>   }
>   
>   static enum typec_cc_status tcpci_to_typec_cc(unsigned int cc, bool sink)
> @@ -178,6 +191,16 @@ 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) {
> +		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 +346,15 @@ static int tcpci_init(struct tcpc_dev *tcpc)
>   	if (time_after(jiffies, timeout))
>   		return -ETIMEDOUT;
>   
> +	/* Handle vendor init */
> +	if (tcpci->data) {
> +		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 +376,16 @@ 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)
> +static irqreturn_t _tcpci_irq(int irq, void *dev_id)
>   {
>   	struct tcpci *tcpci = dev_id;
> +
> +	tcpci_irq(tcpci);
> +	return IRQ_HANDLED;

This should probably be
	return tcpci_irq(tcpci);

> +}
> +
> +int tcpci_irq(struct tcpci *tcpci)

and this should be

irqreturn_t tcpci_irq(struct tcpci *tcpci)

Otherwise it doesn't add value to have tcpci_irq() return anything.
Alternative would be to make it

void tcpci_irq(struct tcpci *tcpci)

if it will always only return IRQ_HANDLED.

Guenter

> +{
>   	u16 status;
>   
>   	tcpci_read16(tcpci, TCPC_ALERT, &status);
> @@ -412,6 +451,7 @@ static irqreturn_t tcpci_irq(int irq, void *dev_id)
>   
>   	return IRQ_HANDLED;
>   }
> +EXPORT_SYMBOL_GPL(tcpci_irq);
>   
>   static const struct regmap_config tcpci_regmap_config = {
>   	.reg_bits = 8,
> @@ -435,22 +475,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 +503,60 @@ static int tcpci_probe(struct i2c_client *client,
>   
>   	err = tcpci_parse_config(tcpci);
>   	if (err < 0)
> -		return err;
> +		return ERR_PTR(err);
>   
>   	/* Disable chip interrupts */
>   	tcpci_write16(tcpci, TCPC_ALERT_MASK, 0);
>   
> -	err = devm_request_threaded_irq(tcpci->dev, client->irq, NULL,
> -					tcpci_irq,
> +	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 *tcpci;
> +	struct tcpci_data *data;
> +	int err;
> +
> +	data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->regmap = devm_regmap_init_i2c(client, &tcpci_regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return PTR_ERR(data->regmap);
> +
> +	tcpci = tcpci_register_port(&client->dev, data);
> +	if (PTR_ERR_OR_ZERO(tcpci))
> +		return PTR_ERR(tcpci);
> +
> +	err = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> +					_tcpci_irq,
>   					IRQF_ONESHOT | IRQF_TRIGGER_LOW,
>   					dev_name(tcpci->dev), tcpci);

I am a bit concerned that this may cause problems, since tcpm_register_port()
will send messages to the driver, which may trigger interrupts. It may be
necessary to register interrupts early, prior to calling tcpci_register_port().

>   	if (err < 0)
>   		return err;

You'll need to call tcpm_unregister_port() here (or register the interrupt first).

Guenter

>   
> -	tcpci->port = tcpm_register_port(tcpci->dev, &tcpci->tcpc);
> -	return PTR_ERR_OR_ZERO(tcpci->port);
> +	i2c_set_clientdata(client, tcpci);
> +	return 0;
>   }
>   
>   static int tcpci_remove(struct i2c_client *client)
>   {
>   	struct tcpci *tcpci = i2c_get_clientdata(client);
>   
> -	tcpm_unregister_port(tcpci->port);
> +	tcpci_unregister_port(tcpci);
>   
>   	return 0;
>   }
> diff --git a/drivers/staging/typec/tcpci.h b/drivers/staging/typec/tcpci.h
> index fdfb06c..c838723 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,16 @@
>   #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);
> +};
> +
> +struct tcpci *tcpci_register_port(struct device *dev, struct tcpci_data *data);
> +void tcpci_unregister_port(struct tcpci *tcpci);
> +int tcpci_irq(struct tcpci *tcpci);
> +
>   #endif /* __LINUX_USB_TCPCI_H */
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread
* [v2] staging: typec: handle vendor defined part and modify drp toggling flow
@ 2018-02-14 14:46   ` Guenter Roeck
  0 siblings, 0 replies; 11+ messages in thread
From: Guenter Roeck @ 2018-02-14 14:46 UTC (permalink / raw)
  To: ShuFanLee, heikki.krogerus; +Cc: linux-kernel, linux-usb, cy_huang, shufan_lee

On 02/14/2018 04:45 AM, ShuFanLee wrote:
> From: ShuFanLee <shufan_lee@richtek.com>
> 
> Handle vendor defined behavior in tcpci_init, tcpci_set_vconn 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 chagned as following:
>    - Write DRP = 0 & Rd/Rd
>    - Write DRP = 1
>    - Set LOOK4CONNECTION command
> 
> Signed-off-by: ShuFanLee <shufan_lee@richtek.com>
> ---
>   drivers/staging/typec/tcpci.c | 111 ++++++++++++++++++++++++++++++++++--------
>   drivers/staging/typec/tcpci.h |  13 +++++
>   2 files changed, 103 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
> index 9bd4412..461fbd48 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,7 @@ struct tcpci {
>   	bool controls_vbus;
>   
>   	struct tcpc_dev tcpc;
> +	struct tcpci_data *data;
>   };
>   
>   static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev *tcpc)
> @@ -98,8 +98,10 @@ 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;
> +	unsigned int reg = (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC1_SHIFT) |
> +			   (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC2_SHIFT);
>   
>   	switch (cc) {
>   	default:
> @@ -116,8 +118,19 @@ static int tcpci_start_drp_toggling(struct tcpc_dev *tcpc,
>   			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;
> +	usleep_range(500, 1000);
> +	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;
>   }
>   
>   static enum typec_cc_status tcpci_to_typec_cc(unsigned int cc, bool sink)
> @@ -178,6 +191,16 @@ 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) {
> +		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 +346,15 @@ static int tcpci_init(struct tcpc_dev *tcpc)
>   	if (time_after(jiffies, timeout))
>   		return -ETIMEDOUT;
>   
> +	/* Handle vendor init */
> +	if (tcpci->data) {
> +		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 +376,16 @@ 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)
> +static irqreturn_t _tcpci_irq(int irq, void *dev_id)
>   {
>   	struct tcpci *tcpci = dev_id;
> +
> +	tcpci_irq(tcpci);
> +	return IRQ_HANDLED;

This should probably be
	return tcpci_irq(tcpci);

> +}
> +
> +int tcpci_irq(struct tcpci *tcpci)

and this should be

irqreturn_t tcpci_irq(struct tcpci *tcpci)

Otherwise it doesn't add value to have tcpci_irq() return anything.
Alternative would be to make it

void tcpci_irq(struct tcpci *tcpci)

if it will always only return IRQ_HANDLED.

Guenter

> +{
>   	u16 status;
>   
>   	tcpci_read16(tcpci, TCPC_ALERT, &status);
> @@ -412,6 +451,7 @@ static irqreturn_t tcpci_irq(int irq, void *dev_id)
>   
>   	return IRQ_HANDLED;
>   }
> +EXPORT_SYMBOL_GPL(tcpci_irq);
>   
>   static const struct regmap_config tcpci_regmap_config = {
>   	.reg_bits = 8,
> @@ -435,22 +475,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 +503,60 @@ static int tcpci_probe(struct i2c_client *client,
>   
>   	err = tcpci_parse_config(tcpci);
>   	if (err < 0)
> -		return err;
> +		return ERR_PTR(err);
>   
>   	/* Disable chip interrupts */
>   	tcpci_write16(tcpci, TCPC_ALERT_MASK, 0);
>   
> -	err = devm_request_threaded_irq(tcpci->dev, client->irq, NULL,
> -					tcpci_irq,
> +	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 *tcpci;
> +	struct tcpci_data *data;
> +	int err;
> +
> +	data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->regmap = devm_regmap_init_i2c(client, &tcpci_regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return PTR_ERR(data->regmap);
> +
> +	tcpci = tcpci_register_port(&client->dev, data);
> +	if (PTR_ERR_OR_ZERO(tcpci))
> +		return PTR_ERR(tcpci);
> +
> +	err = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> +					_tcpci_irq,
>   					IRQF_ONESHOT | IRQF_TRIGGER_LOW,
>   					dev_name(tcpci->dev), tcpci);

I am a bit concerned that this may cause problems, since tcpm_register_port()
will send messages to the driver, which may trigger interrupts. It may be
necessary to register interrupts early, prior to calling tcpci_register_port().

>   	if (err < 0)
>   		return err;

You'll need to call tcpm_unregister_port() here (or register the interrupt first).

Guenter

>   
> -	tcpci->port = tcpm_register_port(tcpci->dev, &tcpci->tcpc);
> -	return PTR_ERR_OR_ZERO(tcpci->port);
> +	i2c_set_clientdata(client, tcpci);
> +	return 0;
>   }
>   
>   static int tcpci_remove(struct i2c_client *client)
>   {
>   	struct tcpci *tcpci = i2c_get_clientdata(client);
>   
> -	tcpm_unregister_port(tcpci->port);
> +	tcpci_unregister_port(tcpci);
>   
>   	return 0;
>   }
> diff --git a/drivers/staging/typec/tcpci.h b/drivers/staging/typec/tcpci.h
> index fdfb06c..c838723 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,16 @@
>   #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);
> +};
> +
> +struct tcpci *tcpci_register_port(struct device *dev, struct tcpci_data *data);
> +void tcpci_unregister_port(struct tcpci *tcpci);
> +int tcpci_irq(struct tcpci *tcpci);
> +
>   #endif /* __LINUX_USB_TCPCI_H */
>
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH v2] staging: typec: handle vendor defined part and modify drp toggling flow
  2018-02-14 14:46   ` [v2] " Guenter Roeck
  (?)
@ 2018-02-14 15:32   ` 李書帆
  -1 siblings, 0 replies; 11+ messages in thread
From: 李書帆 @ 2018-02-14 15:32 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: heikki.krogerus, linux-kernel, linux-usb, cy_huang, shufan_lee, Greg KH

[-- Attachment #1: Type: text/plain, Size: 11855 bytes --]

HI Guenter,

I've thought about registering interrupt early, prior to
tcpci_register_port. Just like you said.
But to make sure interrupt handler will not be invoked, we'll also need to
mask all alerts early.
And we'll need another structure used as dev_id of
devm_request_threaded_irq() because tcpci is not allocated yet at that time.
For example:

struct tcpci_chip {
        struct tcpci *tcpci;
        struct tcpci_data;
       ...
};

static int tcpci_probe(struct i2c_client *client,
                      const struct i2c_device_id *i2c_id)
{
        u16 val = 0;
        struct tcpci_chip *chip;

        chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
....
....
....
        /* Disable chip interrupts */
       regmap_raw_write(chip->data.regmap, TCPC_ALERT_MASK, &val,
sizeof(u16));

       err = devm_request_threaded_irq(&client->dev, client->irq, NULL,
                                       _tcpci_irq,
                                        IRQF_ONESHOT | IRQF_TRIGGER_LOW,
                                        dev_name(&client->dev), chip);
        if (err < 0)
                return err;

       chip->tcpci = tcpci_register_port(&client->dev, &chip->data);
       if (PTR_ERR_OR_ZERO(chip->tcpci))
               return PTR_ERR(chip->tcpci);

If you agree with this flow, I think this one will be better.


2018-02-14 22:46 GMT+08:00 Guenter Roeck <linux@roeck-us.net>:

> On 02/14/2018 04:45 AM, ShuFanLee wrote:
>
>> From: ShuFanLee <shufan_lee@richtek.com>
>>
>> Handle vendor defined behavior in tcpci_init, tcpci_set_vconn 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 chagned as following:
>>    - Write DRP = 0 & Rd/Rd
>>    - Write DRP = 1
>>    - Set LOOK4CONNECTION command
>>
>> Signed-off-by: ShuFanLee <shufan_lee@richtek.com>
>> ---
>>   drivers/staging/typec/tcpci.c | 111 ++++++++++++++++++++++++++++++
>> ++++--------
>>   drivers/staging/typec/tcpci.h |  13 +++++
>>   2 files changed, 103 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.
>> c
>> index 9bd4412..461fbd48 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,7 @@ struct tcpci {
>>         bool controls_vbus;
>>         struct tcpc_dev tcpc;
>> +       struct tcpci_data *data;
>>   };
>>     static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev *tcpc)
>> @@ -98,8 +98,10 @@ 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;
>> +       unsigned int reg = (TCPC_ROLE_CTRL_CC_RD <<
>> TCPC_ROLE_CTRL_CC1_SHIFT) |
>> +                          (TCPC_ROLE_CTRL_CC_RD <<
>> TCPC_ROLE_CTRL_CC2_SHIFT);
>>         switch (cc) {
>>         default:
>> @@ -116,8 +118,19 @@ static int tcpci_start_drp_toggling(struct tcpc_dev
>> *tcpc,
>>                         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;
>> +       usleep_range(500, 1000);
>> +       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;
>>   }
>>     static enum typec_cc_status tcpci_to_typec_cc(unsigned int cc, bool
>> sink)
>> @@ -178,6 +191,16 @@ 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) {
>> +               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 +346,15 @@ static int tcpci_init(struct tcpc_dev *tcpc)
>>         if (time_after(jiffies, timeout))
>>                 return -ETIMEDOUT;
>>   +     /* Handle vendor init */
>> +       if (tcpci->data) {
>> +               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 +376,16 @@ 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)
>> +static irqreturn_t _tcpci_irq(int irq, void *dev_id)
>>   {
>>         struct tcpci *tcpci = dev_id;
>> +
>> +       tcpci_irq(tcpci);
>> +       return IRQ_HANDLED;
>>
>
> This should probably be
>         return tcpci_irq(tcpci);
>
> +}
>> +
>> +int tcpci_irq(struct tcpci *tcpci)
>>
>
> and this should be
>
> irqreturn_t tcpci_irq(struct tcpci *tcpci)
>
> Otherwise it doesn't add value to have tcpci_irq() return anything.
> Alternative would be to make it
>
> void tcpci_irq(struct tcpci *tcpci)
>
> if it will always only return IRQ_HANDLED.
>
> Guenter
>
>
> +{
>>         u16 status;
>>         tcpci_read16(tcpci, TCPC_ALERT, &status);
>> @@ -412,6 +451,7 @@ static irqreturn_t tcpci_irq(int irq, void *dev_id)
>>         return IRQ_HANDLED;
>>   }
>> +EXPORT_SYMBOL_GPL(tcpci_irq);
>>     static const struct regmap_config tcpci_regmap_config = {
>>         .reg_bits = 8,
>> @@ -435,22 +475,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 +503,60 @@ static int tcpci_probe(struct i2c_client *client,
>>         err = tcpci_parse_config(tcpci);
>>         if (err < 0)
>> -               return err;
>> +               return ERR_PTR(err);
>>         /* Disable chip interrupts */
>>         tcpci_write16(tcpci, TCPC_ALERT_MASK, 0);
>>   -     err = devm_request_threaded_irq(tcpci->dev, client->irq, NULL,
>> -                                       tcpci_irq,
>> +       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 *tcpci;
>> +       struct tcpci_data *data;
>> +       int err;
>> +
>> +       data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
>> +       if (!data)
>> +               return -ENOMEM;
>> +
>> +       data->regmap = devm_regmap_init_i2c(client, &tcpci_regmap_config);
>> +       if (IS_ERR(data->regmap))
>> +               return PTR_ERR(data->regmap);
>> +
>> +       tcpci = tcpci_register_port(&client->dev, data);
>> +       if (PTR_ERR_OR_ZERO(tcpci))
>> +               return PTR_ERR(tcpci);
>> +
>> +       err = devm_request_threaded_irq(&client->dev, client->irq, NULL,
>> +                                       _tcpci_irq,
>>                                         IRQF_ONESHOT | IRQF_TRIGGER_LOW,
>>                                         dev_name(tcpci->dev), tcpci);
>>
>
> I am a bit concerned that this may cause problems, since
> tcpm_register_port()
> will send messages to the driver, which may trigger interrupts. It may be
> necessary to register interrupts early, prior to calling
> tcpci_register_port().
>
>         if (err < 0)
>>                 return err;
>>
>
> You'll need to call tcpm_unregister_port() here (or register the interrupt
> first).
>
> Guenter
>
>
>   -     tcpci->port = tcpm_register_port(tcpci->dev, &tcpci->tcpc);
>> -       return PTR_ERR_OR_ZERO(tcpci->port);
>> +       i2c_set_clientdata(client, tcpci);
>> +       return 0;
>>   }
>>     static int tcpci_remove(struct i2c_client *client)
>>   {
>>         struct tcpci *tcpci = i2c_get_clientdata(client);
>>   -     tcpm_unregister_port(tcpci->port);
>> +       tcpci_unregister_port(tcpci);
>>         return 0;
>>   }
>> diff --git a/drivers/staging/typec/tcpci.h b/drivers/staging/typec/tcpci.
>> h
>> index fdfb06c..c838723 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,16 @@
>>   #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);
>> +};
>> +
>> +struct tcpci *tcpci_register_port(struct device *dev, struct tcpci_data
>> *data);
>> +void tcpci_unregister_port(struct tcpci *tcpci);
>> +int tcpci_irq(struct tcpci *tcpci);
>> +
>>   #endif /* __LINUX_USB_TCPCI_H */
>>
>>
>


-- 
Best Regards,
書帆

[-- Attachment #2: Type: text/html, Size: 17458 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH v2] staging: typec: handle vendor defined part and modify drp toggling flow
@ 2018-02-14 15:37       ` Greg KH
  0 siblings, 0 replies; 11+ messages in thread
From: Greg KH @ 2018-02-14 15:37 UTC (permalink / raw)
  To: 李書帆
  Cc: heikki.krogerus, Guenter Roeck, linux-kernel, linux-usb,
	cy_huang, shufan_lee

On Wed, Feb 14, 2018 at 10:43:45PM +0800, 李書帆 wrote:
> Hi Gerg,
> 
> 
> What changed from v1?
> 
> The diff between v1 and v2 is as following:

<snip>

You need to explain what changed below the --- line, as is described in
Documentation/SubmittingPatches.

Please fix that up, and the text for your name, when you resend
version 3 of this patch.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 11+ messages in thread
* [v2] staging: typec: handle vendor defined part and modify drp toggling flow
@ 2018-02-14 15:37       ` Greg KH
  0 siblings, 0 replies; 11+ messages in thread
From: Greg KH @ 2018-02-14 15:37 UTC (permalink / raw)
  To: 李書帆
  Cc: heikki.krogerus, Guenter Roeck, linux-kernel, linux-usb,
	cy_huang, shufan_lee

On Wed, Feb 14, 2018 at 10:43:45PM +0800, 李書帆 wrote:
> Hi Gerg,
> 
> 
> What changed from v1?
> 
> The diff between v1 and v2 is as following:

<snip>

You need to explain what changed below the --- line, as is described in
Documentation/SubmittingPatches.

Please fix that up, and the text for your name, when you resend
version 3 of this patch.

thanks,

greg k-h
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH v2] staging: typec: handle vendor defined part and modify drp toggling flow
  2018-02-14 15:37       ` [v2] " Greg KH
  (?)
@ 2018-02-14 16:55       ` 李書帆
  -1 siblings, 0 replies; 11+ messages in thread
From: 李書帆 @ 2018-02-14 16:55 UTC (permalink / raw)
  To: Greg KH
  Cc: heikki.krogerus, Guenter Roeck, linux-kernel, linux-usb,
	cy_huang, shufan_lee

[-- Attachment #1: Type: text/plain, Size: 10470 bytes --]

Hi Gerg,

  Thank you for your time!

  I'll summarize the difference between v1, v2 and v3 below the --- line
and modify the format of my name in PATCH v3.

  For your reference, the detailed explanation of the difference between v1
and v2 is as following:

diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c

index 305a75d..8bccaa8 100644

--- a/drivers/staging/typec/tcpci.c

+++ b/drivers/staging/typec/tcpci.c

@@ -30,7 +30,7 @@ struct tcpci {

        bool controls_vbus;



        struct tcpc_dev tcpc;

-       struct tcpci_vendor_data *vdata;

+       struct tcpci_data *data;

 };


Change naming of structure of tcpci_vendor_data to tcpci_data
=============================================================

 static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev *tcpc)

@@ -38,29 +38,16 @@ static inline struct tcpci *tcpc_to_tcpci(struct
tcpc_dev *tcpc)
        return container_of(tcpc, struct tcpci, tcpc);


 }



-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));

 }

-EXPORT_SYMBOL_GPL(tcpci_read16);



-int tcpci_write16(struct tcpci *tcpci, unsigned int reg, u16 val)

+static int tcpci_write16(struct tcpci *tcpci, unsigned int reg, u16 val)

 {

        return regmap_raw_write(tcpci->regmap, reg, &val, sizeof(u16));

 }

-EXPORT_SYMBOL_GPL(tcpci_write16);

-

-int tcpci_read8(struct tcpci *tcpci, unsigned int reg, u8 *val)

-{

-       return regmap_raw_read(tcpci->regmap, reg, val, sizeof(u8));

-}

-EXPORT_SYMBOL_GPL(tcpci_read8);

-

-int tcpci_write8(struct tcpci *tcpci, unsigned int reg, u8 val)

-{

-       return regmap_raw_write(tcpci->regmap, reg, &val, sizeof(u8));

-}

-EXPORT_SYMBOL_GPL(tcpci_write8);


Remove exporting the above wrappers like Heikki suggested.
=============================================================



 static int tcpci_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status cc)

 {

@@ -206,6 +193,16 @@ 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) {

+               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)

Add set vconn ops for vendor to do things before standard set vconn
operation
e.x. It is necessary for RT1711H to disable idle mode before enabling vconn
=============================================================

@@ -352,9 +349,9 @@ static int tcpci_init(struct tcpc_dev *tcpc)

                return -ETIMEDOUT;



        /* Handle vendor init */

-       if (tcpci->vdata) {

-               if (tcpci->vdata->init) {

-                       ret = (*tcpci->vdata->init)(tcpci, tcpci->vdata);

+       if (tcpci->data) {

+               if (tcpci->data->init) {

+                       ret = tcpci->data->init(tcpci, tcpci->data);

                        if (ret < 0)

                                return ret;

                }

Fix the type error
=============================================================

@@ -381,20 +378,20 @@ 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)

+static irqreturn_t _tcpci_irq(int irq, void *dev_id)

 {

        struct tcpci *tcpci = dev_id;

+

+       tcpci_irq(tcpci);

+       return IRQ_HANDLED;

+}

+

+int tcpci_irq(struct tcpci *tcpci)

+{

        u16 status;



        tcpci_read16(tcpci, TCPC_ALERT, &status);



-       /* Handle vendor defined interrupt */

-       if (tcpci->vdata) {

-               if (tcpci->vdata->irq_handler)

-                       (*tcpci->vdata->irq_handler)(tcpci, tcpci->vdata,

-                                       &status);

-       }

-

        /*

         * Clear alert status for everything except RX_STATUS, which
shouldn't
         * be cleared until we have successfully retrieved message.

@@ -456,6 +453,7 @@ static irqreturn_t tcpci_irq(int irq, void *dev_id)



        return IRQ_HANDLED;

 }


+EXPORT_SYMBOL_GPL(tcpci_irq);


As Heikki suggested, I export tcpci_irq so that vendor can register their
own IRQ handler and call tcpci_irq in their IRQ handler
=============================================================


 static const struct regmap_config tcpci_regmap_config = {

        .reg_bits = 8,

@@ -479,22 +477,18 @@ static int tcpci_parse_config(struct tcpci *tcpci)

        return 0;

 }


-struct tcpci *tcpci_register_port(struct i2c_client *client,

-                                 struct tcpci_vendor_data *vdata)

+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 ERR_PTR(-ENOMEM);



-       tcpci->client = client;

-       tcpci->dev = &client->dev;

-       tcpci->vdata = vdata;

-       tcpci->regmap = devm_regmap_init_i2c(client, &tcpci_regmap_config);

-       if (IS_ERR(tcpci->regmap))

-               return ERR_CAST(tcpci->regmap);

+       tcpci->dev = dev;

+       tcpci->data = data;

+       tcpci->regmap = data->regmap;



        tcpci->tcpc.init = tcpci_init;

        tcpci->tcpc.get_vbus = tcpci_get_vbus;

@@ -516,13 +510,6 @@ struct tcpci *tcpci_register_port(struct i2c_client
*client,
        /* Disable chip interrupts */

        tcpci_write16(tcpci, TCPC_ALERT_MASK, 0);



-       err = devm_request_threaded_irq(tcpci->dev, client->irq, NULL,

-                                       tcpci_irq,

-                                       IRQF_ONESHOT | IRQF_TRIGGER_LOW,

-                                       dev_name(tcpci->dev), tcpci);

-       if (err < 0)

-               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);

@@ -541,11 +528,28 @@ static int tcpci_probe(struct i2c_client *client,

                       const struct i2c_device_id *i2c_id)

 {

        struct tcpci *tcpci;

+       struct tcpci_data *data;

+       int err;

+

+       data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);

+       if (!data)

+               return -ENOMEM;



-       tcpci = tcpci_register_port(client, NULL);

+       data->regmap = devm_regmap_init_i2c(client, &tcpci_regmap_config);

+       if (IS_ERR(data->regmap))

+               return PTR_ERR(data->regmap);

+

+       tcpci = tcpci_register_port(&client->dev, data);

        if (PTR_ERR_OR_ZERO(tcpci))

                return PTR_ERR(tcpci);



+       err = devm_request_threaded_irq(&client->dev, client->irq, NULL,

+                                       _tcpci_irq,

+                                       IRQF_ONESHOT | IRQF_TRIGGER_LOW,

+                                       dev_name(tcpci->dev), tcpci);

+       if (err < 0)

+               return err;

+

        i2c_set_clientdata(client, tcpci);

        return 0;

 }

Remove i2c_client from tcpci
Move regmap_init & request_threaded_irq to the glue driver
Note: request_threaded_irq may need to be done earlier, prior to
tcpci_register_port. This is still under discussion.
=============================================================


diff --git a/drivers/staging/typec/tcpci.h b/drivers/staging/typec/tcpci.h

index b1fcb4a..c838723 100644

--- a/drivers/staging/typec/tcpci.h

+++ b/drivers/staging/typec/tcpci.h

@@ -123,17 +123,15 @@

 #define TCPC_VBUS_VOLTAGE_ALARM_LO_CFG         0x78




 struct tcpci;

-struct tcpci_vendor_data {

-       int (*init)(struct tcpci *tcpci, struct tcpci_vendor_data *data);

-       int (*irq_handler)(struct tcpci *tcpci, struct tcpci_vendor_data
*data,
-                          u16 *status);

+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 tcpci_read16(struct tcpci *tcpci, unsigned int reg, u16 *val);

-int tcpci_write16(struct tcpci *tcpci, unsigned int reg, u16 val);

-int tcpci_read8(struct tcpci *tcpci, unsigned int reg, u8 *val);

-int tcpci_write8(struct tcpci *tcpci, unsigned int reg, u8 val);

-struct tcpci *tcpci_register_port(struct i2c_client *client,

-                                 struct tcpci_vendor_data *data);

+struct tcpci *tcpci_register_port(struct device *dev, struct tcpci_data
*data);
 void tcpci_unregister_port(struct tcpci *tcpci);

+int tcpci_irq(struct tcpci *tcpci);

+

 #endif /* __LINUX_USB_TCPCI_H */

Modify structure of tcpci_vendor_data to tcpci_data
Only declare tcpci_register_port & tcpci_unregister_port & tcpci_irq in
header file
=============================================================



2018-02-14 23:37 GMT+08:00 Greg KH <greg@kroah.com>:

> On Wed, Feb 14, 2018 at 10:43:45PM +0800, 李書帆 wrote:
> > Hi Gerg,
> >
> >
> > What changed from v1?
> >
> > The diff between v1 and v2 is as following:
>
> <snip>
>
> You need to explain what changed below the --- line, as is described in
> Documentation/SubmittingPatches.
>
> Please fix that up, and the text for your name, when you resend
> version 3 of this patch.
>
> thanks,
>
> greg k-h
>



-- 
Best Regards,
書帆

[-- Attachment #2: Type: text/html, Size: 32797 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-02-14 16:55 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-14 12:45 [PATCH v2] staging: typec: handle vendor defined part and modify drp toggling flow ShuFanLee
2018-02-14 12:45 ` [v2] " ShuFanLee
2018-02-14 12:57 ` [PATCH v2] " Greg KH
2018-02-14 12:57   ` [v2] " Greg KH
2018-02-14 14:43   ` [PATCH v2] " 李書帆
2018-02-14 15:37     ` Greg KH
2018-02-14 15:37       ` [v2] " Greg KH
2018-02-14 16:55       ` [PATCH v2] " 李書帆
2018-02-14 14:46 ` Guenter Roeck
2018-02-14 14:46   ` [v2] " Guenter Roeck
2018-02-14 15:32   ` [PATCH v2] " 李書帆

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.