Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756758AbdIHRoY (ORCPT ); Fri, 8 Sep 2017 13:44:24 -0400 Received: from mail-oi0-f41.google.com ([209.85.218.41]:36454 "EHLO mail-oi0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756685AbdIHRoW (ORCPT ); Fri, 8 Sep 2017 13:44:22 -0400 X-Google-Smtp-Source: AOwi7QDMCK0cix+Ot9H5KIo+swo/pxM63wWeUyg/i+EZ4IEfaq/mgsBDDts7XkIghaSOhgAOMYap+Gm5gUGJgaMe/LY= MIME-Version: 1.0 In-Reply-To: <20170908093629.ob7fnaxlsbrmjksv@mwanda> References: <20170908012214.19047-1-Badhri@google.com> <20170908012214.19047-2-Badhri@google.com> <20170908093629.ob7fnaxlsbrmjksv@mwanda> From: Badhri Jagan Sridharan Date: Fri, 8 Sep 2017 10:43:40 -0700 Message-ID: Subject: Re: [PATCH 2/2] staging: typec: tcpm: Only request matching pdos To: Dan Carpenter Cc: Greg Kroah-Hartman , Guenter Roeck , devel@driverdev.osuosl.org, LKML Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8598 Lines: 221 On Fri, Sep 8, 2017 at 2:36 AM, Dan Carpenter wrote: > On Thu, Sep 07, 2017 at 06:22:14PM -0700, Badhri Jagan Sridharan wrote: >> diff --git a/drivers/staging/typec/tcpm.c b/drivers/staging/typec/tcpm.c >> index 58a2c279f7d1..df0986d9f756 100644 >> --- a/drivers/staging/typec/tcpm.c >> +++ b/drivers/staging/typec/tcpm.c >> @@ -262,6 +262,9 @@ struct tcpm_port { >> unsigned int nr_src_pdo; >> u32 snk_pdo[PDO_MAX_OBJECTS]; >> unsigned int nr_snk_pdo; >> + unsigned int nr_snk_fixed_pdo; >> + unsigned int nr_snk_var_pdo; >> + unsigned int nr_snk_batt_pdo; > > These names are too long. The extra words obscure the parts of the > variable names which have information. It hurts readability. Do this: > > 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; >> >> @@ -1780,37 +1783,77 @@ static int tcpm_pd_check_request(struct tcpm_port *port) >> return 0; >> } >> >> -static int tcpm_pd_select_pdo(struct tcpm_port *port) >> +static int tcpm_pd_select_pdo(struct tcpm_port *port, int *sink_pdo) >> { >> - unsigned int i, max_mw = 0, max_mv = 0; >> + unsigned int i, j, max_mw = 0, max_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. Prefer PDO providing exp > ^^^^^^^ ^^^^^^^^^^^^^ > "matching". What does "providing exp" mean? Actually that was moved forward from the existing code. So not sure what that means ? I can remove it, if needed. > >> */ >> 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_BATT) { >> - mw = pdo_max_power(pdo); >> - } else { >> - ma = min(pdo_max_current(pdo), >> - port->max_snk_ma); >> - mw = ma * mv / 1000; >> + unsigned int mv = 0, ma = 0, mw = 0, selected = 0; >> + >> + if (type == PDO_TYPE_FIXED) { >> + for (j = 0; j < port->nr_snk_fixed_pdo; j++) { >> + if (pdo_fixed_voltage(pdo) == >> + pdo_fixed_voltage(port->snk_pdo[j])) { >> + mv = pdo_fixed_voltage(pdo); >> + selected = j; >> + break; >> + } >> + } >> + } else if (type == PDO_TYPE_BATT && port->nr_snk_batt_pdo) { > > The test for nr_snk_batt_pdo isn't required. If it's zero then the for > loop is just a no-op. Remove it. > >> + for (j = port->nr_snk_fixed_pdo; >> + j < port->nr_snk_fixed_pdo + >> + port->nr_snk_batt_pdo; > > This should be aligned like this: > > for (j = port->nr_snk_fixed_pdo; > j < port->nr_snk_fixed_pdo + > port->nr_snk_batt_pdo; > >> + j++) { >> + if ((pdo_min_voltage(pdo) >= >> + pdo_min_voltage(port->snk_pdo[j])) && >> + pdo_max_voltage(pdo) <= >> + pdo_max_voltage(port->snk_pdo[j])) { > > No need for the extra parentheses around the first part of the &&. The > condition isn't indented right either because the last two lines are > indented one space more than they should be. Just do: > > if (pdo_min_voltage(pdo) >= > pdo_min_voltage(port->snk_pdo[j]) && > pdo_max_voltage(pdo) <= > pdo_max_voltage(port->snk_pdo[j])) { > > >> + mv = pdo_min_voltage(pdo); >> + selected = j; >> + break; > > We always select the first valid voltage? Good catch ! Should be evaluating against all matching sink_caps not just the first one which matches. Will make amends in the next version. > >> + } >> + } >> + } else if (type == PDO_TYPE_VAR && port->nr_snk_var_pdo) { >> + for (j = port->nr_snk_fixed_pdo + >> + port->nr_snk_batt_pdo; >> + j < port->nr_snk_fixed_pdo + >> + port->nr_snk_batt_pdo + >> + port->nr_snk_var_pdo; >> + j++) { >> + if ((pdo_min_voltage(pdo) >= >> + pdo_min_voltage(port->snk_pdo[j])) && >> + pdo_max_voltage(pdo) <= >> + pdo_max_voltage(port->snk_pdo[j])) { >> + mv = pdo_min_voltage(pdo); >> + selected = j; >> + break; >> + } >> + } > > Same stuff. > >> } >> >> + if (mv != 0) { > > Flip this condition around. > > if (mv == 0) > continue; > >> + if (type == PDO_TYPE_BATT) { >> + mw = min(pdo_max_power(pdo), >> + pdo_max_power(port->snk_pdo[selected] >> + )); >> + } else { >> + ma = min(pdo_max_current(pdo), >> + pdo_max_current( >> + port->snk_pdo[selected])); >> + mw = ma * mv / 1000; >> + } > > Then pull this code in one indent level and it looks nicer. > >> + } >> /* Perfer higher voltages if available */ >> - if ((mw > max_mw || (mw == max_mw && mv > max_mv)) && >> - mv <= port->max_snk_mv) { >> + if (mw > max_mw || (mw == max_mw && mv > max_mv)) { >> ret = i; >> + *sink_pdo = selected; >> max_mw = mw; >> max_mv = mv; >> } > > [ snip ] > >> @@ -3609,6 +3659,17 @@ int tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo, >> } >> EXPORT_SYMBOL_GPL(tcpm_update_sink_capabilities); >> >> +static int tcpm_get_nr_type_pdos(const u32 *pdo, unsigned int nr_pdo, > > This function name is awkward. It's quite long and that means we keep > bumping into the 80 character limit. Often times "get_" functions need > to be followed by a "put_" but so the name is a little misleading. It's > static so we don't really need the tcpm_ prefix. Just call it > nr_type_pdos(). > >> + enum pd_pdo_type type) >> +{ >> + unsigned int i = 0; >> + >> + for (i = 0; i < nr_pdo; i++) >> + if (pdo_type(pdo[i] == type)) > > Parentheses are in the wrong place so this is always false. It should > say: > if (pdo_type(pdo[i]) == type) > >> + i++; > > The "i" variable is the iterator. We should be saying "count++"; This > function always returns nr_pdo. Write it like this: > > 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; > } Sure. Will make the about amends and all other readability changes in the next version. > > regards, > dan carpenter >