Re: [PATCH v5 1/8] usb: typec: Manage SVDM version

[Heikki Krogerus]

Hi Kyle,

On Wed, Feb 03, 2021 at 12:17:26AM +0800, Kyle Tso wrote:
> PD Spec Revision 3.0 Version 2.0 + ECNs 2020-12-10
>   6.4.4.2.3 Structured VDM Version
>   "The Structured VDM Version field of the Discover Identity Command
>   sent and received during VDM discovery Shall be used to determine the
>   lowest common Structured VDM Version supported by the Port Partners or
>   Cable Plug and Shall continue to operate using this Specification
>   Revision until they are Detached."
> 
> Add a variable in typec_capability to specify the highest SVDM version
> supported by the port and another variable in typec_port to cache the
> negotiated SVDM version between the port partners.
> 
> Also add setter/getter functions for the negotiated SVDM version.
> 
> Signed-off-by: Kyle Tso <kyletso@xxxxxxxxxx>
> ---
>  drivers/usb/typec/class.c | 13 +++++++++++++
>  include/linux/usb/typec.h | 10 ++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index b6ceab3dc16b..42d1be1eece9 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -51,6 +51,7 @@ struct typec_port {
>  	enum typec_role			vconn_role;
>  	enum typec_pwr_opmode		pwr_opmode;
>  	enum typec_port_type		port_type;
> +	enum usb_pd_svdm_ver		svdm_version;
>  	struct mutex			port_type_lock;

I just realized that you are storing that in the port object. I guess
we don't have to change this right now, but it would have been more
clear to store that in the partner object IMO.

>  	enum typec_orientation		orientation;
> @@ -1841,6 +1842,18 @@ int typec_find_port_data_role(const char *name)
>  }
>  EXPORT_SYMBOL_GPL(typec_find_port_data_role);
>  
> +void typec_set_svdm_version(struct typec_port *port, enum usb_pd_svdm_ver ver)
> +{
> +	port->svdm_version = ver;
> +}
> +EXPORT_SYMBOL_GPL(typec_set_svdm_version);
> +
> +enum usb_pd_svdm_ver typec_get_svdm_version(struct typec_port *port)
> +{
> +	return port->svdm_version;
> +}
> +EXPORT_SYMBOL_GPL(typec_get_svdm_version);

You need to document those exported functions! You need to do that in
any case, but in this case it's very important, because the purpose of
these functions is not clear from the ctx.

I'm sorry for noticing that so late. Since you do need to fix that,
please see if you can also store that detail in the partner device
object instead of the port object.

thanks,

-- 
heikki