linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] usb: typec: tcpm: Create legacy PDOs for PD2 connection
@ 2021-01-15 16:33 Kyle Tso
  2021-01-21  8:41 ` Heikki Krogerus
  2021-01-23 20:01 ` Guenter Roeck
  0 siblings, 2 replies; 5+ messages in thread
From: Kyle Tso @ 2021-01-15 16:33 UTC (permalink / raw)
  To: linux, heikki.krogerus, gregkh; +Cc: badhri, linux-usb, linux-kernel, Kyle Tso

If the port partner is PD2, the PDOs of the local port should follow the
format defined in PD2 Spec. Dynamically modify the pre-defined PD3 PDOs
and transform them into PD2 format before sending them to the PD2 port
partner.

Signed-off-by: Kyle Tso <kyletso@google.com>
---
 drivers/usb/typec/tcpm/tcpm.c | 62 +++++++++++++++++++++++++++++------
 include/linux/usb/pd.h        |  1 +
 2 files changed, 53 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 22a85b396f69..1220ab1ed47d 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -911,13 +911,47 @@ static int tcpm_set_pwr_role(struct tcpm_port *port, enum typec_role role)
 	return 0;
 }
 
+/*
+ * Transform the PDO to be compliant to PD rev2.0.
+ * Return 0 if the PDO type is not defined in PD rev2.0.
+ * Otherwise, return the converted PDO.
+ */
+static u32 tcpm_forge_legacy_pdo(struct tcpm_port *port, u32 pdo, enum typec_role role)
+{
+	switch (pdo_type(pdo)) {
+	case PDO_TYPE_FIXED:
+		if (role == TYPEC_SINK)
+			return pdo & ~PDO_FIXED_FRS_CURR_MASK;
+		else
+			return pdo & ~PDO_FIXED_UNCHUNK_EXT;
+	case PDO_TYPE_VAR:
+	case PDO_TYPE_BATT:
+		return pdo;
+	case PDO_TYPE_APDO:
+	default:
+		return 0;
+	}
+}
+
 static int tcpm_pd_send_source_caps(struct tcpm_port *port)
 {
 	struct pd_message msg;
-	int i;
+	u32 pdo;
+	unsigned int i, nr_pdo = 0;
 
 	memset(&msg, 0, sizeof(msg));
-	if (!port->nr_src_pdo) {
+
+	for (i = 0; i < port->nr_src_pdo; i++) {
+		if (port->negotiated_rev >= PD_REV30) {
+			msg.payload[nr_pdo++] =	cpu_to_le32(port->src_pdo[i]);
+		} else {
+			pdo = tcpm_forge_legacy_pdo(port, port->src_pdo[i], TYPEC_SOURCE);
+			if (pdo)
+				msg.payload[nr_pdo++] = cpu_to_le32(pdo);
+		}
+	}
+
+	if (!nr_pdo) {
 		/* No source capabilities defined, sink only */
 		msg.header = PD_HEADER_LE(PD_CTRL_REJECT,
 					  port->pwr_role,
@@ -930,10 +964,8 @@ static int tcpm_pd_send_source_caps(struct tcpm_port *port)
 					  port->data_role,
 					  port->negotiated_rev,
 					  port->message_id,
-					  port->nr_src_pdo);
+					  nr_pdo);
 	}
-	for (i = 0; i < port->nr_src_pdo; i++)
-		msg.payload[i] = cpu_to_le32(port->src_pdo[i]);
 
 	return tcpm_pd_transmit(port, TCPC_TX_SOP, &msg);
 }
