Re: [RFC PATCH 3/3] usb: typec: ucsi: Helpers for new GET_CONNECTOR_STATUS fields

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

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,

-- 
heikki