On Wed, Aug 28, 2024 at 9:15 AM Heikki Krogerus
<heikki.krogerus@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> ---
> 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).