@@ -941,10 +973,22 @@ static int tcpm_pd_send_source_caps(struct tcpm_port *port)
 static int tcpm_pd_send_sink_caps(struct tcpm_port *port)
 {
 	struct pd_message msg;
-	int i;
+	u32 pdo;
+	unsigned int i, nr_pdo = 0;
 
 	memset(&msg, 0, sizeof(msg));
-	if (!port->nr_snk_pdo) {
+
+	for (i = 0; i < port->nr_snk_pdo; i++) {
+		if (port->negotiated_rev >= PD_REV30) {
+			msg.payload[nr_pdo++] =	cpu_to_le32(port->snk_pdo[i]);
+		} else {
+			pdo = tcpm_forge_legacy_pdo(port, port->snk_pdo[i], TYPEC_SINK);
+			if (pdo)
+				msg.payload[nr_pdo++] = cpu_to_le32(pdo);
+		}
+	}
+
+	if (!nr_pdo) {
 		/* No sink capabilities defined, source only */
 		msg.header = PD_HEADER_LE(PD_CTRL_REJECT,
 					  port->pwr_role,
@@ -957,10 +1001,8 @@ static int tcpm_pd_send_sink_caps(struct tcpm_port *port)
 					  port->data_role,
 					  port->negotiated_rev,
 					  port->message_id,
-					  port->nr_snk_pdo);
+					  nr_pdo);
 	}
-	for (i = 0; i < port->nr_snk_pdo; i++)
-		msg.payload[i] = cpu_to_le32(port->snk_pdo[i]);
 
 	return tcpm_pd_transmit(port, TCPC_TX_SOP, &msg);
 }
diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h
index bb9a782e1411..88f64bce5dea 100644
--- a/include/linux/usb/pd.h
+++ b/include/linux/usb/pd.h
@@ -225,6 +225,7 @@ enum pd_pdo_type {
 #define PDO_FIXED_EXTPOWER		BIT(27) /* Externally powered */
 #define PDO_FIXED_USB_COMM		BIT(26) /* USB communications capable */
 #define PDO_FIXED_DATA_SWAP		BIT(25) /* Data role swap supported */
+#define PDO_FIXED_UNCHUNK_EXT		BIT(24) /* Unchunked Extended Message supported (Source) */
 #define PDO_FIXED_FRS_CURR_MASK		(BIT(24) | BIT(23)) /* FR_Swap Current (Sink) */
 #define PDO_FIXED_FRS_CURR_SHIFT	23
 #define PDO_FIXED_VOLT_SHIFT		10	/* 50mV units */
-- 
2.30.0.284.gd98b1dd5eaa7-goog


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] usb: typec: tcpm: Create legacy PDOs for PD2 connection
  2021-01-15 16:33 [PATCH] usb: typec: tcpm: Create legacy PDOs for PD2 connection Kyle Tso
