[PATCH 0/5] usb: typec: ucsi: glink: rework orientation handling

[PATCH 0/5] usb: typec: ucsi: glink: rework orientation handling

[Dmitry Baryshkov]

Simplify the way the UCSI GLINK driver handles cable orientation. Make
the UCSI core responsible for pinging the driver to get cable status.
Use typec-port API instead of calling typec_switch_set() directly.
Also make the orientation status available via the sysfs.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
Dmitry Baryshkov (5):
      usb: typec: ucsi: add callback for connector status updates
      usb: typec: ucsi: glink: move GPIO reading into connector_status callback
      usb: typec: ucsi: glink: use typec_set_orientation
      usb: typec: ucsi: make it orientation-aware
      usb: typec: ucsi: glink: set orientation aware if supported

 drivers/usb/typec/ucsi/ucsi.c       |  8 ++++++
 drivers/usb/typec/ucsi/ucsi.h       |  4 +++
 drivers/usb/typec/ucsi/ucsi_glink.c | 55 +++++++++++++++----------------------
 3 files changed, 34 insertions(+), 33 deletions(-)
---
base-commit: a7636ecc2a798cf6dfcfe5c993be9deedceb1888
change-id: 20240408-ucsi-orient-aware-9643f2653301

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

[PATCH 1/5] usb: typec: ucsi: add callback for connector status updates

[Dmitry Baryshkov]

Allow UCSI glue driver to perform addtional work to update connector
status. For example, it might check the cable orientation.  This call is
performed after reading new connector statatus, so the platform driver
can peek at new connection status bits.

The callback is called both when registering the port and when the
connector change event is being handled.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/usb/typec/ucsi/ucsi.c | 6 ++++++
 drivers/usb/typec/ucsi/ucsi.h | 3 +++
 2 files changed, 9 insertions(+)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 3106e69050cd..7ad544c968e4 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -1199,6 +1199,9 @@ static void ucsi_handle_connector_change(struct work_struct *work)
 
 	trace_ucsi_connector_change(con->num, &con->status);
 
+	if (ucsi->ops->connector_status)
+		ucsi->ops->connector_status(con);
+
 	role = !!(con->status.flags & UCSI_CONSTAT_PWR_DIR);
 
 	if (con->status.change & UCSI_CONSTAT_POWER_DIR_CHANGE) {
@@ -1588,6 +1591,9 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con)
 	}
 	ret = 0; /* ucsi_send_command() returns length on success */
 
