Re: [PATCH v5 1/8] usb: typec: Manage SVDM version

From: Guenter Roeck <linux@roeck-us.net>
To: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Kyle Tso <kyletso@google.com>,
	gregkh@linuxfoundation.org, hdegoede@redhat.com,
	robh+dt@kernel.org, badhri@google.com, linux-usb@vger.kernel.org,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH v5 1/8] usb: typec: Manage SVDM version
Date: Wed, 3 Feb 2021 06:51:43 -0800	[thread overview]
Message-ID: <20210203145143.GA58095@roeck-us.net> (raw)
In-Reply-To: <20210203124724.GD1687065@kuha.fi.intel.com>

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@google.com>
> > ---
> >  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