[PATCH v5 00/14] staging: typec: tcpci: move out of staging

[PATCH v5 00/14] staging: typec: tcpci: move out of staging

[Li Jun]

This patch set attempts to move the tcpci driver out of staging by fix
some tcpci driver issues and define typec and power delivery device
properties, the changes are verified on NXP PTN5110, which is a standard
tcpci typec port controller device with power delivery support, tested
power source and sink with drp config.

Changes for v5:
- Use "power-role" and "data-role" for typec port power and data capability
  property name.  
- Use macro defintion instead of cryptic numbers for readability of
  power delivery properties(PDO).
- Add one tcpci driver binding string to be "nxp,ptn5110" to follow binding
  name rule "<vendor prefix>,<device>".
- Change operating power property name to be op-sink-microwatt.
- Add one patch(remove unused tcpci_tcpc_config) to resolve bisectablitity
  issue of patch usb: typec: add fwnode to tcpc.
- A minor error handling change to keep the error path and success path
  separate in patch: register port before request irq
- Rebase to latest Greg's tree with max_snk_* removed.

Changes for v4:
- Remove max-sink-* properties as we will purge max_snk_* in tcpm,
  see patch set[4].
- Get typec power and data type value via name string(patch 5).
- Move finding typec and pd properties code from tcpci to tcpm(patch 6)
- Add a compatible string for nxp ptn5110 typec controller in tcpci driver.
  (patch 3)
- Add set cc for drp toggling without try.src/snk in tcpm(patch 10), then
  patch 11 can only update CCx bits for keep disconnect cc line open.
- Update op-sink-microwatt-hours example value to be the right value in
  micorwatts, and accordingly divide 1000 to get its miliwatts value
  in patch 6.
- Add Guenter's Reviewed-by for patch(8/9/12)

[4] https://www.spinics.net/lists/linux-usb/msg167261.html

Changes for v3:
- Use 2 properties to separate power and data capability of typec port:
  "power-type" and "data-type", this is based on Heikki's typec class code
  change[2]. use "try-power-role" to present if the typec port can support
  Try.SNK or Try.SRC.
- 4 sink properties(max_sink_mv/ma/mw and op_sink_mw) are kept because the
  counterpart code is back, see revert patch[3], meanwhile I post a patch
  to fix the reported problem of current source pdo select machinism(which
  completely ignored those 4 sink settings), to see if we can keep current
  code, once it was discussed and have conclusion I can update this
  accordingly.
- Use fwnode to get the connector node for dt setting parse.

Main changes for v2:
- Typec properties are based on general usb connector bindings[1] proposed
  by Andrzej Hajda, use the standard unit suffixes as defined in
  property-units.txt.
- Add 2 infra APIs to get power sink and source config from dt.
- Don't change the set_cc api, to keep the uncontacted cc line open,
  set cc1/cc2 to be open in tcpci driver when set polarity.
- Directly enable vbus detect in tcpci driver rather than add a API.
- Details added in each patch.

[1] https://patchwork.kernel.org/patch/10231447/
[2] https://patchwork.kernel.org/patch/10276483/
[3] https://www.spinics.net/lists/linux-usb/msg166366.html

Li Jun (13):
  dt-bindings: connector: add properties for typec
  dt-bindings: usb: add documentation for typec port controller(TCPCI)
  staging: typec: tcpci: add compatible string for nxp ptn5110
  usb: typec: add fwnode to tcpc
  usb: typec: add API to get typec basic port power and data config
  usb: typec: tcpm: support get typec and pd config from device
    properties
  staging: typec: tcpci: remove unused tcpci_tcpc_config
  staging: typec: tcpci: enable vbus detection
  typec: tcpm: add starting value for drp toggling
  usb: typec: tcpm: set cc for drp toggling attach
  staging: typec: tcpci: keep the not connecting cc line open
  staging: typec: tcpci: Only touch target bit when enable vconn
  staging: typec: tcpci: move tcpci driver out of staging

Peter Chen (1):
  staging: typec: tcpci: register port before request irq

 .../bindings/connector/usb-connector.txt           |  44 ++++++
 .../devicetree/bindings/usb/typec-tcpci.txt        |  49 +++++++
 drivers/staging/Kconfig                            |   2 -
 drivers/staging/Makefile                           |   1 -
 drivers/staging/typec/TODO                         |   5 -
 drivers/usb/typec/Kconfig                          |   7 +
 drivers/usb/typec/Makefile                         |   1 +
 drivers/usb/typec/class.c                          |  52 +++++++
 drivers/{staging => usb}/typec/tcpci.c             |  63 +++++----
 drivers/{staging => usb}/typec/tcpci.h             |   0
 drivers/usb/typec/tcpm.c                           | 149 +++++++++++++++++----
 include/dt-bindings/usb/pd.h                       |  62 +++++++++
 include/linux/usb/tcpm.h                           |   2 +
 include/linux/usb/typec.h                          |   3 +
 14 files changed, 382 insertions(+), 58 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/typec-tcpci.txt
 delete mode 100644 drivers/staging/typec/TODO
 rename drivers/{staging => usb}/typec/tcpci.c (93%)
 rename drivers/{staging => usb}/typec/tcpci.h (100%)
 create mode 100644 include/dt-bindings/usb/pd.h

-- 
2.7.4

[PATCH v5 01/14] dt-bindings: connector: add properties for typec

[Li Jun]

Add bingdings supported by current typec driver, so user can pass
all those properties via dt.

Signed-off-by: Li Jun <jun.li@nxp.com>
---
 .../bindings/connector/usb-connector.txt           | 44 +++++++++++++++
 include/dt-bindings/usb/pd.h                       | 62 ++++++++++++++++++++++
 2 files changed, 106 insertions(+)

diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt
index e1463f1..4b19de6d0 100644
--- a/Documentation/devicetree/bindings/connector/usb-connector.txt
+++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
@@ -15,6 +15,33 @@ Optional properties:
 - type: size of the connector, should be specified in case of USB-A, USB-B
   non-fullsize connectors: "mini", "micro".
 
+Optional properties for usb-c-connector:
+- power-role: should be one of "source", "sink" or "dual"(DRP) if typec
+  connector has power support.
+- try-power-role: preferred power role if "dual"(DRP) can support Try.SNK
+  or Try.SRC, should be "sink" for Try.SNK or "source" for Try.SRC.
+- data-role: should be one of "host", "device", "dual"(DRD) if typec
+  connector supports USB data.
+
+Required properties for usb-c-connector with power delivery support:
+- source-pdos: An array of u32 with each entry providing supported power
+  source data object(PDO), the detailed bit definitions of PDO can be found
+  in "Universal Serial Bus Power Delivery Specification" chapter 6.4.1.2
+  Source_Capabilities Message, the order of each entry(PDO) should follow
+  the PD spec chapter 6.4.1. Required for power source and power dual role.
+  User can specify the source PDO array via PDO_FIXED/BATT/VAR() defined in
+  dt-bindings/usb/pd.h.
+- sink-pdos: An array of u32 with each entry providing supported power
+  sink data object(PDO), the detailed bit definitions of PDO can be found
+  in "Universal Serial Bus Power Delivery Specification" chapter 6.4.1.3
+  Sink Capabilities Message, the order of each entry(PDO) should follow
+  the PD spec chapter 6.4.1. Required for power sink and power dual role.
+  User can specify the sink PDO array via PDO_FIXED/BATT/VAR() defined in
+  dt-bindings/usb/pd.h.
+- op-sink-microwatt: Sink required operating power in microwatt, if source
+  can't offer the power, Capability Mismatch is set, required for power
+  sink and power dual role.
+
 Required nodes:
 - any data bus to the connector should be modeled using the OF graph bindings
   specified in bindings/graph.txt, unless the bus is between parent node and
@@ -73,3 +100,20 @@ ccic: s2mm005@33 {
 		};
 	};
 };
+
+3. USB-C connector attached to a typec port controller(ptn5110), which has
+power delivery support and enables drp.
+
+typec: ptn5110@50 {
+	...
+	usb_con: connector {
+		compatible = "usb-c-connector";
+		label = "USB-C";
+		power-role = "dual";
+		try-power-role = "sink";
+		source-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM)>;
+		sink-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM)
+			     PDO_VAR(5000, 12000, 2000)>;
+		op-sink-microwatt = <10000000>;
+	};
+};
diff --git a/include/dt-bindings/usb/pd.h b/include/dt-bindings/usb/pd.h
new file mode 100644
index 0000000..7b7a92f
--- /dev/null
+++ b/include/dt-bindings/usb/pd.h
@@ -0,0 +1,62 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __DT_POWER_DELIVERY_H
+#define __DT_POWER_DELIVERY_H
+
+/* Power delivery Power Data Object definitions */
+#define PDO_TYPE_FIXED		0
+#define PDO_TYPE_BATT		1
+#define PDO_TYPE_VAR		2
+#define PDO_TYPE_APDO		3
+
+#define PDO_TYPE_SHIFT		30
+#define PDO_TYPE_MASK		0x3
+
+#define PDO_TYPE(t)	((t) << PDO_TYPE_SHIFT)
+
+#define PDO_VOLT_MASK		0x3ff
+#define PDO_CURR_MASK		0x3ff
+#define PDO_PWR_MASK		0x3ff
+
+#define PDO_FIXED_DUAL_ROLE	(1 << 29) /* Power role swap supported */
+#define PDO_FIXED_SUSPEND	(1 << 28) /* USB Suspend supported (Source) */
+#define PDO_FIXED_HIGHER_CAP	(1 << 28) /* Requires more than vSafe5V (Sink) */
+#define PDO_FIXED_EXTPOWER	(1 << 27) /* Externally powered */
+#define PDO_FIXED_USB_COMM	(1 << 26) /* USB communications capable */
+#define PDO_FIXED_DATA_SWAP	(1 << 25) /* Data role swap supported */
+#define PDO_FIXED_VOLT_SHIFT	10	/* 50mV units */
+#define PDO_FIXED_CURR_SHIFT	0	/* 10mA units */
+
+#define PDO_FIXED_VOLT(mv)	((((mv) / 50) & PDO_VOLT_MASK) << PDO_FIXED_VOLT_SHIFT)
+#define PDO_FIXED_CURR(ma)	((((ma) / 10) & PDO_CURR_MASK) << PDO_FIXED_CURR_SHIFT)
+
+#define PDO_FIXED(mv, ma, flags)			\
+	(PDO_TYPE(PDO_TYPE_FIXED) | (flags) |		\
+	 PDO_FIXED_VOLT(mv) | PDO_FIXED_CURR(ma))
+
+#define VSAFE5V 5000 /* mv units */
+
+#define PDO_BATT_MAX_VOLT_SHIFT	20	/* 50mV units */
+#define PDO_BATT_MIN_VOLT_SHIFT	10	/* 50mV units */
+#define PDO_BATT_MAX_PWR_SHIFT	0	/* 250mW units */
+
+#define PDO_BATT_MIN_VOLT(mv) ((((mv) / 50) & PDO_VOLT_MASK) << PDO_BATT_MIN_VOLT_SHIFT)
+#define PDO_BATT_MAX_VOLT(mv) ((((mv) / 50) & PDO_VOLT_MASK) << PDO_BATT_MAX_VOLT_SHIFT)
+#define PDO_BATT_MAX_POWER(mw) ((((mw) / 250) & PDO_PWR_MASK) << PDO_BATT_MAX_PWR_SHIFT)
+
+#define PDO_BATT(min_mv, max_mv, max_mw)			\
+	(PDO_TYPE(PDO_TYPE_BATT) | PDO_BATT_MIN_VOLT(min_mv) |	\
+	 PDO_BATT_MAX_VOLT(max_mv) | PDO_BATT_MAX_POWER(max_mw))
+
+#define PDO_VAR_MAX_VOLT_SHIFT	20	/* 50mV units */
+#define PDO_VAR_MIN_VOLT_SHIFT	10	/* 50mV units */
+#define PDO_VAR_MAX_CURR_SHIFT	0	/* 10mA units */
+
+#define PDO_VAR_MIN_VOLT(mv) ((((mv) / 50) & PDO_VOLT_MASK) << PDO_VAR_MIN_VOLT_SHIFT)
+#define PDO_VAR_MAX_VOLT(mv) ((((mv) / 50) & PDO_VOLT_MASK) << PDO_VAR_MAX_VOLT_SHIFT)
+#define PDO_VAR_MAX_CURR(ma) ((((ma) / 10) & PDO_CURR_MASK) << PDO_VAR_MAX_CURR_SHIFT)
+
+#define PDO_VAR(min_mv, max_mv, max_ma)				\
+	(PDO_TYPE(PDO_TYPE_VAR) | PDO_VAR_MIN_VOLT(min_mv) |	\
+	 PDO_VAR_MAX_VOLT(max_mv) | PDO_VAR_MAX_CURR(max_ma))
+
+ #endif /* __DT_POWER_DELIVERY_H */
-- 
2.7.4

[PATCH v5 02/14] dt-bindings: usb: add documentation for typec port controller(TCPCI)

[Li Jun]

TCPCI stands for typec port controller interface, its implementation
has full typec port control with power delivery support, it's a
standard i2c slave with GPIO input as irq interface, detail see spec
"Universal Serial Bus Type-C Port Controller Interface Specification
Revision 1.0, Version 1.1"

Signed-off-by: Li Jun <jun.li@nxp.com>
---
 .../devicetree/bindings/usb/typec-tcpci.txt        | 49 ++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/typec-tcpci.txt b/Documentation/devicetree/bindings/usb/typec-tcpci.txt
new file mode 100644
index 0000000..0dd1469
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/typec-tcpci.txt
@@ -0,0 +1,49 @@
+TCPCI(Typec port cotroller interface) binding
+---------------------------------------------
+
+Required properties:
+- compatible:       should be set one of following:
+		    - "nxp,ptn5110" for NXP USB PD TCPC PHY IC ptn5110.
+
+- reg:              the i2c slave address of typec port controller device.
+- interrupt-parent: the phandle to the interrupt controller which provides
+                    the interrupt.
+- interrupts:       interrupt specification for tcpci alert.
+
+Required sub-node:
+- connector: The "usb-c-connector" attached to the tcpci chip, the bindings
+  of connector node are specified in
+  Documentation/devicetree/bindings/connector/usb-connector.txt
+
+Example:
+
+ptn5110@50 {
+	compatible = "nxp,ptn5110";
+	reg = <0x50>;
+	interrupt-parent = <&gpio3>;
+	interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
+
+	usb_con: connector {
+		compatible = "usb-c-connector";
+		label = "USB-C";
+		data-role = "dual";
+		power-role = "dual";
+		try-power-role = "sink";
+		source-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM)>;
+		sink-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM)
+			     PDO_VAR(5000, 12000, 2000)>;
+		op-sink-microwatt = <10000000>;
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@1 {
+				reg = <1>;
+				usb_con_ss: endpoint {
+					remote-endpoint = <&usb3_data_ss>;
+				};
+			};
+		};
+	};
+};
-- 
2.7.4

[PATCH v5 03/14] staging: typec: tcpci: add compatible string for nxp ptn5110

[Li Jun]

Add nxp ptn5110 typec controller compatible string: usb-tcpci,ptn5110,
which is a standard tcpci chip with power delivery support.

Signed-off-by: Li Jun <jun.li@nxp.com>
---
 drivers/staging/typec/tcpci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
index 076d97e..741a80a 100644
--- a/drivers/staging/typec/tcpci.c
+++ b/drivers/staging/typec/tcpci.c
@@ -576,6 +576,7 @@ MODULE_DEVICE_TABLE(i2c, tcpci_id);
 #ifdef CONFIG_OF
 static const struct of_device_id tcpci_of_match[] = {
 	{ .compatible = "usb,tcpci", },
+	{ .compatible = "nxp,ptn5110", },
 	{},
 };
 MODULE_DEVICE_TABLE(of, tcpci_of_match);
-- 
2.7.4

[PATCH v5 04/14] usb: typec: add fwnode to tcpc

[Li Jun]

Add fwnode handle to get the fwnode so we can get typec configs
it contains.

Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Li Jun <jun.li@nxp.com>
---
 drivers/staging/typec/tcpci.c | 7 +++++++
 drivers/usb/typec/tcpm.c      | 1 +
 include/linux/usb/tcpm.h      | 2 ++
 3 files changed, 10 insertions(+)

diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
index 741a80a..01d14fb 100644
--- a/drivers/staging/typec/tcpci.c
+++ b/drivers/staging/typec/tcpci.c
@@ -10,6 +10,7 @@
 #include <linux/module.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
+#include <linux/property.h>
 #include <linux/regmap.h>
 #include <linux/usb/pd.h>
 #include <linux/usb/tcpm.h>
@@ -474,6 +475,12 @@ static int tcpci_parse_config(struct tcpci *tcpci)
 
 	/* TODO: Populate struct tcpc_config from ACPI/device-tree */
 	tcpci->tcpc.config = &tcpci_tcpc_config;
+	tcpci->tcpc.fwnode = device_get_named_child_node(tcpci->dev,
+							 "connector");
+	if (!tcpci->tcpc.fwnode) {
+		dev_err(tcpci->dev, "Can't find connector node.\n");
+		return -EINVAL;
+	}
 
 	return 0;
 }
diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
index 7ee417a..80b2303 100644
--- a/drivers/usb/typec/tcpm.c
+++ b/drivers/usb/typec/tcpm.c
@@ -4579,6 +4579,7 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
 	else
 		port->try_role = TYPEC_NO_PREFERRED_ROLE;
 
+	port->typec_caps.fwnode = tcpc->fwnode;
 	port->typec_caps.prefer_role = tcpc->config->default_role;
 	port->typec_caps.type = tcpc->config->type;
 	port->typec_caps.data = tcpc->config->data;
diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
index b231b93..193920a 100644
--- a/include/linux/usb/tcpm.h
+++ b/include/linux/usb/tcpm.h
@@ -110,6 +110,7 @@ enum tcpc_mux_mode {
 /**
  * struct tcpc_dev - Port configuration and callback functions
  * @config:	Pointer to port configuration
+ * @fwnode:	Pointer to port fwnode
  * @get_vbus:	Called to read current VBUS state
  * @get_current_limit:
  *		Optional; called by the tcpm core when configured as a snk
@@ -138,6 +139,7 @@ enum tcpc_mux_mode {
  */
 struct tcpc_dev {
 	const struct tcpc_config *config;
+	struct fwnode_handle *fwnode;
 
 	int (*init)(struct tcpc_dev *dev);
 	int (*get_vbus)(struct tcpc_dev *dev);
-- 
2.7.4

[PATCH v5 05/14] usb: typec: add API to get typec basic port power and data config

[Li Jun]

This patch adds 3 APIs to get the typec port power and data type,
and preferred power role by its name string.

Signed-off-by: Li Jun <jun.li@nxp.com>
---
 drivers/usb/typec/class.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/usb/typec.h |  3 +++
 2 files changed, 55 insertions(+)

diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 53df10d..5981e18 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -9,6 +9,7 @@
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/property.h>
 #include <linux/slab.h>
 #include <linux/usb/typec.h>
 #include <linux/usb/typec_mux.h>
@@ -802,6 +803,12 @@ static const char * const typec_port_types[] = {
 	[TYPEC_PORT_DRP] = "dual",
 };
 
+static const char * const typec_data_types[] = {
+	[TYPEC_PORT_DFP] = "host",
+	[TYPEC_PORT_UFP] = "device",
+	[TYPEC_PORT_DRD] = "dual",
+};
+
 static const char * const typec_port_types_drp[] = {
 	[TYPEC_PORT_SRC] = "dual [source] sink",
 	[TYPEC_PORT_SNK] = "dual source [sink]",
@@ -1252,6 +1259,51 @@ void typec_set_pwr_opmode(struct typec_port *port,
 }
 EXPORT_SYMBOL_GPL(typec_set_pwr_opmode);
 
+/**
+ * typec_find_power_type - Get the typec port power type
+ * @name: port type string
+ *
+ * This routine is used to find the typec_port_type by its string name.
+ *
+ * Returns typec_port_type if success, otherwise negative error code.
+ */
+int typec_find_power_type(const char *name)
+{
+	return match_string(typec_port_types, ARRAY_SIZE(typec_port_types),
+			    name);
+}
+EXPORT_SYMBOL_GPL(typec_find_power_type);
+
+/**
+ * typec_find_preferred_role - Find the typec drp port preferred power role
+ * @name: power role string
+ *
+ * This routine is used to find the typec_role by its string name of
+ * preferred power role(Try.SRC or Try.SNK).
+ *
+ * Returns typec_role if success, otherwise negative error code.
+ */
+int typec_find_preferred_role(const char *name)
+{
+	return match_string(typec_roles, ARRAY_SIZE(typec_roles), name);
+}
+EXPORT_SYMBOL_GPL(typec_find_preferred_role);
+
+/**
+ * typec_find_data_type - Get the typec port data capability
+ * @name: data type string
+ *
+ * This routine is used to find the typec_port_data by its string name.
+ *
+ * Returns typec_port_data if success, otherwise negative error code.
+ */
+int typec_find_data_type(const char *name)
+{
+	return match_string(typec_data_types, ARRAY_SIZE(typec_data_types),
+			    name);
+}
+EXPORT_SYMBOL_GPL(typec_find_data_type);
+
 /* ------------------------------------------ */
 /* API for Multiplexer/DeMultiplexer Switches */
 
diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
index 672b39b..00c93e7 100644
--- a/include/linux/usb/typec.h
+++ b/include/linux/usb/typec.h
@@ -267,4 +267,7 @@ int typec_set_orientation(struct typec_port *port,
 			  enum typec_orientation orientation);
 int typec_set_mode(struct typec_port *port, int mode);
 
+int typec_find_power_type(const char *name);
+int typec_find_preferred_role(const char *name);
+int typec_find_data_type(const char *name);
 #endif /* __LINUX_USB_TYPEC_H */
-- 
2.7.4

[PATCH v5 06/14] usb: typec: tcpm: support get typec and pd config from device properties

[Li Jun]

This patch adds support of get typec and power delivery config from
firmware description.

Signed-off-by: Li Jun <jun.li@nxp.com>
---
 drivers/usb/typec/tcpm.c | 133 +++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 111 insertions(+), 22 deletions(-)

diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
index 80b2303..becedcf 100644
--- a/drivers/usb/typec/tcpm.c
+++ b/drivers/usb/typec/tcpm.c
@@ -14,6 +14,7 @@
 #include <linux/mutex.h>
 #include <linux/power_supply.h>
 #include <linux/proc_fs.h>
+#include <linux/property.h>
 #include <linux/sched/clock.h>
 #include <linux/seq_file.h>
 #include <linux/slab.h>
@@ -4240,6 +4241,81 @@ static int tcpm_copy_vdos(u32 *dest_vdo, const u32 *src_vdo,
 	return nr_vdo;
 }
 
+static int tcpm_fw_get_caps(struct tcpm_port *port,
+			    struct fwnode_handle *fwnode)
+{
+	const char *cap_str;
+	int ret;
+	u32 mw;
+
+	if (!port || !fwnode)
+		return -EINVAL;
+
+	/* USB data support is optional */
+	ret = fwnode_property_read_string(fwnode, "data-role", &cap_str);
+	if (ret == 0) {
+		port->typec_caps.data = typec_find_data_type(cap_str);
+		if (port->typec_caps.data < 0)
+			return -EINVAL;
+	}
+
+	ret = fwnode_property_read_string(fwnode, "power-role", &cap_str);
+	if (ret < 0)
+		return ret;
+
+	port->typec_caps.type = typec_find_power_type(cap_str);
+	if (port->typec_caps.type < 0)
+		return -EINVAL;
+	port->port_type = port->typec_caps.type;
+
+	if (port->port_type == TYPEC_PORT_SNK)
+		goto sink;
+
+	/* Get soruce pdos */
+	ret = fwnode_property_read_u32_array(fwnode, "source-pdos",
+					     NULL, 0);
+	if (ret <= 0)
+		return -EINVAL;
+
+	port->nr_src_pdo = min(ret, PDO_MAX_OBJECTS);
+	ret = fwnode_property_read_u32_array(fwnode, "source-pdos",
+					     port->src_pdo, port->nr_src_pdo);
+	if ((ret < 0) || tcpm_validate_caps(port, port->src_pdo,
+					    port->nr_src_pdo))
+		return -EINVAL;
+
+	if (port->port_type == TYPEC_PORT_SRC)
+		return 0;
+
+	/* Get the preferred power role for DRP */
+	ret = fwnode_property_read_string(fwnode, "try-power-role", &cap_str);
+	if (ret < 0)
+		return ret;
+
+	port->typec_caps.prefer_role = typec_find_preferred_role(cap_str);
+	if (port->typec_caps.prefer_role < 0)
+		return -EINVAL;
+sink:
+	/* Get sink pdos */
+	ret = fwnode_property_read_u32_array(fwnode, "sink-pdos",
+					     NULL, 0);
+	if (ret <= 0)
+		return -EINVAL;
+
+	port->nr_snk_pdo = min(ret, PDO_MAX_OBJECTS);
+	ret = fwnode_property_read_u32_array(fwnode, "sink-pdos",
+					     port->snk_pdo, port->nr_snk_pdo);
+	if ((ret < 0) || tcpm_validate_caps(port, port->snk_pdo,
+					    port->nr_snk_pdo))
+		return -EINVAL;
+
+	if (fwnode_property_read_u32(fwnode, "op-sink-microwatt", &mw) < 0)
+		return -EINVAL;
+	port->operating_snk_mw = mw / 1000;
+
+	return 0;
+}
+
 int tcpm_update_source_capabilities(struct tcpm_port *port, const u32 *pdo,
 				    unsigned int nr_pdo)
 {
@@ -4524,12 +4600,36 @@ static int devm_tcpm_psy_register(struct tcpm_port *port)
 	return PTR_ERR_OR_ZERO(port->psy);
 }
 
+static int tcpm_copy_caps(struct tcpm_port *port,
+			  const struct tcpc_config *tcfg)
+{
+	if (tcpm_validate_caps(port, tcfg->src_pdo, tcfg->nr_src_pdo) ||
+	    tcpm_validate_caps(port, tcfg->snk_pdo, tcfg->nr_snk_pdo))
+		return -EINVAL;
+
+	port->nr_src_pdo = tcpm_copy_pdos(port->src_pdo, tcfg->src_pdo,
+					  tcfg->nr_src_pdo);
+	port->nr_snk_pdo = tcpm_copy_pdos(port->snk_pdo, tcfg->snk_pdo,
+					  tcfg->nr_snk_pdo);
+
+	port->nr_snk_vdo = tcpm_copy_vdos(port->snk_vdo, tcfg->snk_vdo,
+					  tcfg->nr_snk_vdo);
+
+	port->operating_snk_mw = tcfg->operating_snk_mw;
+
+	port->typec_caps.prefer_role = tcfg->default_role;
+	port->typec_caps.type = tcfg->type;
+	port->typec_caps.data = tcfg->data;
+
+	return 0;
+}
+
 struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
 {
 	struct tcpm_port *port;
 	int i, err;
 
-	if (!dev || !tcpc || !tcpc->config ||
+	if (!dev || !tcpc ||
 	    !tcpc->get_vbus || !tcpc->set_cc || !tcpc->get_cc ||
 	    !tcpc->set_polarity || !tcpc->set_vconn || !tcpc->set_vbus ||
 	    !tcpc->set_pd_rx || !tcpc->set_roles || !tcpc->pd_transmit)
@@ -4559,30 +4659,19 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
 	init_completion(&port->pps_complete);
 	tcpm_debugfs_init(port);
 
-	if (tcpm_validate_caps(port, tcpc->config->src_pdo,
-			       tcpc->config->nr_src_pdo) ||
-	    tcpm_validate_caps(port, tcpc->config->snk_pdo,
-			       tcpc->config->nr_snk_pdo)) {
-		err = -EINVAL;
+	if (tcpc->config)
+		err = tcpm_copy_caps(port, tcpc->config);
+	else
+		err = tcpm_fw_get_caps(port, tcpc->fwnode);
+	if (err < 0)
 		goto out_destroy_wq;
-	}
-	port->nr_src_pdo = tcpm_copy_pdos(port->src_pdo, tcpc->config->src_pdo,
-					  tcpc->config->nr_src_pdo);
-	port->nr_snk_pdo = tcpm_copy_pdos(port->snk_pdo, tcpc->config->snk_pdo,
-					  tcpc->config->nr_snk_pdo);
-	port->nr_snk_vdo = tcpm_copy_vdos(port->snk_vdo, tcpc->config->snk_vdo,
-					  tcpc->config->nr_snk_vdo);
-
-	port->operating_snk_mw = tcpc->config->operating_snk_mw;
-	if (!tcpc->config->try_role_hw)
-		port->try_role = tcpc->config->default_role;
+
+	if (!tcpc->config || !tcpc->config->try_role_hw)
+		port->try_role = port->typec_caps.prefer_role;
 	else
 		port->try_role = TYPEC_NO_PREFERRED_ROLE;
 
 	port->typec_caps.fwnode = tcpc->fwnode;
-	port->typec_caps.prefer_role = tcpc->config->default_role;
-	port->typec_caps.type = tcpc->config->type;
-	port->typec_caps.data = tcpc->config->data;
 	port->typec_caps.revision = 0x0120;	/* Type-C spec release 1.2 */
 	port->typec_caps.pd_revision = 0x0300;	/* USB-PD spec release 3.0 */
 	port->typec_caps.dr_set = tcpm_dr_set;
@@ -4592,7 +4681,7 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
 	port->typec_caps.port_type_set = tcpm_port_type_set;
 
 	port->partner_desc.identity = &port->partner_ident;
-	port->port_type = tcpc->config->type;
+	port->port_type = port->typec_caps.type;
 
 	port->role_sw = usb_role_switch_get(port->dev);
 	if (IS_ERR(port->role_sw)) {
@@ -4610,7 +4699,7 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
 		goto out_destroy_wq;
 	}
 
-	if (tcpc->config->alt_modes) {
+	if (tcpc->config && tcpc->config->alt_modes) {
 		const struct typec_altmode_desc *paltmode = tcpc->config->alt_modes;
 
 		i = 0;
-- 
2.7.4

[PATCH v5 07/14] staging: typec: tcpci: remove unused tcpci_tcpc_config

[Li Jun]

Since we will use config settings via device properties, so
remove the hard code tcpci_tcpc_config.

Signed-off-by: Li Jun <jun.li@nxp.com>
---
 drivers/staging/typec/tcpci.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
index 01d14fb..a21aaf5 100644
--- a/drivers/staging/typec/tcpci.c
+++ b/drivers/staging/typec/tcpci.c
@@ -464,17 +464,10 @@ static const struct regmap_config tcpci_regmap_config = {
 	.max_register = 0x7F, /* 0x80 .. 0xFF are vendor defined */
 };
 
-static const struct tcpc_config tcpci_tcpc_config = {
-	.type = TYPEC_PORT_DFP,
-	.default_role = TYPEC_SINK,
-};
-
 static int tcpci_parse_config(struct tcpci *tcpci)
 {
 	tcpci->controls_vbus = true; /* XXX */
 
-	/* TODO: Populate struct tcpc_config from ACPI/device-tree */
-	tcpci->tcpc.config = &tcpci_tcpc_config;
 	tcpci->tcpc.fwnode = device_get_named_child_node(tcpci->dev,
 							 "connector");
 	if (!tcpci->tcpc.fwnode) {
-- 
2.7.4

[PATCH v5 08/14] staging: typec: tcpci: register port before request irq

[Li Jun]

From: Peter Chen <peter.chen@nxp.com>

With that we can clear any pending events and the port is registered
so driver can be ready to handle typec events once we request irq.

Signed-off-by: Peter Chen <peter.chen@nxp.com>
Signed-off-by: Li Jun <jun.li@nxp.com>
---
 drivers/staging/typec/tcpci.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
index a21aaf5..139c2ab 100644
--- a/drivers/staging/typec/tcpci.c
+++ b/drivers/staging/typec/tcpci.c
@@ -537,24 +537,27 @@ static int tcpci_probe(struct i2c_client *client,
 	if (IS_ERR(chip->data.regmap))
 		return PTR_ERR(chip->data.regmap);
 
+	i2c_set_clientdata(client, chip);
+
 	/* Disable chip interrupts before requesting irq */
 	err = regmap_raw_write(chip->data.regmap, TCPC_ALERT_MASK, &val,
 			       sizeof(u16));
 	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);
+
 	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)
+	if (err < 0) {
+		tcpci_unregister_port(chip->tcpci);
 		return err;
+	}
 
-	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;
 }
 
-- 
2.7.4

[PATCH v5 09/14] staging: typec: tcpci: enable vbus detection

[Li Jun]

TCPCI implementation may need SW to enable VBUS detection to generate
power status events.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Li Jun <jun.li@nxp.com>
---
 drivers/staging/typec/tcpci.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
index 139c2ab..5c48810 100644
--- a/drivers/staging/typec/tcpci.c
+++ b/drivers/staging/typec/tcpci.c
@@ -373,6 +373,12 @@ static int tcpci_init(struct tcpc_dev *tcpc)
 	if (ret < 0)
 		return ret;
 
+	/* Enable Vbus detection */
+	ret = regmap_write(tcpci->regmap, TCPC_COMMAND,
+			   TCPC_CMD_ENABLE_VBUS_DETECT);
+	if (ret < 0)
+		return ret;
+
 	reg = TCPC_ALERT_TX_SUCCESS | TCPC_ALERT_TX_FAILED |
 		TCPC_ALERT_TX_DISCARDED | TCPC_ALERT_RX_STATUS |
 		TCPC_ALERT_RX_HARD_RST | TCPC_ALERT_CC_STATUS;
-- 
2.7.4

[PATCH v5 10/14] typec: tcpm: add starting value for drp toggling

[Li Jun]

As DRP port autonomously toggles the Rp/Rd need a start value to
begin with, so add one parameter for it in tcpm_start_drp_toggling.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Li Jun <jun.li@nxp.com>
---
 drivers/usb/typec/tcpm.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
index becedcf..0c7fa14 100644
--- a/drivers/usb/typec/tcpm.c
+++ b/drivers/usb/typec/tcpm.c
@@ -2436,15 +2436,15 @@ static int tcpm_set_charge(struct tcpm_port *port, bool charge)
 	return 0;
 }
 
-static bool tcpm_start_drp_toggling(struct tcpm_port *port)
+static bool tcpm_start_drp_toggling(struct tcpm_port *port,
+				    enum typec_cc_status cc)
 {
 	int ret;
 
 	if (port->tcpc->start_drp_toggling &&
 	    port->port_type == TYPEC_PORT_DRP) {
 		tcpm_log_force(port, "Start DRP toggling");
-		ret = port->tcpc->start_drp_toggling(port->tcpc,
-						     tcpm_rp_cc(port));
+		ret = port->tcpc->start_drp_toggling(port->tcpc, cc);
 		if (!ret)
 			return true;
 	}
@@ -2752,7 +2752,7 @@ static void run_state_machine(struct tcpm_port *port)
 		if (!port->non_pd_role_swap)
 			tcpm_swap_complete(port, -ENOTCONN);
 		tcpm_src_detach(port);
-		if (tcpm_start_drp_toggling(port)) {
+		if (tcpm_start_drp_toggling(port, tcpm_rp_cc(port))) {
 			tcpm_set_state(port, DRP_TOGGLING, 0);
 			break;
 		}
@@ -2927,7 +2927,7 @@ static void run_state_machine(struct tcpm_port *port)
 			tcpm_swap_complete(port, -ENOTCONN);
 		tcpm_pps_complete(port, -ENOTCONN);
 		tcpm_snk_detach(port);
-		if (tcpm_start_drp_toggling(port)) {
+		if (tcpm_start_drp_toggling(port, TYPEC_CC_RD)) {
 			tcpm_set_state(port, DRP_TOGGLING, 0);
 			break;
 		}
-- 
2.7.4

[PATCH v5 11/14] usb: typec: tcpm: set cc for drp toggling attach

[Li Jun]

In case of drp toggling, we may need set correct cc value for role control
after attach as it may never been set.

Signed-off-by: Li Jun <jun.li@nxp.com>
---
 drivers/usb/typec/tcpm.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
index 0c7fa14..7e7eaca 100644
--- a/drivers/usb/typec/tcpm.c
+++ b/drivers/usb/typec/tcpm.c
@@ -2599,6 +2599,7 @@ static void tcpm_reset_port(struct tcpm_port *port)
 	tcpm_set_attached_state(port, false);
 	port->try_src_count = 0;
 	port->try_snk_count = 0;
+	port->cc_req = TYPEC_CC_OPEN;
 	port->supply_voltage = 0;
 	port->current_limit = 0;
 	port->usb_type = POWER_SUPPLY_USB_TYPE_C;
@@ -2831,6 +2832,8 @@ static void run_state_machine(struct tcpm_port *port)
 		break;
 
 	case SRC_ATTACHED:
+		if (port->cc_req == TYPEC_CC_OPEN)
+			tcpm_set_cc(port, tcpm_rp_cc(port));
 		ret = tcpm_src_attach(port);
 		tcpm_set_state(port, SRC_UNATTACHED,
 			       ret < 0 ? 0 : PD_T_PS_SOURCE_ON);
@@ -3004,6 +3007,8 @@ static void run_state_machine(struct tcpm_port *port)
 		tcpm_set_state(port, SNK_UNATTACHED, PD_T_PD_DEBOUNCE);
 		break;
 	case SNK_ATTACHED:
+		if (port->cc_req == TYPEC_CC_OPEN)
+			tcpm_set_cc(port, TYPEC_CC_RD);
 		ret = tcpm_snk_attach(port);
 		if (ret < 0)
 			tcpm_set_state(port, SNK_UNATTACHED, 0);
-- 
2.7.4

[PATCH v5 12/14] staging: typec: tcpci: keep the not connecting cc line open

[Li Jun]

While set polarity, we should keep the not connecting cc line to be
open.

Signed-off-by: Li Jun <jun.li@nxp.com>
---
 drivers/staging/typec/tcpci.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
index 5c48810..5c0c5e3 100644
--- a/drivers/staging/typec/tcpci.c
+++ b/drivers/staging/typec/tcpci.c
@@ -185,15 +185,25 @@ static int tcpci_set_polarity(struct tcpc_dev *tcpc,
 			      enum typec_cc_polarity polarity)
 {
 	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
+	unsigned int reg;
 	int ret;
 
-	ret = regmap_write(tcpci->regmap, TCPC_TCPC_CTRL,
-			   (polarity == TYPEC_POLARITY_CC2) ?
-			   TCPC_TCPC_CTRL_ORIENTATION : 0);
+	/* Keep the disconnect cc line open */
+	ret = regmap_read(tcpci->regmap, TCPC_ROLE_CTRL, &reg);
 	if (ret < 0)
 		return ret;
 
-	return 0;
+	if (polarity == TYPEC_POLARITY_CC2)
+		reg |= TCPC_ROLE_CTRL_CC_OPEN << TCPC_ROLE_CTRL_CC1_SHIFT;
+	else
+		reg |= TCPC_ROLE_CTRL_CC_OPEN << 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_TCPC_CTRL,
+			   (polarity == TYPEC_POLARITY_CC2) ?
+			   TCPC_TCPC_CTRL_ORIENTATION : 0);
 }
 
 static int tcpci_set_vconn(struct tcpc_dev *tcpc, bool enable)
-- 
2.7.4

[PATCH v5 13/14] staging: typec: tcpci: Only touch target bit when enable vconn

[Li Jun]

We need regmap_update_bits to avoid touch any other bits when
enable or disable vconn.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Li Jun <jun.li@nxp.com>
---
 drivers/staging/typec/tcpci.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
index 5c0c5e3..2818a4d 100644
--- a/drivers/staging/typec/tcpci.c
+++ b/drivers/staging/typec/tcpci.c
@@ -218,12 +218,9 @@ static int tcpci_set_vconn(struct tcpc_dev *tcpc, bool enable)
 			return ret;
 	}
 
-	ret = regmap_write(tcpci->regmap, TCPC_POWER_CTRL,
-			   enable ? TCPC_POWER_CTRL_VCONN_ENABLE : 0);
-	if (ret < 0)
-		return ret;
-
-	return 0;
+	return regmap_update_bits(tcpci->regmap, TCPC_POWER_CTRL,
+				TCPC_POWER_CTRL_VCONN_ENABLE,
+				enable ? TCPC_POWER_CTRL_VCONN_ENABLE : 0);
 }
 
 static int tcpci_set_roles(struct tcpc_dev *tcpc, bool attached,
-- 
2.7.4

[PATCH v5 14/14] staging: typec: tcpci: move tcpci driver out of staging

[Li Jun]

Move TCPCI(Typec port controller interface) driver out of staging.

Signed-off-by: Li Jun <jun.li@nxp.com>
---
 drivers/staging/Kconfig                | 2 --
 drivers/staging/Makefile               | 1 -
 drivers/staging/typec/TODO             | 5 -----
 drivers/usb/typec/Kconfig              | 7 +++++++
 drivers/usb/typec/Makefile             | 1 +
 drivers/{staging => usb}/typec/tcpci.c | 0
 drivers/{staging => usb}/typec/tcpci.h | 0
 7 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/Kconfig b/drivers/staging/Kconfig
index d5926f0..d83ff66 100644
--- a/drivers/staging/Kconfig
+++ b/drivers/staging/Kconfig
@@ -112,8 +112,6 @@ source "drivers/staging/greybus/Kconfig"
 
 source "drivers/staging/vc04_services/Kconfig"
 
-source "drivers/staging/typec/Kconfig"
-
 source "drivers/staging/vboxvideo/Kconfig"
 
 source "drivers/staging/pi433/Kconfig"
diff --git a/drivers/staging/Makefile b/drivers/staging/Makefile
index 919753c..a71ec1f 100644
--- a/drivers/staging/Makefile
+++ b/drivers/staging/Makefile
@@ -2,7 +2,6 @@
 # Makefile for staging directory
 
 obj-y				+= media/
-obj-y				+= typec/
 obj-$(CONFIG_IPX)		+= ipx/
 obj-$(CONFIG_NCP_FS)		+= ncpfs/
 obj-$(CONFIG_PRISM2_USB)	+= wlan-ng/
diff --git a/drivers/staging/typec/TODO b/drivers/staging/typec/TODO
deleted file mode 100644
index 53fe2f7..0000000
--- a/drivers/staging/typec/TODO
+++ /dev/null
@@ -1,5 +0,0 @@
-tcpci:
-- Test with real hardware
-
-Please send patches to Guenter Roeck <linux@roeck-us.net> and copy
-Heikki Krogerus <heikki.krogerus@linux.intel.com>.
diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
index 2c8eab1..0a862fc 100644
--- a/drivers/usb/typec/Kconfig
+++ b/drivers/usb/typec/Kconfig
@@ -56,6 +56,13 @@ config TYPEC_TCPM
 
 if TYPEC_TCPM
 
+config TYPEC_TCPCI
+	tristate "Type-C Port Controller Interface driver"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  Type-C Port Controller driver for TCPCI-compliant controller.
+
 source "drivers/usb/typec/fusb302/Kconfig"
 
 config TYPEC_WCOVE
diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
index 1f599a6..02758a1 100644
--- a/drivers/usb/typec/Makefile
+++ b/drivers/usb/typec/Makefile
@@ -7,3 +7,4 @@ obj-$(CONFIG_TYPEC_WCOVE)	+= typec_wcove.o
 obj-$(CONFIG_TYPEC_UCSI)	+= ucsi/
 obj-$(CONFIG_TYPEC_TPS6598X)	+= tps6598x.o
 obj-$(CONFIG_TYPEC)		+= mux/
+obj-$(CONFIG_TYPEC_TCPCI)	+= tcpci.o
diff --git a/drivers/staging/typec/tcpci.c b/drivers/usb/typec/tcpci.c
similarity index 100%
rename from drivers/staging/typec/tcpci.c
rename to drivers/usb/typec/tcpci.c
diff --git a/drivers/staging/typec/tcpci.h b/drivers/usb/typec/tcpci.h
similarity index 100%
rename from drivers/staging/typec/tcpci.h
rename to drivers/usb/typec/tcpci.h
-- 
2.7.4

Re: [PATCH v5 01/14] dt-bindings: connector: add properties for typec

[Oliver Neukum]

Am Donnerstag, den 03.05.2018, 08:24 +0800 schrieb Li Jun:
> +Optional properties for usb-c-connector:
> +- power-role: should be one of "source", "sink" or "dual"(DRP) if typec
> +  connector has power support.
> +- try-power-role: preferred power role if "dual"(DRP) can support Try.SNK
> +  or Try.SRC, should be "sink" for Try.SNK or "source" for Try.SRC.
> +- data-role: should be one of "host", "device", "dual"(DRD) if typec
> +  connector supports USB data.

Hi,

is this really correct?

Can one implement a device that can operate as either DFP or UFP,
but not implements the dynamic role switch that a DRP must support?

	Regards
		Oliver

Re: [PATCH v5 08/14] staging: typec: tcpci: register port before request irq

[Sergei Shtylyov]

Hello!

On 5/3/2018 3:24 AM, Li Jun wrote:

> From: Peter Chen <peter.chen@nxp.com>
> 
> With that we can clear any pending events and the port is registered
> so driver can be ready to handle typec events once we request irq.
> 
> Signed-off-by: Peter Chen <peter.chen@nxp.com>
> Signed-off-by: Li Jun <jun.li@nxp.com>
> ---
>   drivers/staging/typec/tcpci.c | 15 +++++++++------
>   1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
> index a21aaf5..139c2ab 100644
> --- a/drivers/staging/typec/tcpci.c
> +++ b/drivers/staging/typec/tcpci.c
> @@ -537,24 +537,27 @@ static int tcpci_probe(struct i2c_client *client,
>   	if (IS_ERR(chip->data.regmap))
>   		return PTR_ERR(chip->data.regmap);
>   
> +	i2c_set_clientdata(client, chip);
> +
>   	/* Disable chip interrupts before requesting irq */
>   	err = regmap_raw_write(chip->data.regmap, TCPC_ALERT_MASK, &val,
>   			       sizeof(u16));
>   	if (err < 0)
>   		return err;
>   
> +	chip->tcpci = tcpci_register_port(&client->dev, &chip->data);
> +	if (PTR_ERR_OR_ZERO(chip->tcpci))

    Is NULL an error indication too?

> +		return PTR_ERR(chip->tcpci);

    If so, you'll return a success indication here.

> +
>   	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)
> +	if (err < 0) {
> +		tcpci_unregister_port(chip->tcpci);
>   		return err;
> +	}
>   
> -	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;
>   }
>   

MBR, Sergei

Re: [PATCH v5 08/14] staging: typec: tcpci: register port before request irq

[Sergei Shtylyov]

On 5/3/2018 11:29 AM, Sergei Shtylyov wrote:

>> From: Peter Chen <peter.chen@nxp.com>
>>
>> With that we can clear any pending events and the port is registered
>> so driver can be ready to handle typec events once we request irq.
>>
>> Signed-off-by: Peter Chen <peter.chen@nxp.com>
>> Signed-off-by: Li Jun <jun.li@nxp.com>
>> ---
>>   drivers/staging/typec/tcpci.c | 15 +++++++++------
>>   1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
>> index a21aaf5..139c2ab 100644
>> --- a/drivers/staging/typec/tcpci.c
>> +++ b/drivers/staging/typec/tcpci.c
>> @@ -537,24 +537,27 @@ static int tcpci_probe(struct i2c_client *client,
>>       if (IS_ERR(chip->data.regmap))
>>           return PTR_ERR(chip->data.regmap);
>> +    i2c_set_clientdata(client, chip);
>> +
>>       /* Disable chip interrupts before requesting irq */
>>       err = regmap_raw_write(chip->data.regmap, TCPC_ALERT_MASK, &val,
>>                      sizeof(u16));
>>       if (err < 0)
>>           return err;
>> +    chip->tcpci = tcpci_register_port(&client->dev, &chip->data);
>> +    if (PTR_ERR_OR_ZERO(chip->tcpci))
> 
>     Is NULL an error indication too?
> 
>> +        return PTR_ERR(chip->tcpci);
> 
>     If so, you'll return a success indication here.

    ... when chip->tcpci is NULL, I mean. Seems wrong...

>> +
>>       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)
>> +    if (err < 0) {
>> +        tcpci_unregister_port(chip->tcpci);
>>           return err;
>> +    }
>> -    chip->tcpci = tcpci_register_port(&client->dev, &chip->data);
>> -    if (PTR_ERR_OR_ZERO(chip->tcpci))
>> -        return PTR_ERR(chip->tcpci);
>> -

    Ah, you're only moving the questionable code...

>> -    i2c_set_clientdata(client, chip);
>>       return 0;
>>   }

MBR, Sergei

RE: [PATCH v5 01/14] dt-bindings: connector: add properties for typec

[Jun Li]

Hi
> -----Original Message-----
> From: Oliver Neukum [mailto:oneukum@suse.com]
> Sent: 2018年5月3日 15:27
> To: Jun Li <jun.li@nxp.com>; robh+dt@kernel.org;
> heikki.krogerus@linux.intel.com; gregkh@linuxfoundation.org;
> linux@roeck-us.net
> Cc: gsomlo@gmail.com; dl-linux-imx <linux-imx@nxp.com>; Peter Chen
> <peter.chen@nxp.com>; shufan_lee@richtek.com; a.hajda@samsung.com;
> cw00.choi@samsung.com; devicetree@vger.kernel.org;
> linux-usb@vger.kernel.org
> Subject: Re: [PATCH v5 01/14] dt-bindings: connector: add properties for typec
> 
> Am Donnerstag, den 03.05.2018, 08:24 +0800 schrieb Li Jun:
> > +Optional properties for usb-c-connector:
> > +- power-role: should be one of "source", "sink" or "dual"(DRP) if
> > +typec
> > +  connector has power support.
> > +- try-power-role: preferred power role if "dual"(DRP) can support
> > +Try.SNK
> > +  or Try.SRC, should be "sink" for Try.SNK or "source" for Try.SRC.
> > +- data-role: should be one of "host", "device", "dual"(DRD) if typec
> > +  connector supports USB data.
> 
> Hi,
> 
> is this really correct?
> 
> Can one implement a device that can operate as either DFP or UFP, but not
> implements the dynamic role switch that a DRP must support?

You mean a port with DRD on data but not DRP on power?

The data-role is newly added as the data role is not coupled with power
role in new PD spec, in practice, I think this is very normal, Heikki already
added a patch to separate the power and data role in class driver, but tcpm
hasn't changed accordingly for this yet, so current tcpm may not allow your
question case, but from property definition point view, we should not limit
this and let the 2 roles independent each other.

Thanks
Jun
> 
> 	Regards
> 		Oliver

Re: [PATCH v5 01/14] dt-bindings: connector: add properties for typec

[Oliver Neukum]

Am Donnerstag, den 03.05.2018, 08:35 +0000 schrieb Jun Li:
> Hi
> > -----Original Message-----
> > From: Oliver Neukum [mailto:oneukum@suse.com]
> > Sent: 2018年5月3日 15:27
> > To: Jun Li <jun.li@nxp.com>; robh+dt@kernel.org;
> > heikki.krogerus@linux.intel.com; gregkh@linuxfoundation.org;
> > linux@roeck-us.net
> > Cc: gsomlo@gmail.com; dl-linux-imx <linux-imx@nxp.com>; Peter Chen
> > <peter.chen@nxp.com>; shufan_lee@richtek.com; a.hajda@samsung.com;
> > cw00.choi@samsung.com; devicetree@vger.kernel.org;
> > linux-usb@vger.kernel.org
> > Subject: Re: [PATCH v5 01/14] dt-bindings: connector: add properties for typec
> > 
> > Am Donnerstag, den 03.05.2018, 08:24 +0800 schrieb Li Jun:
> > > +Optional properties for usb-c-connector:
> > > +- power-role: should be one of "source", "sink" or "dual"(DRP) if
> > > +typec
> > > +  connector has power support.
> > > +- try-power-role: preferred power role if "dual"(DRP) can support
> > > +Try.SNK
> > > +  or Try.SRC, should be "sink" for Try.SNK or "source" for Try.SRC.
> > > +- data-role: should be one of "host", "device", "dual"(DRD) if typec
> > > +  connector supports USB data.
> > 
> > Hi,
> > 
> > is this really correct?
> > 
> > Can one implement a device that can operate as either DFP or UFP, but not
> > implements the dynamic role switch that a DRP must support?
> 
> You mean a port with DRD on data but not DRP on power?
> 
> The data-role is newly added as the data role is not coupled with power

No, I meant data role. As far as I can tell for a DRP you need to
implement the detection logic described in chapter 4 of the spec.
I can see no reason why you couldn't build a port that can be switched
between the data roles but not implement that logic.

	Regards
		Oliver

RE: [PATCH v5 01/14] dt-bindings: connector: add properties for typec

[Jun Li]

Hi
> -----Original Message-----
> From: Oliver Neukum [mailto:oneukum@suse.com]
> Sent: 2018年5月3日 17:18
> To: Jun Li <jun.li@nxp.com>; robh+dt@kernel.org;
> heikki.krogerus@linux.intel.com; gregkh@linuxfoundation.org;
> linux@roeck-us.net
> Cc: gsomlo@gmail.com; dl-linux-imx <linux-imx@nxp.com>; Peter Chen
> <peter.chen@nxp.com>; shufan_lee@richtek.com; a.hajda@samsung.com;
> cw00.choi@samsung.com; devicetree@vger.kernel.org;
> linux-usb@vger.kernel.org
> Subject: Re: [PATCH v5 01/14] dt-bindings: connector: add properties for typec
> 
> Am Donnerstag, den 03.05.2018, 08:35 +0000 schrieb Jun Li:
> > Hi
> > > -----Original Message-----
> > > From: Oliver Neukum [mailto:oneukum@suse.com]
> > > Sent: 2018年5月3日 15:27
> > > To: Jun Li <jun.li@nxp.com>; robh+dt@kernel.org;
> > > heikki.krogerus@linux.intel.com; gregkh@linuxfoundation.org;
> > > linux@roeck-us.net
> > > Cc: gsomlo@gmail.com; dl-linux-imx <linux-imx@nxp.com>; Peter Chen
> > > <peter.chen@nxp.com>; shufan_lee@richtek.com;
> a.hajda@samsung.com;
> > > cw00.choi@samsung.com; devicetree@vger.kernel.org;
> > > linux-usb@vger.kernel.org
> > > Subject: Re: [PATCH v5 01/14] dt-bindings: connector: add properties
> > > for typec
> > >
> > > Am Donnerstag, den 03.05.2018, 08:24 +0800 schrieb Li Jun:
> > > > +Optional properties for usb-c-connector:
> > > > +- power-role: should be one of "source", "sink" or "dual"(DRP) if
> > > > +typec
> > > > +  connector has power support.
> > > > +- try-power-role: preferred power role if "dual"(DRP) can support
> > > > +Try.SNK
> > > > +  or Try.SRC, should be "sink" for Try.SNK or "source" for Try.SRC.
> > > > +- data-role: should be one of "host", "device", "dual"(DRD) if
> > > > +typec
> > > > +  connector supports USB data.
> > >
> > > Hi,
> > >
> > > is this really correct?
> > >
> > > Can one implement a device that can operate as either DFP or UFP,
> > > but not implements the dynamic role switch that a DRP must support?
> >
> > You mean a port with DRD on data but not DRP on power?
> >
> > The data-role is newly added as the data role is not coupled with
> > power
> 
> No, I meant data role. As far as I can tell for a DRP you need to implement the
> detection logic described in chapter 4 of the spec.

Could you please point me the "detection logic" of typec spec chapter 4
you are referring to?

> I can see no reason why you couldn't build a port that can be switched between
> the data roles but not implement that logic.

I see there is dr_swap handling for data role swap in tcpm already, maybe
I misunderstood the "logic" you want here.

Regards
Li Jun
 
> 
> 	Regards
> 		Oliver

Re: [PATCH v5 01/14] dt-bindings: connector: add properties for typec

[Rob Herring]

On Thu, May 03, 2018 at 08:24:41AM +0800, Li Jun wrote:
> Add bingdings supported by current typec driver, so user can pass
> all those properties via dt.
> 
> Signed-off-by: Li Jun <jun.li@nxp.com>
> ---
>  .../bindings/connector/usb-connector.txt           | 44 +++++++++++++++
>  include/dt-bindings/usb/pd.h                       | 62 ++++++++++++++++++++++
>  2 files changed, 106 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt
> index e1463f1..4b19de6d0 100644
> --- a/Documentation/devicetree/bindings/connector/usb-connector.txt
> +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
> @@ -15,6 +15,33 @@ Optional properties:
>  - type: size of the connector, should be specified in case of USB-A, USB-B
>    non-fullsize connectors: "mini", "micro".
>  
> +Optional properties for usb-c-connector:
> +- power-role: should be one of "source", "sink" or "dual"(DRP) if typec
> +  connector has power support.
> +- try-power-role: preferred power role if "dual"(DRP) can support Try.SNK
> +  or Try.SRC, should be "sink" for Try.SNK or "source" for Try.SRC.
> +- data-role: should be one of "host", "device", "dual"(DRD) if typec
> +  connector supports USB data.

Need to define what are the defaults on these if the properties aren't 
present (ideally, those would be the common case)?

Rob

Re: [PATCH v5 02/14] dt-bindings: usb: add documentation for typec port controller(TCPCI)

[Rob Herring]

On Thu, May 03, 2018 at 08:24:42AM +0800, Li Jun wrote:
> TCPCI stands for typec port controller interface, its implementation
> has full typec port control with power delivery support, it's a
> standard i2c slave with GPIO input as irq interface, detail see spec
> "Universal Serial Bus Type-C Port Controller Interface Specification
> Revision 1.0, Version 1.1"
> 
> Signed-off-by: Li Jun <jun.li@nxp.com>
> ---
>  .../devicetree/bindings/usb/typec-tcpci.txt        | 49 ++++++++++++++++++++++
>  1 file changed, 49 insertions(+)

The example may need to change based on discussion on the prior patch, 
otherwise:

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

RE: [PATCH v5 01/14] dt-bindings: connector: add properties for typec

[Jun Li]

> -----Original Message-----
> From: Rob Herring [mailto:robh@kernel.org]
> Sent: 2018年5月7日 23:59
> To: Jun Li <jun.li@nxp.com>
> Cc: gregkh@linuxfoundation.org; heikki.krogerus@linux.intel.com;
> linux@roeck-us.net; a.hajda@samsung.com; cw00.choi@samsung.com;
> shufan_lee@richtek.com; Peter Chen <peter.chen@nxp.com>;
> gsomlo@gmail.com; devicetree@vger.kernel.org; linux-usb@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v5 01/14] dt-bindings: connector: add properties for typec
> 
> On Thu, May 03, 2018 at 08:24:41AM +0800, Li Jun wrote:
> > Add bingdings supported by current typec driver, so user can pass all
> > those properties via dt.
> >
> > Signed-off-by: Li Jun <jun.li@nxp.com>
> > ---
> >  .../bindings/connector/usb-connector.txt           | 44
> +++++++++++++++
> >  include/dt-bindings/usb/pd.h                       | 62
> ++++++++++++++++++++++
> >  2 files changed, 106 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/connector/usb-connector.txt
> > b/Documentation/devicetree/bindings/connector/usb-connector.txt
> > index e1463f1..4b19de6d0 100644
> > --- a/Documentation/devicetree/bindings/connector/usb-connector.txt
> > +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
> > @@ -15,6 +15,33 @@ Optional properties:
> >  - type: size of the connector, should be specified in case of USB-A, USB-B
> >    non-fullsize connectors: "mini", "micro".
> >
> > +Optional properties for usb-c-connector:
> > +- power-role: should be one of "source", "sink" or "dual"(DRP) if
> > +typec
> > +  connector has power support.
> > +- try-power-role: preferred power role if "dual"(DRP) can support
> > +Try.SNK
> > +  or Try.SRC, should be "sink" for Try.SNK or "source" for Try.SRC.
> > +- data-role: should be one of "host", "device", "dual"(DRD) if typec
> > +  connector supports USB data.
> 
> Need to define what are the defaults on these if the properties aren't present
> (ideally, those would be the common case)?

If one of above properties isn't present, means the port doesn't have
corresponding capability. I think the common case is a port has a
specific property value, but it's also possible it doesn't support that
function(e.g. a typec port only for power but no data communication),
so I use "not present" for this case instead of some default value).

Thanks
Li Jun
> 
> Rob

Re: [PATCH v5 01/14] dt-bindings: connector: add properties for typec

[Oliver Neukum]

Am Freitag, den 04.05.2018, 08:59 +0000 schrieb Jun Li:
> 
> > > > Can one implement a device that can operate as either DFP or UFP,
> > > > but not implements the dynamic role switch that a DRP must support?
> > > 
> > > You mean a port with DRD on data but not DRP on power?
> > > 
> > > The data-role is newly added as the data role is not coupled with
> > > power
> > 
> > No, I meant data role. As far as I can tell for a DRP you need to implement the
> > detection logic described in chapter 4 of the spec.
> 
> Could you please point me the "detection logic" of typec spec chapter 4
> you are referring to?

Chapter 4.5.2.2, especially state diagramms 4.15 and 4.16

It just seems to me that a DRP and a physical port that can be switched
between UFP and DFP are not the same thing, but can be implemented.

	Regards
		Oliver

RE: [PATCH v5 01/14] dt-bindings: connector: add properties for typec

[Jun Li]

Hi
> -----Original Message-----
> From: Oliver Neukum [mailto:oneukum@suse.com]
> Sent: 2018年5月8日 18:03
> To: Jun Li <jun.li@nxp.com>; robh+dt@kernel.org;
> heikki.krogerus@linux.intel.com; gregkh@linuxfoundation.org;
> linux@roeck-us.net
> Cc: gsomlo@gmail.com; dl-linux-imx <linux-imx@nxp.com>; Peter Chen
> <peter.chen@nxp.com>; shufan_lee@richtek.com; a.hajda@samsung.com;
> cw00.choi@samsung.com; devicetree@vger.kernel.org;
> linux-usb@vger.kernel.org
> Subject: Re: [PATCH v5 01/14] dt-bindings: connector: add properties for typec
> 
> Am Freitag, den 04.05.2018, 08:59 +0000 schrieb Jun Li:
> >
> > > > > Can one implement a device that can operate as either DFP or
> > > > > UFP, but not implements the dynamic role switch that a DRP must
> support?
> > > >
> > > > You mean a port with DRD on data but not DRP on power?
> > > >
> > > > The data-role is newly added as the data role is not coupled with
> > > > power
> > >
> > > No, I meant data role. As far as I can tell for a DRP you need to
> > > implement the detection logic described in chapter 4 of the spec.
> >
> > Could you please point me the "detection logic" of typec spec chapter
> > 4 you are referring to?
> 
> Chapter 4.5.2.2, especially state diagramms 4.15 and 4.16

Figure 4-15 Connection State Diagram: DRP
That's DRP case without try-power-role present, a simple DRP(Dual-Role-Power).

Figure 4-16 Connection State Diagram: DRP with Accessory and Try.SRC Support
That's DRP with try-power-role = "source"

> 
> It just seems to me that a DRP and a physical port that can be switched
> between UFP and DFP are not the same thing, but can be implemented.

Yes, they are different, the former is DRP(Dual-Role-Power, power-role = "dual"),
and the latter is DRD(Dual-Role-Data, data-role = "dual"). If a port can support
both DRP and DRD, I think current tcpm already can support it.
If a typec port only can sink power(power-role = "sink") but can support UFP
and UFP switch on data (data-role = "dual"), in this case, we only can switch
the data roles via dr_swap or sys, current tcpm may need more change to
adapt the separation of power role and data role, but anyway all this can be
implemented as you said.

Thanks
Li Jun
> 
> 	Regards
> 		Oliver

Re: [PATCH v5 01/14] dt-bindings: connector: add properties for typec

[Mats Karrman]

Hi Li Jun,

On 2018-05-03 02:24, Li Jun wrote:

> Add bingdings supported by current typec driver, so user can pass
> all those properties via dt.
>
> Signed-off-by: Li Jun <jun.li@nxp.com>
> ---
>   .../bindings/connector/usb-connector.txt           | 44 +++++++++++++++
>   include/dt-bindings/usb/pd.h                       | 62 ++++++++++++++++++++++
>   2 files changed, 106 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt
> index e1463f1..4b19de6d0 100644
> --- a/Documentation/devicetree/bindings/connector/usb-connector.txt
> +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
> @@ -15,6 +15,33 @@ Optional properties:
>   - type: size of the connector, should be specified in case of USB-A, USB-B
>     non-fullsize connectors: "mini", "micro".
>   
> +Optional properties for usb-c-connector:
> +- power-role: should be one of "source", "sink" or "dual"(DRP) if typec
> +  connector has power support.
> +- try-power-role: preferred power role if "dual"(DRP) can support Try.SNK
> +  or Try.SRC, should be "sink" for Try.SNK or "source" for Try.SRC.
> +- data-role: should be one of "host", "device", "dual"(DRD) if typec
> +  connector supports USB data.
> +
> +Required properties for usb-c-connector with power delivery support:
> +- source-pdos: An array of u32 with each entry providing supported power
> +  source data object(PDO), the detailed bit definitions of PDO can be found
> +  in "Universal Serial Bus Power Delivery Specification" chapter 6.4.1.2
> +  Source_Capabilities Message, the order of each entry(PDO) should follow
> +  the PD spec chapter 6.4.1. Required for power source and power dual role.
> +  User can specify the source PDO array via PDO_FIXED/BATT/VAR() defined in
> +  dt-bindings/usb/pd.h.
> +- sink-pdos: An array of u32 with each entry providing supported power
> +  sink data object(PDO), the detailed bit definitions of PDO can be found
> +  in "Universal Serial Bus Power Delivery Specification" chapter 6.4.1.3
> +  Sink Capabilities Message, the order of each entry(PDO) should follow
> +  the PD spec chapter 6.4.1. Required for power sink and power dual role.
> +  User can specify the sink PDO array via PDO_FIXED/BATT/VAR() defined in
> +  dt-bindings/usb/pd.h.
> +- op-sink-microwatt: Sink required operating power in microwatt, if source
> +  can't offer the power, Capability Mismatch is set, required for power

...set. Required...
(new sentence, otherwise it's unclear what is required; op-sink-microwatt or
Capability Mismatch set)

BR // Mats

> +  sink and power dual role.
> +
>   Required nodes:
>   - any data bus to the connector should be modeled using the OF graph bindings
>     specified in bindings/graph.txt, unless the bus is between parent node and
> @@ -73,3 +100,20 @@ ccic: s2mm005@33 {
>   		};
>   	};
>   };
> +
> +3. USB-C connector attached to a typec port controller(ptn5110), which has
> +power delivery support and enables drp.
> +
> +typec: ptn5110@50 {
> +	...
> +	usb_con: connector {
> +		compatible = "usb-c-connector";
> +		label = "USB-C";
> +		power-role = "dual";
> +		try-power-role = "sink";
> +		source-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM)>;
> +		sink-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM)
> +			     PDO_VAR(5000, 12000, 2000)>;
> +		op-sink-microwatt = <10000000>;
> +	};
> +};
> diff --git a/include/dt-bindings/usb/pd.h b/include/dt-bindings/usb/pd.h
> new file mode 100644
> index 0000000..7b7a92f
> --- /dev/null
> +++ b/include/dt-bindings/usb/pd.h
> @@ -0,0 +1,62 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __DT_POWER_DELIVERY_H
> +#define __DT_POWER_DELIVERY_H
> +
> +/* Power delivery Power Data Object definitions */
> +#define PDO_TYPE_FIXED		0
> +#define PDO_TYPE_BATT		1
> +#define PDO_TYPE_VAR		2
> +#define PDO_TYPE_APDO		3
> +
> +#define PDO_TYPE_SHIFT		30
> +#define PDO_TYPE_MASK		0x3
> +
> +#define PDO_TYPE(t)	((t) << PDO_TYPE_SHIFT)
> +
> +#define PDO_VOLT_MASK		0x3ff
> +#define PDO_CURR_MASK		0x3ff
> +#define PDO_PWR_MASK		0x3ff
> +
> +#define PDO_FIXED_DUAL_ROLE	(1 << 29) /* Power role swap supported */
> +#define PDO_FIXED_SUSPEND	(1 << 28) /* USB Suspend supported (Source) */
> +#define PDO_FIXED_HIGHER_CAP	(1 << 28) /* Requires more than vSafe5V (Sink) */
> +#define PDO_FIXED_EXTPOWER	(1 << 27) /* Externally powered */
> +#define PDO_FIXED_USB_COMM	(1 << 26) /* USB communications capable */
> +#define PDO_FIXED_DATA_SWAP	(1 << 25) /* Data role swap supported */
> +#define PDO_FIXED_VOLT_SHIFT	10	/* 50mV units */
> +#define PDO_FIXED_CURR_SHIFT	0	/* 10mA units */
> +
> +#define PDO_FIXED_VOLT(mv)	((((mv) / 50) & PDO_VOLT_MASK) << PDO_FIXED_VOLT_SHIFT)
> +#define PDO_FIXED_CURR(ma)	((((ma) / 10) & PDO_CURR_MASK) << PDO_FIXED_CURR_SHIFT)
> +
> +#define PDO_FIXED(mv, ma, flags)			\
> +	(PDO_TYPE(PDO_TYPE_FIXED) | (flags) |		\
> +	 PDO_FIXED_VOLT(mv) | PDO_FIXED_CURR(ma))
> +
> +#define VSAFE5V 5000 /* mv units */
> +
> +#define PDO_BATT_MAX_VOLT_SHIFT	20	/* 50mV units */
> +#define PDO_BATT_MIN_VOLT_SHIFT	10	/* 50mV units */
> +#define PDO_BATT_MAX_PWR_SHIFT	0	/* 250mW units */
> +
> +#define PDO_BATT_MIN_VOLT(mv) ((((mv) / 50) & PDO_VOLT_MASK) << PDO_BATT_MIN_VOLT_SHIFT)
> +#define PDO_BATT_MAX_VOLT(mv) ((((mv) / 50) & PDO_VOLT_MASK) << PDO_BATT_MAX_VOLT_SHIFT)
> +#define PDO_BATT_MAX_POWER(mw) ((((mw) / 250) & PDO_PWR_MASK) << PDO_BATT_MAX_PWR_SHIFT)
> +
> +#define PDO_BATT(min_mv, max_mv, max_mw)			\
> +	(PDO_TYPE(PDO_TYPE_BATT) | PDO_BATT_MIN_VOLT(min_mv) |	\
> +	 PDO_BATT_MAX_VOLT(max_mv) | PDO_BATT_MAX_POWER(max_mw))
> +
> +#define PDO_VAR_MAX_VOLT_SHIFT	20	/* 50mV units */
> +#define PDO_VAR_MIN_VOLT_SHIFT	10	/* 50mV units */
> +#define PDO_VAR_MAX_CURR_SHIFT	0	/* 10mA units */
> +
> +#define PDO_VAR_MIN_VOLT(mv) ((((mv) / 50) & PDO_VOLT_MASK) << PDO_VAR_MIN_VOLT_SHIFT)
> +#define PDO_VAR_MAX_VOLT(mv) ((((mv) / 50) & PDO_VOLT_MASK) << PDO_VAR_MAX_VOLT_SHIFT)
> +#define PDO_VAR_MAX_CURR(ma) ((((ma) / 10) & PDO_CURR_MASK) << PDO_VAR_MAX_CURR_SHIFT)
> +
> +#define PDO_VAR(min_mv, max_mv, max_ma)				\
> +	(PDO_TYPE(PDO_TYPE_VAR) | PDO_VAR_MIN_VOLT(min_mv) |	\
> +	 PDO_VAR_MAX_VOLT(max_mv) | PDO_VAR_MAX_CURR(max_ma))
> +
> + #endif /* __DT_POWER_DELIVERY_H */