@ 2021-01-21  8:41 ` Heikki Krogerus
  2021-01-21  9:48   ` Kyle Tso
  2021-01-23 20:01 ` Guenter Roeck
  1 sibling, 1 reply; 5+ messages in thread
From: Heikki Krogerus @ 2021-01-21  8:41 UTC (permalink / raw)
  To: Kyle Tso; +Cc: linux, gregkh, badhri, linux-usb, linux-kernel

Hi Kyle,

On Sat, Jan 16, 2021 at 12:33:11AM +0800, Kyle Tso wrote:
> If the port partner is PD2, the PDOs of the local port should follow the
> format defined in PD2 Spec. Dynamically modify the pre-defined PD3 PDOs
> and transform them into PD2 format before sending them to the PD2 port
> partner.

I guess it's not possible for the system to supply separate static
PDOs for each PD revision?

> Signed-off-by: Kyle Tso <kyletso@google.com>
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 62 +++++++++++++++++++++++++++++------
>  include/linux/usb/pd.h        |  1 +
>  2 files changed, 53 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 22a85b396f69..1220ab1ed47d 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -911,13 +911,47 @@ static int tcpm_set_pwr_role(struct tcpm_port *port, enum typec_role role)
>  	return 0;
>  }
>  
> +/*
> + * Transform the PDO to be compliant to PD rev2.0.
> + * Return 0 if the PDO type is not defined in PD rev2.0.
> + * Otherwise, return the converted PDO.
> + */
> +static u32 tcpm_forge_legacy_pdo(struct tcpm_port *port, u32 pdo, enum typec_role role)
> +{
> +	switch (pdo_type(pdo)) {
> +	case PDO_TYPE_FIXED:
> +		if (role == TYPEC_SINK)
> +			return pdo & ~PDO_FIXED_FRS_CURR_MASK;
> +		else
> +			return pdo & ~PDO_FIXED_UNCHUNK_EXT;
> +	case PDO_TYPE_VAR:
> +	case PDO_TYPE_BATT:
> +		return pdo;
> +	case PDO_TYPE_APDO:
> +	default:
> +		return 0;
> +	}
> +}
> +
>  static int tcpm_pd_send_source_caps(struct tcpm_port *port)
>  {
>  	struct pd_message msg;
> -	int i;
> +	u32 pdo;
> +	unsigned int i, nr_pdo = 0;

Side note. I think this driver uses the "reverse christmas tree"
style with the variables.

>  	memset(&msg, 0, sizeof(msg));
> -	if (!port->nr_src_pdo) {
> +
> +	for (i = 0; i < port->nr_src_pdo; i++) {
> +		if (port->negotiated_rev >= PD_REV30) {
> +			msg.payload[nr_pdo++] =	cpu_to_le32(port->src_pdo[i]);
> +		} else {
> +			pdo = tcpm_forge_legacy_pdo(port, port->src_pdo[i], TYPEC_SOURCE);
> +			if (pdo)
> +				msg.payload[nr_pdo++] = cpu_to_le32(pdo);
> +		}
> +	}

Why not just generate the PD_REV20 PDOs during probe time and then
just use the ones that fit the negotiated_rev here?

> +
> +	if (!nr_pdo) {
>  		/* No source capabilities defined, sink only */
>  		msg.header = PD_HEADER_LE(PD_CTRL_REJECT,
>  					  port->pwr_role,
> @@ -930,10 +964,8 @@ static int tcpm_pd_send_source_caps(struct tcpm_port *port)
>  					  port->data_role,
>  					  port->negotiated_rev,
>  					  port->message_id,
> -					  port->nr_src_pdo);
> +					  nr_pdo);
>  	}
> -	for (i = 0; i < port->nr_src_pdo; i++)
> -		msg.payload[i] = cpu_to_le32(port->src_pdo[i]);
>  
>  	return tcpm_pd_transmit(port, TCPC_TX_SOP, &msg);
>  }
> @@ -941,10 +973,22 @@ static int tcpm_pd_send_source_caps(struct tcpm_port *port)
>  static int tcpm_pd_send_sink_caps(struct tcpm_port *port)
>  {
>  	struct pd_message msg;
> -	int i;
> +	u32 pdo;
> +	unsigned int i, nr_pdo = 0;
>  
>  	memset(&msg, 0, sizeof(msg));
> -	if (!port->nr_snk_pdo) {
> +
> +	for (i = 0; i < port->nr_snk_pdo; i++) {
> +		if (port->negotiated_rev >= PD_REV30) {
> +			msg.payload[nr_pdo++] =	cpu_to_le32(port->snk_pdo[i]);
> +		} else {
> +			pdo = tcpm_forge_legacy_pdo(port, port->snk_pdo[i], TYPEC_SINK);
> +			if (pdo)
> +				msg.payload[nr_pdo++] = cpu_to_le32(pdo);
> +		}
> +	}

ditto.

> +	if (!nr_pdo) {
>  		/* No sink capabilities defined, source only */
>  		msg.header = PD_HEADER_LE(PD_CTRL_REJECT,
>  					  port->pwr_role,
> @@ -957,10 +1001,8 @@ static int tcpm_pd_send_sink_caps(struct tcpm_port *port)
>  					  port->data_role,
>  					  port->negotiated_rev,
>  					  port->message_id,
> -					  port->nr_snk_pdo);
> +					  nr_pdo);
>  	}
> -	for (i = 0; i < port->nr_snk_pdo; i++)
> -		msg.payload[i] = cpu_to_le32(port->snk_pdo[i]);
>  
>  	return tcpm_pd_transmit(port, TCPC_TX_SOP, &msg);
>  }
> diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h
> index bb9a782e1411..88f64bce5dea 100644
> --- a/include/linux/usb/pd.h
> +++ b/include/linux/usb/pd.h
> @@ -225,6 +225,7 @@ enum pd_pdo_type {
>  #define PDO_FIXED_EXTPOWER		BIT(27) /* Externally powered */
>  #define PDO_FIXED_USB_COMM		BIT(26) /* USB communications capable */
>  #define PDO_FIXED_DATA_SWAP		BIT(25) /* Data role swap supported */
> +#define PDO_FIXED_UNCHUNK_EXT		BIT(24) /* Unchunked Extended Message supported (Source) */
>  #define PDO_FIXED_FRS_CURR_MASK		(BIT(24) | BIT(23)) /* FR_Swap Current (Sink) */
>  #define PDO_FIXED_FRS_CURR_SHIFT	23
>  #define PDO_FIXED_VOLT_SHIFT		10	/* 50mV units */

thanks,

-- 
heikki

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] usb: typec: tcpm: Create legacy PDOs for PD2 connection
  2021-01-21  8:41 ` Heikki Krogerus
