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?
> */
> 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?
> + }
> + }
> + } 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;
}
regards,
dan carpenter
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