Re: [PATCH v5 03/14] staging: typec: tcpci: add compatible string for nxp ptn5110

[Mats Karrman]

Hi Li Jun,

On 2018-05-03 02:24, Li Jun wrote:

> Add nxp ptn5110 typec controller compatible string: usb-tcpci,ptn5110,
> which is a standard tcpci chip with power delivery support.
>
> Signed-off-by: Li Jun <jun.li@nxp.com>
> ---
>   drivers/staging/typec/tcpci.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
> index 076d97e..741a80a 100644
> --- a/drivers/staging/typec/tcpci.c
> +++ b/drivers/staging/typec/tcpci.c
> @@ -576,6 +576,7 @@ MODULE_DEVICE_TABLE(i2c, tcpci_id);
>   #ifdef CONFIG_OF
>   static const struct of_device_id tcpci_of_match[] = {
>   	{ .compatible = "usb,tcpci", },

Either this line should go away, or a "generic TCPCI controller" line should be
added to the DT documentation.

BR // Mats

> +	{ .compatible = "nxp,ptn5110", },
>   	{},
>   };
>   MODULE_DEVICE_TABLE(of, tcpci_of_match);
>

Re: [PATCH v5 05/14] usb: typec: add API to get typec basic port power and data config

[Mats Karrman]

Hi Li Jun,

