Re: [PATCH v6] usb: typec: ucsi: add support for separate DP altmode devices

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

[-- Attachment #1: Type: text/plain, Size: 2161 bytes --]

On Thu, Dec 12, 2019 at 05:42:28PM +0000, Ajay Gupta wrote:
> Hi Heikki,
> 
> > -----Original Message-----
> > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Sent: Thursday, December 12, 2019 5:44 AM
> > To: Ajay Gupta <ajaykuee@gmail.com>
> > Cc: linux-usb@vger.kernel.org; Ajay Gupta <ajayg@nvidia.com>
> > Subject: Re: [PATCH v6] usb: typec: ucsi: add support for separate DP altmode
> > devices
> > 
> > 
> > Hi Ajay,
> > 
> > On Fri, Nov 22, 2019 at 04:43:47PM -0800, Ajay Gupta wrote:
> > > From: Ajay Gupta <ajayg@nvidia.com>
> > >
> > > CCGx controller used on NVIDIA GPU card has two separate display
> > > altmode for two DP pin assignments. UCSI specification doesn't
> > > prohibits using separate display altmode.
> > >
> > > Current UCSI Type-C framework expects only one display altmode for all
> > > DP pin assignment. This patch squashes two separate display altmode
> > > into single altmode to support controllers with separate display
> > > altmode. We first read all the alternate modes of connector and then
> > > run through it to know if there are separate display altmodes. If so,
> > > it prepares a new port altmode set after squashing two or more
> > > separate altmodes into one.
> > 
> > I didn't see any major issues with this. There were still few extra spaces etc.,
> > but I can clean those. Maybe it would have been good to mention in the
> > commit message that the reason why we need those two separate alt modes,
> > for what is in reality two separate pin configurations, is limitations in UCSI
> > specification, but never mind.
> > 
> > I still don't like the approach, but since I'm unable to explain my idea, or have
> > time to write something for this myself, I don't want block this any longer. It
> > does not add that much code, so once I have time, I can always try to improve
> > it myself, right?
> > 
> > Otherwise it's OK by me. I'll queue it up.
> Thanks for reviewing. Please feel free to improve the patch or commit message.

One thing that I do want to do is I want to isolate the changes done
to ucsi.c. Can you test the attached diff? It applies on top of this
patch.

thanks,

-- 
heikki

[-- Attachment #2: isolate_the_quirk.diff --]
[-- Type: text/plain, Size: 4491 bytes --]

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index f028658d1b1a..2bab97a07031 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -318,29 +318,22 @@ static int ucsi_register_altmode(struct ucsi_connector *con,
 	return ret;
 }
 
-static int ucsi_register_altmodes(struct ucsi_connector *con, u8 recipient)
+static int
+ucsi_register_altmodes_nvidia(struct ucsi_connector *con, u8 recipient)
 {
 	int max_altmodes = UCSI_MAX_ALTMODES;
 	struct typec_altmode_desc desc;
-	struct ucsi_altmode alt[2];
+	struct ucsi_altmode alt;
 	struct ucsi_altmode orig[UCSI_MAX_ALTMODES];
 	struct ucsi_altmode updated[UCSI_MAX_ALTMODES];
 	struct ucsi *ucsi = con->ucsi;
 	bool multi_dp = false;
 	u64 command;
-	int num = 1;
 	int ret;
 	int len;
-	int j;
 	int i;
 	int k = 0;
 
-	if (!(con->ucsi->cap.features & UCSI_CAP_ALT_MODE_DETAILS))
-		return 0;
-
-	if (recipient == UCSI_RECIPIENT_SOP && con->partner_altmode[0])
-		return 0;
-
 	if (recipient == UCSI_RECIPIENT_CON)
 		max_altmodes = con->ucsi->cap.num_alt_modes;
 
@@ -348,13 +341,13 @@ static int ucsi_register_altmodes(struct ucsi_connector *con, u8 recipient)
 	memset(updated, 0, sizeof(updated));
 
 	/* First get all the alternate modes */
-	for (i = 0; i < max_altmodes;) {
-		memset(alt, 0, sizeof(alt));
+	for (i = 0; i < max_altmodes; i++) {
+		memset(&alt, 0, sizeof(alt));
 		command = UCSI_GET_ALTERNATE_MODES;
 		command |= UCSI_GET_ALTMODE_RECIPIENT(recipient);
 		command |= UCSI_GET_ALTMODE_CONNECTOR_NUMBER(con->num);
 		command |= UCSI_GET_ALTMODE_OFFSET(i);
-		len = ucsi_run_command(con->ucsi, command, alt, sizeof(alt));
+		len = ucsi_run_command(con->ucsi, command, &alt, sizeof(alt));
 		/*
 		 * We are collecting all altmodes first and then registering.
 		 * Some type-C device will return zero length data beyond last
@@ -367,27 +360,15 @@ static int ucsi_register_altmodes(struct ucsi_connector *con, u8 recipient)
 		if (!len)
 			break;
 
-		/*
-		 * This code is requesting one alt mode at a time, but some PPMs
-		 * may still return two. If that happens both alt modes need be
-		 * saved and the offset for the next alt mode has to be
-		 * incremented.
-		 */
-		num = len / sizeof(alt[0]);
-		i += num;
-
-		for (j = 0; j < num; j++) {
-
-			if (!alt[j].svid) {
-				/* break out of outer loop and register */
-				i = max_altmodes;
-				break;
-			}
-
-			orig[k].mid = alt[j].mid;
-			orig[k].svid = alt[j].svid;
-			k++;
+		if (!alt.svid) {
+			/* break out of outer loop and register */
+			i = max_altmodes;
+			break;
 		}
+
+		orig[k].mid = alt.mid;
+		orig[k].svid = alt.svid;
+		k++;
 	}
 	/*
 	 * Update the original altmode table as some ppms may report
@@ -419,6 +400,67 @@ static int ucsi_register_altmodes(struct ucsi_connector *con, u8 recipient)
 	return 0;
 }
 
+static int ucsi_register_altmodes(struct ucsi_connector *con, u8 recipient)
+{
+	int max_altmodes = UCSI_MAX_ALTMODES;
+	struct typec_altmode_desc desc;
+	struct ucsi_altmode alt[2];
+	u64 command;
+	int num = 1;
+	int ret;
+	int len;
+	int j;
+	int i;
+
+	if (!(con->ucsi->cap.features & UCSI_CAP_ALT_MODE_DETAILS))
+		return 0;
+
+	if (recipient == UCSI_RECIPIENT_SOP && con->partner_altmode[0])
+		return 0;
+
+	if (recipient == UCSI_RECIPIENT_CON)
+		max_altmodes = con->ucsi->cap.num_alt_modes;
+
+	if (con->ucsi->ops->update_altmodes)
+		return ucsi_register_altmodes_nvidia(con, recipient);
+
+	for (i = 0; i < max_altmodes;) {
+		memset(alt, 0, sizeof(alt));
+		command = UCSI_GET_ALTERNATE_MODES;
+		command |= UCSI_GET_ALTMODE_RECIPIENT(recipient);
+		command |= UCSI_GET_ALTMODE_CONNECTOR_NUMBER(con->num);
+		command |= UCSI_GET_ALTMODE_OFFSET(i);
+		len = ucsi_run_command(con->ucsi, command, alt, sizeof(alt));
+		if (len <= 0)
+			return len;
+
+		/*
+		 * This code is requesting one alt mode at a time, but some PPMs
+		 * may still return two. If that happens both alt modes need be
+		 * registered and the offset for the next alt mode has to be
+		 * incremented.
+		 */
+		num = len / sizeof(alt[0]);
+		i += num;
+
+		for (j = 0; j < num; j++) {
+			if (!alt[j].svid)
+				return 0;
+
+			memset(&desc, 0, sizeof(desc));
+			desc.vdo = alt[j].mid;
+			desc.svid = alt[j].svid;
+			desc.roles = TYPEC_PORT_DRD;
+
+			ret = ucsi_register_altmode(con, &desc, recipient);
+			if (ret)
+				return ret;
+		}
+	}
+
+	return 0;
+}
+
 static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient)
 {
 	const struct typec_altmode *pdev;