Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Guenter Roeck <groeck7@gmail.com>
Message-ID: <0ea7ae64-c4dd-929d-2ca1-68598af13bc4@roeck-us.net>
Date:   Wed, 2 Aug 2023 07:18:17 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH] usb: typec: tcpm: Refactor the PPS APDO selection
Content-Language: en-US
To:     Kyle Tso <kyletso@google.com>, heikki.krogerus@linux.intel.com,
        gregkh@linuxfoundation.org
Cc:     badhri@google.com, linux-kernel@vger.kernel.org,
        linux-usb@vger.kernel.org
References: <20230731162159.19483-1-kyletso@google.com>
From:   Guenter Roeck <linux@roeck-us.net>
In-Reply-To: <20230731162159.19483-1-kyletso@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 7/31/23 09:21, Kyle Tso wrote:
> In current design of the PPS APDO selection, TCPM power supply only
> accepts the requested voltage which is inside the range of the selected
> PPS profile. To extend the flexibility and usability, remove the checks
> about the voltage range in current profile. And try to search all PPS
> APDOs of the Source that fit the requested voltage.
> 
> Also remove some redundant checks in tcpm_pd_build_pps_request.
> 
> Signed-off-by: Kyle Tso <kyletso@google.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/usb/typec/tcpm/tcpm.c | 122 ++++++----------------------------
>   1 file changed, 21 insertions(+), 101 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 829d75ebab42..9c496b8302b4 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -3253,23 +3253,12 @@ static int tcpm_pd_select_pdo(struct tcpm_port *port, int *sink_pdo,
>   	return ret;
>   }
>   
> -#define min_pps_apdo_current(x, y)	\
> -	min(pdo_pps_apdo_max_current(x), pdo_pps_apdo_max_current(y))
> -
>   static unsigned int tcpm_pd_select_pps_apdo(struct tcpm_port *port)
>   {
> -	unsigned int i, j, max_mw = 0, max_mv = 0;
> -	unsigned int min_src_mv, max_src_mv, src_ma, src_mw;
> -	unsigned int min_snk_mv, max_snk_mv;
> -	unsigned int max_op_mv;
> -	u32 pdo, src, snk;
> -	unsigned int src_pdo = 0, snk_pdo = 0;
> +	unsigned int i, src_ma, max_temp_mw = 0, max_op_ma, op_mw;
> +	unsigned int src_pdo = 0;
> +	u32 pdo, src;
>   
> -	/*
> -	 * Select the source PPS APDO providing the most power while staying
> -	 * within the board's limits. We skip the first PDO as this is always
> -	 * 5V 3A.
> -	 */
>   	for (i = 1; i < port->nr_source_caps; ++i) {
>   		pdo = port->source_caps[i];
>   
> @@ -3280,54 +3269,17 @@ static unsigned int tcpm_pd_select_pps_apdo(struct tcpm_port *port)
>   				continue;
>   			}
>   
> -			min_src_mv = pdo_pps_apdo_min_voltage(pdo);
> -			max_src_mv = pdo_pps_apdo_max_voltage(pdo);
> -			src_ma = pdo_pps_apdo_max_current(pdo);
> -			src_mw = (src_ma * max_src_mv) / 1000;
> -
> -			/*
> -			 * Now search through the sink PDOs to find a matching
> -			 * PPS APDO. Again skip the first sink PDO as this will
> -			 * always be 5V 3A.
> -			 */
> -			for (j = 1; j < port->nr_snk_pdo; j++) {
> -				pdo = port->snk_pdo[j];
> -
> -				switch (pdo_type(pdo)) {
> -				case PDO_TYPE_APDO:
> -					if (pdo_apdo_type(pdo) != APDO_TYPE_PPS) {
> -						tcpm_log(port,
> -							 "Not PPS APDO (sink), ignoring");
> -						continue;
> -					}
> -
> -					min_snk_mv =
> -						pdo_pps_apdo_min_voltage(pdo);
> -					max_snk_mv =
> -						pdo_pps_apdo_max_voltage(pdo);
> -					break;
> -				default:
> -					tcpm_log(port,
> -						 "Not APDO type (sink), ignoring");
> -					continue;
> -				}
> +			if (port->pps_data.req_out_volt > pdo_pps_apdo_max_voltage(pdo) ||
> +			    port->pps_data.req_out_volt < pdo_pps_apdo_min_voltage(pdo))
> +				continue;
>   
> -				if (min_src_mv <= max_snk_mv &&
> -				    max_src_mv >= min_snk_mv) {
> -					max_op_mv = min(max_src_mv, max_snk_mv);
> -					src_mw = (max_op_mv * src_ma) / 1000;
> -					/* Prefer higher voltages if available */
> -					if ((src_mw == max_mw &&
> -					     max_op_mv > max_mv) ||
> -					    src_mw > max_mw) {
> -						src_pdo = i;
> -						snk_pdo = j;
> -						max_mw = src_mw;
> -						max_mv = max_op_mv;
> -					}
> -				}
> +			src_ma = pdo_pps_apdo_max_current(pdo);
> +			max_op_ma = min(src_ma, port->pps_data.req_op_curr);
> +			op_mw = max_op_ma * port->pps_data.req_out_volt / 1000;
> +			if (op_mw > max_temp_mw) {
> +				src_pdo = i;
> +				max_temp_mw = op_mw;
>   			}
> -
>   			break;
>   		default:
>   			tcpm_log(port, "Not APDO type (source), ignoring");
> @@ -3337,16 +3289,10 @@ static unsigned int tcpm_pd_select_pps_apdo(struct tcpm_port *port)
>   
>   	if (src_pdo) {
>   		src = port->source_caps[src_pdo];
> -		snk = port->snk_pdo[snk_pdo];
> -
> -		port->pps_data.req_min_volt = max(pdo_pps_apdo_min_voltage(src),
> -						  pdo_pps_apdo_min_voltage(snk));
> -		port->pps_data.req_max_volt = min(pdo_pps_apdo_max_voltage(src),
> -						  pdo_pps_apdo_max_voltage(snk));
> -		port->pps_data.req_max_curr = min_pps_apdo_current(src, snk);
> -		port->pps_data.req_out_volt = min(port->pps_data.req_max_volt,
> -						  max(port->pps_data.req_min_volt,
> -						      port->pps_data.req_out_volt));
> +
> +		port->pps_data.req_min_volt = pdo_pps_apdo_min_voltage(src);
> +		port->pps_data.req_max_volt = pdo_pps_apdo_max_voltage(src);
> +		port->pps_data.req_max_curr = pdo_pps_apdo_max_current(src);
>   		port->pps_data.req_op_curr = min(port->pps_data.req_max_curr,
>   						 port->pps_data.req_op_curr);
>   	}
> @@ -3464,32 +3410,16 @@ static int tcpm_pd_send_request(struct tcpm_port *port)
>   static int tcpm_pd_build_pps_request(struct tcpm_port *port, u32 *rdo)
>   {
>   	unsigned int out_mv, op_ma, op_mw, max_mv, max_ma, flags;
> -	enum pd_pdo_type type;
>   	unsigned int src_pdo_index;
> -	u32 pdo;
>   
>   	src_pdo_index = tcpm_pd_select_pps_apdo(port);
>   	if (!src_pdo_index)
>   		return -EOPNOTSUPP;
>   
> -	pdo = port->source_caps[src_pdo_index];
> -	type = pdo_type(pdo);
> -
> -	switch (type) {
> -	case PDO_TYPE_APDO:
> -		if (pdo_apdo_type(pdo) != APDO_TYPE_PPS) {
> -			tcpm_log(port, "Invalid APDO selected!");
> -			return -EINVAL;
> -		}
> -		max_mv = port->pps_data.req_max_volt;
> -		max_ma = port->pps_data.req_max_curr;
> -		out_mv = port->pps_data.req_out_volt;
> -		op_ma = port->pps_data.req_op_curr;
> -		break;
> -	default:
> -		tcpm_log(port, "Invalid PDO selected!");
> -		return -EINVAL;
> -	}
> +	max_mv = port->pps_data.req_max_volt;
> +	max_ma = port->pps_data.req_max_curr;
> +	out_mv = port->pps_data.req_out_volt;
> +	op_ma = port->pps_data.req_op_curr;
>   
>   	flags = RDO_USB_COMM | RDO_NO_SUSPEND;
>   
> @@ -5882,12 +5812,6 @@ static int tcpm_pps_set_out_volt(struct tcpm_port *port, u16 req_out_volt)
>   		goto port_unlock;
>   	}
>   
> -	if (req_out_volt < port->pps_data.min_volt ||
> -	    req_out_volt > port->pps_data.max_volt) {
> -		ret = -EINVAL;
> -		goto port_unlock;
> -	}
> -
>   	target_mw = (port->current_limit * req_out_volt) / 1000;
>   	if (target_mw < port->operating_snk_mw) {
>   		ret = -EINVAL;
> @@ -6440,11 +6364,7 @@ static int tcpm_psy_set_prop(struct power_supply *psy,
>   		ret = tcpm_psy_set_online(port, val);
>   		break;
>   	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> -		if (val->intval < port->pps_data.min_volt * 1000 ||
> -		    val->intval > port->pps_data.max_volt * 1000)
> -			ret = -EINVAL;
> -		else
> -			ret = tcpm_pps_set_out_volt(port, val->intval / 1000);
> +		ret = tcpm_pps_set_out_volt(port, val->intval / 1000);
>   		break;
>   	case POWER_SUPPLY_PROP_CURRENT_NOW:
>   		if (val->intval > port->pps_data.max_curr * 1000)