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

[Dan Carpenter <dan.carpenter@oracle.com>]

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