On 2018-05-03 02:24, Li Jun wrote:

> This patch adds 3 APIs to get the typec port power and data type,
> and preferred power role by its name string.
>
> Signed-off-by: Li Jun <jun.li@nxp.com>
> ---
>   drivers/usb/typec/class.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++
>   include/linux/usb/typec.h |  3 +++
>   2 files changed, 55 insertions(+)
>
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index 53df10d..5981e18 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -9,6 +9,7 @@
>   #include <linux/device.h>
>   #include <linux/module.h>
>   #include <linux/mutex.h>
> +#include <linux/property.h>
>   #include <linux/slab.h>
>   #include <linux/usb/typec.h>
>   #include <linux/usb/typec_mux.h>
> @@ -802,6 +803,12 @@ static const char * const typec_port_types[] = {
>   	[TYPEC_PORT_DRP] = "dual",
>   };
>   
> +static const char * const typec_data_types[] = {
> +	[TYPEC_PORT_DFP] = "host",
> +	[TYPEC_PORT_UFP] = "device",
> +	[TYPEC_PORT_DRD] = "dual",
> +};
> +
>   static const char * const typec_port_types_drp[] = {
>   	[TYPEC_PORT_SRC] = "dual [source] sink",
>   	[TYPEC_PORT_SNK] = "dual source [sink]",
> @@ -1252,6 +1259,51 @@ void typec_set_pwr_opmode(struct typec_port *port,
>   }
>   EXPORT_SYMBOL_GPL(typec_set_pwr_opmode);
>   
> +/**
> + * typec_find_power_type - Get the typec port power type

Why is this function called typec_find_power_type() and not typec_find_port_type()?
It's called port_type in sysfs, having different names just adds confusion.
(Otherwise I agree power_type is a better name but...)

> + * @name: port type string
> + *
> + * This routine is used to find the typec_port_type by its string name.
> + *
> + * Returns typec_port_type if success, otherwise negative error code.
> + */
> +int typec_find_power_type(const char *name)
> +{
> +	return match_string(typec_port_types, ARRAY_SIZE(typec_port_types),
> +			    name);
> +}
> +EXPORT_SYMBOL_GPL(typec_find_power_type);
> +
> +/**
> + * typec_find_preferred_role - Find the typec drp port preferred power role

Why typec_find_preferred_role()? Could be used for any power_role so
why not typec_find_power_role()?

BR // Mats

> + * @name: power role string
> + *
> + * This routine is used to find the typec_role by its string name of
> + * preferred power role(Try.SRC or Try.SNK).
> + *
> + * Returns typec_role if success, otherwise negative error code.
> + */
> +int typec_find_preferred_role(const char *name)
> +{
> +	return match_string(typec_roles, ARRAY_SIZE(typec_roles), name);
> +}
> +EXPORT_SYMBOL_GPL(typec_find_preferred_role);
> +
> +/**
> + * typec_find_data_type - Get the typec port data capability
> + * @name: data type string
> + *
> + * This routine is used to find the typec_port_data by its string name.
> + *
> + * Returns typec_port_data if success, otherwise negative error code.
> + */
> +int typec_find_data_type(const char *name)
> +{
> +	return match_string(typec_data_types, ARRAY_SIZE(typec_data_types),
> +			    name);
> +}
> +EXPORT_SYMBOL_GPL(typec_find_data_type);
> +
>   /* ------------------------------------------ */
>   /* API for Multiplexer/DeMultiplexer Switches */
>   
> diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
> index 672b39b..00c93e7 100644
> --- a/include/linux/usb/typec.h
> +++ b/include/linux/usb/typec.h
> @@ -267,4 +267,7 @@ int typec_set_orientation(struct typec_port *port,
>   			  enum typec_orientation orientation);
>   int typec_set_mode(struct typec_port *port, int mode);
>   
> +int typec_find_power_type(const char *name);
> +int typec_find_preferred_role(const char *name);
> +int typec_find_data_type(const char *name);
>   #endif /* __LINUX_USB_TYPEC_H */
>

Re: [PATCH v5 14/14] staging: typec: tcpci: move tcpci driver out of staging

[Mats Karrman]

Hi Li Jun,

This patch takes away building the entire staging/typec tree but this is still
not empty after your patch, another driver "tcpci_rt1711h" is there.
Better just remove tcpci from staging/typec/{Kconfig,Makefile}

// Mats

On 2018-05-03 02:24, Li Jun wrote:

> Move TCPCI(Typec port controller interface) driver out of staging.
>
> Signed-off-by: Li Jun <jun.li@nxp.com>
> ---
>   drivers/staging/Kconfig                | 2 --
>   drivers/staging/Makefile               | 1 -
>   drivers/staging/typec/TODO             | 5 -----
>   drivers/usb/typec/Kconfig              | 7 +++++++
>   drivers/usb/typec/Makefile             | 1 +
>   drivers/{staging => usb}/typec/tcpci.c | 0
>   drivers/{staging => usb}/typec/tcpci.h | 0
>   7 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/Kconfig b/drivers/staging/Kconfig
> index d5926f0..d83ff66 100644
> --- a/drivers/staging/Kconfig
> +++ b/drivers/staging/Kconfig
> @@ -112,8 +112,6 @@ source "drivers/staging/greybus/Kconfig"
>   
>   source "drivers/staging/vc04_services/Kconfig"
>   
> -source "drivers/staging/typec/Kconfig"
> -
>   source "drivers/staging/vboxvideo/Kconfig"
>   
>   source "drivers/staging/pi433/Kconfig"
> diff --git a/drivers/staging/Makefile b/drivers/staging/Makefile
> index 919753c..a71ec1f 100644
> --- a/drivers/staging/Makefile
> +++ b/drivers/staging/Makefile
> @@ -2,7 +2,6 @@
>   # Makefile for staging directory
>   
>   obj-y				+= media/
> -obj-y				+= typec/
>   obj-$(CONFIG_IPX)		+= ipx/
>   obj-$(CONFIG_NCP_FS)		+= ncpfs/
>   obj-$(CONFIG_PRISM2_USB)	+= wlan-ng/
> diff --git a/drivers/staging/typec/TODO b/drivers/staging/typec/TODO
> deleted file mode 100644
> index 53fe2f7..0000000
> --- a/drivers/staging/typec/TODO
> +++ /dev/null
> @@ -1,5 +0,0 @@
> -tcpci:
> -- Test with real hardware
> -
> -Please send patches to Guenter Roeck <linux@roeck-us.net> and copy
> -Heikki Krogerus <heikki.krogerus@linux.intel.com>.
> diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
> index 2c8eab1..0a862fc 100644
> --- a/drivers/usb/typec/Kconfig
> +++ b/drivers/usb/typec/Kconfig
> @@ -56,6 +56,13 @@ config TYPEC_TCPM
>   
>   if TYPEC_TCPM
>   
> +config TYPEC_TCPCI
> +	tristate "Type-C Port Controller Interface driver"
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	  Type-C Port Controller driver for TCPCI-compliant controller.
> +
>   source "drivers/usb/typec/fusb302/Kconfig"
>   
>   config TYPEC_WCOVE
> diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
> index 1f599a6..02758a1 100644
> --- a/drivers/usb/typec/Makefile
> +++ b/drivers/usb/typec/Makefile
> @@ -7,3 +7,4 @@ obj-$(CONFIG_TYPEC_WCOVE)	+= typec_wcove.o
>   obj-$(CONFIG_TYPEC_UCSI)	+= ucsi/
>   obj-$(CONFIG_TYPEC_TPS6598X)	+= tps6598x.o
>   obj-$(CONFIG_TYPEC)		+= mux/
> +obj-$(CONFIG_TYPEC_TCPCI)	+= tcpci.o
> diff --git a/drivers/staging/typec/tcpci.c b/drivers/usb/typec/tcpci.c
> similarity index 100%
> rename from drivers/staging/typec/tcpci.c
> rename to drivers/usb/typec/tcpci.c
> diff --git a/drivers/staging/typec/tcpci.h b/drivers/usb/typec/tcpci.h
> similarity index 100%
> rename from drivers/staging/typec/tcpci.h
> rename to drivers/usb/typec/tcpci.h

RE: [PATCH v5 01/14] dt-bindings: connector: add properties for typec

[Jun Li]

Hi
> -----Original Message-----
> From: Mats Karrman [mailto:mats.dev.list@gmail.com]
> Sent: 2018年5月12日 3:49
> To: Jun Li <jun.li@nxp.com>; robh+dt@kernel.org; gregkh@linuxfoundation.org;
> heikki.krogerus@linux.intel.com; linux@roeck-us.net
> Cc: a.hajda@samsung.com; cw00.choi@samsung.com;
> shufan_lee@richtek.com; Peter Chen <peter.chen@nxp.com>;
> gsomlo@gmail.com; devicetree@vger.kernel.org; linux-usb@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v5 01/14] dt-bindings: connector: add properties for typec
> 
> Hi Li Jun,
> 
> On 2018-05-03 02:24, Li Jun wrote:
> 
> > Add bingdings supported by current typec driver, so user can pass all
> > those properties via dt.
> >
> > Signed-off-by: Li Jun <jun.li@nxp.com>
> > ---
> >   .../bindings/connector/usb-connector.txt           | 44
> +++++++++++++++
> >   include/dt-bindings/usb/pd.h                       | 62
> ++++++++++++++++++++++
> >   2 files changed, 106 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/connector/usb-connector.txt
> > b/Documentation/devicetree/bindings/connector/usb-connector.txt
> > index e1463f1..4b19de6d0 100644
> > --- a/Documentation/devicetree/bindings/connector/usb-connector.txt
> > +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
> > @@ -15,6 +15,33 @@ Optional properties:
> >   - type: size of the connector, should be specified in case of USB-A, USB-B
> >     non-fullsize connectors: "mini", "micro".
> >
> > +Optional properties for usb-c-connector:
> > +- power-role: should be one of "source", "sink" or "dual"(DRP) if
> > +typec
> > +  connector has power support.
> > +- try-power-role: preferred power role if "dual"(DRP) can support
> > +Try.SNK
> > +  or Try.SRC, should be "sink" for Try.SNK or "source" for Try.SRC.
> > +- data-role: should be one of "host", "device", "dual"(DRD) if typec
> > +  connector supports USB data.
> > +
> > +Required properties for usb-c-connector with power delivery support:
> > +- source-pdos: An array of u32 with each entry providing supported
> > +power
> > +  source data object(PDO), the detailed bit definitions of PDO can be
> > +found
> > +  in "Universal Serial Bus Power Delivery Specification" chapter
> > +6.4.1.2
> > +  Source_Capabilities Message, the order of each entry(PDO) should
> > +follow
> > +  the PD spec chapter 6.4.1. Required for power source and power dual
> role.
> > +  User can specify the source PDO array via PDO_FIXED/BATT/VAR()
> > +defined in
> > +  dt-bindings/usb/pd.h.
> > +- sink-pdos: An array of u32 with each entry providing supported
> > +power
> > +  sink data object(PDO), the detailed bit definitions of PDO can be
> > +found
> > +  in "Universal Serial Bus Power Delivery Specification" chapter
> > +6.4.1.3
> > +  Sink Capabilities Message, the order of each entry(PDO) should
> > +follow
> > +  the PD spec chapter 6.4.1. Required for power sink and power dual role.
> > +  User can specify the sink PDO array via PDO_FIXED/BATT/VAR()
> > +defined in
> > +  dt-bindings/usb/pd.h.
> > +- op-sink-microwatt: Sink required operating power in microwatt, if
> > +source
> > +  can't offer the power, Capability Mismatch is set, required for
> > +power
> 
> ...set. Required...
> (new sentence, otherwise it's unclear what is required; op-sink-microwatt or
> Capability Mismatch set)

