mbox series

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

Message ID 1521817127-23061-1-git-send-email-jun.li@nxp.com
Headers show
Series usb: typec: remove max_snk_mv/ma/mw | expand

Message

Jun Li March 23, 2018, 2:58 p.m. UTC
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(-)

Comments

Hans de Goede April 3, 2018, 3:17 p.m. UTC | #1
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


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hans de Goede April 3, 2018, 3:25 p.m. UTC | #2
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hans de Goede April 3, 2018, 3:27 p.m. UTC | #3
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(-)
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mats Karrman April 4, 2018, 12:06 p.m. UTC | #4
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.
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hans de Goede April 4, 2018, 8:12 p.m. UTC | #5
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.
>>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jun Li April 9, 2018, 6:06 a.m. UTC | #6
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
>
Jun Li April 9, 2018, 7:04 a.m. UTC | #7
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
Jun Li April 9, 2018, 8:48 a.m. UTC | #8
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.
> >