[PATCH v2 0/5] usb: typec: remove max_snk_mv/ma/mw

[PATCH v2 0/5] usb: typec: remove max_snk_mv/ma/mw

[Li Jun]

This patch set is to remove max_snk_mv/ma/mw configs, as we should
define the sink capability by sink PDOs, the first patch update
the source PDO match policy by compare the voltage range between
source and sink PDOs no matter what type they are, the following
patchs remove those 3 variables from 2 existing users by adding
a variable PDO, then finial patch remove the max_snk_* from tcpm.

Changes for v2:
- rebase the 1st patch to be based on commit 6f566af34628
  ("Revert "typec: tcpm: Only request matching pdos"").
- Convert the device properties passing max_snk_* to be a
  variable sink pdo for fusb302.
 
Li Jun (5):
  usb: typec: tcpm: pdo matching optimization
  usb: typec: fusb302:  remove max_snk_* for sink config
  dt-bindings: usb: fusb302: remove max-sink-* properties
  usb: typec: wcove: remove max_snk_* for sink config
  usb: typec: tcpm: remove max_snk_mv/ma/mw

 .../devicetree/bindings/usb/fcs,fusb302.txt        |   6 -
 drivers/usb/typec/fusb302/fusb302.c                |  51 +++++---
 drivers/usb/typec/tcpm.c                           | 139 +++++++++++++--------
 drivers/usb/typec/typec_wcove.c                    |   4 +-
 include/linux/usb/tcpm.h                           |   9 --
 5 files changed, 128 insertions(+), 81 deletions(-)

-- 
2.7.4

[PATCH v2 1/5] usb: typec: tcpm: pdo matching optimization

[Li Jun]

This patch is a combination of commit 57e6f0d7b804
("typec: tcpm: Only request matching pdos") and source
pdo selection optimization based on it, instead of only
compare between the same pdo type of sink and source,
we should check source pdo voltage range is within the
voltage range of one sink pdo.

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

diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
index 677d121..50a1979 100644
--- a/drivers/usb/typec/tcpm.c
+++ b/drivers/usb/typec/tcpm.c
@@ -254,6 +254,9 @@ struct tcpm_port {
 	unsigned int nr_src_pdo;
 	u32 snk_pdo[PDO_MAX_OBJECTS];
 	unsigned int nr_snk_pdo;
+	unsigned int nr_fixed; /* number of fixed sink PDOs */
+	unsigned int nr_var; /* number of variable sink PDOs */
+	unsigned int nr_batt; /* number of battery sink PDOs */
 	u32 snk_vdo[VDO_MAX_OBJECTS];
 	unsigned int nr_snk_vdo;
 
@@ -1772,40 +1775,65 @@ static int tcpm_pd_check_request(struct tcpm_port *port)
 	return 0;
 }
 