OK, will update as you suggested.

Thanks
Li Jun

RE: [PATCH v5 03/14] staging: typec: tcpci: add compatible string for nxp ptn5110

[Jun Li]

Hi
> -----Original Message-----
> From: Mats Karrman [mailto:mats.dev.list@gmail.com]
> Sent: 2018年5月12日 3:52
> To: Jun Li <jun.li@nxp.com>; robh+dt@kernel.org; gregkh@linuxfoundation.org;
> heikki.krogerus@linux.intel.com; linux@roeck-us.net
> Cc: a.hajda@samsung.com; cw00.choi@samsung.com;
> shufan_lee@richtek.com; Peter Chen <peter.chen@nxp.com>;
> gsomlo@gmail.com; devicetree@vger.kernel.org; linux-usb@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v5 03/14] staging: typec: tcpci: add compatible string for
> nxp ptn5110
> 
> Hi Li Jun,
> 
> On 2018-05-03 02:24, Li Jun wrote:
> 
> > Add nxp ptn5110 typec controller compatible string: usb-tcpci,ptn5110,
> > which is a standard tcpci chip with power delivery support.
> >
> > Signed-off-by: Li Jun <jun.li@nxp.com>
> > ---
> >   drivers/staging/typec/tcpci.c | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/staging/typec/tcpci.c
> > b/drivers/staging/typec/tcpci.c index 076d97e..741a80a 100644
> > --- a/drivers/staging/typec/tcpci.c
> > +++ b/drivers/staging/typec/tcpci.c
> > @@ -576,6 +576,7 @@ MODULE_DEVICE_TABLE(i2c, tcpci_id);
> >   #ifdef CONFIG_OF
> >   static const struct of_device_id tcpci_of_match[] = {
> >   	{ .compatible = "usb,tcpci", },
> 
> Either this line should go away, or a "generic TCPCI controller" line should be
> added to the DT documentation.

This binding name didn't follow the rule "<vender prefix>, <chip specific>",
AFAIK there is no user on upstream kernel, if no objection, I can remove it.

Li Jun
> 
> BR // Mats
> 
> > +	{ .compatible = "nxp,ptn5110", },
> >   	{},
> >   };
> >   MODULE_DEVICE_TABLE(of, tcpci_of_match);
> >

RE: [PATCH v5 05/14] usb: typec: add API to get typec basic port power and data config

[Jun Li]

Hi
> -----Original Message-----
> From: Mats Karrman [mailto:mats.dev.list@gmail.com]
> Sent: 2018年5月12日 3:56
> To: Jun Li <jun.li@nxp.com>; robh+dt@kernel.org; gregkh@linuxfoundation.org;
> heikki.krogerus@linux.intel.com; linux@roeck-us.net
> Cc: a.hajda@samsung.com; cw00.choi@samsung.com;
> shufan_lee@richtek.com; Peter Chen <peter.chen@nxp.com>;
> gsomlo@gmail.com; devicetree@vger.kernel.org; linux-usb@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v5 05/14] usb: typec: add API to get typec basic port power
> and data config
> 
> Hi Li Jun,
> 
> On 2018-05-03 02:24, Li Jun wrote:
> 
> > This patch adds 3 APIs to get the typec port power and data type, and
> > preferred power role by its name string.
> >
> > Signed-off-by: Li Jun <jun.li@nxp.com>
> > ---
> >   drivers/usb/typec/class.c | 52
> +++++++++++++++++++++++++++++++++++++++++++++++
> >   include/linux/usb/typec.h |  3 +++
> >   2 files changed, 55 insertions(+)
> >
> > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> > index 53df10d..5981e18 100644
> > --- a/drivers/usb/typec/class.c
> > +++ b/drivers/usb/typec/class.c
> > @@ -9,6 +9,7 @@
> >   #include <linux/device.h>
> >   #include <linux/module.h>
> >   #include <linux/mutex.h>
> > +#include <linux/property.h>
> >   #include <linux/slab.h>
> >   #include <linux/usb/typec.h>
> >   #include <linux/usb/typec_mux.h>
> > @@ -802,6 +803,12 @@ static const char * const typec_port_types[] = {
> >   	[TYPEC_PORT_DRP] = "dual",
> >   };
> >
> > +static const char * const typec_data_types[] = {
> > +	[TYPEC_PORT_DFP] = "host",
> > +	[TYPEC_PORT_UFP] = "device",
> > +	[TYPEC_PORT_DRD] = "dual",
> > +};
> > +
> >   static const char * const typec_port_types_drp[] = {
> >   	[TYPEC_PORT_SRC] = "dual [source] sink",
> >   	[TYPEC_PORT_SNK] = "dual source [sink]", @@ -1252,6 +1259,51
> @@
> > void typec_set_pwr_opmode(struct typec_port *port,
> >   }
> >   EXPORT_SYMBOL_GPL(typec_set_pwr_opmode);
> >
> > +/**
> > + * typec_find_power_type - Get the typec port power type
> 
> Why is this function called typec_find_power_type() and not
> typec_find_port_type()?
> It's called port_type in sysfs, having different names just adds confusion.
> (Otherwise I agree power_type is a better name but...)

We have "port type" before the power and data role separation,
this API name's intention is to reflect the power cap, anyway I
leave this to be decided by Heikki then.

> 
> > + * @name: port type string
> > + *
> > + * This routine is used to find the typec_port_type by its string name.
> > + *
> > + * Returns typec_port_type if success, otherwise negative error code.
> > + */
> > +int typec_find_power_type(const char *name) {
> > +	return match_string(typec_port_types, ARRAY_SIZE(typec_port_types),
> > +			    name);
> > +}
> > +EXPORT_SYMBOL_GPL(typec_find_power_type);
> > +
> > +/**
> > + * typec_find_preferred_role - Find the typec drp port preferred
> > +power role
> 
> Why typec_find_preferred_role()? Could be used for any power_role so why not
> typec_find_power_role()?

I am not sure if I catch your point of this comment.
For preferred role(if support try.sink or try.src) the only allowed power roles are 
"sink"
"source"
But for power role, the allowed type are 
"sink"
"source"
"dual"

Thanks
Li Jun
> 
> BR // Mats
> 
> > + * @name: power role string
> > + *
> > + * This routine is used to find the typec_role by its string name of
> > + * preferred power role(Try.SRC or Try.SNK).
> > + *
> > + * Returns typec_role if success, otherwise negative error code.
> > + */
> > +int typec_find_preferred_role(const char *name) {
> > +	return match_string(typec_roles, ARRAY_SIZE(typec_roles), name); }
> > +EXPORT_SYMBOL_GPL(typec_find_preferred_role);
> > +
> > +/**
> > + * typec_find_data_type - Get the typec port data capability
> > + * @name: data type string
> > + *
> > + * This routine is used to find the typec_port_data by its string name.
> > + *
> > + * Returns typec_port_data if success, otherwise negative error code.
> > + */
> > +int typec_find_data_type(const char *name) {
> > +	return match_string(typec_data_types, ARRAY_SIZE(typec_data_types),
> > +			    name);
> > +}
> > +EXPORT_SYMBOL_GPL(typec_find_data_type);
> > +
> >   /* ------------------------------------------ */
> >   /* API for Multiplexer/DeMultiplexer Switches */
> >
> > diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
> > index 672b39b..00c93e7 100644
> > --- a/include/linux/usb/typec.h
> > +++ b/include/linux/usb/typec.h
> > @@ -267,4 +267,7 @@ int typec_set_orientation(struct typec_port *port,
> >   			  enum typec_orientation orientation);
> >   int typec_set_mode(struct typec_port *port, int mode);
> >
> > +int typec_find_power_type(const char *name); int
> > +typec_find_preferred_role(const char *name); int
> > +typec_find_data_type(const char *name);
> >   #endif /* __LINUX_USB_TYPEC_H */
> >

RE: [PATCH v5 14/14] staging: typec: tcpci: move tcpci driver out of staging

[Jun Li]

Hi
> -----Original Message-----
> From: Mats Karrman [mailto:mats.dev.list@gmail.com]
> Sent: 2018年5月12日 5:37
> To: Jun Li <jun.li@nxp.com>; robh+dt@kernel.org; gregkh@linuxfoundation.org;
> heikki.krogerus@linux.intel.com; linux@roeck-us.net
> Cc: a.hajda@samsung.com; cw00.choi@samsung.com;
> shufan_lee@richtek.com; Peter Chen <peter.chen@nxp.com>;
> gsomlo@gmail.com; devicetree@vger.kernel.org; linux-usb@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v5 14/14] staging: typec: tcpci: move tcpci driver out of
> staging
> 
> Hi Li Jun,
> 
> This patch takes away building the entire staging/typec tree but this is still not
> empty after your patch, another driver "tcpci_rt1711h" is there.
> Better just remove tcpci from staging/typec/{Kconfig,Makefile}