@ 2021-01-21  9:48   ` Kyle Tso
  2021-01-25  9:21     ` Heikki Krogerus
  0 siblings, 1 reply; 5+ messages in thread
From: Kyle Tso @ 2021-01-21  9:48 UTC (permalink / raw)
  To: Heikki Krogerus; +Cc: Guenter Roeck, Greg KH, Badhri Jagan Sridharan, USB, LKML

On Thu, Jan 21, 2021 at 4:41 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> Hi Kyle,
>
> On Sat, Jan 16, 2021 at 12:33:11AM +0800, Kyle Tso wrote:
> > If the port partner is PD2, the PDOs of the local port should follow the
> > format defined in PD2 Spec. Dynamically modify the pre-defined PD3 PDOs
> > and transform them into PD2 format before sending them to the PD2 port
> > partner.
>
> I guess it's not possible for the system to supply separate static
> PDOs for each PD revision?
>
We can do that for sure. But a problem is that if there are more PD
revisions in the future, we will need to add more PDO arrays.
For backward compatibility, the new revision usually uses the
previously-reserved bits for the new features.
From my perspective, the better way to achieve the backward
compatibility is to just clear the bits if those are reserved in the
previous revision.

I can submit another patch which adds another PDO array for PD2 if you
think it is more appropriate.

