| Message ID | 20240828161502.2774996-4-heikki.krogerus@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: typec: ucsi: New way to handle GET_CONNECTOR_STATUS | expand |
> > UCSI v2 introduced new fields to GET_CONNECTOR_STATUS. > Adding a helper for each field. The helpers check that the > UCSI version is v2 or above. I believe this approach helps us from ensuring new fields are only used in supported versions. > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > drivers/usb/typec/ucsi/ucsi.h | 66 +++++++++++++++++++++++++++++++++++ > 1 file changed, 66 insertions(+) > > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h > index dfbc0f6c1b9b..adcbfc96dfcf 100644 > --- a/drivers/usb/typec/ucsi/ucsi.h > +++ b/drivers/usb/typec/ucsi/ucsi.h > @@ -524,4 +524,70 @@ static inline void ucsi_debugfs_unregister(struct ucsi *ucsi) { } > #define USB_TYPEC_NVIDIA_VLINK_DP_VDO 0x1 > #define USB_TYPEC_NVIDIA_VLINK_DBG_VDO 0x3 > > +/* -------------------------------------------------------------------------- */ > + > +static inline enum typec_orientation ucsi_orientation(struct ucsi_connector *con) > +{ > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > + return TYPEC_ORIENTATION_NONE; > + return UCSI_FIELD(con->status, 86, 1) ? TYPEC_ORIENTATION_REVERSE : > + TYPEC_ORIENTATION_NORMAL; > +} > + > +static inline int ucsi_sink_path_status(struct ucsi_connector *con) > +{ > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > + return -EINVAL; > + return UCSI_FIELD(con->status, 87, 1); > +} > + > +static inline int ucsi_reverse_current_protection_status(struct ucsi_connector *con) > +{ > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > + return -EINVAL; > + return UCSI_FIELD(con->status, 88, 1); > +} > + I think the below fields are supported from UCSI v2.1 onwards. Can you please perform version check accordingly. > +static inline int ucsi_power_reading_ready(struct ucsi_connector *con) > +{ > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > + return -EINVAL; > + return UCSI_FIELD(con->status, 89, 1); > +} > + > +static inline int ucsi_current_scale(struct ucsi_connector *con) > +{ > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > + return -EINVAL; > + return UCSI_FIELD(con->status, 90, 3) * 5; /* mA */ > +} > + > +static inline int ucsi_peak_current(struct ucsi_connector *con) > +{ > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > + return -EINVAL; > + return UCSI_FIELD(con->status, 93, 16); > +} > + > +static inline int ucsi_average_current(struct ucsi_connector *con) > +{ > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > + return -EINVAL; > + return UCSI_FIELD(con->status, 109, 16); > +} > + > +static inline int ucsi_voltage_scale(struct ucsi_connector *con) > +{ > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > + return -EINVAL; > + return UCSI_FIELD(con->status, 125, 4) * 5; /* mV */ > +} > + > +static inline int ucsi_voltage_reading(struct ucsi_connector *con) > +{ > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > + return -EINVAL; > + return UCSI_FIELD(con->status, 129, 16); > +} > + > #endif /* __DRIVER_USB_TYPEC_UCSI_H */ > -- > 2.45.2 -Thanks, Siva.
On Wed, Aug 28, 2024 at 11:54:32PM +0000, Pilla, Siva sai kumar wrote: > > > > UCSI v2 introduced new fields to GET_CONNECTOR_STATUS. > > Adding a helper for each field. The helpers check that the > > UCSI version is v2 or above. > > I believe this approach helps us from ensuring new fields are only used in supported versions. > > > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > --- > > drivers/usb/typec/ucsi/ucsi.h | 66 +++++++++++++++++++++++++++++++++++ > > 1 file changed, 66 insertions(+) > > > > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h > > index dfbc0f6c1b9b..adcbfc96dfcf 100644 > > --- a/drivers/usb/typec/ucsi/ucsi.h > > +++ b/drivers/usb/typec/ucsi/ucsi.h > > @@ -524,4 +524,70 @@ static inline void ucsi_debugfs_unregister(struct ucsi *ucsi) { } > > #define USB_TYPEC_NVIDIA_VLINK_DP_VDO 0x1 > > #define USB_TYPEC_NVIDIA_VLINK_DBG_VDO 0x3 > > > > +/* -------------------------------------------------------------------------- */ > > + > > +static inline enum typec_orientation ucsi_orientation(struct ucsi_connector *con) > > +{ > > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > > + return TYPEC_ORIENTATION_NONE; > > + return UCSI_FIELD(con->status, 86, 1) ? TYPEC_ORIENTATION_REVERSE : > > + TYPEC_ORIENTATION_NORMAL; > > +} > > + > > +static inline int ucsi_sink_path_status(struct ucsi_connector *con) > > +{ > > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > > + return -EINVAL; > > + return UCSI_FIELD(con->status, 87, 1); > > +} > > + > > +static inline int ucsi_reverse_current_protection_status(struct ucsi_connector *con) > > +{ > > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > > + return -EINVAL; > > + return UCSI_FIELD(con->status, 88, 1); > > +} > > + > > I think the below fields are supported from UCSI v2.1 onwards. Can you please perform version check accordingly. Good catch! thanks, > > +static inline int ucsi_power_reading_ready(struct ucsi_connector *con) > > +{ > > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > > + return -EINVAL; > > + return UCSI_FIELD(con->status, 89, 1); > > +} > > + > > +static inline int ucsi_current_scale(struct ucsi_connector *con) > > +{ > > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > > + return -EINVAL; > > + return UCSI_FIELD(con->status, 90, 3) * 5; /* mA */ > > +} > > + > > +static inline int ucsi_peak_current(struct ucsi_connector *con) > > +{ > > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > > + return -EINVAL; > > + return UCSI_FIELD(con->status, 93, 16); > > +} > > + > > +static inline int ucsi_average_current(struct ucsi_connector *con) > > +{ > > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > > + return -EINVAL; > > + return UCSI_FIELD(con->status, 109, 16); > > +} > > + > > +static inline int ucsi_voltage_scale(struct ucsi_connector *con) > > +{ > > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > > + return -EINVAL; > > + return UCSI_FIELD(con->status, 125, 4) * 5; /* mV */ > > +} > > + > > +static inline int ucsi_voltage_reading(struct ucsi_connector *con) > > +{ > > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > > + return -EINVAL; > > + return UCSI_FIELD(con->status, 129, 16); > > +} > > + > > #endif /* __DRIVER_USB_TYPEC_UCSI_H */
On Wed, Aug 28, 2024 at 9:15 AM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > UCSI v2 introduced new fields to GET_CONNECTOR_STATUS. > Adding a helper for each field. The helpers check that the > UCSI version is v2 or above. > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > drivers/usb/typec/ucsi/ucsi.h | 66 +++++++++++++++++++++++++++++++++++ > 1 file changed, 66 insertions(+) > > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h > index dfbc0f6c1b9b..adcbfc96dfcf 100644 > --- a/drivers/usb/typec/ucsi/ucsi.h > +++ b/drivers/usb/typec/ucsi/ucsi.h > @@ -524,4 +524,70 @@ static inline void ucsi_debugfs_unregister(struct ucsi *ucsi) { } > #define USB_TYPEC_NVIDIA_VLINK_DP_VDO 0x1 > #define USB_TYPEC_NVIDIA_VLINK_DBG_VDO 0x3 > > +/* -------------------------------------------------------------------------- */ > + > +static inline enum typec_orientation ucsi_orientation(struct ucsi_connector *con) > +{ > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > + return TYPEC_ORIENTATION_NONE; > + return UCSI_FIELD(con->status, 86, 1) ? TYPEC_ORIENTATION_REVERSE : > + TYPEC_ORIENTATION_NORMAL; > +} > + > +static inline int ucsi_sink_path_status(struct ucsi_connector *con) > +{ > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > + return -EINVAL; > + return UCSI_FIELD(con->status, 87, 1); > +} > + > +static inline int ucsi_reverse_current_protection_status(struct ucsi_connector *con) > +{ > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > + return -EINVAL; > + return UCSI_FIELD(con->status, 88, 1); > +} > + > +static inline int ucsi_power_reading_ready(struct ucsi_connector *con) > +{ > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > + return -EINVAL; > + return UCSI_FIELD(con->status, 89, 1); > +} > + > +static inline int ucsi_current_scale(struct ucsi_connector *con) > +{ > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > + return -EINVAL; > + return UCSI_FIELD(con->status, 90, 3) * 5; /* mA */ > +} > + > +static inline int ucsi_peak_current(struct ucsi_connector *con) > +{ > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > + return -EINVAL; > + return UCSI_FIELD(con->status, 93, 16); > +} > + > +static inline int ucsi_average_current(struct ucsi_connector *con) > +{ > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > + return -EINVAL; > + return UCSI_FIELD(con->status, 109, 16); > +} > + > +static inline int ucsi_voltage_scale(struct ucsi_connector *con) > +{ > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > + return -EINVAL; > + return UCSI_FIELD(con->status, 125, 4) * 5; /* mV */ > +} > + > +static inline int ucsi_voltage_reading(struct ucsi_connector *con) > +{ > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > + return -EINVAL; > + return UCSI_FIELD(con->status, 129, 16); > +} > + > #endif /* __DRIVER_USB_TYPEC_UCSI_H */ > -- > 2.45.2 > I think it would be nice to be able to declare what version a field was added when we describe the data structure rather than having to add helper functions for every single access of it. Using the patch I had previously attempted as reference here too: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/5274219 We could add a minimum version to each field definition like so: #define UCSI_CONSTAT_SINK_PATH_STATUS \ UCSI_FIELD_FOR(/*struct=*/ucsi_connector_status, /*min_version=*/UCSI_2_0, /*offset=*/87, /*size=*/1) These can default to WARN_ON + return -EINVAL when the minimum version doesn't match. Pros: Minimum versions are alongside the field definitions, easier to keep track of and enforce. Cons: - WARN may not easily identify which field is failing to access. This could be fixable. - Version check for every field (and not just ones that need it).
On Thu, Aug 29, 2024 at 02:49:34PM -0700, Abhishek Pandit-Subedi wrote: > On Wed, Aug 28, 2024 at 9:15 AM Heikki Krogerus > <heikki.krogerus@linux.intel.com> wrote: > > > > UCSI v2 introduced new fields to GET_CONNECTOR_STATUS. > > Adding a helper for each field. The helpers check that the > > UCSI version is v2 or above. > > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > --- > > drivers/usb/typec/ucsi/ucsi.h | 66 +++++++++++++++++++++++++++++++++++ > > 1 file changed, 66 insertions(+) > > > > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h > > index dfbc0f6c1b9b..adcbfc96dfcf 100644 > > --- a/drivers/usb/typec/ucsi/ucsi.h > > +++ b/drivers/usb/typec/ucsi/ucsi.h > > @@ -524,4 +524,70 @@ static inline void ucsi_debugfs_unregister(struct ucsi *ucsi) { } > > #define USB_TYPEC_NVIDIA_VLINK_DP_VDO 0x1 > > #define USB_TYPEC_NVIDIA_VLINK_DBG_VDO 0x3 > > > > +/* -------------------------------------------------------------------------- */ > > + > > +static inline enum typec_orientation ucsi_orientation(struct ucsi_connector *con) > > +{ > > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > > + return TYPEC_ORIENTATION_NONE; > > + return UCSI_FIELD(con->status, 86, 1) ? TYPEC_ORIENTATION_REVERSE : > > + TYPEC_ORIENTATION_NORMAL; > > +} > > + > > +static inline int ucsi_sink_path_status(struct ucsi_connector *con) > > +{ > > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > > + return -EINVAL; > > + return UCSI_FIELD(con->status, 87, 1); > > +} > > + > > +static inline int ucsi_reverse_current_protection_status(struct ucsi_connector *con) > > +{ > > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > > + return -EINVAL; > > + return UCSI_FIELD(con->status, 88, 1); > > +} > > + > > +static inline int ucsi_power_reading_ready(struct ucsi_connector *con) > > +{ > > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > > + return -EINVAL; > > + return UCSI_FIELD(con->status, 89, 1); > > +} > > + > > +static inline int ucsi_current_scale(struct ucsi_connector *con) > > +{ > > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > > + return -EINVAL; > > + return UCSI_FIELD(con->status, 90, 3) * 5; /* mA */ > > +} > > + > > +static inline int ucsi_peak_current(struct ucsi_connector *con) > > +{ > > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > > + return -EINVAL; > > + return UCSI_FIELD(con->status, 93, 16); > > +} > > + > > +static inline int ucsi_average_current(struct ucsi_connector *con) > > +{ > > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > > + return -EINVAL; > > + return UCSI_FIELD(con->status, 109, 16); > > +} > > + > > +static inline int ucsi_voltage_scale(struct ucsi_connector *con) > > +{ > > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > > + return -EINVAL; > > + return UCSI_FIELD(con->status, 125, 4) * 5; /* mV */ > > +} > > + > > +static inline int ucsi_voltage_reading(struct ucsi_connector *con) > > +{ > > + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) > > + return -EINVAL; > > + return UCSI_FIELD(con->status, 129, 16); > > +} > > + > > #endif /* __DRIVER_USB_TYPEC_UCSI_H */ > > -- > > 2.45.2 > > > > I think it would be nice to be able to declare what version a field > was added when we describe the data structure rather than having to > add helper functions for every single access of it. > > Using the patch I had previously attempted as reference here too: > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/5274219 > > We could add a minimum version to each field definition like so: > > #define UCSI_CONSTAT_SINK_PATH_STATUS \ > UCSI_FIELD_FOR(/*struct=*/ucsi_connector_status, > /*min_version=*/UCSI_2_0, /*offset=*/87, /*size=*/1) > > These can default to WARN_ON + return -EINVAL when the minimum version > doesn't match. > > Pros: Minimum versions are alongside the field definitions, easier to > keep track of and enforce. > Cons: > - WARN may not easily identify which field is failing to access. > This could be fixable. > - Version check for every field (and not just ones that need it). I agree that the helpers are not very good in the end, and yes, the macro needs to check the version for every field. Yesterday I actually already reworked that macro to do exactly that. It does not really need to do anything else - bitmap_read() does the rest. Structures I don't see any use with these. We should use data structures if we could still use bitfields, but since they had to be dropped, structures only make things difficult. I'll send v2 today, or at least try to. There was an internal report about bogus data in sysfs. I found one issues with the pd_revision attribute file that is actually caused by the problem at hand - the Partner PD Revision field in GET_CONNECTOR_CAPABILITY even when it's not supported. I need to fix that at least, but there may be some other problems too. So I will in any case convert also struct ucsi_connector_capability to bitmap while at it in v2. Let's continue the discussion from there. Br,
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h index dfbc0f6c1b9b..adcbfc96dfcf 100644 --- a/drivers/usb/typec/ucsi/ucsi.h +++ b/drivers/usb/typec/ucsi/ucsi.h @@ -524,4 +524,70 @@ static inline void ucsi_debugfs_unregister(struct ucsi *ucsi) { } #define USB_TYPEC_NVIDIA_VLINK_DP_VDO 0x1 #define USB_TYPEC_NVIDIA_VLINK_DBG_VDO 0x3 +/* -------------------------------------------------------------------------- */ + +static inline enum typec_orientation ucsi_orientation(struct ucsi_connector *con) +{ + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) + return TYPEC_ORIENTATION_NONE; + return UCSI_FIELD(con->status, 86, 1) ? TYPEC_ORIENTATION_REVERSE : + TYPEC_ORIENTATION_NORMAL; +} + +static inline int ucsi_sink_path_status(struct ucsi_connector *con) +{ + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) + return -EINVAL; + return UCSI_FIELD(con->status, 87, 1); +} + +static inline int ucsi_reverse_current_protection_status(struct ucsi_connector *con) +{ + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) + return -EINVAL; + return UCSI_FIELD(con->status, 88, 1); +} + +static inline int ucsi_power_reading_ready(struct ucsi_connector *con) +{ + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) + return -EINVAL; + return UCSI_FIELD(con->status, 89, 1); +} + +static inline int ucsi_current_scale(struct ucsi_connector *con) +{ + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) + return -EINVAL; + return UCSI_FIELD(con->status, 90, 3) * 5; /* mA */ +} + +static inline int ucsi_peak_current(struct ucsi_connector *con) +{ + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) + return -EINVAL; + return UCSI_FIELD(con->status, 93, 16); +} + +static inline int ucsi_average_current(struct ucsi_connector *con) +{ + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) + return -EINVAL; + return UCSI_FIELD(con->status, 109, 16); +} + +static inline int ucsi_voltage_scale(struct ucsi_connector *con) +{ + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) + return -EINVAL; + return UCSI_FIELD(con->status, 125, 4) * 5; /* mV */ +} + +static inline int ucsi_voltage_reading(struct ucsi_connector *con) +{ + if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0)) + return -EINVAL; + return UCSI_FIELD(con->status, 129, 16); +} + #endif /* __DRIVER_USB_TYPEC_UCSI_H */
UCSI v2 introduced new fields to GET_CONNECTOR_STATUS. Adding a helper for each field. The helpers check that the UCSI version is v2 or above. Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> --- drivers/usb/typec/ucsi/ucsi.h | 66 +++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+)