Oh, yes, I should also move out tcpci_rt1711h driver, will update in next version.

Thanks
Li Jun
> 
> // Mats
> 
> On 2018-05-03 02:24, Li Jun wrote:
> 
> > Move TCPCI(Typec port controller interface) driver out of staging.
> >
> > Signed-off-by: Li Jun <jun.li@nxp.com>
> > ---
> >   drivers/staging/Kconfig                | 2 --
> >   drivers/staging/Makefile               | 1 -
> >   drivers/staging/typec/TODO             | 5 -----
> >   drivers/usb/typec/Kconfig              | 7 +++++++
> >   drivers/usb/typec/Makefile             | 1 +
> >   drivers/{staging => usb}/typec/tcpci.c | 0
> >   drivers/{staging => usb}/typec/tcpci.h | 0
> >   7 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/staging/Kconfig b/drivers/staging/Kconfig index
> > d5926f0..d83ff66 100644
> > --- a/drivers/staging/Kconfig
> > +++ b/drivers/staging/Kconfig
> > @@ -112,8 +112,6 @@ source "drivers/staging/greybus/Kconfig"
> >
> >   source "drivers/staging/vc04_services/Kconfig"
> >
> > -source "drivers/staging/typec/Kconfig"
> > -
> >   source "drivers/staging/vboxvideo/Kconfig"
> >
> >   source "drivers/staging/pi433/Kconfig"
> > diff --git a/drivers/staging/Makefile b/drivers/staging/Makefile index
> > 919753c..a71ec1f 100644
> > --- a/drivers/staging/Makefile
> > +++ b/drivers/staging/Makefile
> > @@ -2,7 +2,6 @@
> >   # Makefile for staging directory
> >
> >   obj-y				+= media/
> > -obj-y				+= typec/
> >   obj-$(CONFIG_IPX)		+= ipx/
> >   obj-$(CONFIG_NCP_FS)		+= ncpfs/
> >   obj-$(CONFIG_PRISM2_USB)	+= wlan-ng/
> > diff --git a/drivers/staging/typec/TODO b/drivers/staging/typec/TODO
> > deleted file mode 100644 index 53fe2f7..0000000
> > --- a/drivers/staging/typec/TODO
> > +++ /dev/null
> > @@ -1,5 +0,0 @@
> > -tcpci:
> > -- Test with real hardware
> > -
> > -Please send patches to Guenter Roeck <linux@roeck-us.net> and copy
> > -Heikki Krogerus <heikki.krogerus@linux.intel.com>.
> > diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
> > index 2c8eab1..0a862fc 100644
> > --- a/drivers/usb/typec/Kconfig
> > +++ b/drivers/usb/typec/Kconfig
> > @@ -56,6 +56,13 @@ config TYPEC_TCPM
> >
> >   if TYPEC_TCPM
> >
> > +config TYPEC_TCPCI
> > +	tristate "Type-C Port Controller Interface driver"
> > +	depends on I2C
> > +	select REGMAP_I2C
> > +	help
> > +	  Type-C Port Controller driver for TCPCI-compliant controller.
> > +
> >   source "drivers/usb/typec/fusb302/Kconfig"
> >
> >   config TYPEC_WCOVE
> > diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
> > index 1f599a6..02758a1 100644
> > --- a/drivers/usb/typec/Makefile
> > +++ b/drivers/usb/typec/Makefile
> > @@ -7,3 +7,4 @@ obj-$(CONFIG_TYPEC_WCOVE)	+= typec_wcove.o
> >   obj-$(CONFIG_TYPEC_UCSI)	+= ucsi/
> >   obj-$(CONFIG_TYPEC_TPS6598X)	+= tps6598x.o
> >   obj-$(CONFIG_TYPEC)		+= mux/
> > +obj-$(CONFIG_TYPEC_TCPCI)	+= tcpci.o
> > diff --git a/drivers/staging/typec/tcpci.c b/drivers/usb/typec/tcpci.c
> > similarity index 100% rename from drivers/staging/typec/tcpci.c rename
> > to drivers/usb/typec/tcpci.c diff --git
> > a/drivers/staging/typec/tcpci.h b/drivers/usb/typec/tcpci.h similarity
> > index 100% rename from drivers/staging/typec/tcpci.h rename to
> > drivers/usb/typec/tcpci.h

Re: [PATCH v5 05/14] usb: typec: add API to get typec basic port power and data config

[Mats Karrman]

Hi,

On 05/14/2018 11:36 AM, Jun Li wrote:

> Hi
>> -----Original Message-----
>> From: Mats Karrman [mailto:mats.dev.list@gmail.com]
>> Sent: 2018年5月12日 3:56
>> To: Jun Li <jun.li@nxp.com>; robh+dt@kernel.org; gregkh@linuxfoundation.org;
>> heikki.krogerus@linux.intel.com; linux@roeck-us.net
>> Cc: a.hajda@samsung.com; cw00.choi@samsung.com;
>> shufan_lee@richtek.com; Peter Chen <peter.chen@nxp.com>;
>> gsomlo@gmail.com; devicetree@vger.kernel.org; linux-usb@vger.kernel.org;
>> dl-linux-imx <linux-imx@nxp.com>
>> Subject: Re: [PATCH v5 05/14] usb: typec: add API to get typec basic port power
>> and data config
>>
>> Hi Li Jun,
>>
>> On 2018-05-03 02:24, Li Jun wrote:
>>
>>> This patch adds 3 APIs to get the typec port power and data type, and
>>> preferred power role by its name string.
>>>
>>> Signed-off-by: Li Jun <jun.li@nxp.com>
>>> ---
>>>   drivers/usb/typec/class.c | 52
>> +++++++++++++++++++++++++++++++++++++++++++++++
>>>   include/linux/usb/typec.h |  3 +++
>>>   2 files changed, 55 insertions(+)
>>>
>>> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
>>> index 53df10d..5981e18 100644
>>> --- a/drivers/usb/typec/class.c
>>> +++ b/drivers/usb/typec/class.c
>>> @@ -9,6 +9,7 @@
>>>   #include <linux/device.h>
>>>   #include <linux/module.h>
>>>   #include <linux/mutex.h>
>>> +#include <linux/property.h>
>>>   #include <linux/slab.h>
>>>   #include <linux/usb/typec.h>
>>>   #include <linux/usb/typec_mux.h>
>>> @@ -802,6 +803,12 @@ static const char * const typec_port_types[] = {
>>>   	[TYPEC_PORT_DRP] = "dual",
>>>   };
>>>
>>> +static const char * const typec_data_types[] = {
>>> +	[TYPEC_PORT_DFP] = "host",
>>> +	[TYPEC_PORT_UFP] = "device",
>>> +	[TYPEC_PORT_DRD] = "dual",
>>> +};
>>> +
>>>   static const char * const typec_port_types_drp[] = {
>>>   	[TYPEC_PORT_SRC] = "dual [source] sink",
>>>   	[TYPEC_PORT_SNK] = "dual source [sink]", @@ -1252,6 +1259,51
>> @@
>>> void typec_set_pwr_opmode(struct typec_port *port,
>>>   }
>>>   EXPORT_SYMBOL_GPL(typec_set_pwr_opmode);
>>>
>>> +/**
>>> + * typec_find_power_type - Get the typec port power type
>> Why is this function called typec_find_power_type() and not
>> typec_find_port_type()?
>> It's called port_type in sysfs, having different names just adds confusion.
>> (Otherwise I agree power_type is a better name but...)
> We have "port type" before the power and data role separation,
> this API name's intention is to reflect the power cap, anyway I
> leave this to be decided by Heikki then.
>>> + * @name: port type string
>>> + *
>>> + * This routine is used to find the typec_port_type by its string name.
>>> + *
>>> + * Returns typec_port_type if success, otherwise negative error code.
>>> + */
>>> +int typec_find_power_type(const char *name) {
>>> +	return match_string(typec_port_types, ARRAY_SIZE(typec_port_types),
>>> +			    name);
>>> +}
>>> +EXPORT_SYMBOL_GPL(typec_find_power_type);
>>> +
>>> +/**
>>> + * typec_find_preferred_role - Find the typec drp port preferred
>>> +power role
>> Why typec_find_preferred_role()? Could be used for any power_role so why not
>> typec_find_power_role()?
> I am not sure if I catch your point of this comment.
> For preferred role(if support try.sink or try.src) the only allowed power roles are 
> "sink"
> "source"
> But for power role, the allowed type are 
> "sink"
> "source"
> "dual"

Uhm, typing too fast again, I am. A better name would be just typec_find_role().
What I mean is that the function could be used for any situation when
someone wants to map a string to a TYPEC_{SOURCE,SINK} constant so it
is unnecessary to limit its usage to just preferred role.
// Mats

> Thanks
> Li Jun
>> BR // Mats
>>
>>> + * @name: power role string
>>> + *
>>> + * This routine is used to find the typec_role by its string name of
>>> + * preferred power role(Try.SRC or Try.SNK).
>>> + *
>>> + * Returns typec_role if success, otherwise negative error code.
>>> + */
>>> +int typec_find_preferred_role(const char *name) {
>>> +	return match_string(typec_roles, ARRAY_SIZE(typec_roles), name); }
>>> +EXPORT_SYMBOL_GPL(typec_find_preferred_role);
>>> +
>>> +/**
>>> + * typec_find_data_type - Get the typec port data capability
>>> + * @name: data type string
>>> + *
>>> + * This routine is used to find the typec_port_data by its string name.
>>> + *
>>> + * Returns typec_port_data if success, otherwise negative error code.
>>> + */
>>> +int typec_find_data_type(const char *name) {
>>> +	return match_string(typec_data_types, ARRAY_SIZE(typec_data_types),
>>> +			    name);
>>> +}
>>> +EXPORT_SYMBOL_GPL(typec_find_data_type);
>>> +
>>>   /* ------------------------------------------ */
>>>   /* API for Multiplexer/DeMultiplexer Switches */
>>>
>>> diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
>>> index 672b39b..00c93e7 100644
>>> --- a/include/linux/usb/typec.h
>>> +++ b/include/linux/usb/typec.h
>>> @@ -267,4 +267,7 @@ int typec_set_orientation(struct typec_port *port,
>>>   			  enum typec_orientation orientation);
>>>   int typec_set_mode(struct typec_port *port, int mode);
>>>
>>> +int typec_find_power_type(const char *name); int
>>> +typec_find_preferred_role(const char *name); int
>>> +typec_find_data_type(const char *name);
>>>   #endif /* __LINUX_USB_TYPEC_H */

RE: [PATCH v5 01/14] dt-bindings: connector: add properties for typec

[Peter Chen]

  
> Add bingdings supported by current typec driver, so user can pass all those
> properties via dt.
> 

%s/bingdings/bindings

Peter

RE: [PATCH v5 12/14] staging: typec: tcpci: keep the not connecting cc line open

[Peter Chen]

 
> 
> While set polarity, we should keep the not connecting cc line to be open.
> 

keep the disconnected cc line open?

Peter

> Signed-off-by: Li Jun <jun.li@nxp.com>
> ---
>  drivers/staging/typec/tcpci.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c index
> 5c48810..5c0c5e3 100644
> --- a/drivers/staging/typec/tcpci.c
> +++ b/drivers/staging/typec/tcpci.c
> @@ -185,15 +185,25 @@ static int tcpci_set_polarity(struct tcpc_dev *tcpc,
>  			      enum typec_cc_polarity polarity)  {
>  	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> +	unsigned int reg;
>  	int ret;
> 
> -	ret = regmap_write(tcpci->regmap, TCPC_TCPC_CTRL,
> -			   (polarity == TYPEC_POLARITY_CC2) ?
> -			   TCPC_TCPC_CTRL_ORIENTATION : 0);
> +	/* Keep the disconnect cc line open */
> +	ret = regmap_read(tcpci->regmap, TCPC_ROLE_CTRL, &reg);
>  	if (ret < 0)
>  		return ret;
> 
> -	return 0;
> +	if (polarity == TYPEC_POLARITY_CC2)
> +		reg |= TCPC_ROLE_CTRL_CC_OPEN <<
> TCPC_ROLE_CTRL_CC1_SHIFT;
> +	else
> +		reg |= TCPC_ROLE_CTRL_CC_OPEN <<
> 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_TCPC_CTRL,
> +			   (polarity == TYPEC_POLARITY_CC2) ?
> +			   TCPC_TCPC_CTRL_ORIENTATION : 0);
>  }
> 
>  static int tcpci_set_vconn(struct tcpc_dev *tcpc, bool enable)
> --
> 2.7.4

Re: [PATCH v5 05/14] usb: typec: add API to get typec basic port power and data config

[Heikki Krogerus]

Hi guys,

On Tue, May 15, 2018 at 10:52:57PM +0200, Mats Karrman wrote:
> Hi,
> 
> On 05/14/2018 11:36 AM, Jun Li wrote:
> 
> > Hi
> >> -----Original Message-----
> >> From: Mats Karrman [mailto:mats.dev.list@gmail.com]
> >> Sent: 2018???5???12??? 3:56
> >> To: Jun Li <jun.li@nxp.com>; robh+dt@kernel.org; gregkh@linuxfoundation.org;
> >> heikki.krogerus@linux.intel.com; linux@roeck-us.net
> >> Cc: a.hajda@samsung.com; cw00.choi@samsung.com;
> >> shufan_lee@richtek.com; Peter Chen <peter.chen@nxp.com>;
> >> gsomlo@gmail.com; devicetree@vger.kernel.org; linux-usb@vger.kernel.org;
> >> dl-linux-imx <linux-imx@nxp.com>
> >> Subject: Re: [PATCH v5 05/14] usb: typec: add API to get typec basic port power
> >> and data config
> >>
> >> Hi Li Jun,
> >>
> >> On 2018-05-03 02:24, Li Jun wrote:
> >>
> >>> This patch adds 3 APIs to get the typec port power and data type, and
> >>> preferred power role by its name string.
> >>>
> >>> Signed-off-by: Li Jun <jun.li@nxp.com>
> >>> ---
> >>>   drivers/usb/typec/class.c | 52
> >> +++++++++++++++++++++++++++++++++++++++++++++++
> >>>   include/linux/usb/typec.h |  3 +++
> >>>   2 files changed, 55 insertions(+)
> >>>
> >>> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> >>> index 53df10d..5981e18 100644
> >>> --- a/drivers/usb/typec/class.c
> >>> +++ b/drivers/usb/typec/class.c
> >>> @@ -9,6 +9,7 @@
> >>>   #include <linux/device.h>
> >>>   #include <linux/module.h>
> >>>   #include <linux/mutex.h>
> >>> +#include <linux/property.h>

I don't think you need that anymore.

