Re: [PATCH 2/2] staging: typec: tcpm: Only request matching pdos

[Badhri Jagan Sridharan]

On Fri, Sep 8, 2017 at 2:36 AM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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
>