> -----Original Message----- > From: Hans de Goede [mailto:hdegoede@xxxxxxxxxx] > Sent: 2018年4月14日 0:54 > To: Guenter Roeck <linux@xxxxxxxxxxxx>; Jun Li <jun.li@xxxxxxx> > Cc: gregkh@xxxxxxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx; > heikki.krogerus@xxxxxxxxxxxxxxx; rmfrfs@xxxxxxxxx; yueyao.zhu@xxxxxxxxx; > linux-usb@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx> > Subject: Re: [PATCH v4 1/5] usb: typec: tcpm: pdo matching optimization > > Hi, > > On 13-04-18 17:58, Guenter Roeck wrote: > > 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. > > Good catch, this indeed looks like this was not tested ? I had this patch of old version tested, with new version only did rebase and remove unused variable, I assumed it should work the same way, didn't realize I actually removed one line code "ret = 0", sorry for this. > > Next time please always test if possible. Sure. > > we need to set ret = 0 in tcpm_pd_select_pdo() when we assign a value to the > src and sink pdos. > > Regards, > > Hans > > > > > > > >> - 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 > >> ��.n��������+%������w��{.n�����{���)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