-static int tcpm_pd_select_pdo(struct tcpm_port *port)
+#define min_power(x, y) min(pdo_max_power(x), pdo_max_power(y))
+#define min_current(x, y) min(pdo_max_current(x), pdo_max_current(y))
+
+static int tcpm_pd_select_pdo(struct tcpm_port *port, int *sink_pdo,
+			      int *src_pdo)
 {
-	unsigned int i, max_mw = 0, max_mv = 0;
+	unsigned int i, j, max_src_mv = 0, min_src_mv = 0, max_mw = 0,
+		     max_mv = 0, src_mw = 0, src_ma = 0, max_snk_mv = 0,
+		     min_snk_mv = 0;
 	int ret = -EINVAL;
 
 	/*
-	 * Select the source PDO providing the most power while staying within
-	 * the board's voltage limits. Prefer PDO providing exp
+	 * Select the source PDO providing the most power which has a
+	 * matchig sink cap.
 	 */
 	for (i = 0; i < port->nr_source_caps; i++) {
 		u32 pdo = port->source_caps[i];
 		enum pd_pdo_type type = pdo_type(pdo);
-		unsigned int mv, ma, mw;
 
-		if (type == PDO_TYPE_FIXED)
-			mv = pdo_fixed_voltage(pdo);
-		else
-			mv = pdo_min_voltage(pdo);
+		if (type == PDO_TYPE_FIXED) {
+			max_src_mv = pdo_fixed_voltage(pdo);
+			min_src_mv = max_src_mv;
+		} else {
+			max_src_mv = pdo_max_voltage(pdo);
+			min_src_mv = pdo_min_voltage(pdo);
+		}
 
 		if (type == PDO_TYPE_BATT) {
-			mw = pdo_max_power(pdo);
+			src_mw = pdo_max_power(pdo);
 		} else {
-			ma = min(pdo_max_current(pdo),
-				 port->max_snk_ma);
-			mw = ma * mv / 1000;
+			src_ma = pdo_max_current(pdo);
+			src_mw = src_ma * min_src_mv / 1000;
 		}
 
-		/* Perfer higher voltages if available */
-		if ((mw > max_mw || (mw == max_mw && mv > max_mv)) &&
-		    mv <= port->max_snk_mv) {
-			ret = i;
-			max_mw = mw;
-			max_mv = mv;
+		for (j = 0; j < port->nr_snk_pdo; j++) {
+			pdo = port->snk_pdo[j];
+
+			if (pdo_type(pdo) == PDO_TYPE_FIXED) {
+				min_snk_mv = pdo_fixed_voltage(pdo);
+				max_snk_mv = pdo_fixed_voltage(pdo);
+			} else {
+				min_snk_mv = pdo_min_voltage(pdo);
+				max_snk_mv = pdo_max_voltage(pdo);
+			}
+
+			if (max_src_mv <= max_snk_mv &&
+				min_src_mv >= min_snk_mv) {
+				/* Prefer higher voltages if available */
+				if ((src_mw == max_mw && min_src_mv > max_mv) ||
+							src_mw > max_mw) {
+					*src_pdo = i;
+					*sink_pdo = j;
+					max_mw = src_mw;
+					max_mv = min_src_mv;
+				}
+				break;
+			}
 		}
+
 	}
 
 	return ret;
@@ -1816,13 +1844,14 @@ static int tcpm_pd_build_request(struct tcpm_port *port, u32 *rdo)
 	unsigned int mv, ma, mw, flags;
 	unsigned int max_ma, max_mw;
 	enum pd_pdo_type type;
-	int index;
-	u32 pdo;
+	int src_pdo_index, snk_pdo_index;
+	u32 pdo, matching_snk_pdo;
 
-	index = tcpm_pd_select_pdo(port);
-	if (index < 0)
+	if (tcpm_pd_select_pdo(port, &snk_pdo_index, &src_pdo_index) < 0)
 		return -EINVAL;
-	pdo = port->source_caps[index];
+
+	pdo = port->source_caps[src_pdo_index];
+	matching_snk_pdo = port->snk_pdo[snk_pdo_index];
 	type = pdo_type(pdo);
 
 	if (type == PDO_TYPE_FIXED)
@@ -1830,26 +1859,28 @@ static int tcpm_pd_build_request(struct tcpm_port *port, u32 *rdo)
 	else
 		mv = pdo_min_voltage(pdo);
 
-	/* Select maximum available current within the board's power limit */
+	/* Select maximum available current within the sink pdo's limit */
 	if (type == PDO_TYPE_BATT) {
-		mw = pdo_max_power(pdo);
-		ma = 1000 * min(mw, port->max_snk_mw) / mv;
+		mw = min_power(pdo, matching_snk_pdo);
+		ma = 1000 * mw / mv;
 	} else {
-		ma = min(pdo_max_current(pdo),
-			 1000 * port->max_snk_mw / mv);
+		ma = min_current(pdo, matching_snk_pdo);
+		mw = ma * mv / 1000;
 	}
-	ma = min(ma, port->max_snk_ma);
 
 	flags = RDO_USB_COMM | RDO_NO_SUSPEND;
 
 	/* Set mismatch bit if offered power is less than operating power */
-	mw = ma * mv / 1000;
 	max_ma = ma;
 	max_mw = mw;
 	if (mw < port->operating_snk_mw) {
 		flags |= RDO_CAP_MISMATCH;
-		max_mw = port->operating_snk_mw;
-		max_ma = max_mw * 1000 / mv;
+		if (type == PDO_TYPE_BATT &&
+		    (pdo_max_power(matching_snk_pdo) > pdo_max_power(pdo)))
+			max_mw = pdo_max_power(matching_snk_pdo);
+		else if (pdo_max_current(matching_snk_pdo) >
+			 pdo_max_current(pdo))
+			max_ma = pdo_max_current(matching_snk_pdo);
 	}
 
 	tcpm_log(port, "cc=%d cc1=%d cc2=%d vbus=%d vconn=%s polarity=%d",
@@ -1858,16 +1889,16 @@ static int tcpm_pd_build_request(struct tcpm_port *port, u32 *rdo)
 		 port->polarity);
 
 	if (type == PDO_TYPE_BATT) {
-		*rdo = RDO_BATT(index + 1, mw, max_mw, flags);
+		*rdo = RDO_BATT(src_pdo_index + 1, mw, max_mw, flags);
 
 		tcpm_log(port, "Requesting PDO %d: %u mV, %u mW%s",
-			 index, mv, mw,
+			 src_pdo_index, mv, mw,
 			 flags & RDO_CAP_MISMATCH ? " [mismatch]" : "");
 	} else {
-		*rdo = RDO_FIXED(index + 1, ma, max_ma, flags);
+		*rdo = RDO_FIXED(src_pdo_index + 1, ma, max_ma, flags);
 
 		tcpm_log(port, "Requesting PDO %d: %u mV, %u mA%s",
-			 index, mv, ma,
+			 src_pdo_index, mv, ma,
 			 flags & RDO_CAP_MISMATCH ? " [mismatch]" : "");
 	}
 
@@ -3599,6 +3630,19 @@ int tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo,
 }
 EXPORT_SYMBOL_GPL(tcpm_update_sink_capabilities);
 
+static int nr_type_pdos(const u32 *pdo, unsigned int nr_pdo,
+			enum pd_pdo_type type)
+{
+	int count = 0;
+	int i;
+
+	for (i = 0; i < nr_pdo; i++) {
+		if (pdo_type(pdo[i]) == type)
+			count++;
+	}
+	return count;
+}
+
 struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
 {
 	struct tcpm_port *port;
@@ -3644,6 +3688,15 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
 					  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_fixed =  nr_type_pdos(port->snk_pdo,
+				       port->nr_snk_pdo,
+				       PDO_TYPE_FIXED);
+	port->nr_var = nr_type_pdos(port->snk_pdo,
+				    port->nr_snk_pdo,
+				    PDO_TYPE_VAR);
+	port->nr_batt = nr_type_pdos(port->snk_pdo,
+				     port->nr_snk_pdo,
+				     PDO_TYPE_BATT);
 	port->nr_snk_vdo = tcpm_copy_vdos(port->snk_vdo, tcpc->config->snk_vdo,
 					  tcpc->config->nr_snk_vdo);
 
-- 
2.7.4

[PATCH v2 2/5] usb: typec: fusb302: remove max_snk_* for sink config

[Li Jun]

Since max_snk_* is to be deprecated, so remove max_snk_* by adding a
variable PDO for sink config.

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

diff --git a/drivers/usb/typec/fusb302/fusb302.c b/drivers/usb/typec/fusb302/fusb302.c
index 7036171..db4d9cd 100644
--- a/drivers/usb/typec/fusb302/fusb302.c
+++ b/drivers/usb/typec/fusb302/fusb302.c
@@ -120,6 +120,7 @@ struct fusb302_chip {
 	enum typec_cc_polarity cc_polarity;
 	enum typec_cc_status cc1;
 	enum typec_cc_status cc2;
+	u32 snk_pdo[PDO_MAX_OBJECTS];
 
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *dentry;
@@ -1212,11 +1213,6 @@ static const u32 snk_pdo[] = {
 static const struct tcpc_config fusb302_tcpc_config = {
 	.src_pdo = src_pdo,
 	.nr_src_pdo = ARRAY_SIZE(src_pdo),
-	.snk_pdo = snk_pdo,
-	.nr_snk_pdo = ARRAY_SIZE(snk_pdo),
-	.max_snk_mv = 5000,
-	.max_snk_ma = 3000,
-	.max_snk_mw = 15000,
 	.operating_snk_mw = 2500,
 	.type = TYPEC_PORT_DRP,
 	.data = TYPEC_PORT_DRD,
@@ -1756,6 +1752,38 @@ static int init_gpio(struct fusb302_chip *chip)
 	return 0;
 }
 
+static int fusb302_composite_snk_pdo_array(struct fusb302_chip *chip)
+{
+	struct device *dev = chip->dev;
+	u32 mv, ma, mw, min_mv;
+	unsigned int i;
+
+	/* Copy the static snk pdo */
+	for (i = 0; i < ARRAY_SIZE(snk_pdo); i++)
+		chip->snk_pdo[i] = snk_pdo[i];
+
+	if (device_property_read_u32(dev, "fcs,max-sink-microvolt", &mv) ||
+	    device_property_read_u32(dev, "fcs,max-sink-microamp", &ma) ||
+	    device_property_read_u32(dev, "fcs,max-sink-microwatt", &mw))
+		return i;
+
+	mv = mv / 1000;
+	ma = ma / 1000;
+	mw = mw / 1000;
+
+	min_mv = 1000 * chip->tcpc_config.operating_snk_mw / ma;
+	if (pdo_type(snk_pdo[i-1] == PDO_TYPE_FIXED))
+		min_mv = max(min_mv, pdo_fixed_voltage(snk_pdo[i-1]));
+	else
+		min_mv = max(min_mv, pdo_max_voltage(snk_pdo[i-1]));
+	ma = min(ma, 1000 * mw / min_mv);
+
+	/* Insert the new pdo to the end */
+	chip->snk_pdo[i] = PDO_VAR(min_mv, mv, ma);
+
+	return i+1;
+}
+
 static int fusb302_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -1784,18 +1812,13 @@ static int fusb302_probe(struct i2c_client *client,
 	chip->tcpc_dev.config = &chip->tcpc_config;
 	mutex_init(&chip->lock);
 
-	if (!device_property_read_u32(dev, "fcs,max-sink-microvolt", &v))
-		chip->tcpc_config.max_snk_mv = v / 1000;
-
-	if (!device_property_read_u32(dev, "fcs,max-sink-microamp", &v))
-		chip->tcpc_config.max_snk_ma = v / 1000;
-
-	if (!device_property_read_u32(dev, "fcs,max-sink-microwatt", &v))
-		chip->tcpc_config.max_snk_mw = v / 1000;
-
 	if (!device_property_read_u32(dev, "fcs,operating-sink-microwatt", &v))
 		chip->tcpc_config.operating_snk_mw = v / 1000;
 
+	/* Composite sink PDO */
+	chip->tcpc_config.nr_snk_pdo = fusb302_composite_snk_pdo_array(chip);
+	chip->tcpc_config.snk_pdo = chip->snk_pdo;
+
 	/*
 	 * Devicetree platforms should get extcon via phandle (not yet
 	 * supported). On ACPI platforms, we get the name from a device prop.
-- 
2.7.4

[PATCH v2 3/5] dt-bindings: usb: fusb302: remove max-sink-* properties

[Li Jun]

Remove max-sink-* properties since they are deprecated.

Signed-off-by: Li Jun <jun.li@nxp.com>
---
 Documentation/devicetree/bindings/usb/fcs,fusb302.txt | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/fcs,fusb302.txt b/Documentation/devicetree/bindings/usb/fcs,fusb302.txt
index 472facf..6087dc7 100644
--- a/Documentation/devicetree/bindings/usb/fcs,fusb302.txt
+++ b/Documentation/devicetree/bindings/usb/fcs,fusb302.txt
@@ -6,12 +6,6 @@ Required properties :
 - interrupts             : Interrupt specifier
 
 Optional properties :
-- fcs,max-sink-microvolt : Maximum voltage to negotiate when configured as sink
-- fcs,max-sink-microamp  : Maximum current to negotiate when configured as sink
-- fcs,max-sink-microwatt : Maximum power to negotiate when configured as sink
-			   If this is less then max-sink-microvolt *
-			   max-sink-microamp then the configured current will
-			   be clamped.
 - fcs,operating-sink-microwatt :
 			   Minimum amount of power accepted from a sink
 			   when negotiating
-- 
2.7.4

[PATCH v2 4/5] usb: typec: wcove: remove max_snk_* for sink config

[Li Jun]

Since max_snk_* is to be deprecated, so remove max_snk_* by adding a
variable PDO for sink config.

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

diff --git a/drivers/usb/typec/typec_wcove.c b/drivers/usb/typec/typec_wcove.c
index 19cca7f..39cff11 100644
--- a/drivers/usb/typec/typec_wcove.c
+++ b/drivers/usb/typec/typec_wcove.c
@@ -558,6 +558,7 @@ static const u32 src_pdo[] = {
 static const u32 snk_pdo[] = {
 	PDO_FIXED(5000, 500, PDO_FIXED_DUAL_ROLE | PDO_FIXED_DATA_SWAP |
 		  PDO_FIXED_USB_COMM),
+	PDO_VAR(5000, 12000, 3000),
 };
 
 static struct tcpc_config wcove_typec_config = {
@@ -566,9 +567,6 @@ static struct tcpc_config wcove_typec_config = {
 	.snk_pdo = snk_pdo,
 	.nr_snk_pdo = ARRAY_SIZE(snk_pdo),
 
-	.max_snk_mv = 12000,
-	.max_snk_ma = 3000,
-	.max_snk_mw = 36000,
 	.operating_snk_mw = 15000,
 
 	.type = TYPEC_PORT_DRP,
-- 
2.7.4

[PATCH v2 5/5] usb: typec: tcpm: remove max_snk_mv/ma/mw

[Li Jun]

Since there is no user of max_snk_*, so we can remove them from tcpm.

Signed-off-by: Li Jun <jun.li@nxp.com>
---
 drivers/usb/typec/tcpm.c | 12 ------------
 include/linux/usb/tcpm.h |  9 ---------
 2 files changed, 21 deletions(-)

diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
index 50a1979..72e7a65 100644
--- a/drivers/usb/typec/tcpm.c
+++ b/drivers/usb/typec/tcpm.c
@@ -260,9 +260,6 @@ struct tcpm_port {
 	u32 snk_vdo[VDO_MAX_OBJECTS];
 	unsigned int nr_snk_vdo;
 
-	unsigned int max_snk_mv;
-	unsigned int max_snk_ma;
-	unsigned int max_snk_mw;
 	unsigned int operating_snk_mw;
 
 	/* Requested current / voltage */
@@ -3600,9 +3597,6 @@ EXPORT_SYMBOL_GPL(tcpm_update_source_capabilities);
 
 int tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo,
 				  unsigned int nr_pdo,
-				  unsigned int max_snk_mv,
-				  unsigned int max_snk_ma,
-				  unsigned int max_snk_mw,
 				  unsigned int operating_snk_mw)
 {
 	if (tcpm_validate_caps(port, pdo, nr_pdo))
@@ -3610,9 +3604,6 @@ int tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo,
 
 	mutex_lock(&port->lock);
 	port->nr_snk_pdo = tcpm_copy_pdos(port->snk_pdo, pdo, nr_pdo);
-	port->max_snk_mv = max_snk_mv;
-	port->max_snk_ma = max_snk_ma;
-	port->max_snk_mw = max_snk_mw;
 	port->operating_snk_mw = operating_snk_mw;
 
 	switch (port->state) {
@@ -3700,9 +3691,6 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
 	port->nr_snk_vdo = tcpm_copy_vdos(port->snk_vdo, tcpc->config->snk_vdo,
 					  tcpc->config->nr_snk_vdo);
 
-	port->max_snk_mv = tcpc->config->max_snk_mv;
-	port->max_snk_ma = tcpc->config->max_snk_ma;
-	port->max_snk_mw = tcpc->config->max_snk_mw;
 	port->operating_snk_mw = tcpc->config->operating_snk_mw;
 	if (!tcpc->config->try_role_hw)
 		port->try_role = tcpc->config->default_role;
diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
index f0d839d..f5bda9a 100644
--- a/include/linux/usb/tcpm.h
+++ b/include/linux/usb/tcpm.h
@@ -62,9 +62,6 @@ enum tcpm_transmit_type {
  * @snk_pdo:	PDO parameters sent to partner as response to
  *		PD_CTRL_GET_SINK_CAP message
  * @nr_snk_pdo:	Number of entries in @snk_pdo
- * @max_snk_mv:	Maximum acceptable sink voltage in mV
- * @max_snk_ma:	Maximum sink current in mA
- * @max_snk_mw:	Maximum required sink power in mW
  * @operating_snk_mw:
  *		Required operating sink power in mW
  * @type:	Port type (TYPEC_PORT_DFP, TYPEC_PORT_UFP, or
@@ -85,9 +82,6 @@ struct tcpc_config {
 	const u32 *snk_vdo;
 	unsigned int nr_snk_vdo;
 
-	unsigned int max_snk_mv;
-	unsigned int max_snk_ma;
-	unsigned int max_snk_mw;
 	unsigned int operating_snk_mw;
 
 	enum typec_port_type type;
@@ -174,9 +168,6 @@ int tcpm_update_source_capabilities(struct tcpm_port *port, const u32 *pdo,
 				    unsigned int nr_pdo);
 int tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo,
 				  unsigned int nr_pdo,
-				  unsigned int max_snk_mv,
-				  unsigned int max_snk_ma,
-				  unsigned int max_snk_mw,
 				  unsigned int operating_snk_mw);
 
 void tcpm_vbus_change(struct tcpm_port *port);
-- 
2.7.4

Re: [PATCH v2 3/5] dt-bindings: usb: fusb302: remove max-sink-* properties

[Rob Herring]

On Fri, Mar 23, 2018 at 10:58:45PM +0800, Li Jun wrote:
> Remove max-sink-* properties since they are deprecated.
> 
> Signed-off-by: Li Jun <jun.li@nxp.com>
> ---
>  Documentation/devicetree/bindings/usb/fcs,fusb302.txt | 6 ------
>  1 file changed, 6 deletions(-)

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

Re: [PATCH v2 1/5] usb: typec: tcpm: pdo matching optimization

[Hans de Goede]

Hi,

On 23-03-18 15:58, Li Jun wrote:
> This patch is a combination of commit 57e6f0d7b804
> ("typec: tcpm: Only request matching pdos") and source
> pdo selection optimization based on it, instead of only
> compare between the same pdo type of sink and source,
> we should check source pdo voltage range is within the
> voltage range of one sink pdo.
> 
> Signed-off-by: Li Jun <jun.li@nxp.com>
> ---
>   drivers/usb/typec/tcpm.c | 127 +++++++++++++++++++++++++++++++++--------------
>   1 file changed, 90 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> index 677d121..50a1979 100644
> --- a/drivers/usb/typec/tcpm.c
> +++ b/drivers/usb/typec/tcpm.c
> @@ -254,6 +254,9 @@ struct tcpm_port {
>   	unsigned int nr_src_pdo;
>   	u32 snk_pdo[PDO_MAX_OBJECTS];
>   	unsigned int nr_snk_pdo;
> +	unsigned int nr_fixed; /* number of fixed sink PDOs */
> +	unsigned int nr_var; /* number of variable sink PDOs */
> +	unsigned int nr_batt; /* number of battery sink PDOs */
>   	u32 snk_vdo[VDO_MAX_OBJECTS];
>   	unsigned int nr_snk_vdo;
>  

These 3 variables are now no longer needed.

> @@ -1772,40 +1775,65 @@ static int tcpm_pd_check_request(struct tcpm_port *port)
>   	return 0;
>   }
>   
> -static int tcpm_pd_select_pdo(struct tcpm_port *port)
> +#define min_power(x, y) min(pdo_max_power(x), pdo_max_power(y))
> +#define min_current(x, y) min(pdo_max_current(x), pdo_max_current(y))
> +
> +static int tcpm_pd_select_pdo(struct tcpm_port *port, int *sink_pdo,
> +			      int *src_pdo)
>   {
> -	unsigned int i, max_mw = 0, max_mv = 0;
> +	unsigned int i, j, max_src_mv = 0, min_src_mv = 0, max_mw = 0,
> +		     max_mv = 0, src_mw = 0, src_ma = 0, max_snk_mv = 0,
> +		     min_snk_mv = 0;
>   	int ret = -EINVAL;
>   
>   	/*
> -	 * Select the source PDO providing the most power while staying within
> -	 * the board's voltage limits. Prefer PDO providing exp
> +	 * Select the source PDO providing the most power which has a
> +	 * matchig sink cap.
>   	 */
>   	for (i = 0; i < port->nr_source_caps; i++) {
>   		u32 pdo = port->source_caps[i];
>   		enum pd_pdo_type type = pdo_type(pdo);
> -		unsigned int mv, ma, mw;
>   
> -		if (type == PDO_TYPE_FIXED)
> -			mv = pdo_fixed_voltage(pdo);
> -		else
> -			mv = pdo_min_voltage(pdo);
> +		if (type == PDO_TYPE_FIXED) {
> +			max_src_mv = pdo_fixed_voltage(pdo);
> +			min_src_mv = max_src_mv;
> +		} else {
> +			max_src_mv = pdo_max_voltage(pdo);
> +			min_src_mv = pdo_min_voltage(pdo);
> +		}
>   
>   		if (type == PDO_TYPE_BATT) {
> -			mw = pdo_max_power(pdo);
> +			src_mw = pdo_max_power(pdo);
>   		} else {
> -			ma = min(pdo_max_current(pdo),
> -				 port->max_snk_ma);
> -			mw = ma * mv / 1000;
> +			src_ma = pdo_max_current(pdo);
> +			src_mw = src_ma * min_src_mv / 1000;
>   		}
>   
> -		/* Perfer higher voltages if available */
> -		if ((mw > max_mw || (mw == max_mw && mv > max_mv)) &&
> -		    mv <= port->max_snk_mv) {
> -			ret = i;
> -			max_mw = mw;
> -			max_mv = mv;
> +		for (j = 0; j < port->nr_snk_pdo; j++) {
> +			pdo = port->snk_pdo[j];
> +
> +			if (pdo_type(pdo) == PDO_TYPE_FIXED) {
> +				min_snk_mv = pdo_fixed_voltage(pdo);
> +				max_snk_mv = pdo_fixed_voltage(pdo);
> +			} else {
> +				min_snk_mv = pdo_min_voltage(pdo);
> +				max_snk_mv = pdo_max_voltage(pdo);
> +			}
> +
> +			if (max_src_mv <= max_snk_mv &&
> +				min_src_mv >= min_snk_mv) {
> +				/* Prefer higher voltages if available */
> +				if ((src_mw == max_mw && min_src_mv > max_mv) ||
> +							src_mw > max_mw) {
> +					*src_pdo = i;
> +					*sink_pdo = j;
> +					max_mw = src_mw;
> +					max_mv = min_src_mv;
> +				}
> +				break;
> +			}
>   		}
> +
>   	}
>   
>   	return ret;
> @@ -1816,13 +1844,14 @@ static int tcpm_pd_build_request(struct tcpm_port *port, u32 *rdo)
>   	unsigned int mv, ma, mw, flags;
>   	unsigned int max_ma, max_mw;
>   	enum pd_pdo_type type;
> -	int index;
> -	u32 pdo;
> +	int src_pdo_index, snk_pdo_index;
> +	u32 pdo, matching_snk_pdo;
>   
> -	index = tcpm_pd_select_pdo(port);
> -	if (index < 0)
> +	if (tcpm_pd_select_pdo(port, &snk_pdo_index, &src_pdo_index) < 0)
>   		return -EINVAL;
> -	pdo = port->source_caps[index];
> +
> +	pdo = port->source_caps[src_pdo_index];
> +	matching_snk_pdo = port->snk_pdo[snk_pdo_index];
>   	type = pdo_type(pdo);
>   
>   	if (type == PDO_TYPE_FIXED)
> @@ -1830,26 +1859,28 @@ static int tcpm_pd_build_request(struct tcpm_port *port, u32 *rdo)
>   	else
>   		mv = pdo_min_voltage(pdo);
>   
> -	/* Select maximum available current within the board's power limit */
> +	/* Select maximum available current within the sink pdo's limit */
>   	if (type == PDO_TYPE_BATT) {
> -		mw = pdo_max_power(pdo);
> -		ma = 1000 * min(mw, port->max_snk_mw) / mv;
> +		mw = min_power(pdo, matching_snk_pdo);
> +		ma = 1000 * mw / mv;
>   	} else {
> -		ma = min(pdo_max_current(pdo),
> -			 1000 * port->max_snk_mw / mv);
> +		ma = min_current(pdo, matching_snk_pdo);
> +		mw = ma * mv / 1000;
>   	}
> -	ma = min(ma, port->max_snk_ma);
>   
>   	flags = RDO_USB_COMM | RDO_NO_SUSPEND;
>   
>   	/* Set mismatch bit if offered power is less than operating power */
> -	mw = ma * mv / 1000;
>   	max_ma = ma;
>   	max_mw = mw;
>   	if (mw < port->operating_snk_mw) {
>   		flags |= RDO_CAP_MISMATCH;
> -		max_mw = port->operating_snk_mw;
> -		max_ma = max_mw * 1000 / mv;
> +		if (type == PDO_TYPE_BATT &&
> +		    (pdo_max_power(matching_snk_pdo) > pdo_max_power(pdo)))
> +			max_mw = pdo_max_power(matching_snk_pdo);
> +		else if (pdo_max_current(matching_snk_pdo) >
> +			 pdo_max_current(pdo))
> +			max_ma = pdo_max_current(matching_snk_pdo);
>   	}
>   
>   	tcpm_log(port, "cc=%d cc1=%d cc2=%d vbus=%d vconn=%s polarity=%d",
> @@ -1858,16 +1889,16 @@ static int tcpm_pd_build_request(struct tcpm_port *port, u32 *rdo)
>   		 port->polarity);
>   
>   	if (type == PDO_TYPE_BATT) {
> -		*rdo = RDO_BATT(index + 1, mw, max_mw, flags);
> +		*rdo = RDO_BATT(src_pdo_index + 1, mw, max_mw, flags);
>   
>   		tcpm_log(port, "Requesting PDO %d: %u mV, %u mW%s",
> -			 index, mv, mw,
> +			 src_pdo_index, mv, mw,
>   			 flags & RDO_CAP_MISMATCH ? " [mismatch]" : "");
>   	} else {
> -		*rdo = RDO_FIXED(index + 1, ma, max_ma, flags);
> +		*rdo = RDO_FIXED(src_pdo_index + 1, ma, max_ma, flags);
>   
>   		tcpm_log(port, "Requesting PDO %d: %u mV, %u mA%s",
> -			 index, mv, ma,
> +			 src_pdo_index, mv, ma,
>   			 flags & RDO_CAP_MISMATCH ? " [mismatch]" : "");
>   	}
>   
> @@ -3599,6 +3630,19 @@ int tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo,
>   }
>   EXPORT_SYMBOL_GPL(tcpm_update_sink_capabilities);
>   
> +static int nr_type_pdos(const u32 *pdo, unsigned int nr_pdo,
> +			enum pd_pdo_type type)
> +{
> +	int count = 0;
> +	int i;
> +
> +	for (i = 0; i < nr_pdo; i++) {
> +		if (pdo_type(pdo[i]) == type)
> +			count++;
> +	}
> +	return count;
> +}
> +
>   struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
>   {
>   	struct tcpm_port *port;
> @@ -3644,6 +3688,15 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
>   					  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_fixed =  nr_type_pdos(port->snk_pdo,
> +				       port->nr_snk_pdo,
> +				       PDO_TYPE_FIXED);
> +	port->nr_var = nr_type_pdos(port->snk_pdo,
> +				    port->nr_snk_pdo,
> +				    PDO_TYPE_VAR);
> +	port->nr_batt = nr_type_pdos(port->snk_pdo,
> +				     port->nr_snk_pdo,
> +				     PDO_TYPE_BATT);
>   	port->nr_snk_vdo = tcpm_copy_vdos(port->snk_vdo, tcpc->config->snk_vdo,
>   					  tcpc->config->nr_snk_vdo);
>   
> 

And the code setting them thus also is no longer needed.

Otherwise this looks good to me now.

Regards,

Hans

Re: [PATCH v2 2/5] usb: typec: fusb302: remove max_snk_* for sink config

[Hans de Goede]

Hi,

On 23-03-18 15:58, Li Jun wrote:
> Since max_snk_* is to be deprecated, so remove max_snk_* by adding a
> variable PDO for sink config.
> 
> Signed-off-by: Li Jun <jun.li@nxp.com>
> ---
>   drivers/usb/typec/fusb302/fusb302.c | 51 +++++++++++++++++++++++++++----------
>   1 file changed, 37 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/typec/fusb302/fusb302.c b/drivers/usb/typec/fusb302/fusb302.c
> index 7036171..db4d9cd 100644
> --- a/drivers/usb/typec/fusb302/fusb302.c
> +++ b/drivers/usb/typec/fusb302/fusb302.c
> @@ -120,6 +120,7 @@ struct fusb302_chip {
>   	enum typec_cc_polarity cc_polarity;
>   	enum typec_cc_status cc1;
>   	enum typec_cc_status cc2;
> +	u32 snk_pdo[PDO_MAX_OBJECTS];
>   
>   #ifdef CONFIG_DEBUG_FS
>   	struct dentry *dentry;
> @@ -1212,11 +1213,6 @@ static const u32 snk_pdo[] = {
>   static const struct tcpc_config fusb302_tcpc_config = {
>   	.src_pdo = src_pdo,
>   	.nr_src_pdo = ARRAY_SIZE(src_pdo),
> -	.snk_pdo = snk_pdo,
> -	.nr_snk_pdo = ARRAY_SIZE(snk_pdo),
> -	.max_snk_mv = 5000,
> -	.max_snk_ma = 3000,
> -	.max_snk_mw = 15000,
>   	.operating_snk_mw = 2500,
>   	.type = TYPEC_PORT_DRP,
>   	.data = TYPEC_PORT_DRD,
> @@ -1756,6 +1752,38 @@ static int init_gpio(struct fusb302_chip *chip)
>   	return 0;
>   }
>   
> +static int fusb302_composite_snk_pdo_array(struct fusb302_chip *chip)
> +{
> +	struct device *dev = chip->dev;
> +	u32 mv, ma, mw, min_mv;
> +	unsigned int i;
> +
> +	/* Copy the static snk pdo */
> +	for (i = 0; i < ARRAY_SIZE(snk_pdo); i++)
> +		chip->snk_pdo[i] = snk_pdo[i];

The static snk PDO is constant and only contains:
	PDO_FIXED(5000, 400, PDO_FIXED_FLAGS),

So you can just do:

	chip->snk_pdo[0] = PDO_FIXED(5000, 400, PDO_FIXED_FLAGS);

Here.

> +
> +	if (device_property_read_u32(dev, "fcs,max-sink-microvolt", &mv) ||
> +	    device_property_read_u32(dev, "fcs,max-sink-microamp", &ma) ||
> +	    device_property_read_u32(dev, "fcs,max-sink-microwatt", &mw))

You can drop the reading of "fcs,max-sink-microwatt" here, it was only
ever added because the tcpm code used to depend on max_mw being set,
now that that is no longer the case, we don't need it.

So this entire function can be simplified to:

static int fusb302_composite_snk_pdo_array(struct fusb302_chip *chip)
{
	struct device *dev = chip->dev;
	u32 max_uv, max_ua;

	chip->snk_pdo[0] = PDO_FIXED(5000, 400, PDO_FIXED_FLAGS);

	if (device_property_read_u32(dev, "fcs,max-sink-microvolt", &max_uv) ||
	    device_property_read_u32(dev, "fcs,max-sink-microamp", &max_ua))
		return 1;

	chip->snk_pdo[1] = PDO_VAR(5000, max_uv / 1000, max_ua / 1000);
	return 2;
}

> @@ -1784,18 +1812,13 @@ static int fusb302_probe(struct i2c_client *client,
>   	chip->tcpc_dev.config = &chip->tcpc_config;
>   	mutex_init(&chip->lock);
>   
> -	if (!device_property_read_u32(dev, "fcs,max-sink-microvolt", &v))
> -		chip->tcpc_config.max_snk_mv = v / 1000;
> -
> -	if (!device_property_read_u32(dev, "fcs,max-sink-microamp", &v))
> -		chip->tcpc_config.max_snk_ma = v / 1000;
> -
> -	if (!device_property_read_u32(dev, "fcs,max-sink-microwatt", &v))
> -		chip->tcpc_config.max_snk_mw = v / 1000;
> -
>   	if (!device_property_read_u32(dev, "fcs,operating-sink-microwatt", &v))
>   		chip->tcpc_config.operating_snk_mw = v / 1000;
>   
> +	/* Composite sink PDO */
> +	chip->tcpc_config.nr_snk_pdo = fusb302_composite_snk_pdo_array(chip);
> +	chip->tcpc_config.snk_pdo = chip->snk_pdo;
> +
>   	/*
>   	 * Devicetree platforms should get extcon via phandle (not yet
>   	 * supported). On ACPI platforms, we get the name from a device prop.
> 

Regards,

Hans

Re: [PATCH v2 0/5] usb: typec: remove max_snk_mv/ma/mw

[Hans de Goede]

Hi,

On 23-03-18 15:58, Li Jun wrote:
> This patch set is to remove max_snk_mv/ma/mw configs, as we should
> define the sink capability by sink PDOs, the first patch update
> the source PDO match policy by compare the voltage range between
> source and sink PDOs no matter what type they are, the following
> patchs remove those 3 variables from 2 existing users by adding
> a variable PDO, then finial patch remove the max_snk_* from tcpm.
> 
> Changes for v2:
> - rebase the 1st patch to be based on commit 6f566af34628
>    ("Revert "typec: tcpm: Only request matching pdos"").
> - Convert the device properties passing max_snk_* to be a
>    variable sink pdo for fusb302.

Thank you for the new version.

I've replied with some comments to patches 1 and 2, the other
3 patches look good to me.

Regards,

Hans



>   
> Li Jun (5):
>    usb: typec: tcpm: pdo matching optimization
>    usb: typec: fusb302:  remove max_snk_* for sink config
>    dt-bindings: usb: fusb302: remove max-sink-* properties
>    usb: typec: wcove: remove max_snk_* for sink config
>    usb: typec: tcpm: remove max_snk_mv/ma/mw
> 
>   .../devicetree/bindings/usb/fcs,fusb302.txt        |   6 -
>   drivers/usb/typec/fusb302/fusb302.c                |  51 +++++---
>   drivers/usb/typec/tcpm.c                           | 139 +++++++++++++--------
>   drivers/usb/typec/typec_wcove.c                    |   4 +-
>   include/linux/usb/tcpm.h                           |   9 --
>   5 files changed, 128 insertions(+), 81 deletions(-)
> 

Re: [PATCH v2 2/5] usb: typec: fusb302: remove max_snk_* for sink config

[Mats Karrman]

Hi Li,

On 2018-03-23 15:58, Li Jun wrote:

> Since max_snk_* is to be deprecated, so remove max_snk_* by adding a
> variable PDO for sink config.
>
> Signed-off-by: Li Jun <jun.li@nxp.com>
> ---
>   drivers/usb/typec/fusb302/fusb302.c | 51 +++++++++++++++++++++++++++----------
>   1 file changed, 37 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/usb/typec/fusb302/fusb302.c b/drivers/usb/typec/fusb302/fusb302.c
> index 7036171..db4d9cd 100644
> --- a/drivers/usb/typec/fusb302/fusb302.c
> +++ b/drivers/usb/typec/fusb302/fusb302.c
> @@ -120,6 +120,7 @@ struct fusb302_chip {
>   	enum typec_cc_polarity cc_polarity;
>   	enum typec_cc_status cc1;
>   	enum typec_cc_status cc2;
> +	u32 snk_pdo[PDO_MAX_OBJECTS];
>   
>   #ifdef CONFIG_DEBUG_FS
>   	struct dentry *dentry;
> @@ -1212,11 +1213,6 @@ static const u32 snk_pdo[] = {
>   static const struct tcpc_config fusb302_tcpc_config = {
>   	.src_pdo = src_pdo,
>   	.nr_src_pdo = ARRAY_SIZE(src_pdo),
> -	.snk_pdo = snk_pdo,
> -	.nr_snk_pdo = ARRAY_SIZE(snk_pdo),
> -	.max_snk_mv = 5000,
> -	.max_snk_ma = 3000,
> -	.max_snk_mw = 15000,
>   	.operating_snk_mw = 2500,
>   	.type = TYPEC_PORT_DRP,
>   	.data = TYPEC_PORT_DRD,
> @@ -1756,6 +1752,38 @@ static int init_gpio(struct fusb302_chip *chip)
>   	return 0;
>   }
>   
> +static int fusb302_composite_snk_pdo_array(struct fusb302_chip *chip)
> +{
> +	struct device *dev = chip->dev;
> +	u32 mv, ma, mw, min_mv;
> +	unsigned int i;
> +
> +	/* Copy the static snk pdo */
> +	for (i = 0; i < ARRAY_SIZE(snk_pdo); i++)
> +		chip->snk_pdo[i] = snk_pdo[i];
> +
> +	if (device_property_read_u32(dev, "fcs,max-sink-microvolt", &mv) ||
> +	    device_property_read_u32(dev, "fcs,max-sink-microamp", &ma) ||
> +	    device_property_read_u32(dev, "fcs,max-sink-microwatt", &mw))
> +		return i;
> +
> +	mv = mv / 1000;
> +	ma = ma / 1000;
> +	mw = mw / 1000;
> +
> +	min_mv = 1000 * chip->tcpc_config.operating_snk_mw / ma;
> +	if (pdo_type(snk_pdo[i-1] == PDO_TYPE_FIXED))

You've got the parentheses wrong.

Apart from that I don't like/understand why the PDO's should be fixed.
Every product should be able to have its own settings, including the first PDO (e.g. it
might need to specify a higher current and/or set the "Higher Capability" bit).

I think this would be best solved the same way as in your TCPCI driver patch series [1]
with a list freely specified by a property.

[1] https://www.spinics.net/lists/linux-usb/msg167398.html

> +		min_mv = max(min_mv, pdo_fixed_voltage(snk_pdo[i-1]));
> +	else
> +		min_mv = max(min_mv, pdo_max_voltage(snk_pdo[i-1]));
> +	ma = min(ma, 1000 * mw / min_mv);
> +
> +	/* Insert the new pdo to the end */
> +	chip->snk_pdo[i] = PDO_VAR(min_mv, mv, ma);
> +
> +	return i+1;
> +}
> +
>   static int fusb302_probe(struct i2c_client *client,
>   			 const struct i2c_device_id *id)
>   {
> @@ -1784,18 +1812,13 @@ static int fusb302_probe(struct i2c_client *client,
>   	chip->tcpc_dev.config = &chip->tcpc_config;
>   	mutex_init(&chip->lock);
>   
> -	if (!device_property_read_u32(dev, "fcs,max-sink-microvolt", &v))
> -		chip->tcpc_config.max_snk_mv = v / 1000;
> -
> -	if (!device_property_read_u32(dev, "fcs,max-sink-microamp", &v))
> -		chip->tcpc_config.max_snk_ma = v / 1000;
> -
> -	if (!device_property_read_u32(dev, "fcs,max-sink-microwatt", &v))
> -		chip->tcpc_config.max_snk_mw = v / 1000;
> -
>   	if (!device_property_read_u32(dev, "fcs,operating-sink-microwatt", &v))
>   		chip->tcpc_config.operating_snk_mw = v / 1000;
>   
> +	/* Composite sink PDO */
> +	chip->tcpc_config.nr_snk_pdo = fusb302_composite_snk_pdo_array(chip);
> +	chip->tcpc_config.snk_pdo = chip->snk_pdo;
> +
>   	/*
>   	 * Devicetree platforms should get extcon via phandle (not yet
>   	 * supported). On ACPI platforms, we get the name from a device prop.
>

Re: [PATCH v2 2/5] usb: typec: fusb302: remove max_snk_* for sink config

[Hans de Goede]

Hi,

On 04-04-18 14:06, Mats Karrman wrote:
> Hi Li,
> 
> On 2018-03-23 15:58, Li Jun wrote:
> 
>> Since max_snk_* is to be deprecated, so remove max_snk_* by adding a
>> variable PDO for sink config.
>>
>> Signed-off-by: Li Jun <jun.li@nxp.com>
>> ---
>>   drivers/usb/typec/fusb302/fusb302.c | 51 +++++++++++++++++++++++++++----------
>>   1 file changed, 37 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/usb/typec/fusb302/fusb302.c b/drivers/usb/typec/fusb302/fusb302.c
>> index 7036171..db4d9cd 100644
>> --- a/drivers/usb/typec/fusb302/fusb302.c
>> +++ b/drivers/usb/typec/fusb302/fusb302.c
>> @@ -120,6 +120,7 @@ struct fusb302_chip {
>>       enum typec_cc_polarity cc_polarity;
>>       enum typec_cc_status cc1;
>>       enum typec_cc_status cc2;
>> +    u32 snk_pdo[PDO_MAX_OBJECTS];
>>   #ifdef CONFIG_DEBUG_FS
>>       struct dentry *dentry;
>> @@ -1212,11 +1213,6 @@ static const u32 snk_pdo[] = {
>>   static const struct tcpc_config fusb302_tcpc_config = {
>>       .src_pdo = src_pdo,
>>       .nr_src_pdo = ARRAY_SIZE(src_pdo),
>> -    .snk_pdo = snk_pdo,
>> -    .nr_snk_pdo = ARRAY_SIZE(snk_pdo),
>> -    .max_snk_mv = 5000,
>> -    .max_snk_ma = 3000,
>> -    .max_snk_mw = 15000,
>>       .operating_snk_mw = 2500,
>>       .type = TYPEC_PORT_DRP,
>>       .data = TYPEC_PORT_DRD,
>> @@ -1756,6 +1752,38 @@ static int init_gpio(struct fusb302_chip *chip)
>>       return 0;
>>   }
>> +static int fusb302_composite_snk_pdo_array(struct fusb302_chip *chip)
>> +{
>> +    struct device *dev = chip->dev;
>> +    u32 mv, ma, mw, min_mv;
>> +    unsigned int i;
>> +
>> +    /* Copy the static snk pdo */
>> +    for (i = 0; i < ARRAY_SIZE(snk_pdo); i++)
>> +        chip->snk_pdo[i] = snk_pdo[i];
>> +
>> +    if (device_property_read_u32(dev, "fcs,max-sink-microvolt", &mv) ||
>> +        device_property_read_u32(dev, "fcs,max-sink-microamp", &ma) ||
>> +        device_property_read_u32(dev, "fcs,max-sink-microwatt", &mw))
>> +        return i;
>> +
>> +    mv = mv / 1000;
>> +    ma = ma / 1000;
>> +    mw = mw / 1000;
>> +
>> +    min_mv = 1000 * chip->tcpc_config.operating_snk_mw / ma;
>> +    if (pdo_type(snk_pdo[i-1] == PDO_TYPE_FIXED))
> 
> You've got the parentheses wrong.
> 
> Apart from that I don't like/understand why the PDO's should be fixed.
> Every product should be able to have its own settings, including the first PDO (e.g. it
> might need to specify a higher current and/or set the "Higher Capability" bit).
> 
> I think this would be best solved the same way as in your TCPCI driver patch series [1]
> with a list freely specified by a property.
> 
> [1] https://www.spinics.net/lists/linux-usb/msg167398.html

Hmm, interesting, for the x86 use-case that would require updating
the properties for the fusb302 defined in:

drivers/platform/x86/intel_cht_int33fe.c

In tandem, which is easily doable.

But what about other users of the fusb302 driver? Since this driver
was added before I started using it for some x86 boards, I assume
there are some non x86 users, so we should preserve compatibility
for DTB files which don't define any sink PDOs in their properties,
I guess we could fall to a default fixed 5V sink pdo for those.

Regards,

Hans



> 
>> +        min_mv = max(min_mv, pdo_fixed_voltage(snk_pdo[i-1]));
>> +    else
>> +        min_mv = max(min_mv, pdo_max_voltage(snk_pdo[i-1]));
>> +    ma = min(ma, 1000 * mw / min_mv);
>> +
>> +    /* Insert the new pdo to the end */
>> +    chip->snk_pdo[i] = PDO_VAR(min_mv, mv, ma);
>> +
>> +    return i+1;
>> +}
>> +
>>   static int fusb302_probe(struct i2c_client *client,
>>                const struct i2c_device_id *id)
>>   {
>> @@ -1784,18 +1812,13 @@ static int fusb302_probe(struct i2c_client *client,
>>       chip->tcpc_dev.config = &chip->tcpc_config;
>>       mutex_init(&chip->lock);
>> -    if (!device_property_read_u32(dev, "fcs,max-sink-microvolt", &v))
>> -        chip->tcpc_config.max_snk_mv = v / 1000;
>> -
>> -    if (!device_property_read_u32(dev, "fcs,max-sink-microamp", &v))
>> -        chip->tcpc_config.max_snk_ma = v / 1000;
>> -
>> -    if (!device_property_read_u32(dev, "fcs,max-sink-microwatt", &v))
>> -        chip->tcpc_config.max_snk_mw = v / 1000;
>> -
>>       if (!device_property_read_u32(dev, "fcs,operating-sink-microwatt", &v))
>>           chip->tcpc_config.operating_snk_mw = v / 1000;
>> +    /* Composite sink PDO */
>> +    chip->tcpc_config.nr_snk_pdo = fusb302_composite_snk_pdo_array(chip);
>> +    chip->tcpc_config.snk_pdo = chip->snk_pdo;
>> +
>>       /*
>>        * Devicetree platforms should get extcon via phandle (not yet
>>        * supported). On ACPI platforms, we get the name from a device prop.
>>

RE: [PATCH v2 1/5] usb: typec: tcpm: pdo matching optimization

[Jun Li]

Hi
> -----Original Message-----
> From: Hans de Goede [mailto:hdegoede@redhat.com]
> Sent: 2018年4月3日 23:17
> To: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org; robh+dt@kernel.org;
> mark.rutland@arm.com; heikki.krogerus@linux.intel.com
> Cc: linux@roeck-us.net; rmfrfs@gmail.com; yueyao.zhu@gmail.com;
> linux-usb@vger.kernel.org; devicetree@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v2 1/5] usb: typec: tcpm: pdo matching optimization
> 
> Hi,
> 
> On 23-03-18 15:58, Li Jun wrote:
> > This patch is a combination of commit 57e6f0d7b804
> > ("typec: tcpm: Only request matching pdos") and source pdo selection
> > optimization based on it, instead of only compare between the same pdo
> > type of sink and source, we should check source pdo voltage range is
> > within the voltage range of one sink pdo.
> >
> > Signed-off-by: Li Jun <jun.li@nxp.com>
> > ---
> >   drivers/usb/typec/tcpm.c | 127
> +++++++++++++++++++++++++++++++++--------------
> >   1 file changed, 90 insertions(+), 37 deletions(-)
> >
> > diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c index
> > 677d121..50a1979 100644
> > --- a/drivers/usb/typec/tcpm.c
> > +++ b/drivers/usb/typec/tcpm.c
> > @@ -254,6 +254,9 @@ struct tcpm_port {
> >   	unsigned int nr_src_pdo;
> >   	u32 snk_pdo[PDO_MAX_OBJECTS];
> >   	unsigned int nr_snk_pdo;
> > +	unsigned int nr_fixed; /* number of fixed sink PDOs */
> > +	unsigned int nr_var; /* number of variable sink PDOs */
> > +	unsigned int nr_batt; /* number of battery sink PDOs */
> >   	u32 snk_vdo[VDO_MAX_OBJECTS];
> >   	unsigned int nr_snk_vdo;
> >
> 
> These 3 variables are now no longer needed.

Yes, I will remove them.

> 
> > @@ -1772,40 +1775,65 @@ static int tcpm_pd_check_request(struct
> tcpm_port *port)
> >   	return 0;
> >   }
> >
> > -static int tcpm_pd_select_pdo(struct tcpm_port *port)
> > +#define min_power(x, y) min(pdo_max_power(x), pdo_max_power(y))
> > +#define min_current(x, y) min(pdo_max_current(x), pdo_max_current(y))
> > +
> > +static int tcpm_pd_select_pdo(struct tcpm_port *port, int *sink_pdo,
> > +			      int *src_pdo)
> >   {
> > -	unsigned int i, max_mw = 0, max_mv = 0;
> > +	unsigned int i, j, max_src_mv = 0, min_src_mv = 0, max_mw = 0,
> > +		     max_mv = 0, src_mw = 0, src_ma = 0, max_snk_mv = 0,
> > +		     min_snk_mv = 0;
> >   	int ret = -EINVAL;
> >
> >   	/*
> > -	 * Select the source PDO providing the most power while staying within
> > -	 * the board's voltage limits. Prefer PDO providing exp
> > +	 * Select the source PDO providing the most power which has a
> > +	 * matchig sink cap.
> >   	 */
> >   	for (i = 0; i < port->nr_source_caps; i++) {
> >   		u32 pdo = port->source_caps[i];
> >   		enum pd_pdo_type type = pdo_type(pdo);
> > -		unsigned int mv, ma, mw;
> >
> > -		if (type == PDO_TYPE_FIXED)
> > -			mv = pdo_fixed_voltage(pdo);
> > -		else
> > -			mv = pdo_min_voltage(pdo);
> > +		if (type == PDO_TYPE_FIXED) {
> > +			max_src_mv = pdo_fixed_voltage(pdo);
> > +			min_src_mv = max_src_mv;
> > +		} else {
> > +			max_src_mv = pdo_max_voltage(pdo);
> > +			min_src_mv = pdo_min_voltage(pdo);
> > +		}
> >
> >   		if (type == PDO_TYPE_BATT) {
> > -			mw = pdo_max_power(pdo);
> > +			src_mw = pdo_max_power(pdo);
> >   		} else {
> > -			ma = min(pdo_max_current(pdo),
> > -				 port->max_snk_ma);
> > -			mw = ma * mv / 1000;
> > +			src_ma = pdo_max_current(pdo);
> > +			src_mw = src_ma * min_src_mv / 1000;
> >   		}
> >
> > -		/* Perfer higher voltages if available */
> > -		if ((mw > max_mw || (mw == max_mw && mv > max_mv)) &&
> > -		    mv <= port->max_snk_mv) {
> > -			ret = i;
> > -			max_mw = mw;
> > -			max_mv = mv;
> > +		for (j = 0; j < port->nr_snk_pdo; j++) {
> > +			pdo = port->snk_pdo[j];
> > +
> > +			if (pdo_type(pdo) == PDO_TYPE_FIXED) {
> > +				min_snk_mv = pdo_fixed_voltage(pdo);
> > +				max_snk_mv = pdo_fixed_voltage(pdo);
> > +			} else {
> > +				min_snk_mv = pdo_min_voltage(pdo);
> > +				max_snk_mv = pdo_max_voltage(pdo);
> > +			}
> > +
> > +			if (max_src_mv <= max_snk_mv &&
> > +				min_src_mv >= min_snk_mv) {
> > +				/* Prefer higher voltages if available */
> > +				if ((src_mw == max_mw && min_src_mv > max_mv) ||
> > +							src_mw > max_mw) {
> > +					*src_pdo = i;
> > +					*sink_pdo = j;
> > +					max_mw = src_mw;
> > +					max_mv = min_src_mv;
> > +				}
> > +				break;
> > +			}
> >   		}
> > +
> >   	}
> >
> >   	return ret;
> > @@ -1816,13 +1844,14 @@ static int tcpm_pd_build_request(struct tcpm_port
> *port, u32 *rdo)
> >   	unsigned int mv, ma, mw, flags;
> >   	unsigned int max_ma, max_mw;
> >   	enum pd_pdo_type type;
> > -	int index;
> > -	u32 pdo;
> > +	int src_pdo_index, snk_pdo_index;
> > +	u32 pdo, matching_snk_pdo;
> >
> > -	index = tcpm_pd_select_pdo(port);
> > -	if (index < 0)
> > +	if (tcpm_pd_select_pdo(port, &snk_pdo_index, &src_pdo_index) < 0)
> >   		return -EINVAL;
> > -	pdo = port->source_caps[index];
> > +
> > +	pdo = port->source_caps[src_pdo_index];
> > +	matching_snk_pdo = port->snk_pdo[snk_pdo_index];
> >   	type = pdo_type(pdo);
> >
> >   	if (type == PDO_TYPE_FIXED)
> > @@ -1830,26 +1859,28 @@ static int tcpm_pd_build_request(struct tcpm_port
> *port, u32 *rdo)
> >   	else
> >   		mv = pdo_min_voltage(pdo);
> >
> > -	/* Select maximum available current within the board's power limit */
> > +	/* Select maximum available current within the sink pdo's limit */
> >   	if (type == PDO_TYPE_BATT) {
> > -		mw = pdo_max_power(pdo);
> > -		ma = 1000 * min(mw, port->max_snk_mw) / mv;
> > +		mw = min_power(pdo, matching_snk_pdo);
> > +		ma = 1000 * mw / mv;
> >   	} else {
> > -		ma = min(pdo_max_current(pdo),
> > -			 1000 * port->max_snk_mw / mv);
> > +		ma = min_current(pdo, matching_snk_pdo);
> > +		mw = ma * mv / 1000;
> >   	}
> > -	ma = min(ma, port->max_snk_ma);
> >
> >   	flags = RDO_USB_COMM | RDO_NO_SUSPEND;
> >
> >   	/* Set mismatch bit if offered power is less than operating power */
> > -	mw = ma * mv / 1000;
> >   	max_ma = ma;
> >   	max_mw = mw;
> >   	if (mw < port->operating_snk_mw) {
> >   		flags |= RDO_CAP_MISMATCH;
> > -		max_mw = port->operating_snk_mw;
> > -		max_ma = max_mw * 1000 / mv;
> > +		if (type == PDO_TYPE_BATT &&
> > +		    (pdo_max_power(matching_snk_pdo) > pdo_max_power(pdo)))
> > +			max_mw = pdo_max_power(matching_snk_pdo);
> > +		else if (pdo_max_current(matching_snk_pdo) >
> > +			 pdo_max_current(pdo))
> > +			max_ma = pdo_max_current(matching_snk_pdo);
> >   	}
> >
> >   	tcpm_log(port, "cc=%d cc1=%d cc2=%d vbus=%d vconn=%s polarity=%d",
> > @@ -1858,16 +1889,16 @@ static int tcpm_pd_build_request(struct tcpm_port
> *port, u32 *rdo)
> >   		 port->polarity);
> >
> >   	if (type == PDO_TYPE_BATT) {
> > -		*rdo = RDO_BATT(index + 1, mw, max_mw, flags);
> > +		*rdo = RDO_BATT(src_pdo_index + 1, mw, max_mw, flags);
> >
> >   		tcpm_log(port, "Requesting PDO %d: %u mV, %u mW%s",
> > -			 index, mv, mw,
> > +			 src_pdo_index, mv, mw,
> >   			 flags & RDO_CAP_MISMATCH ? " [mismatch]" : "");
> >   	} else {
> > -		*rdo = RDO_FIXED(index + 1, ma, max_ma, flags);
> > +		*rdo = RDO_FIXED(src_pdo_index + 1, ma, max_ma, flags);
> >
> >   		tcpm_log(port, "Requesting PDO %d: %u mV, %u mA%s",
> > -			 index, mv, ma,
> > +			 src_pdo_index, mv, ma,
> >   			 flags & RDO_CAP_MISMATCH ? " [mismatch]" : "");
> >   	}
> >
> > @@ -3599,6 +3630,19 @@ int tcpm_update_sink_capabilities(struct tcpm_port
> *port, const u32 *pdo,
> >   }
> >   EXPORT_SYMBOL_GPL(tcpm_update_sink_capabilities);
> >
> > +static int nr_type_pdos(const u32 *pdo, unsigned int nr_pdo,
> > +			enum pd_pdo_type type)
> > +{
> > +	int count = 0;
> > +	int i;
> > +
> > +	for (i = 0; i < nr_pdo; i++) {
> > +		if (pdo_type(pdo[i]) == type)
> > +			count++;
> > +	}
> > +	return count;
> > +}
> > +
> >   struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev
> *tcpc)
> >   {
> >   	struct tcpm_port *port;
> > @@ -3644,6 +3688,15 @@ struct tcpm_port *tcpm_register_port(struct device
> *dev, struct tcpc_dev *tcpc)
> >   					  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_fixed =  nr_type_pdos(port->snk_pdo,
> > +				       port->nr_snk_pdo,
> > +				       PDO_TYPE_FIXED);
> > +	port->nr_var = nr_type_pdos(port->snk_pdo,
> > +				    port->nr_snk_pdo,
> > +				    PDO_TYPE_VAR);
> > +	port->nr_batt = nr_type_pdos(port->snk_pdo,
> > +				     port->nr_snk_pdo,
> > +				     PDO_TYPE_BATT);
> >   	port->nr_snk_vdo = tcpm_copy_vdos(port->snk_vdo,
> tcpc->config->snk_vdo,
> >   					  tcpc->config->nr_snk_vdo);
> >
> >
> 
> And the code setting them thus also is no longer needed.

I will remove them.

> 
> Otherwise this looks good to me now.

Thanks for review.

Jun
> 
> Regards,
> 
> Hans
> 

RE: [PATCH v2 2/5] usb: typec: fusb302: remove max_snk_* for sink config

[Jun Li]

Hi
> -----Original Message-----
> From: Hans de Goede [mailto:hdegoede@redhat.com]
> Sent: 2018年4月3日 23:26
> To: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org; robh+dt@kernel.org;
> mark.rutland@arm.com; heikki.krogerus@linux.intel.com
> Cc: linux@roeck-us.net; rmfrfs@gmail.com; yueyao.zhu@gmail.com;
> linux-usb@vger.kernel.org; devicetree@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v2 2/5] usb: typec: fusb302: remove max_snk_* for sink
> config
> 
> Hi,
> 
> On 23-03-18 15:58, Li Jun wrote:
> > Since max_snk_* is to be deprecated, so remove max_snk_* by adding a
> > variable PDO for sink config.
> >
> > Signed-off-by: Li Jun <jun.li@nxp.com>
> > ---
> >   drivers/usb/typec/fusb302/fusb302.c | 51
> +++++++++++++++++++++++++++----------
> >   1 file changed, 37 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/usb/typec/fusb302/fusb302.c
> > b/drivers/usb/typec/fusb302/fusb302.c
> > index 7036171..db4d9cd 100644
> > --- a/drivers/usb/typec/fusb302/fusb302.c
> > +++ b/drivers/usb/typec/fusb302/fusb302.c
> > @@ -120,6 +120,7 @@ struct fusb302_chip {
> >   	enum typec_cc_polarity cc_polarity;
> >   	enum typec_cc_status cc1;
> >   	enum typec_cc_status cc2;
> > +	u32 snk_pdo[PDO_MAX_OBJECTS];
> >
> >   #ifdef CONFIG_DEBUG_FS
> >   	struct dentry *dentry;
> > @@ -1212,11 +1213,6 @@ static const u32 snk_pdo[] = {
> >   static const struct tcpc_config fusb302_tcpc_config = {
> >   	.src_pdo = src_pdo,
> >   	.nr_src_pdo = ARRAY_SIZE(src_pdo),
> > -	.snk_pdo = snk_pdo,
> > -	.nr_snk_pdo = ARRAY_SIZE(snk_pdo),
> > -	.max_snk_mv = 5000,
> > -	.max_snk_ma = 3000,
> > -	.max_snk_mw = 15000,
> >   	.operating_snk_mw = 2500,
> >   	.type = TYPEC_PORT_DRP,
> >   	.data = TYPEC_PORT_DRD,
> > @@ -1756,6 +1752,38 @@ static int init_gpio(struct fusb302_chip *chip)
> >   	return 0;
> >   }
> >
> > +static int fusb302_composite_snk_pdo_array(struct fusb302_chip *chip)
> > +{
> > +	struct device *dev = chip->dev;
> > +	u32 mv, ma, mw, min_mv;
> > +	unsigned int i;
> > +
> > +	/* Copy the static snk pdo */
> > +	for (i = 0; i < ARRAY_SIZE(snk_pdo); i++)
> > +		chip->snk_pdo[i] = snk_pdo[i];
> 
> The static snk PDO is constant and only contains:
> 	PDO_FIXED(5000, 400, PDO_FIXED_FLAGS),
> 
> So you can just do:
> 
> 	chip->snk_pdo[0] = PDO_FIXED(5000, 400, PDO_FIXED_FLAGS);
> 
> Here.
> 
> > +
> > +	if (device_property_read_u32(dev, "fcs,max-sink-microvolt", &mv) ||
> > +	    device_property_read_u32(dev, "fcs,max-sink-microamp", &ma) ||
> > +	    device_property_read_u32(dev, "fcs,max-sink-microwatt", &mw))
> 
> You can drop the reading of "fcs,max-sink-microwatt" here, it was only ever
> added because the tcpm code used to depend on max_mw being set, now that
> that is no longer the case, we don't need it.
> 
> So this entire function can be simplified to:
> 
> static int fusb302_composite_snk_pdo_array(struct fusb302_chip *chip) {
> 	struct device *dev = chip->dev;
> 	u32 max_uv, max_ua;
> 
> 	chip->snk_pdo[0] = PDO_FIXED(5000, 400, PDO_FIXED_FLAGS);
> 
> 	if (device_property_read_u32(dev, "fcs,max-sink-microvolt", &max_uv) ||
> 	    device_property_read_u32(dev, "fcs,max-sink-microamp", &max_ua))
> 		return 1;
> 
> 	chip->snk_pdo[1] = PDO_VAR(5000, max_uv / 1000, max_ua / 1000);
> 	return 2;
> }

OK, I will change in v3.

Thanks
Jun

> 
> > @@ -1784,18 +1812,13 @@ static int fusb302_probe(struct i2c_client *client,
> >   	chip->tcpc_dev.config = &chip->tcpc_config;
> >   	mutex_init(&chip->lock);
> >
> > -	if (!device_property_read_u32(dev, "fcs,max-sink-microvolt", &v))
> > -		chip->tcpc_config.max_snk_mv = v / 1000;
> > -
> > -	if (!device_property_read_u32(dev, "fcs,max-sink-microamp", &v))
> > -		chip->tcpc_config.max_snk_ma = v / 1000;
> > -
> > -	if (!device_property_read_u32(dev, "fcs,max-sink-microwatt", &v))
> > -		chip->tcpc_config.max_snk_mw = v / 1000;
> > -
> >   	if (!device_property_read_u32(dev, "fcs,operating-sink-microwatt", &v))
> >   		chip->tcpc_config.operating_snk_mw = v / 1000;
> >
> > +	/* Composite sink PDO */
> > +	chip->tcpc_config.nr_snk_pdo = fusb302_composite_snk_pdo_array(chip);
> > +	chip->tcpc_config.snk_pdo = chip->snk_pdo;
> > +
> >   	/*
> >   	 * Devicetree platforms should get extcon via phandle (not yet
> >   	 * supported). On ACPI platforms, we get the name from a device prop.
> >
> 
> Regards,
> 
> Hans

RE: [PATCH v2 2/5] usb: typec: fusb302: remove max_snk_* for sink config

[Jun Li]

Hi
> -----Original Message-----
> From: Mats Karrman [mailto:mats.dev.list@gmail.com]
> Sent: 2018年4月4日 20:07
> To: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org; robh+dt@kernel.org;
> mark.rutland@arm.com; heikki.krogerus@linux.intel.com;
> hdegoede@redhat.com
> Cc: linux@roeck-us.net; rmfrfs@gmail.com; yueyao.zhu@gmail.com;
> linux-usb@vger.kernel.org; devicetree@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v2 2/5] usb: typec: fusb302: remove max_snk_* for sink
> config
> 
> Hi Li,
> 
> On 2018-03-23 15:58, Li Jun wrote:
> 
> > Since max_snk_* is to be deprecated, so remove max_snk_* by adding a
> > variable PDO for sink config.
> >
> > Signed-off-by: Li Jun <jun.li@nxp.com>
> > ---
> >   drivers/usb/typec/fusb302/fusb302.c | 51
> +++++++++++++++++++++++++++----------
> >   1 file changed, 37 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/usb/typec/fusb302/fusb302.c
> > b/drivers/usb/typec/fusb302/fusb302.c
> > index 7036171..db4d9cd 100644
> > --- a/drivers/usb/typec/fusb302/fusb302.c
> > +++ b/drivers/usb/typec/fusb302/fusb302.c
> > @@ -120,6 +120,7 @@ struct fusb302_chip {
> >   	enum typec_cc_polarity cc_polarity;
> >   	enum typec_cc_status cc1;
> >   	enum typec_cc_status cc2;
> > +	u32 snk_pdo[PDO_MAX_OBJECTS];
> >
> >   #ifdef CONFIG_DEBUG_FS
> >   	struct dentry *dentry;
> > @@ -1212,11 +1213,6 @@ static const u32 snk_pdo[] = {
> >   static const struct tcpc_config fusb302_tcpc_config = {
> >   	.src_pdo = src_pdo,
> >   	.nr_src_pdo = ARRAY_SIZE(src_pdo),
> > -	.snk_pdo = snk_pdo,
> > -	.nr_snk_pdo = ARRAY_SIZE(snk_pdo),
> > -	.max_snk_mv = 5000,
> > -	.max_snk_ma = 3000,
> > -	.max_snk_mw = 15000,
> >   	.operating_snk_mw = 2500,
> >   	.type = TYPEC_PORT_DRP,
> >   	.data = TYPEC_PORT_DRD,
> > @@ -1756,6 +1752,38 @@ static int init_gpio(struct fusb302_chip *chip)
> >   	return 0;
> >   }
> >
> > +static int fusb302_composite_snk_pdo_array(struct fusb302_chip *chip)
> > +{
> > +	struct device *dev = chip->dev;
> > +	u32 mv, ma, mw, min_mv;
> > +	unsigned int i;
> > +
> > +	/* Copy the static snk pdo */
> > +	for (i = 0; i < ARRAY_SIZE(snk_pdo); i++)
> > +		chip->snk_pdo[i] = snk_pdo[i];
> > +
> > +	if (device_property_read_u32(dev, "fcs,max-sink-microvolt", &mv) ||
> > +	    device_property_read_u32(dev, "fcs,max-sink-microamp", &ma) ||
> > +	    device_property_read_u32(dev, "fcs,max-sink-microwatt", &mw))
> > +		return i;
> > +
> > +	mv = mv / 1000;
> > +	ma = ma / 1000;
> > +	mw = mw / 1000;
> > +
> > +	min_mv = 1000 * chip->tcpc_config.operating_snk_mw / ma;
> > +	if (pdo_type(snk_pdo[i-1] == PDO_TYPE_FIXED))
> 
> You've got the parentheses wrong.

Good catch.

> 
> Apart from that I don't like/understand why the PDO's should be fixed.

This is only for legacy setting(a fixed PDO) and properties(fcs,max-sink-*),
to make it can still work after using the new PDO matching policy.

> Every product should be able to have its own settings, including the first PDO (e.g.
> it might need to specify a higher current and/or set the "Higher Capability" bit).
> 
> I think this would be best solved the same way as in your TCPCI driver patch
> series [1] with a list freely specified by a property.
> 
> [1]
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.s
> pinics.net%2Flists%2Flinux-usb%2Fmsg167398.html&data=02%7C01%7Cjun.li%40
> nxp.com%7C94bfdd71fa87479f54c808d59a24873a%7C686ea1d3bc2b4c6fa92cd99c
> 5c301635%7C0%7C0%7C636584404043415663&sdata=RbOoMtJM9XtmvhHYNQ9a
> nhPMn%2BJpdqwY3UagRPCgM9k%3D&reserved=0

Agreed, new code(dt) should use[1] to describe the power properties.
Fusb302 driver just need pass a fwnode at init, tcpm will read and parse
all the properties when register port.

Thanks
Jun
> 
> > +		min_mv = max(min_mv, pdo_fixed_voltage(snk_pdo[i-1]));
> > +	else
> > +		min_mv = max(min_mv, pdo_max_voltage(snk_pdo[i-1]));
> > +	ma = min(ma, 1000 * mw / min_mv);
> > +
> > +	/* Insert the new pdo to the end */
> > +	chip->snk_pdo[i] = PDO_VAR(min_mv, mv, ma);
> > +
> > +	return i+1;
> > +}
> > +
> >   static int fusb302_probe(struct i2c_client *client,
> >   			 const struct i2c_device_id *id)
> >   {
> > @@ -1784,18 +1812,13 @@ static int fusb302_probe(struct i2c_client *client,
> >   	chip->tcpc_dev.config = &chip->tcpc_config;
> >   	mutex_init(&chip->lock);
> >
> > -	if (!device_property_read_u32(dev, "fcs,max-sink-microvolt", &v))
> > -		chip->tcpc_config.max_snk_mv = v / 1000;
> > -
> > -	if (!device_property_read_u32(dev, "fcs,max-sink-microamp", &v))
> > -		chip->tcpc_config.max_snk_ma = v / 1000;
> > -
> > -	if (!device_property_read_u32(dev, "fcs,max-sink-microwatt", &v))
> > -		chip->tcpc_config.max_snk_mw = v / 1000;
> > -
> >   	if (!device_property_read_u32(dev, "fcs,operating-sink-microwatt", &v))
> >   		chip->tcpc_config.operating_snk_mw = v / 1000;
> >
> > +	/* Composite sink PDO */
> > +	chip->tcpc_config.nr_snk_pdo = fusb302_composite_snk_pdo_array(chip);
> > +	chip->tcpc_config.snk_pdo = chip->snk_pdo;
> > +
> >   	/*
> >   	 * Devicetree platforms should get extcon via phandle (not yet
> >   	 * supported). On ACPI platforms, we get the name from a device prop.
> >

RE: [PATCH v2 2/5] usb: typec: fusb302: remove max_snk_* for sink config

[Jun Li]

> -----Original Message-----
> From: Hans de Goede [mailto:hdegoede@redhat.com]
> Sent: 2018年4月5日 4:13
> To: Mats Karrman <mats.dev.list@gmail.com>; Jun Li <jun.li@nxp.com>;
> gregkh@linuxfoundation.org; robh+dt@kernel.org; mark.rutland@arm.com;
> heikki.krogerus@linux.intel.com
> Cc: linux@roeck-us.net; rmfrfs@gmail.com; yueyao.zhu@gmail.com;
> linux-usb@vger.kernel.org; devicetree@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v2 2/5] usb: typec: fusb302: remove max_snk_* for sink
> config
> 
> Hi,
> 
> On 04-04-18 14:06, Mats Karrman wrote:
> > Hi Li,
> >
> > On 2018-03-23 15:58, Li Jun wrote:
> >
> >> Since max_snk_* is to be deprecated, so remove max_snk_* by adding a
> >> variable PDO for sink config.
> >>
> >> Signed-off-by: Li Jun <jun.li@nxp.com>
> >> ---
> >>   drivers/usb/typec/fusb302/fusb302.c | 51
> >> +++++++++++++++++++++++++++----------
> >>   1 file changed, 37 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/drivers/usb/typec/fusb302/fusb302.c
> >> b/drivers/usb/typec/fusb302/fusb302.c
> >> index 7036171..db4d9cd 100644
> >> --- a/drivers/usb/typec/fusb302/fusb302.c
> >> +++ b/drivers/usb/typec/fusb302/fusb302.c
> >> @@ -120,6 +120,7 @@ struct fusb302_chip {
> >>       enum typec_cc_polarity cc_polarity;
> >>       enum typec_cc_status cc1;
> >>       enum typec_cc_status cc2;
> >> +    u32 snk_pdo[PDO_MAX_OBJECTS];
> >>   #ifdef CONFIG_DEBUG_FS
> >>       struct dentry *dentry;
> >> @@ -1212,11 +1213,6 @@ static const u32 snk_pdo[] = {
> >>   static const struct tcpc_config fusb302_tcpc_config = {
> >>       .src_pdo = src_pdo,
> >>       .nr_src_pdo = ARRAY_SIZE(src_pdo),
> >> -    .snk_pdo = snk_pdo,
> >> -    .nr_snk_pdo = ARRAY_SIZE(snk_pdo),
> >> -    .max_snk_mv = 5000,
> >> -    .max_snk_ma = 3000,
> >> -    .max_snk_mw = 15000,
> >>       .operating_snk_mw = 2500,
> >>       .type = TYPEC_PORT_DRP,
> >>       .data = TYPEC_PORT_DRD,
> >> @@ -1756,6 +1752,38 @@ static int init_gpio(struct fusb302_chip
> >> *chip)
> >>       return 0;
> >>   }
> >> +static int fusb302_composite_snk_pdo_array(struct fusb302_chip
> >> +*chip) {
> >> +    struct device *dev = chip->dev;
> >> +    u32 mv, ma, mw, min_mv;
> >> +    unsigned int i;
> >> +
> >> +    /* Copy the static snk pdo */
> >> +    for (i = 0; i < ARRAY_SIZE(snk_pdo); i++)
> >> +        chip->snk_pdo[i] = snk_pdo[i];
> >> +
> >> +    if (device_property_read_u32(dev, "fcs,max-sink-microvolt", &mv)
> >> +||
> >> +        device_property_read_u32(dev, "fcs,max-sink-microamp", &ma)
> >> +||
> >> +        device_property_read_u32(dev, "fcs,max-sink-microwatt",
> >> +&mw))
> >> +        return i;
> >> +
> >> +    mv = mv / 1000;
> >> +    ma = ma / 1000;
> >> +    mw = mw / 1000;
> >> +
> >> +    min_mv = 1000 * chip->tcpc_config.operating_snk_mw / ma;
> >> +    if (pdo_type(snk_pdo[i-1] == PDO_TYPE_FIXED))
> >
> > You've got the parentheses wrong.
> >
> > Apart from that I don't like/understand why the PDO's should be fixed.
> > Every product should be able to have its own settings, including the
> > first PDO (e.g. it might need to specify a higher current and/or set the "Higher
> Capability" bit).
> >
> > I think this would be best solved the same way as in your TCPCI driver
> > patch series [1] with a list freely specified by a property.
> >
> > [1]
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww
> > .spinics.net%2Flists%2Flinux-usb%2Fmsg167398.html&data=02%7C01%7Cjun.l
> >
> i%40nxp.com%7C678fb35afff542581d3808d59a68708b%7C686ea1d3bc2b4c6fa92c
> d
> >
> 99c5c301635%7C0%7C0%7C636584695710093111&sdata=ZyAJQUR8ZbAq8VWsZkF
> PGLg
> > F9P5j5QpvF3yeGyNoyH8%3D&reserved=0
> 
> Hmm, interesting, for the x86 use-case that would require updating the
> properties for the fusb302 defined in:
> 
> drivers/platform/x86/intel_cht_int33fe.c
> 
> In tandem, which is easily doable.
> 
> But what about other users of the fusb302 driver? Since this driver was added
> before I started using it for some x86 boards, I assume there are some non x86
> users, so we should preserve compatibility for DTB files which don't define any
> sink PDOs in their properties, I guess we could fall to a default fixed 5V sink pdo
> for those.

If we can't elaborate all existing users, we have to change the driver
like this to preserve compatibility.

Thanks
Jun
> 
> Regards,
> 
> Hans
> 
> 
> 
> >
> >> +        min_mv = max(min_mv, pdo_fixed_voltage(snk_pdo[i-1]));
> >> +    else
> >> +        min_mv = max(min_mv, pdo_max_voltage(snk_pdo[i-1]));
> >> +    ma = min(ma, 1000 * mw / min_mv);
> >> +
> >> +    /* Insert the new pdo to the end */
> >> +    chip->snk_pdo[i] = PDO_VAR(min_mv, mv, ma);
> >> +
> >> +    return i+1;
> >> +}
> >> +
> >>   static int fusb302_probe(struct i2c_client *client,
> >>                const struct i2c_device_id *id)
> >>   {
> >> @@ -1784,18 +1812,13 @@ static int fusb302_probe(struct i2c_client
> >> *client,
> >>       chip->tcpc_dev.config = &chip->tcpc_config;
> >>       mutex_init(&chip->lock);
> >> -    if (!device_property_read_u32(dev, "fcs,max-sink-microvolt",
> >> &v))
> >> -        chip->tcpc_config.max_snk_mv = v / 1000;
> >> -
> >> -    if (!device_property_read_u32(dev, "fcs,max-sink-microamp", &v))
> >> -        chip->tcpc_config.max_snk_ma = v / 1000;
> >> -
> >> -    if (!device_property_read_u32(dev, "fcs,max-sink-microwatt",
> >> &v))
> >> -        chip->tcpc_config.max_snk_mw = v / 1000;
> >> -
> >>       if (!device_property_read_u32(dev,
> >> "fcs,operating-sink-microwatt", &v))
> >>           chip->tcpc_config.operating_snk_mw = v / 1000;
> >> +    /* Composite sink PDO */
> >> +    chip->tcpc_config.nr_snk_pdo =
> >> +fusb302_composite_snk_pdo_array(chip);
> >> +    chip->tcpc_config.snk_pdo = chip->snk_pdo;
> >> +
> >>       /*
> >>        * Devicetree platforms should get extcon via phandle (not yet
> >>        * supported). On ACPI platforms, we get the name from a device
> prop.
> >>