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 <[email protected]>
---
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 <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
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 <[email protected]>
[1]:
https://lore.kernel.org/all/[email protected]/
> ---
> 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);
> }
>
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
On Tue, 9 Apr 2024 at 11:05, Heikki Krogerus
<[email protected]> 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 <[email protected]>
> > ---
> > 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