On Fri, Apr 13, 2018 at 10:18:41PM +0800, 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.
>
> Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> Signed-off-by: Li Jun <jun.li@xxxxxxx>
> ---
> drivers/usb/typec/tcpm.c | 102 ++++++++++++++++++++++++++++++-----------------
> 1 file changed, 65 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> index 677d121..c127339 100644
> --- a/drivers/usb/typec/tcpm.c
> +++ b/drivers/usb/typec/tcpm.c
> @@ -1772,40 +1772,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;
> + }
> }
> +
Nitpick, but that added empty line is uneceessary.
> }
>
> return ret;
> @@ -1816,13 +1841,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;
This may save you a variable, but tcpm_pd_select_pdo() may at some point return
a different error. Either case, this may trigger a static analyzer warning that
you are overriding a return value.
Besides, does this even work ? I don't see where the function would return
anything but -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 +1856,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 +1886,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]" : "");
> }
>
> --
> 2.7.4
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
