Re: [PATCH v2] usb: typec: tcpm: Use configured PD revision for negotiation

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

On Tue, May 13, 2025 at 09:08:34PM +0800, Cosmo Chou wrote:
> Initialize negotiated_rev and negotiated_rev_prime based on the port's
> configured PD revision (rev_major) rather than always defaulting to
> PD_MAX_REV. This ensures ports start PD communication using their
> appropriate revision level.
> 
> This allows proper communication with devices that require specific
> PD revision levels, especially for the hardware designed for PD 1.0
> or 2.0 specifications.
> 
> Signed-off-by: Cosmo Chou <chou.cosmo@gmail.com>
> ---
> Change log:
> 
> v2:
>   - Add PD_CAP_REVXX macros and use switch-case for better readability.
> 
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 29 +++++++++++++++++++++++++----
>  1 file changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 8adf6f954633..48e9cfc2b49a 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -313,6 +313,10 @@ struct pd_data {
>  	unsigned int operating_snk_mw;
>  };
>  
> +#define PD_CAP_REV10	0x1
> +#define PD_CAP_REV20	0x2
> +#define PD_CAP_REV30	0x3
> +
>  struct pd_revision_info {
>  	u8 rev_major;
>  	u8 rev_minor;
> @@ -4665,6 +4669,25 @@ static void tcpm_set_initial_svdm_version(struct tcpm_port *port)
>  	}
>  }
>  
> +static void tcpm_set_initial_negotiated_rev(struct tcpm_port *port)
> +{
> +	switch (port->pd_rev.rev_major) {
> +	case PD_CAP_REV10:
> +		port->negotiated_rev = PD_REV10;
> +		break;
> +	case PD_CAP_REV20:
> +		port->negotiated_rev = PD_REV20;
> +		break;
> +	case PD_CAP_REV30:
> +		port->negotiated_rev = PD_REV30;
> +		break;
> +	default:
> +		port->negotiated_rev = PD_MAX_REV;
> +		break;
> +	}
> +	port->negotiated_rev_prime = port->negotiated_rev;
> +}

Do we need this? Couldn't you just add one to rev_major?

        port->negotiated_rev = port->pd_rev.rev_major + 1;
        port->negotiated_rev_prime = port->pd_rev.rev_major + 1;

Or am I missing something?

thanks,

>  static void run_state_machine(struct tcpm_port *port)
>  {
>  	int ret;
> @@ -4782,8 +4805,7 @@ static void run_state_machine(struct tcpm_port *port)
>  		typec_set_pwr_opmode(port->typec_port, opmode);
>  		port->pwr_opmode = TYPEC_PWR_MODE_USB;
>  		port->caps_count = 0;
> -		port->negotiated_rev = PD_MAX_REV;
> -		port->negotiated_rev_prime = PD_MAX_REV;
> +		tcpm_set_initial_negotiated_rev(port);
>  		port->message_id = 0;
>  		port->message_id_prime = 0;
>  		port->rx_msgid = -1;
> @@ -5048,8 +5070,7 @@ static void run_state_machine(struct tcpm_port *port)
>  					      port->cc2 : port->cc1);
>  		typec_set_pwr_opmode(port->typec_port, opmode);
>  		port->pwr_opmode = TYPEC_PWR_MODE_USB;
> -		port->negotiated_rev = PD_MAX_REV;
> -		port->negotiated_rev_prime = PD_MAX_REV;
> +		tcpm_set_initial_negotiated_rev(port);
>  		port->message_id = 0;
>  		port->message_id_prime = 0;
>  		port->rx_msgid = -1;
> -- 
> 2.43.0

-- 
heikki