> > Signed-off-by: Kyle Tso <kyletso@google.com>
> > ---
> >  drivers/usb/typec/tcpm/tcpm.c | 62 +++++++++++++++++++++++++++++------
> >  include/linux/usb/pd.h        |  1 +
> >  2 files changed, 53 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> > index 22a85b396f69..1220ab1ed47d 100644
> > --- a/drivers/usb/typec/tcpm/tcpm.c
> > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > @@ -911,13 +911,47 @@ static int tcpm_set_pwr_role(struct tcpm_port *port, enum typec_role role)
> >       return 0;
> >  }
> >
> > +/*
> > + * Transform the PDO to be compliant to PD rev2.0.
> > + * Return 0 if the PDO type is not defined in PD rev2.0.
> > + * Otherwise, return the converted PDO.
> > + */
> > +static u32 tcpm_forge_legacy_pdo(struct tcpm_port *port, u32 pdo, enum typec_role role)
> > +{
> > +     switch (pdo_type(pdo)) {
> > +     case PDO_TYPE_FIXED:
> > +             if (role == TYPEC_SINK)
> > +                     return pdo & ~PDO_FIXED_FRS_CURR_MASK;
> > +             else
> > +                     return pdo & ~PDO_FIXED_UNCHUNK_EXT;
> > +     case PDO_TYPE_VAR:
> > +     case PDO_TYPE_BATT:
> > +             return pdo;
> > +     case PDO_TYPE_APDO:
> > +     default:
> > +             return 0;
> > +     }
> > +}
> > +
> >  static int tcpm_pd_send_source_caps(struct tcpm_port *port)
> >  {
> >       struct pd_message msg;
> > -     int i;
> > +     u32 pdo;
> > +     unsigned int i, nr_pdo = 0;
>
> Side note. I think this driver uses the "reverse christmas tree"
> style with the variables.

I will change the order (if there is a next version)

thanks,
Kyle

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] usb: typec: tcpm: Create legacy PDOs for PD2 connection
  2021-01-15 16:33 [PATCH] usb: typec: tcpm: Create legacy PDOs for PD2 connection Kyle Tso
  2021-01-21  8:41 ` Heikki Krogerus
@ 2021-01-23 20:01 ` Guenter Roeck
  1 sibling, 0 replies; 5+ messages in thread
From: Guenter Roeck @ 2021-01-23 20:01 UTC (permalink / raw)
  To: Kyle Tso, heikki.krogerus, gregkh; +Cc: badhri, linux-usb, linux-kernel

On 1/15/21 8:33 AM, Kyle Tso wrote:
> If the port partner is PD2, the PDOs of the local port should follow the
> format defined in PD2 Spec. Dynamically modify the pre-defined PD3 PDOs
> and transform them into PD2 format before sending them to the PD2 port
> partner.
> 
> Signed-off-by: Kyle Tso <kyletso@google.com>

I don't like this too much, but I don't have a better idea. Thus,

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

Guenter