> >>>   #include <linux/slab.h>
> >>>   #include <linux/usb/typec.h>
> >>>   #include <linux/usb/typec_mux.h>
> >>> @@ -802,6 +803,12 @@ static const char * const typec_port_types[] = {
> >>>   	[TYPEC_PORT_DRP] = "dual",
> >>>   };
> >>>
> >>> +static const char * const typec_data_types[] = {
> >>> +	[TYPEC_PORT_DFP] = "host",
> >>> +	[TYPEC_PORT_UFP] = "device",
> >>> +	[TYPEC_PORT_DRD] = "dual",
> >>> +};
> >>> +
> >>>   static const char * const typec_port_types_drp[] = {
> >>>   	[TYPEC_PORT_SRC] = "dual [source] sink",
> >>>   	[TYPEC_PORT_SNK] = "dual source [sink]", @@ -1252,6 +1259,51
> >> @@
> >>> void typec_set_pwr_opmode(struct typec_port *port,
> >>>   }
> >>>   EXPORT_SYMBOL_GPL(typec_set_pwr_opmode);
> >>>
> >>> +/**
> >>> + * typec_find_power_type - Get the typec port power type
> >> Why is this function called typec_find_power_type() and not
> >> typec_find_port_type()?
> >> It's called port_type in sysfs, having different names just adds confusion.
> >> (Otherwise I agree power_type is a better name but...)
> > We have "port type" before the power and data role separation,
> > this API name's intention is to reflect the power cap, anyway I
> > leave this to be decided by Heikki then.

I really hate the "*_type" naming. It was understandable when there
was no separate power and data roles defined in the specification, but
now that there are, it's just confusing. IMO we should not use it
anywhere.

So to me typec_find_type() is just as bad as typec_find_power_type()
because it has the "type" in it. I wonder if this function is
necessary at all? If it is, then perhaps we can think of some better
name for it, name that gives a better hint what it is used for.

> >>> + * @name: port type string
> >>> + *
> >>> + * This routine is used to find the typec_port_type by its string name.
> >>> + *
> >>> + * Returns typec_port_type if success, otherwise negative error code.
> >>> + */
> >>> +int typec_find_power_type(const char *name) {
> >>> +	return match_string(typec_port_types, ARRAY_SIZE(typec_port_types),
> >>> +			    name);
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(typec_find_power_type);
> >>> +
> >>> +/**
> >>> + * typec_find_preferred_role - Find the typec drp port preferred
> >>> +power role
> >> Why typec_find_preferred_role()? Could be used for any power_role so why not
> >> typec_find_power_role()?
> > I am not sure if I catch your point of this comment.
> > For preferred role(if support try.sink or try.src) the only allowed power roles are 
> > "sink"
> > "source"
> > But for power role, the allowed type are 
> > "sink"
> > "source"
> > "dual"
> 
> Uhm, typing too fast again, I am. A better name would be just typec_find_role().
> What I mean is that the function could be used for any situation when
> someone wants to map a string to a TYPEC_{SOURCE,SINK} constant so it
> is unnecessary to limit its usage to just preferred role.

That sounds reasonable to me.


Thanks,

-- 
heikki

Re: [PATCH v5 05/14] usb: typec: add API to get typec basic port power and data config

[Mats Karrman]

Hi,

On 05/16/2018 02:25 PM, Heikki Krogerus wrote:

> Hi guys,
>
> On Tue, May 15, 2018 at 10:52:57PM +0200, Mats Karrman wrote:
>> Hi,
>>
>> On 05/14/2018 11:36 AM, Jun Li wrote:
>>
>>> Hi
>>>> -----Original Message-----
>>>> From: Mats Karrman [mailto:mats.dev.list@gmail.com]
>>>> Sent: 2018???5???12??? 3:56
>>>> To: Jun Li <jun.li@nxp.com>; robh+dt@kernel.org; gregkh@linuxfoundation.org;
>>>> heikki.krogerus@linux.intel.com; linux@roeck-us.net
>>>> Cc: a.hajda@samsung.com; cw00.choi@samsung.com;
>>>> shufan_lee@richtek.com; Peter Chen <peter.chen@nxp.com>;
>>>> gsomlo@gmail.com; devicetree@vger.kernel.org; linux-usb@vger.kernel.org;
>>>> dl-linux-imx <linux-imx@nxp.com>
>>>> Subject: Re: [PATCH v5 05/14] usb: typec: add API to get typec basic port power
>>>> and data config
>>>>
>>>> Hi Li Jun,
>>>>
>>>> On 2018-05-03 02:24, Li Jun wrote:
>>>>
>>>>> This patch adds 3 APIs to get the typec port power and data type, and
>>>>> preferred power role by its name string.
>>>>>
>>>>> Signed-off-by: Li Jun <jun.li@nxp.com>
>>>>> ---
>>>>>   drivers/usb/typec/class.c | 52
>>>> +++++++++++++++++++++++++++++++++++++++++++++++
>>>>>   include/linux/usb/typec.h |  3 +++
>>>>>   2 files changed, 55 insertions(+)
>>>>>
>>>>> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
>>>>> index 53df10d..5981e18 100644
>>>>> --- a/drivers/usb/typec/class.c
>>>>> +++ b/drivers/usb/typec/class.c
>>>>> @@ -9,6 +9,7 @@
>>>>>   #include <linux/device.h>
>>>>>   #include <linux/module.h>
>>>>>   #include <linux/mutex.h>
>>>>> +#include <linux/property.h>
> I don't think you need that anymore.
>
>>>>>   #include <linux/slab.h>
>>>>>   #include <linux/usb/typec.h>
>>>>>   #include <linux/usb/typec_mux.h>
>>>>> @@ -802,6 +803,12 @@ static const char * const typec_port_types[] = {
>>>>>   	[TYPEC_PORT_DRP] = "dual",
>>>>>   };
>>>>>
>>>>> +static const char * const typec_data_types[] = {
>>>>> +	[TYPEC_PORT_DFP] = "host",
>>>>> +	[TYPEC_PORT_UFP] = "device",
>>>>> +	[TYPEC_PORT_DRD] = "dual",
>>>>> +};
>>>>> +
>>>>>   static const char * const typec_port_types_drp[] = {
>>>>>   	[TYPEC_PORT_SRC] = "dual [source] sink",
>>>>>   	[TYPEC_PORT_SNK] = "dual source [sink]", @@ -1252,6 +1259,51
>>>> @@
>>>>> void typec_set_pwr_opmode(struct typec_port *port,
>>>>>   }
>>>>>   EXPORT_SYMBOL_GPL(typec_set_pwr_opmode);
>>>>>
>>>>> +/**
>>>>> + * typec_find_power_type - Get the typec port power type
>>>> Why is this function called typec_find_power_type() and not
>>>> typec_find_port_type()?
>>>> It's called port_type in sysfs, having different names just adds confusion.
>>>> (Otherwise I agree power_type is a better name but...)
>>> We have "port type" before the power and data role separation,
>>> this API name's intention is to reflect the power cap, anyway I
>>> leave this to be decided by Heikki then.
> I really hate the "*_type" naming. It was understandable when there
> was no separate power and data roles defined in the specification, but
> now that there are, it's just confusing. IMO we should not use it
> anywhere.
>
> So to me typec_find_type() is just as bad as typec_find_power_type()
> because it has the "type" in it. I wonder if this function is
> necessary at all? If it is, then perhaps we can think of some better
> name for it, name that gives a better hint what it is used for.

I reread this patch and tried to see it more in the context of the other
patches and the existing code. The naming of the existing string tables
doesn't help in getting this right, however I have a proposal:

typec_find_port_power_role() to get to TYPEC_PORT_SRC/SNK/DRP
typec_find_port_data_role() to get to TYPEC_PORT_DFP/UFP/DRD
typec_find_power_role() to get to TYPEC_SINK/SOURCE

and sometime, if the use should arise 

typec_find_data_role() to get to TYPEC_DEVICE/HOST

I think it is fairly comprehensible, *_port_* concerns a capability and
without *_port_* it is an actual state. Plus it matches the names of the
constants.

BR // Mats

>>>>> + * @name: port type string
>>>>> + *
>>>>> + * This routine is used to find the typec_port_type by its string name.
>>>>> + *
>>>>> + * Returns typec_port_type if success, otherwise negative error code.
>>>>> + */
>>>>> +int typec_find_power_type(const char *name) {
>>>>> +	return match_string(typec_port_types, ARRAY_SIZE(typec_port_types),
>>>>> +			    name);
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(typec_find_power_type);
>>>>> +
>>>>> +/**
>>>>> + * typec_find_preferred_role - Find the typec drp port preferred
>>>>> +power role
>>>> Why typec_find_preferred_role()? Could be used for any power_role so why not
>>>> typec_find_power_role()?
>>> I am not sure if I catch your point of this comment.
>>> For preferred role(if support try.sink or try.src) the only allowed power roles are 
>>> "sink"
>>> "source"
>>> But for power role, the allowed type are 
>>> "sink"
>>> "source"
>>> "dual"
>> Uhm, typing too fast again, I am. A better name would be just typec_find_role().
>> What I mean is that the function could be used for any situation when
>> someone wants to map a string to a TYPEC_{SOURCE,SINK} constant so it
>> is unnecessary to limit its usage to just preferred role.
> That sounds reasonable to me.
>
>
> Thanks,
>

Re: [PATCH v5 05/14] usb: typec: add API to get typec basic port power and data config

[Heikki Krogerus]

On Wed, May 16, 2018 at 10:55:36PM +0200, Mats Karrman wrote:
> Hi,
> 
> On 05/16/2018 02:25 PM, Heikki Krogerus wrote:
> 
> > Hi guys,
> >
> > On Tue, May 15, 2018 at 10:52:57PM +0200, Mats Karrman wrote:
> >> Hi,
> >>
> >> On 05/14/2018 11:36 AM, Jun Li wrote:
> >>
> >>> Hi
> >>>> -----Original Message-----
> >>>> From: Mats Karrman [mailto:mats.dev.list@gmail.com]
> >>>> Sent: 2018???5???12??? 3:56
> >>>> To: Jun Li <jun.li@nxp.com>; robh+dt@kernel.org; gregkh@linuxfoundation.org;
> >>>> heikki.krogerus@linux.intel.com; linux@roeck-us.net
> >>>> Cc: a.hajda@samsung.com; cw00.choi@samsung.com;
> >>>> shufan_lee@richtek.com; Peter Chen <peter.chen@nxp.com>;
> >>>> gsomlo@gmail.com; devicetree@vger.kernel.org; linux-usb@vger.kernel.org;
> >>>> dl-linux-imx <linux-imx@nxp.com>
> >>>> Subject: Re: [PATCH v5 05/14] usb: typec: add API to get typec basic port power
> >>>> and data config
> >>>>
> >>>> Hi Li Jun,
> >>>>
> >>>> On 2018-05-03 02:24, Li Jun wrote:
> >>>>
> >>>>> This patch adds 3 APIs to get the typec port power and data type, and
> >>>>> preferred power role by its name string.
> >>>>>
> >>>>> Signed-off-by: Li Jun <jun.li@nxp.com>
> >>>>> ---
> >>>>>   drivers/usb/typec/class.c | 52
> >>>> +++++++++++++++++++++++++++++++++++++++++++++++
> >>>>>   include/linux/usb/typec.h |  3 +++
> >>>>>   2 files changed, 55 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> >>>>> index 53df10d..5981e18 100644
> >>>>> --- a/drivers/usb/typec/class.c
> >>>>> +++ b/drivers/usb/typec/class.c
> >>>>> @@ -9,6 +9,7 @@
> >>>>>   #include <linux/device.h>
> >>>>>   #include <linux/module.h>
> >>>>>   #include <linux/mutex.h>
> >>>>> +#include <linux/property.h>
> > I don't think you need that anymore.
> >
> >>>>>   #include <linux/slab.h>
> >>>>>   #include <linux/usb/typec.h>
> >>>>>   #include <linux/usb/typec_mux.h>
> >>>>> @@ -802,6 +803,12 @@ static const char * const typec_port_types[] = {
> >>>>>   	[TYPEC_PORT_DRP] = "dual",
> >>>>>   };
> >>>>>
> >>>>> +static const char * const typec_data_types[] = {
> >>>>> +	[TYPEC_PORT_DFP] = "host",
> >>>>> +	[TYPEC_PORT_UFP] = "device",
> >>>>> +	[TYPEC_PORT_DRD] = "dual",
> >>>>> +};
> >>>>> +
> >>>>>   static const char * const typec_port_types_drp[] = {
> >>>>>   	[TYPEC_PORT_SRC] = "dual [source] sink",
> >>>>>   	[TYPEC_PORT_SNK] = "dual source [sink]", @@ -1252,6 +1259,51
> >>>> @@
> >>>>> void typec_set_pwr_opmode(struct typec_port *port,
> >>>>>   }
> >>>>>   EXPORT_SYMBOL_GPL(typec_set_pwr_opmode);
> >>>>>
> >>>>> +/**
> >>>>> + * typec_find_power_type - Get the typec port power type
> >>>> Why is this function called typec_find_power_type() and not
> >>>> typec_find_port_type()?
> >>>> It's called port_type in sysfs, having different names just adds confusion.
> >>>> (Otherwise I agree power_type is a better name but...)
> >>> We have "port type" before the power and data role separation,
> >>> this API name's intention is to reflect the power cap, anyway I
> >>> leave this to be decided by Heikki then.
> > I really hate the "*_type" naming. It was understandable when there
> > was no separate power and data roles defined in the specification, but
> > now that there are, it's just confusing. IMO we should not use it
> > anywhere.
> >
> > So to me typec_find_type() is just as bad as typec_find_power_type()
> > because it has the "type" in it. I wonder if this function is
> > necessary at all? If it is, then perhaps we can think of some better
> > name for it, name that gives a better hint what it is used for.
> 
> I reread this patch and tried to see it more in the context of the other
> patches and the existing code. The naming of the existing string tables
> doesn't help in getting this right, however I have a proposal:
> 
> typec_find_port_power_role() to get to TYPEC_PORT_SRC/SNK/DRP
> typec_find_port_data_role() to get to TYPEC_PORT_DFP/UFP/DRD
> typec_find_power_role() to get to TYPEC_SINK/SOURCE
> 
> and sometime, if the use should arise 
> 
> typec_find_data_role() to get to TYPEC_DEVICE/HOST
> 
> I think it is fairly comprehensible, *_port_* concerns a capability and
> without *_port_* it is an actual state. Plus it matches the names of the
> constants.

Well, there are now four things to consider:

1) the roles (power and data) the port is capable of supporting
2) Try.SRC and Try.SNK, i.e. the preferred role
3) the current roles
4) the role(s) the user want's to limit the use of a port with DRP ports

The last one I don't know if it's relevant with these functions. It's
not information that we would get for example from firmware.

I also don't think we need to use these functions with the current
roles the port is in.

If the preferred role is "sink" or "source", so just like the power
role, we don't need separate function for it here.

So isn't two functions all we need here: one for the power and one for
data role?


Thanks,

-- 
heikki

RE: [PATCH v5 05/14] usb: typec: add API to get typec basic port power and data config

[Jun Li]

Hi Mats,
> 
> Uhm, typing too fast again, I am. A better name would be just
> typec_find_role().
> What I mean is that the function could be used for any situation when
> someone wants to map a string to a TYPEC_{SOURCE,SINK} constant so it is
> unnecessary to limit its usage to just preferred role.

Get your idea, that will be a more general API for typec class.

Thanks
Li Jun

RE: [PATCH v5 01/14] dt-bindings: connector: add properties for typec

[Jun Li]

Hi
> -----Original Message-----
> From: Peter Chen
> Sent: 2018年5月16日 15:22
> To: Jun Li <jun.li@nxp.com>; robh+dt@kernel.org; gregkh@linuxfoundation.org;
> heikki.krogerus@linux.intel.com; linux@roeck-us.net
> Cc: a.hajda@samsung.com; cw00.choi@samsung.com;
> shufan_lee@richtek.com; gsomlo@gmail.com; devicetree@vger.kernel.org;
> linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: RE: [PATCH v5 01/14] dt-bindings: connector: add properties for typec
> 
> 
> > Add bingdings supported by current typec driver, so user can pass all
> > those properties via dt.
> >
> 
> %s/bingdings/bindings

Will change

Thanks
Li Jun
> 
> Peter

RE: [PATCH v5 12/14] staging: typec: tcpci: keep the not connecting cc line open

[Jun Li]

> -----Original Message-----
> From: Peter Chen
> Sent: 2018年5月16日 16:36
> To: Jun Li <jun.li@nxp.com>; robh+dt@kernel.org; gregkh@linuxfoundation.org;
> heikki.krogerus@linux.intel.com; linux@roeck-us.net
> Cc: a.hajda@samsung.com; cw00.choi@samsung.com;
> shufan_lee@richtek.com; gsomlo@gmail.com; devicetree@vger.kernel.org;
> linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: RE: [PATCH v5 12/14] staging: typec: tcpci: keep the not connecting cc
> line open
> 
> 
> >
> > While set polarity, we should keep the not connecting cc line to be open.
> >
> 
> keep the disconnected cc line open?

Okay, I will change.

Thanks
Li Jun
> 
> Peter
> 
> > Signed-off-by: Li Jun <jun.li@nxp.com>
> > ---
> >  drivers/staging/typec/tcpci.c | 18 ++++++++++++++----
> >  1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c index
> > 5c48810..5c0c5e3 100644
> > --- a/drivers/staging/typec/tcpci.c
> > +++ b/drivers/staging/typec/tcpci.c
> > @@ -185,15 +185,25 @@ static int tcpci_set_polarity(struct tcpc_dev *tcpc,
> >  			      enum typec_cc_polarity polarity)  {
> >  	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> > +	unsigned int reg;
> >  	int ret;
> >
> > -	ret = regmap_write(tcpci->regmap, TCPC_TCPC_CTRL,
> > -			   (polarity == TYPEC_POLARITY_CC2) ?
> > -			   TCPC_TCPC_CTRL_ORIENTATION : 0);
> > +	/* Keep the disconnect cc line open */
> > +	ret = regmap_read(tcpci->regmap, TCPC_ROLE_CTRL, &reg);
> >  	if (ret < 0)
> >  		return ret;
> >
> > -	return 0;
> > +	if (polarity == TYPEC_POLARITY_CC2)
> > +		reg |= TCPC_ROLE_CTRL_CC_OPEN <<
> > TCPC_ROLE_CTRL_CC1_SHIFT;
> > +	else
> > +		reg |= TCPC_ROLE_CTRL_CC_OPEN <<
> > 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_TCPC_CTRL,
> > +			   (polarity == TYPEC_POLARITY_CC2) ?
> > +			   TCPC_TCPC_CTRL_ORIENTATION : 0);
> >  }
> >
> >  static int tcpci_set_vconn(struct tcpc_dev *tcpc, bool enable)
> > --
> > 2.7.4

RE: [PATCH v5 05/14] usb: typec: add API to get typec basic port power and data config

[Jun Li]

Hi
> -----Original Message-----
> From: Heikki Krogerus [mailto:heikki.krogerus@linux.intel.com]
> Sent: 2018年5月16日 20:25
> To: Jun Li <jun.li@nxp.com>; Mats Karrman <mats.dev.list@gmail.com>
> Cc: robh+dt@kernel.org; gregkh@linuxfoundation.org; linux@roeck-us.net;
> a.hajda@samsung.com; cw00.choi@samsung.com; shufan_lee@richtek.com;
> Peter Chen <peter.chen@nxp.com>; gsomlo@gmail.com;
> devicetree@vger.kernel.org; linux-usb@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v5 05/14] usb: typec: add API to get typec basic port power
> and data config
> 
> Hi guys,
> 
> On Tue, May 15, 2018 at 10:52:57PM +0200, Mats Karrman wrote:
> > Hi,
> >
> > On 05/14/2018 11:36 AM, Jun Li wrote:
> >
> > > Hi
> > >> -----Original Message-----
> > >> From: Mats Karrman [mailto:mats.dev.list@gmail.com]
> > >> Sent: 2018???5???12??? 3:56
> > >> To: Jun Li <jun.li@nxp.com>; robh+dt@kernel.org;
> > >> gregkh@linuxfoundation.org; heikki.krogerus@linux.intel.com;
> > >> linux@roeck-us.net
> > >> Cc: a.hajda@samsung.com; cw00.choi@samsung.com;
> > >> shufan_lee@richtek.com; Peter Chen <peter.chen@nxp.com>;
> > >> gsomlo@gmail.com; devicetree@vger.kernel.org;
> > >> linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> > >> Subject: Re: [PATCH v5 05/14] usb: typec: add API to get typec
> > >> basic port power and data config
> > >>
> > >> Hi Li Jun,
> > >>
> > >> On 2018-05-03 02:24, Li Jun wrote:
> > >>
> > >>> This patch adds 3 APIs to get the typec port power and data type,
> > >>> and preferred power role by its name string.
> > >>>
> > >>> Signed-off-by: Li Jun <jun.li@nxp.com>
> > >>> ---
> > >>>   drivers/usb/typec/class.c | 52
> > >> +++++++++++++++++++++++++++++++++++++++++++++++
> > >>>   include/linux/usb/typec.h |  3 +++
> > >>>   2 files changed, 55 insertions(+)
> > >>>
> > >>> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> > >>> index 53df10d..5981e18 100644
> > >>> --- a/drivers/usb/typec/class.c
> > >>> +++ b/drivers/usb/typec/class.c
> > >>> @@ -9,6 +9,7 @@
> > >>>   #include <linux/device.h>
> > >>>   #include <linux/module.h>
> > >>>   #include <linux/mutex.h>
> > >>> +#include <linux/property.h>
> 
> I don't think you need that anymore.

I will remove it.

