| Message ID | 20240408-ucsi-orient-aware-v1-4-95a74a163a10@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: typec: ucsi: glink: rework orientation handling | expand |
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,
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
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
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(+)