> ---
>  drivers/usb/typec/tcpm/tcpm.c | 62 +++++++++++++++++++++++++++++------
>  include/linux/usb/pd.h        |  1 +
>  2 files changed, 53 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 22a85b396f69..1220ab1ed47d 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -911,13 +911,47 @@ static int tcpm_set_pwr_role(struct tcpm_port *port, enum typec_role role)
>  	return 0;
>  }
>  
> +/*
> + * Transform the PDO to be compliant to PD rev2.0.
> + * Return 0 if the PDO type is not defined in PD rev2.0.
> + * Otherwise, return the converted PDO.
> + */
> +static u32 tcpm_forge_legacy_pdo(struct tcpm_port *port, u32 pdo, enum typec_role role)
> +{
> +	switch (pdo_type(pdo)) {
> +	case PDO_TYPE_FIXED:
> +		if (role == TYPEC_SINK)
> +			return pdo & ~PDO_FIXED_FRS_CURR_MASK;
> +		else
> +			return pdo & ~PDO_FIXED_UNCHUNK_EXT;
> +	case PDO_TYPE_VAR:
> +	case PDO_TYPE_BATT:
> +		return pdo;
> +	case PDO_TYPE_APDO:
> +	default:
> +		return 0;
> +	}
> +}
> +
>  static int tcpm_pd_send_source_caps(struct tcpm_port *port)
>  {
>  	struct pd_message msg;
> -	int i;
> +	u32 pdo;
> +	unsigned int i, nr_pdo = 0;
>  
>  	memset(&msg, 0, sizeof(msg));
> -	if (!port->nr_src_pdo) {
> +
> +	for (i = 0; i < port->nr_src_pdo; i++) {
> +		if (port->negotiated_rev >= PD_REV30) {
> +			msg.payload[nr_pdo++] =	cpu_to_le32(port->src_pdo[i]);
> +		} else {
> +			pdo = tcpm_forge_legacy_pdo(port, port->src_pdo[i], TYPEC_SOURCE);
> +			if (pdo)
> +				msg.payload[nr_pdo++] = cpu_to_le32(pdo);
> +		}
> +	}
> +
> +	if (!nr_pdo) {
>  		/* No source capabilities defined, sink only */
>  		msg.header = PD_HEADER_LE(PD_CTRL_REJECT,
>  					  port->pwr_role,
> @@ -930,10 +964,8 @@ static int tcpm_pd_send_source_caps(struct tcpm_port *port)
>  					  port->data_role,
>  					  port->negotiated_rev,
>  					  port->message_id,
> -					  port->nr_src_pdo);
> +					  nr_pdo);
>  	}
> -	for (i = 0; i < port->nr_src_pdo; i++)
> -		msg.payload[i] = cpu_to_le32(port->src_pdo[i]);
>  
>  	return tcpm_pd_transmit(port, TCPC_TX_SOP, &msg);
>  }
> @@ -941,10 +973,22 @@ static int tcpm_pd_send_source_caps(struct tcpm_port *port)
>  static int tcpm_pd_send_sink_caps(struct tcpm_port *port)
>  {
>  	struct pd_message msg;
> -	int i;
> +	u32 pdo;
> +	unsigned int i, nr_pdo = 0;
>  
>  	memset(&msg, 0, sizeof(msg));
> -	if (!port->nr_snk_pdo) {
> +
> +	for (i = 0; i < port->nr_snk_pdo; i++) {
> +		if (port->negotiated_rev >= PD_REV30) {
> +			msg.payload[nr_pdo++] =	cpu_to_le32(port->snk_pdo[i]);
> +		} else {
> +			pdo = tcpm_forge_legacy_pdo(port, port->snk_pdo[i], TYPEC_SINK);
> +			if (pdo)
> +				msg.payload[nr_pdo++] = cpu_to_le32(pdo);
> +		}
> +	}
> +
> +	if (!nr_pdo) {
>  		/* No sink capabilities defined, source only */
>  		msg.header = PD_HEADER_LE(PD_CTRL_REJECT,
>  					  port->pwr_role,
> @@ -957,10 +1001,8 @@ static int tcpm_pd_send_sink_caps(struct tcpm_port *port)
>  					  port->data_role,
>  					  port->negotiated_rev,
>  					  port->message_id,
> -					  port->nr_snk_pdo);
> +					  nr_pdo);
>  	}
> -	for (i = 0; i < port->nr_snk_pdo; i++)
> -		msg.payload[i] = cpu_to_le32(port->snk_pdo[i]);
>  
>  	return tcpm_pd_transmit(port, TCPC_TX_SOP, &msg);
>  }
> diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h
> index bb9a782e1411..88f64bce5dea 100644
> --- a/include/linux/usb/pd.h
> +++ b/include/linux/usb/pd.h
> @@ -225,6 +225,7 @@ enum pd_pdo_type {
>  #define PDO_FIXED_EXTPOWER		BIT(27) /* Externally powered */
>  #define PDO_FIXED_USB_COMM		BIT(26) /* USB communications capable */
>  #define PDO_FIXED_DATA_SWAP		BIT(25) /* Data role swap supported */
> +#define PDO_FIXED_UNCHUNK_EXT		BIT(24) /* Unchunked Extended Message supported (Source) */
>  #define PDO_FIXED_FRS_CURR_MASK		(BIT(24) | BIT(23)) /* FR_Swap Current (Sink) */
>  #define PDO_FIXED_FRS_CURR_SHIFT	23
>  #define PDO_FIXED_VOLT_SHIFT		10	/* 50mV units */
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] usb: typec: tcpm: Create legacy PDOs for PD2 connection
  2021-01-21  9:48   ` Kyle Tso
@ 2021-01-25  9:21     ` Heikki Krogerus
  0 siblings, 0 replies; 5+ messages in thread
