On Wed, Feb 03, 2021 at 02:47:24PM +0200, Heikki Krogerus wrote:
> Hi Kyle,
>
> On Wed, Feb 03, 2021 at 12:17:26AM +0800, Kyle Tso wrote:
> > PD Spec Revision 3.0 Version 2.0 + ECNs 2020-12-10
> > 6.4.4.2.3 Structured VDM Version
> > "The Structured VDM Version field of the Discover Identity Command
> > sent and received during VDM discovery Shall be used to determine the
> > lowest common Structured VDM Version supported by the Port Partners or
> > Cable Plug and Shall continue to operate using this Specification
> > Revision until they are Detached."
> >
> > Add a variable in typec_capability to specify the highest SVDM version
> > supported by the port and another variable in typec_port to cache the
> > negotiated SVDM version between the port partners.
> >
> > Also add setter/getter functions for the negotiated SVDM version.
> >
> > Signed-off-by: Kyle Tso <kyletso@xxxxxxxxxx>
> > ---
> > drivers/usb/typec/class.c | 13 +++++++++++++
> > include/linux/usb/typec.h | 10 ++++++++++
> > 2 files changed, 23 insertions(+)
> >
> > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> > index b6ceab3dc16b..42d1be1eece9 100644
> > --- a/drivers/usb/typec/class.c
> > +++ b/drivers/usb/typec/class.c
> > @@ -51,6 +51,7 @@ struct typec_port {
> > enum typec_role vconn_role;
> > enum typec_pwr_opmode pwr_opmode;
> > enum typec_port_type port_type;
> > + enum usb_pd_svdm_ver svdm_version;
> > struct mutex port_type_lock;
>
> I just realized that you are storing that in the port object. I guess
> we don't have to change this right now, but it would have been more
> clear to store that in the partner object IMO.
>
> > enum typec_orientation orientation;
> > @@ -1841,6 +1842,18 @@ int typec_find_port_data_role(const char *name)
> > }
> > EXPORT_SYMBOL_GPL(typec_find_port_data_role);
> >
> > +void typec_set_svdm_version(struct typec_port *port, enum usb_pd_svdm_ver ver)
> > +{
> > + port->svdm_version = ver;
> > +}
> > +EXPORT_SYMBOL_GPL(typec_set_svdm_version);
> > +
> > +enum usb_pd_svdm_ver typec_get_svdm_version(struct typec_port *port)
> > +{
> > + return port->svdm_version;
> > +}
> > +EXPORT_SYMBOL_GPL(typec_get_svdm_version);
>
> You need to document those exported functions! You need to do that in
> any case, but in this case it's very important, because the purpose of
> these functions is not clear from the ctx.
Thinking about it, would it make make sense to define the functions as
static inline ?
Thanks,
Guenter
>
> I'm sorry for noticing that so late. Since you do need to fix that,
> please see if you can also store that detail in the partner device
> object instead of the port object.
>
> thanks,
>
> --
> heikki