> 
> > >>>   #include <linux/slab.h>
> > >>>   #include <linux/usb/typec.h>
> > >>>   #include <linux/usb/typec_mux.h> @@ -802,6 +803,12 @@ static
> > >>> const char * const typec_port_types[] = {
> > >>>   	[TYPEC_PORT_DRP] = "dual",
> > >>>   };
> > >>>
> > >>> +static const char * const typec_data_types[] = {
> > >>> +	[TYPEC_PORT_DFP] = "host",
> > >>> +	[TYPEC_PORT_UFP] = "device",
> > >>> +	[TYPEC_PORT_DRD] = "dual",
> > >>> +};
> > >>> +
> > >>>   static const char * const typec_port_types_drp[] = {
> > >>>   	[TYPEC_PORT_SRC] = "dual [source] sink",
> > >>>   	[TYPEC_PORT_SNK] = "dual source [sink]", @@ -1252,6 +1259,51
> > >> @@
> > >>> void typec_set_pwr_opmode(struct typec_port *port,
> > >>>   }
> > >>>   EXPORT_SYMBOL_GPL(typec_set_pwr_opmode);
> > >>>
> > >>> +/**
> > >>> + * typec_find_power_type - Get the typec port power type
> > >> Why is this function called typec_find_power_type() and not
> > >> typec_find_port_type()?
> > >> It's called port_type in sysfs, having different names just adds confusion.
> > >> (Otherwise I agree power_type is a better name but...)
> > > We have "port type" before the power and data role separation, this
> > > API name's intention is to reflect the power cap, anyway I leave
> > > this to be decided by Heikki then.
> 
> I really hate the "*_type" naming. It was understandable when there was no
> separate power and data roles defined in the specification, but now that there
> are, it's just confusing. IMO we should not use it anywhere.

OK, know your preference now.

> 
> So to me typec_find_type() is just as bad as typec_find_power_type() because it
> has the "type" in it. I wonder if this function is necessary at all? If it is, then
> perhaps we can think of some better name for it, name that gives a better hint
> what it is used for.

We need a way to match a string to a value which has to be done via typec
class.
 
> 
> > >>> + * @name: port type string
> > >>> + *
> > >>> + * This routine is used to find the typec_port_type by its string name.
> > >>> + *
> > >>> + * Returns typec_port_type if success, otherwise negative error code.
> > >>> + */
> > >>> +int typec_find_power_type(const char *name) {
> > >>> +	return match_string(typec_port_types,
> ARRAY_SIZE(typec_port_types),
> > >>> +			    name);
> > >>> +}
> > >>> +EXPORT_SYMBOL_GPL(typec_find_power_type);
> > >>> +
> > >>> +/**
> > >>> + * typec_find_preferred_role - Find the typec drp port preferred
> > >>> +power role
> > >> Why typec_find_preferred_role()? Could be used for any power_role
> > >> so why not typec_find_power_role()?
> > > I am not sure if I catch your point of this comment.
> > > For preferred role(if support try.sink or try.src) the only allowed
> > > power roles are "sink"
> > > "source"
> > > But for power role, the allowed type are "sink"
> > > "source"
> > > "dual"
> >
> > Uhm, typing too fast again, I am. A better name would be just
> typec_find_role().
> > What I mean is that the function could be used for any situation when
> > someone wants to map a string to a TYPEC_{SOURCE,SINK} constant so it
> > is unnecessary to limit its usage to just preferred role.
> 
> That sounds reasonable to me.

Agreed

thanks
Li Jun
> 
> 
> Thanks,
> 
> --
> heikki

RE: [PATCH v5 05/14] usb: typec: add API to get typec basic port power and data config

[Jun Li]

Hi Mats

> I reread this patch and tried to see it more in the context of the other patches
> and the existing code. The naming of the existing string tables doesn't help in
> getting this right, however I have a proposal:
> 
> typec_find_port_power_role() to get to TYPEC_PORT_SRC/SNK/DRP
> typec_find_port_data_role() to get to TYPEC_PORT_DFP/UFP/DRD
> typec_find_power_role() to get to TYPEC_SINK/SOURCE
> 
> and sometime, if the use should arise
> 
> typec_find_data_role() to get to TYPEC_DEVICE/HOST
> 
> I think it is fairly comprehensible, *_port_* concerns a capability and without
> *_port_* it is an actual state. Plus it matches the names of the constants.
> 

Reasonable to me, I was using "type" to concern a capability but it was not
preferred, if Heikki agree this proposal, I will change the API names like this
and include 4 APIs above.
 
Thanks
Li Jun

RE: [PATCH v5 05/14] usb: typec: add API to get typec basic port power and data config

[Jun Li]

Hi Heikki,

> > I reread this patch and tried to see it more in the context of the
> > other patches and the existing code. The naming of the existing string
> > tables doesn't help in getting this right, however I have a proposal:
> >
> > typec_find_port_power_role() to get to TYPEC_PORT_SRC/SNK/DRP
> > typec_find_port_data_role() to get to TYPEC_PORT_DFP/UFP/DRD
> > typec_find_power_role() to get to TYPEC_SINK/SOURCE
> >
> > and sometime, if the use should arise
> >
> > typec_find_data_role() to get to TYPEC_DEVICE/HOST
> >
> > I think it is fairly comprehensible, *_port_* concerns a capability
> > and without *_port_* it is an actual state. Plus it matches the names
> > of the constants.
> 
> Well, there are now four things to consider:
> 
> 1) the roles (power and data) the port is capable of supporting
> 2) Try.SRC and Try.SNK, i.e. the preferred role
> 3) the current roles
> 4) the role(s) the user want's to limit the use of a port with DRP ports
> 
> The last one I don't know if it's relevant with these functions. It's not
> information that we would get for example from firmware.
> 
> I also don't think we need to use these functions with the current roles the port
> is in.
> 
> If the preferred role is "sink" or "source", so just like the power role, we don't
> need separate function for it here.
> 
> So isn't two functions all we need here: one for the power and one for data
> role?

Take power as an example, can we use one function to only look up
typec_port_types[]? as capability(typec_port_type) and state(typec_role)
are different enum, and the allowed strings are different.

static const char * const typec_roles[] = {
	[TYPEC_SINK]    = "sink",
	[TYPEC_SOURCE]  = "source",
};

static const char * const typec_port_types[] = {
	[TYPEC_PORT_SRC] = "source",
	[TYPEC_PORT_SNK] = "sink",
	[TYPEC_PORT_DRP] = "dual",
};

Thanks
Li Jun
> 
> 
> Thanks,
> 
> --
> heikki

Re: [PATCH v5 05/14] usb: typec: add API to get typec basic port power and data config

[Heikki Krogerus]

On Thu, May 17, 2018 at 02:05:41PM +0000, Jun Li wrote:
> Hi Heikki,
> 
> > > I reread this patch and tried to see it more in the context of the
> > > other patches and the existing code. The naming of the existing string
> > > tables doesn't help in getting this right, however I have a proposal:
> > >
> > > typec_find_port_power_role() to get to TYPEC_PORT_SRC/SNK/DRP
> > > typec_find_port_data_role() to get to TYPEC_PORT_DFP/UFP/DRD
> > > typec_find_power_role() to get to TYPEC_SINK/SOURCE
> > >
> > > and sometime, if the use should arise
> > >
> > > typec_find_data_role() to get to TYPEC_DEVICE/HOST
> > >
> > > I think it is fairly comprehensible, *_port_* concerns a capability
> > > and without *_port_* it is an actual state. Plus it matches the names
> > > of the constants.
> > 
> > Well, there are now four things to consider:
> > 
> > 1) the roles (power and data) the port is capable of supporting
> > 2) Try.SRC and Try.SNK, i.e. the preferred role
> > 3) the current roles
> > 4) the role(s) the user want's to limit the use of a port with DRP ports
> > 
> > The last one I don't know if it's relevant with these functions. It's not
> > information that we would get for example from firmware.
> > 
> > I also don't think we need to use these functions with the current roles the port
> > is in.
> > 
> > If the preferred role is "sink" or "source", so just like the power role, we don't
> > need separate function for it here.
> > 
> > So isn't two functions all we need here: one for the power and one for data
> > role?
> 
> Take power as an example, can we use one function to only look up
> typec_port_types[]? as capability(typec_port_type) and state(typec_role)
> are different enum, and the allowed strings are different.
> 
> static const char * const typec_roles[] = {
> 	[TYPEC_SINK]    = "sink",
> 	[TYPEC_SOURCE]  = "source",
> };
> 
> static const char * const typec_port_types[] = {
> 	[TYPEC_PORT_SRC] = "source",
> 	[TYPEC_PORT_SNK] = "sink",
> 	[TYPEC_PORT_DRP] = "dual",
> };

Where out side the class code we need to convert the current role, the
"state", with these functions?


Thanks,

-- 
heikki

RE: [PATCH v5 05/14] usb: typec: add API to get typec basic port power and data config

[Jun Li]

Hi
> -----Original Message-----
> From: Heikki Krogerus [mailto:heikki.krogerus@linux.intel.com]
> Sent: 2018年5月17日 22:24
> To: Jun Li <jun.li@nxp.com>
> Cc: Mats Karrman <mats.dev.list@gmail.com>; robh+dt@kernel.org;
> gregkh@linuxfoundation.org; linux@roeck-us.net; a.hajda@samsung.com;
> cw00.choi@samsung.com; shufan_lee@richtek.com; Peter Chen
> <peter.chen@nxp.com>; gsomlo@gmail.com; devicetree@vger.kernel.org;
> linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v5 05/14] usb: typec: add API to get typec basic port power
> and data config
> 
> On Thu, May 17, 2018 at 02:05:41PM +0000, Jun Li wrote:
> > Hi Heikki,
> >
> > > > I reread this patch and tried to see it more in the context of the
> > > > other patches and the existing code. The naming of the existing
> > > > string tables doesn't help in getting this right, however I have a proposal:
> > > >
> > > > typec_find_port_power_role() to get to TYPEC_PORT_SRC/SNK/DRP
> > > > typec_find_port_data_role() to get to TYPEC_PORT_DFP/UFP/DRD
> > > > typec_find_power_role() to get to TYPEC_SINK/SOURCE
> > > >
> > > > and sometime, if the use should arise
> > > >
> > > > typec_find_data_role() to get to TYPEC_DEVICE/HOST
> > > >
> > > > I think it is fairly comprehensible, *_port_* concerns a
> > > > capability and without *_port_* it is an actual state. Plus it
> > > > matches the names of the constants.
> > >
> > > Well, there are now four things to consider:
> > >
> > > 1) the roles (power and data) the port is capable of supporting
> > > 2) Try.SRC and Try.SNK, i.e. the preferred role
> > > 3) the current roles
> > > 4) the role(s) the user want's to limit the use of a port with DRP
> > > ports
> > >
> > > The last one I don't know if it's relevant with these functions.
> > > It's not information that we would get for example from firmware.
> > >
> > > I also don't think we need to use these functions with the current
> > > roles the port is in.
> > >
> > > If the preferred role is "sink" or "source", so just like the power
> > > role, we don't need separate function for it here.
> > >
> > > So isn't two functions all we need here: one for the power and one
> > > for data role?
> >
> > Take power as an example, can we use one function to only look up
> > typec_port_types[]? as capability(typec_port_type) and
> > state(typec_role) are different enum, and the allowed strings are different.
> >
> > static const char * const typec_roles[] = {
> > 	[TYPEC_SINK]    = "sink",
> > 	[TYPEC_SOURCE]  = "source",
> > };
> >
> > static const char * const typec_port_types[] = {
> > 	[TYPEC_PORT_SRC] = "source",
> > 	[TYPEC_PORT_SNK] = "sink",
> > 	[TYPEC_PORT_DRP] = "dual",
> > };
> 
> Where out side the class code we need to convert the current role, the "state",
> with these functions?

Currently, the only call site is getting the preferred power role from firmware.

Thanks
Li Jun
> 
> 
> Thanks,
> 
> --
> heikki

Re: [PATCH v5 05/14] usb: typec: add API to get typec basic port power and data config

[Heikki Krogerus]

Hi Jun,

Sorry for the delay.

On Thu, May 17, 2018 at 02:41:31PM +0000, Jun Li wrote:
> Hi
> > -----Original Message-----
> > From: Heikki Krogerus [mailto:heikki.krogerus@linux.intel.com]
> > Sent: 2018??5??17?? 22:24
> > To: Jun Li <jun.li@nxp.com>
> > Cc: Mats Karrman <mats.dev.list@gmail.com>; robh+dt@kernel.org;
> > gregkh@linuxfoundation.org; linux@roeck-us.net; a.hajda@samsung.com;
> > cw00.choi@samsung.com; shufan_lee@richtek.com; Peter Chen
> > <peter.chen@nxp.com>; gsomlo@gmail.com; devicetree@vger.kernel.org;
> > linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> > Subject: Re: [PATCH v5 05/14] usb: typec: add API to get typec basic port power
> > and data config
> > 
> > On Thu, May 17, 2018 at 02:05:41PM +0000, Jun Li wrote:
> > > Hi Heikki,
> > >
> > > > > I reread this patch and tried to see it more in the context of the
> > > > > other patches and the existing code. The naming of the existing
> > > > > string tables doesn't help in getting this right, however I have a proposal:
> > > > >
> > > > > typec_find_port_power_role() to get to TYPEC_PORT_SRC/SNK/DRP
> > > > > typec_find_port_data_role() to get to TYPEC_PORT_DFP/UFP/DRD
> > > > > typec_find_power_role() to get to TYPEC_SINK/SOURCE
> > > > >
> > > > > and sometime, if the use should arise
> > > > >
> > > > > typec_find_data_role() to get to TYPEC_DEVICE/HOST
> > > > >
> > > > > I think it is fairly comprehensible, *_port_* concerns a
> > > > > capability and without *_port_* it is an actual state. Plus it
> > > > > matches the names of the constants.
> > > >
> > > > Well, there are now four things to consider:
> > > >
> > > > 1) the roles (power and data) the port is capable of supporting
> > > > 2) Try.SRC and Try.SNK, i.e. the preferred role
> > > > 3) the current roles
> > > > 4) the role(s) the user want's to limit the use of a port with DRP
> > > > ports
> > > >
> > > > The last one I don't know if it's relevant with these functions.
> > > > It's not information that we would get for example from firmware.
> > > >
> > > > I also don't think we need to use these functions with the current
> > > > roles the port is in.
> > > >
> > > > If the preferred role is "sink" or "source", so just like the power
> > > > role, we don't need separate function for it here.
> > > >
> > > > So isn't two functions all we need here: one for the power and one
> > > > for data role?
> > >
> > > Take power as an example, can we use one function to only look up
> > > typec_port_types[]? as capability(typec_port_type) and
> > > state(typec_role) are different enum, and the allowed strings are different.
> > >
> > > static const char * const typec_roles[] = {
> > > 	[TYPEC_SINK]    = "sink",
> > > 	[TYPEC_SOURCE]  = "source",
> > > };
> > >
> > > static const char * const typec_port_types[] = {
> > > 	[TYPEC_PORT_SRC] = "source",
> > > 	[TYPEC_PORT_SNK] = "sink",
> > > 	[TYPEC_PORT_DRP] = "dual",
> > > };
> > 
> > Where out side the class code we need to convert the current role, the "state",
> > with these functions?
> 
> Currently, the only call site is getting the preferred power role from firmware.

My point was that if we used enum typec_port_type with the prefered
role, we could use the helper for power role with prefered role. But
never mind.


Thanks,

-- 
heikki

RE: [PATCH v5 05/14] usb: typec: add API to get typec basic port power and data config

[Jun Li]

Hi Heikki,

> -----Original Message-----
> From: Heikki Krogerus [mailto:heikki.krogerus@linux.intel.com]
> Sent: 2018年5月21日 21:13
> To: Jun Li <jun.li@nxp.com>
> Cc: Mats Karrman <mats.dev.list@gmail.com>; robh+dt@kernel.org;
> gregkh@linuxfoundation.org; linux@roeck-us.net; a.hajda@samsung.com;
> cw00.choi@samsung.com; shufan_lee@richtek.com; Peter Chen
> <peter.chen@nxp.com>; gsomlo@gmail.com; devicetree@vger.kernel.org;
> linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v5 05/14] usb: typec: add API to get typec basic port power
> and data config
> 
> Hi Jun,
> 
> Sorry for the delay.
> 
> On Thu, May 17, 2018 at 02:41:31PM +0000, Jun Li wrote:
> > Hi
> > > -----Original Message-----
> > > From: Heikki Krogerus [mailto:heikki.krogerus@linux.intel.com]
> > > Sent: 2018??5??17?? 22:24
> > > To: Jun Li <jun.li@nxp.com>
> > > Cc: Mats Karrman <mats.dev.list@gmail.com>; robh+dt@kernel.org;
> > > gregkh@linuxfoundation.org; linux@roeck-us.net; a.hajda@samsung.com;
> > > cw00.choi@samsung.com; shufan_lee@richtek.com; Peter Chen
> > > <peter.chen@nxp.com>; gsomlo@gmail.com; devicetree@vger.kernel.org;
> > > linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> > > Subject: Re: [PATCH v5 05/14] usb: typec: add API to get typec basic
> > > port power and data config
> > >
> > > On Thu, May 17, 2018 at 02:05:41PM +0000, Jun Li wrote:
> > > > Hi Heikki,
> > > >
> > > > > > I reread this patch and tried to see it more in the context of
> > > > > > the other patches and the existing code. The naming of the
> > > > > > existing string tables doesn't help in getting this right, however I have
> a proposal:
> > > > > >
> > > > > > typec_find_port_power_role() to get to TYPEC_PORT_SRC/SNK/DRP
> > > > > > typec_find_port_data_role() to get to TYPEC_PORT_DFP/UFP/DRD
> > > > > > typec_find_power_role() to get to TYPEC_SINK/SOURCE
> > > > > >
> > > > > > and sometime, if the use should arise
> > > > > >
> > > > > > typec_find_data_role() to get to TYPEC_DEVICE/HOST
> > > > > >
> > > > > > I think it is fairly comprehensible, *_port_* concerns a
> > > > > > capability and without *_port_* it is an actual state. Plus it
> > > > > > matches the names of the constants.
> > > > >
> > > > > Well, there are now four things to consider:
> > > > >
> > > > > 1) the roles (power and data) the port is capable of supporting
> > > > > 2) Try.SRC and Try.SNK, i.e. the preferred role
> > > > > 3) the current roles
> > > > > 4) the role(s) the user want's to limit the use of a port with
> > > > > DRP ports
> > > > >
> > > > > The last one I don't know if it's relevant with these functions.
> > > > > It's not information that we would get for example from firmware.
> > > > >
> > > > > I also don't think we need to use these functions with the
> > > > > current roles the port is in.
> > > > >
> > > > > If the preferred role is "sink" or "source", so just like the
> > > > > power role, we don't need separate function for it here.
> > > > >
> > > > > So isn't two functions all we need here: one for the power and
> > > > > one for data role?
> > > >
> > > > Take power as an example, can we use one function to only look up
> > > > typec_port_types[]? as capability(typec_port_type) and
> > > > state(typec_role) are different enum, and the allowed strings are
> different.
> > > >
> > > > static const char * const typec_roles[] = {
> > > > 	[TYPEC_SINK]    = "sink",
> > > > 	[TYPEC_SOURCE]  = "source",
> > > > };
> > > >
> > > > static const char * const typec_port_types[] = {
> > > > 	[TYPEC_PORT_SRC] = "source",
> > > > 	[TYPEC_PORT_SNK] = "sink",
> > > > 	[TYPEC_PORT_DRP] = "dual",
> > > > };
> > >
> > > Where out side the class code we need to convert the current role,
> > > the "state", with these functions?
> >
> > Currently, the only call site is getting the preferred power role from firmware.
> 
> My point was that if we used enum typec_port_type with the prefered role, we
> could use the helper for power role with prefered role. But never mind.

Got your point. So let's follow Mats's suggestion on this, I will update a new version.

Thanks
Li Jun
> 
> 
> Thanks,
> 
> --
> heikki