From: Heikki Krogerus @ 2021-01-25  9:21 UTC (permalink / raw)
  To: Kyle Tso; +Cc: Guenter Roeck, Greg KH, Badhri Jagan Sridharan, USB, LKML

Hi,

On Thu, Jan 21, 2021 at 05:48:46PM +0800, Kyle Tso wrote:
> On Thu, Jan 21, 2021 at 4:41 PM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > Hi Kyle,
> >
> > On Sat, Jan 16, 2021 at 12:33:11AM +0800, Kyle Tso wrote:
> > > If the port partner is PD2, the PDOs of the local port should follow the
> > > format defined in PD2 Spec. Dynamically modify the pre-defined PD3 PDOs
> > > and transform them into PD2 format before sending them to the PD2 port
> > > partner.
> >
> > I guess it's not possible for the system to supply separate static
> > PDOs for each PD revision?
> >
> We can do that for sure. But a problem is that if there are more PD
> revisions in the future, we will need to add more PDO arrays.
> For backward compatibility, the new revision usually uses the
> previously-reserved bits for the new features.
> >From my perspective, the better way to achieve the backward
> compatibility is to just clear the bits if those are reserved in the
> previous revision.

I was trying to think of something better for this, but I got nothing.
I'm not completely comfortable with this, but never mind. Let's just go
with this.

> I can submit another patch which adds another PDO array for PD2 if you
> think it is more appropriate.

Not necessary.

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> > > Signed-off-by: Kyle Tso <kyletso@google.com>
> > > ---
> > >  drivers/usb/typec/tcpm/tcpm.c | 62 +++++++++++++++++++++++++++++------
> > >  include/linux/usb/pd.h        |  1 +
> > >  2 files changed, 53 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> > > index 22a85b396f69..1220ab1ed47d 100644
> > > --- a/drivers/usb/typec/tcpm/tcpm.c
> > > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > > @@ -911,13 +911,47 @@ static int tcpm_set_pwr_role(struct tcpm_port *port, enum typec_role role)
> > >       return 0;
> > >  }
> > >
> > > +/*
> > > + * Transform the PDO to be compliant to PD rev2.0.
> > > + * Return 0 if the PDO type is not defined in PD rev2.0.
> > > + * Otherwise, return the converted PDO.
> > > + */
> > > +static u32 tcpm_forge_legacy_pdo(struct tcpm_port *port, u32 pdo, enum typec_role role)
> > > +{
> > > +     switch (pdo_type(pdo)) {
> > > +     case PDO_TYPE_FIXED:
> > > +             if (role == TYPEC_SINK)
> > > +                     return pdo & ~PDO_FIXED_FRS_CURR_MASK;
> > > +             else
> > > +                     return pdo & ~PDO_FIXED_UNCHUNK_EXT;
> > > +     case PDO_TYPE_VAR:
> > > +     case PDO_TYPE_BATT:
> > > +             return pdo;
> > > +     case PDO_TYPE_APDO:
> > > +     default:
> > > +             return 0;
> > > +     }
> > > +}
> > > +
> > >  static int tcpm_pd_send_source_caps(struct tcpm_port *port)
> > >  {
> > >       struct pd_message msg;
> > > -     int i;
> > > +     u32 pdo;
> > > +     unsigned int i, nr_pdo = 0;
> >
> > Side note. I think this driver uses the "reverse christmas tree"
> > style with the variables.
> 
> I will change the order (if there is a next version)
> 
> thanks,
> Kyle

-- 
heikki

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-01-26 19:36 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-15 16:33 [PATCH] usb: typec: tcpm: Create legacy PDOs for PD2 connection Kyle Tso
2021-01-21  8:41 ` Heikki Krogerus
2021-01-21  9:48   ` Kyle Tso
2021-01-25  9:21     ` Heikki Krogerus
2021-01-23 20:01 ` Guenter Roeck

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).