Re: [PATCH v2 1/3] usb: typec: Determine common SVDM Versions

On Mon, Feb 1, 2021 at 12:21 AM Guenter Roeck [off-list ref] wrote:

On 1/31/21 7:18 AM, Kyle Tso wrote:

quoted

Changes since v1:
- removed the "local" variables (svdm_version) in tcpm.c and
  (altmodes/ucsi)/displayport.c
- added a member "svdm_version" in struct typec_capabilities indicating
  the default SVDM version of the port
- added a member "common_svdm_ver" in struct typec_port indicating the
  common SVDM version between the port and the partner

I personally find the "common" in the variable and function names unnecessary.
I would prefer using something like svdm_version instead of common_svdm_ver.

The reason for the common_ prefix is just for the readability.
That's totally fine with me to remove the prefix.
Will fix this in the next version.

quoted

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 0afd8ef692e8..403a483645dd 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c

@@ -1470,11 +1470,13 @@ static void tcpm_register_partner_altmodes(struct tcpm_port *port)
 }

 #define supports_modal(port) PD_IDH_MODAL_SUPP((port)->partner_ident.id_header)
+#define common_svdm_ver(typec)       (typec_get_common_svdm_version(typec))

I think that is unnecessary and confusing. We now have typec_get_common_svdm_version()
as well as common_svdm_ver() and COMMON_SVDM_VER() macros. I would suggest to just use
the function name (and maybe drop the 'common_' prefix from it).

will fix this in the next version.

quoted

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index ca3f4194ad90..b8d693cc7b77 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c

@@ -764,6 +764,7 @@ static void ucsi_handle_connector_change(struct work_struct *work)

      if (con->status.change & UCSI_CONSTAT_CONNECT_CHANGE) {
              typec_set_pwr_role(con->port, role);
+             typec_set_common_svdm_version(con->port, con->typec_cap.svdm_version);

I am a bit concerned that svdm_version is added to typec_capabilities but not
consistently used by all drivers registering with typec. I am not sure I
understand if and how the value in typec_capabilities is used by the typec core.

I am not sure about it as well :p
From my POV, that is just the same nature as the "pd_revision" is in
typec_capabilities which means the capabilities the port has
regardless of the port partners.
The port needs to reset the operating mode to it's designed SVDM
version upon detach. I think typec_capabilities is a good place to
store this information.
What do you think?

BTW, the reset value of the variable "negotiated_rev" in tcpm/tcpm.c
looks weird to me.
It is reset to "PD_MAX_REV" in SNK_STARTUP and SRC_STARTUP.
However, the tcpm.c might not always support the max revision of PD.
IMO, the pd_revision in typec_capabilities is a better choice compared
to PD_MAX_REV.

quoted

diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
index 54475323f83b..df0cb1e595a1 100644
--- a/include/linux/usb/typec.h
+++ b/include/linux/usb/typec.h

@@ -206,12 +206,19 @@ struct typec_operations {
                           enum typec_port_type type);
 };

+enum usb_pd_svdm_ver {
+     SVDM_VER_1_0 = 0,
+     SVDM_VER_2_0 = 1,
+     SVDM_VER_MAX = SVDM_VER_2_0,
+};
+
 /*
  * struct typec_capability - USB Type-C Port Capabilities
  * @type: Supported power role of the port
  * @data: Supported data role of the port
  * @revision: USB Type-C Specification release. Binary coded decimal
  * @pd_revision: USB Power Delivery Specification revision if supported
+ * @svdm_version: USB PD Structured VDM version if supported
  * @prefer_role: Initial role preference (DRP ports).
  * @accessory: Supported Accessory Modes
  * @fwnode: Optional fwnode of the port

@@ -225,6 +232,7 @@ struct typec_capability {
      enum typec_port_data    data;
      u16                     revision; /* 0120H = "1.2" */
      u16                     pd_revision; /* 0300H = "3.0" */
+     enum usb_pd_svdm_ver    svdm_version;
      int                     prefer_role;
      enum typec_accessory    accessory[TYPEC_MAX_ACCESSORY];
      unsigned int            orientation_aware:1;

@@ -275,4 +283,6 @@ int typec_find_orientation(const char *name);
 int typec_find_port_power_role(const char *name);
 int typec_find_power_role(const char *name);
 int typec_find_port_data_role(const char *name);
+void typec_set_common_svdm_version(struct typec_port *port, enum usb_pd_svdm_ver);
+enum usb_pd_svdm_ver typec_get_common_svdm_version(struct typec_port *port);
 #endif /* __LINUX_USB_TYPEC_H */

Thanks,
Kyle