+	if (ucsi->ops->connector_status)
+		ucsi->ops->connector_status(con);
+
 	switch (UCSI_CONSTAT_PARTNER_TYPE(con->status.flags)) {
 	case UCSI_CONSTAT_PARTNER_TYPE_UFP:
 	case UCSI_CONSTAT_PARTNER_TYPE_CABLE_AND_UFP:
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index 2caf2969668c..6599fbd09bee 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -53,12 +53,14 @@ struct dentry;
 #define UCSI_CCI_ERROR			BIT(30)
 #define UCSI_CCI_COMMAND_COMPLETE	BIT(31)
 
+struct ucsi_connector;
 /**
  * struct ucsi_operations - UCSI I/O operations
  * @read: Read operation
  * @sync_write: Blocking write operation
  * @async_write: Non-blocking write operation
  * @update_altmodes: Squashes duplicate DP altmodes
+ * @connector_status: Updates connector status, called holding connector lock
  *
  * Read and write routines for UCSI interface. @sync_write must wait for the
  * Command Completion Event from the PPM before returning, and @async_write must
@@ -73,6 +75,7 @@ struct ucsi_operations {
 			   const void *val, size_t val_len);
 	bool (*update_altmodes)(struct ucsi *ucsi, struct ucsi_altmode *orig,
 				struct ucsi_altmode *updated);
+	void (*connector_status)(struct ucsi_connector *con);
 };
 
 struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops);

-- 
2.39.2

Re: [PATCH 1/5] usb: typec: ucsi: add callback for connector status updates

[Heikki Krogerus]

On Mon, Apr 08, 2024 at 07:30:49AM +0300, Dmitry Baryshkov wrote:
> Allow UCSI glue driver to perform addtional work to update connector
> status. For example, it might check the cable orientation.  This call is
> performed after reading new connector statatus, so the platform driver
> can peek at new connection status bits.
> 
> The callback is called both when registering the port and when the
> connector change event is being handled.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/usb/typec/ucsi/ucsi.c | 6 ++++++
>  drivers/usb/typec/ucsi/ucsi.h | 3 +++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 3106e69050cd..7ad544c968e4 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -1199,6 +1199,9 @@ static void ucsi_handle_connector_change(struct work_struct *work)
>  
>  	trace_ucsi_connector_change(con->num, &con->status);
>  
> +	if (ucsi->ops->connector_status)
> +		ucsi->ops->connector_status(con);
> +
>  	role = !!(con->status.flags & UCSI_CONSTAT_PWR_DIR);
>  
>  	if (con->status.change & UCSI_CONSTAT_POWER_DIR_CHANGE) {
> @@ -1588,6 +1591,9 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con)
>  	}
>  	ret = 0; /* ucsi_send_command() returns length on success */
>  
> +	if (ucsi->ops->connector_status)
> +		ucsi->ops->connector_status(con);
> +
>  	switch (UCSI_CONSTAT_PARTNER_TYPE(con->status.flags)) {
>  	case UCSI_CONSTAT_PARTNER_TYPE_UFP:
>  	case UCSI_CONSTAT_PARTNER_TYPE_CABLE_AND_UFP:
> diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> index 2caf2969668c..6599fbd09bee 100644
> --- a/drivers/usb/typec/ucsi/ucsi.h
> +++ b/drivers/usb/typec/ucsi/ucsi.h
> @@ -53,12 +53,14 @@ struct dentry;
>  #define UCSI_CCI_ERROR			BIT(30)
>  #define UCSI_CCI_COMMAND_COMPLETE	BIT(31)
>  
> +struct ucsi_connector;

Let's keep the forward declarations in one place. Please move that to
the beginning of the file where the other forward declarations are.

thanks,

>  /**
>   * struct ucsi_operations - UCSI I/O operations
>   * @read: Read operation
>   * @sync_write: Blocking write operation
>   * @async_write: Non-blocking write operation
>   * @update_altmodes: Squashes duplicate DP altmodes
> + * @connector_status: Updates connector status, called holding connector lock
>   *
>   * Read and write routines for UCSI interface. @sync_write must wait for the
>   * Command Completion Event from the PPM before returning, and @async_write must
> @@ -73,6 +75,7 @@ struct ucsi_operations {
>  			   const void *val, size_t val_len);
>  	bool (*update_altmodes)(struct ucsi *ucsi, struct ucsi_altmode *orig,
>  				struct ucsi_altmode *updated);
> +	void (*connector_status)(struct ucsi_connector *con);
>  };
>  
>  struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops);

-- 
heikki

[PATCH 2/5] usb: typec: ucsi: glink: move GPIO reading into connector_status callback

[Dmitry Baryshkov]

To simplify the platform code move Type-C orientation handling into the
connector_status callback. As it is called both during connector
registration and on connector change events, duplicated code from
pmic_glink_ucsi_register() can be dropped.

Also this moves operations that can sleep into a worker thread,
removing the only sleeping operation from pmic_glink_ucsi_notify().

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/usb/typec/ucsi/ucsi_glink.c | 48 ++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 28 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi_glink.c b/drivers/usb/typec/ucsi/ucsi_glink.c
index b91d2d15d7d9..d21f8cd2fe35 100644
--- a/drivers/usb/typec/ucsi/ucsi_glink.c
+++ b/drivers/usb/typec/ucsi/ucsi_glink.c
@@ -187,10 +187,28 @@ static int pmic_glink_ucsi_sync_write(struct ucsi *__ucsi, unsigned int offset,
 	return ret;
 }
 
+static void pmic_glink_ucsi_connector_status(struct ucsi_connector *con)
+{
+	struct pmic_glink_ucsi *ucsi = ucsi_get_drvdata(con->ucsi);
+	int orientation;
+
+	if (con->num >= PMIC_GLINK_MAX_PORTS ||
+	    !ucsi->port_orientation[con->num - 1])
+		return;
+
+	orientation = gpiod_get_value(ucsi->port_orientation[con->num - 1]);
+	if (orientation >= 0) {
+		typec_switch_set(ucsi->port_switch[con->num - 1],
+				 orientation ? TYPEC_ORIENTATION_REVERSE
+				 : TYPEC_ORIENTATION_NORMAL);
+	}
+}
+
 static const struct ucsi_operations pmic_glink_ucsi_ops = {
 	.read = pmic_glink_ucsi_read,
 	.sync_write = pmic_glink_ucsi_sync_write,
-	.async_write = pmic_glink_ucsi_async_write
+	.async_write = pmic_glink_ucsi_async_write,
+	.connector_status = pmic_glink_ucsi_connector_status,
 };
 
 static void pmic_glink_ucsi_read_ack(struct pmic_glink_ucsi *ucsi, const void *data, int len)
@@ -229,20 +247,8 @@ static void pmic_glink_ucsi_notify(struct work_struct *work)
 	}
 
 	con_num = UCSI_CCI_CONNECTOR(cci);
-	if (con_num) {
-		if (con_num <= PMIC_GLINK_MAX_PORTS &&
-		    ucsi->port_orientation[con_num - 1]) {
-			int orientation = gpiod_get_value(ucsi->port_orientation[con_num - 1]);
-
-			if (orientation >= 0) {
-				typec_switch_set(ucsi->port_switch[con_num - 1],
-						 orientation ? TYPEC_ORIENTATION_REVERSE
-							     : TYPEC_ORIENTATION_NORMAL);
-			}
-		}
-
+	if (con_num)
 		ucsi_connector_change(ucsi->ucsi, con_num);
-	}
 
 	if (ucsi->sync_pending &&
 		   (cci & (UCSI_CCI_ACK_COMPLETE | UCSI_CCI_COMMAND_COMPLETE))) {
@@ -253,20 +259,6 @@ static void pmic_glink_ucsi_notify(struct work_struct *work)
 static void pmic_glink_ucsi_register(struct work_struct *work)
 {
 	struct pmic_glink_ucsi *ucsi = container_of(work, struct pmic_glink_ucsi, register_work);
-	int orientation;
-	int i;
-
-	for (i = 0; i < PMIC_GLINK_MAX_PORTS; i++) {
-		if (!ucsi->port_orientation[i])
-			continue;
-		orientation = gpiod_get_value(ucsi->port_orientation[i]);
-
-		if (orientation >= 0) {
-			typec_switch_set(ucsi->port_switch[i],
-					 orientation ? TYPEC_ORIENTATION_REVERSE
-					     : TYPEC_ORIENTATION_NORMAL);
-		}
-	}
 
 	ucsi_register(ucsi->ucsi);
 }

-- 
2.39.2

Re: [PATCH 2/5] usb: typec: ucsi: glink: move GPIO reading into connector_status callback

[Krishna Kurapati PSSNV]

On 4/8/2024 10:00 AM, Dmitry Baryshkov wrote:
> To simplify the platform code move Type-C orientation handling into the
> connector_status callback. As it is called both during connector
> registration and on connector change events, duplicated code from
> pmic_glink_ucsi_register() can be dropped.
> 
> Also this moves operations that can sleep into a worker thread,
> removing the only sleeping operation from pmic_glink_ucsi_notify().
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Verified working of device mode (ADB) in SS in both orientations on 
QCS6490 RB3Gen2.

As an experiment, removed the connector status call being done in 
ucsi_register_port to see if the issue fixed by [1] would be 
reproducible or not and it is. So this patch is taking care of proper 
enumeration in bootup in host mode in reverse orientation as well.

Tested-by: Krishna Kurapati <quic_kriskura@quicinc.com>

[1]: 
https://lore.kernel.org/all/20240301040914.458492-1-quic_kriskura@quicinc.com/

> ---
>   drivers/usb/typec/ucsi/ucsi_glink.c | 48 ++++++++++++++++---------------------
>   1 file changed, 20 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi_glink.c b/drivers/usb/typec/ucsi/ucsi_glink.c
> index b91d2d15d7d9..d21f8cd2fe35 100644
> --- a/drivers/usb/typec/ucsi/ucsi_glink.c
> +++ b/drivers/usb/typec/ucsi/ucsi_glink.c
> @@ -187,10 +187,28 @@ static int pmic_glink_ucsi_sync_write(struct ucsi *__ucsi, unsigned int offset,
>   	return ret;
>   }
>   
> +static void pmic_glink_ucsi_connector_status(struct ucsi_connector *con)
> +{
> +	struct pmic_glink_ucsi *ucsi = ucsi_get_drvdata(con->ucsi);
> +	int orientation;
> +
> +	if (con->num >= PMIC_GLINK_MAX_PORTS ||
> +	    !ucsi->port_orientation[con->num - 1])
> +		return;
> +
> +	orientation = gpiod_get_value(ucsi->port_orientation[con->num - 1]);
> +	if (orientation >= 0) {
> +		typec_switch_set(ucsi->port_switch[con->num - 1],
> +				 orientation ? TYPEC_ORIENTATION_REVERSE
> +				 : TYPEC_ORIENTATION_NORMAL);
> +	}
> +}
> +
>   static const struct ucsi_operations pmic_glink_ucsi_ops = {
>   	.read = pmic_glink_ucsi_read,
>   	.sync_write = pmic_glink_ucsi_sync_write,
> -	.async_write = pmic_glink_ucsi_async_write
> +	.async_write = pmic_glink_ucsi_async_write,
> +	.connector_status = pmic_glink_ucsi_connector_status,
>   };
>   
>   static void pmic_glink_ucsi_read_ack(struct pmic_glink_ucsi *ucsi, const void *data, int len)
> @@ -229,20 +247,8 @@ static void pmic_glink_ucsi_notify(struct work_struct *work)
>   	}
>   
>   	con_num = UCSI_CCI_CONNECTOR(cci);
> -	if (con_num) {
> -		if (con_num <= PMIC_GLINK_MAX_PORTS &&
> -		    ucsi->port_orientation[con_num - 1]) {
> -			int orientation = gpiod_get_value(ucsi->port_orientation[con_num - 1]);
> -
> -			if (orientation >= 0) {
> -				typec_switch_set(ucsi->port_switch[con_num - 1],
> -						 orientation ? TYPEC_ORIENTATION_REVERSE
> -							     : TYPEC_ORIENTATION_NORMAL);
> -			}
> -		}
> -
> +	if (con_num)
>   		ucsi_connector_change(ucsi->ucsi, con_num);
> -	}
>   
>   	if (ucsi->sync_pending &&
>   		   (cci & (UCSI_CCI_ACK_COMPLETE | UCSI_CCI_COMMAND_COMPLETE))) {
> @@ -253,20 +259,6 @@ static void pmic_glink_ucsi_notify(struct work_struct *work)
>   static void pmic_glink_ucsi_register(struct work_struct *work)
>   {
>   	struct pmic_glink_ucsi *ucsi = container_of(work, struct pmic_glink_ucsi, register_work);
> -	int orientation;
> -	int i;
> -
> -	for (i = 0; i < PMIC_GLINK_MAX_PORTS; i++) {
> -		if (!ucsi->port_orientation[i])
> -			continue;
> -		orientation = gpiod_get_value(ucsi->port_orientation[i]);
> -
> -		if (orientation >= 0) {
> -			typec_switch_set(ucsi->port_switch[i],
> -					 orientation ? TYPEC_ORIENTATION_REVERSE
> -					     : TYPEC_ORIENTATION_NORMAL);
> -		}
> -	}
>   
>   	ucsi_register(ucsi->ucsi);
>   }
> 

[PATCH 3/5] usb: typec: ucsi: glink: use typec_set_orientation

[Dmitry Baryshkov]

Use typec_set_orientation() instead of calling typec_switch_set()
manually. This way the rest of the typec framework and the userspace are
notified about the orientation change.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/usb/typec/ucsi/ucsi_glink.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi_glink.c b/drivers/usb/typec/ucsi/ucsi_glink.c
index d21f8cd2fe35..d279e2cf9bba 100644
--- a/drivers/usb/typec/ucsi/ucsi_glink.c
+++ b/drivers/usb/typec/ucsi/ucsi_glink.c
@@ -58,7 +58,6 @@ struct pmic_glink_ucsi {
 	struct device *dev;
 
 	struct gpio_desc *port_orientation[PMIC_GLINK_MAX_PORTS];
-	struct typec_switch *port_switch[PMIC_GLINK_MAX_PORTS];
 
 	struct pmic_glink_client *client;
 
@@ -198,9 +197,10 @@ static void pmic_glink_ucsi_connector_status(struct ucsi_connector *con)
 
 	orientation = gpiod_get_value(ucsi->port_orientation[con->num - 1]);
 	if (orientation >= 0) {
-		typec_switch_set(ucsi->port_switch[con->num - 1],
-				 orientation ? TYPEC_ORIENTATION_REVERSE
-				 : TYPEC_ORIENTATION_NORMAL);
+		typec_set_orientation(con->port,
+				      orientation ?
+				      TYPEC_ORIENTATION_REVERSE :
+				      TYPEC_ORIENTATION_NORMAL);
 	}
 }
 
@@ -378,11 +378,6 @@ static int pmic_glink_ucsi_probe(struct auxiliary_device *adev,
 			return dev_err_probe(dev, PTR_ERR(desc),
 					     "unable to acquire orientation gpio\n");
 		ucsi->port_orientation[port] = desc;
-
-		ucsi->port_switch[port] = fwnode_typec_switch_get(fwnode);
-		if (IS_ERR(ucsi->port_switch[port]))
-			return dev_err_probe(dev, PTR_ERR(ucsi->port_switch[port]),
-					"failed to acquire orientation-switch\n");
 	}
 
 	ucsi->client = devm_pmic_glink_register_client(dev,

-- 
2.39.2

[PATCH 4/5] usb: typec: ucsi: make it orientation-aware

[Dmitry Baryshkov]

The UCSI 1.0 is not orientation aware. Only UCSI 2.0 has added
orientation status to GET_CONNECTOR_STATUS data. However the glue code
can be able to detect cable orientation on its own (and report it via
corresponding typec API). Add a flag to let UCSI mark registered ports
as orientation aware.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/usb/typec/ucsi/ucsi.c | 2 ++
 drivers/usb/typec/ucsi/ucsi.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 7ad544c968e4..6f5adc335980 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -1551,6 +1551,8 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con)
 	cap->svdm_version = SVDM_VER_2_0;
 	cap->prefer_role = TYPEC_NO_PREFERRED_ROLE;
 
+	cap->orientation_aware = !!(ucsi->quirks & UCSI_ORIENTATION_AWARE);
+
 	if (con->cap.op_mode & UCSI_CONCAP_OPMODE_AUDIO_ACCESSORY)
 		*accessory++ = TYPEC_ACCESSORY_AUDIO;
 	if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY)
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index 6599fbd09bee..e92be45e4c1c 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -410,6 +410,7 @@ struct ucsi {
 	unsigned long quirks;
 #define UCSI_NO_PARTNER_PDOS	BIT(0)	/* Don't read partner's PDOs */
 #define UCSI_DELAY_DEVICE_PDOS	BIT(1)	/* Reading PDOs fails until the parter is in PD mode */
+#define UCSI_ORIENTATION_AWARE	BIT(2)	/* UCSI is orientation aware */
 };
 
 #define UCSI_MAX_SVID		5

-- 
2.39.2

Re: [PATCH 4/5] usb: typec: ucsi: make it orientation-aware

[Heikki Krogerus]

Hi Dmitry,

On Mon, Apr 08, 2024 at 07:30:52AM +0300, Dmitry Baryshkov wrote:
> The UCSI 1.0 is not orientation aware. Only UCSI 2.0 has added
> orientation status to GET_CONNECTOR_STATUS data. However the glue code
> can be able to detect cable orientation on its own (and report it via
> corresponding typec API). Add a flag to let UCSI mark registered ports
> as orientation aware.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/usb/typec/ucsi/ucsi.c | 2 ++
>  drivers/usb/typec/ucsi/ucsi.h | 1 +
>  2 files changed, 3 insertions(+)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 7ad544c968e4..6f5adc335980 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -1551,6 +1551,8 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con)
>  	cap->svdm_version = SVDM_VER_2_0;
>  	cap->prefer_role = TYPEC_NO_PREFERRED_ROLE;
>  
> +	cap->orientation_aware = !!(ucsi->quirks & UCSI_ORIENTATION_AWARE);
> +
>  	if (con->cap.op_mode & UCSI_CONCAP_OPMODE_AUDIO_ACCESSORY)
>  		*accessory++ = TYPEC_ACCESSORY_AUDIO;
>  	if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY)
> diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> index 6599fbd09bee..e92be45e4c1c 100644
> --- a/drivers/usb/typec/ucsi/ucsi.h
> +++ b/drivers/usb/typec/ucsi/ucsi.h
> @@ -410,6 +410,7 @@ struct ucsi {
>  	unsigned long quirks;
>  #define UCSI_NO_PARTNER_PDOS	BIT(0)	/* Don't read partner's PDOs */
>  #define UCSI_DELAY_DEVICE_PDOS	BIT(1)	/* Reading PDOs fails until the parter is in PD mode */
> +#define UCSI_ORIENTATION_AWARE	BIT(2)	/* UCSI is orientation aware */

You are not using that flag anywhere in this series. But why would
orientation need a "quirk" in the first place?

I'm not sure where you are going with this, but please try to avoid
the quirk flags in general in this driver rather than considering them
as the first way of solving things. Use them only as the last resort.

I don't want this driver to end up like xhci and some other drivers,
where refactoring is almost impossible because every place is full of
conditions checking the quirks, and where in worst case a quirk meant
to solve a problem on one hardware causes problems on another.

thanks,

-- 
heikki

Re: [PATCH 4/5] usb: typec: ucsi: make it orientation-aware

[Dmitry Baryshkov]

On Tue, 9 Apr 2024 at 11:05, Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> Hi Dmitry,
>
> On Mon, Apr 08, 2024 at 07:30:52AM +0300, Dmitry Baryshkov wrote:
> > The UCSI 1.0 is not orientation aware. Only UCSI 2.0 has added
> > orientation status to GET_CONNECTOR_STATUS data. However the glue code
> > can be able to detect cable orientation on its own (and report it via
> > corresponding typec API). Add a flag to let UCSI mark registered ports
> > as orientation aware.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  drivers/usb/typec/ucsi/ucsi.c | 2 ++
> >  drivers/usb/typec/ucsi/ucsi.h | 1 +
> >  2 files changed, 3 insertions(+)
> >
> > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> > index 7ad544c968e4..6f5adc335980 100644
> > --- a/drivers/usb/typec/ucsi/ucsi.c
> > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > @@ -1551,6 +1551,8 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con)
> >       cap->svdm_version = SVDM_VER_2_0;
> >       cap->prefer_role = TYPEC_NO_PREFERRED_ROLE;
> >
> > +     cap->orientation_aware = !!(ucsi->quirks & UCSI_ORIENTATION_AWARE);
> > +
> >       if (con->cap.op_mode & UCSI_CONCAP_OPMODE_AUDIO_ACCESSORY)
> >               *accessory++ = TYPEC_ACCESSORY_AUDIO;
> >       if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY)
> > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> > index 6599fbd09bee..e92be45e4c1c 100644
> > --- a/drivers/usb/typec/ucsi/ucsi.h
> > +++ b/drivers/usb/typec/ucsi/ucsi.h
> > @@ -410,6 +410,7 @@ struct ucsi {
> >       unsigned long quirks;
> >  #define UCSI_NO_PARTNER_PDOS BIT(0)  /* Don't read partner's PDOs */
> >  #define UCSI_DELAY_DEVICE_PDOS       BIT(1)  /* Reading PDOs fails until the parter is in PD mode */
> > +#define UCSI_ORIENTATION_AWARE       BIT(2)  /* UCSI is orientation aware */
>
> You are not using that flag anywhere in this series. But why would
> orientation need a "quirk" in the first place?

Patch 5 sets this flag.

> I'm not sure where you are going with this, but please try to avoid
> the quirk flags in general in this driver rather than considering them
> as the first way of solving things. Use them only as the last resort.
>
> I don't want this driver to end up like xhci and some other drivers,
> where refactoring is almost impossible because every place is full of
> conditions checking the quirks, and where in worst case a quirk meant
> to solve a problem on one hardware causes problems on another.

Enabling the orientation_aware flag in the capabilities enables the
`class/typec/portN/orientation` attribute to be visible. This way
userspace (and more importantly the developer) can detect in which way
the cable is plugged. We have had several issues with the driver
mis-detecting the orientation and having the valid orientation
attribute helped us a lot.

Note, the UCSI 1.x doesn't report orientation at all. So by default
the UCSI driver isn't orientation aware, it doesn't call
typec_set_orientation(), etc. UCSI 2.0 introduced the orientation flag
in the GET_CONNECTOR_STATUS data, but currently the driver just
ignores it. If we enable orientation_aware by default this will result
in the useless 'unknown' value for that attribute. I think this is
what Badhri tried to evade when he introduced the orientation_aware
flag.

We can probably get the same result by adding the port_capabilities()
callback to be called just before ucsi_register_port_psy() and
typec_register_port(). Would it be better from your point of view?

--
With best wishes
Dmitry

[PATCH 5/5] usb: typec: ucsi: glink: set orientation aware if supported

[Dmitry Baryshkov]

If the PMIC-GLINK device has orientation GPIOs declared, then it will
report connection orientation. In this case set the flag to mark
registered ports as orientation-aware.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/usb/typec/ucsi/ucsi_glink.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/typec/ucsi/ucsi_glink.c b/drivers/usb/typec/ucsi/ucsi_glink.c
index d279e2cf9bba..87a8b5f88da4 100644
--- a/drivers/usb/typec/ucsi/ucsi_glink.c
+++ b/drivers/usb/typec/ucsi/ucsi_glink.c
@@ -378,6 +378,8 @@ static int pmic_glink_ucsi_probe(struct auxiliary_device *adev,
 			return dev_err_probe(dev, PTR_ERR(desc),
 					     "unable to acquire orientation gpio\n");
 		ucsi->port_orientation[port] = desc;
+
+		ucsi->ucsi->quirks |= UCSI_ORIENTATION_AWARE;
 	}
 
 	ucsi->client = devm_pmic_glink_register_client(dev,

-- 
2.39.2