| Message ID | 20210202161733.932215-2-kyletso@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | common SVDM version and VDO from dt | expand |
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. 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,
Hi Kyle, On Wed, Feb 03, 2021 at 02:47:28PM +0200, Heikki Krogerus wrote: > 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. > > 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. I'm attaching here my (quite crude) proposal how to do this. It should give you an idea what I'm after here. Br,
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
On Wed, Feb 03, 2021 at 06:51:43AM -0800, Guenter Roeck wrote: > Thinking about it, would it make make sense to define the functions as > static inline ? I (and I believe Guenter too) want to keep these structures protected for now. If the API starts to get too bloated, then I guess I have to reconsider that. But I don't think we are there yet. I have been thinking about moving the USB PD negotiation details to a separate structure that could be more accessible for everybody. That should allow me continue to protect my precious structures. But I have not yet put much though into that. thanks,
On Wed, Feb 03, 2021 at 05:01:26PM +0200, Heikki Krogerus wrote: > On Wed, Feb 03, 2021 at 06:51:43AM -0800, Guenter Roeck wrote: > > Thinking about it, would it make make sense to define the functions as > > static inline ? > > I (and I believe Guenter too) s/I believe Guenter too/I thought you too/ > want to keep these structures protected > for now. If the API starts to get too bloated, then I guess I have to > reconsider that. But I don't think we are there yet. > > I have been thinking about moving the USB PD negotiation details to a > separate structure that could be more accessible for everybody. That > should allow me continue to protect my precious structures. But I have > not yet put much though into that.
Thank you Heikki and Guenter for these valuable reviews. I will update it in the next version. thanks, Kyle On Wed, Feb 3, 2021 at 11:04 PM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > On Wed, Feb 03, 2021 at 05:01:26PM +0200, Heikki Krogerus wrote: > > On Wed, Feb 03, 2021 at 06:51:43AM -0800, Guenter Roeck wrote: > > > Thinking about it, would it make make sense to define the functions as > > > static inline ? > > > > I (and I believe Guenter too) > > s/I believe Guenter too/I thought you too/ > > > want to keep these structures protected > > for now. If the API starts to get too bloated, then I guess I have to > > reconsider that. But I don't think we are there yet. > > > > I have been thinking about moving the USB PD negotiation details to a > > separate structure that could be more accessible for everybody. That > > should allow me continue to protect my precious structures. But I have > > not yet put much though into that. > > > -- > heikki
On 2/3/21 7:04 AM, Heikki Krogerus wrote: > On Wed, Feb 03, 2021 at 05:01:26PM +0200, Heikki Krogerus wrote: >> On Wed, Feb 03, 2021 at 06:51:43AM -0800, Guenter Roeck wrote: >>> Thinking about it, would it make make sense to define the functions as >>> static inline ? >> >> I (and I believe Guenter too) > > s/I believe Guenter too/I thought you too/ > Oops, you are correct. I somehow thought the structure holding the variable was defined in an include file. Sorry, my bad. Please ignore the noise I am making. Guenter >> want to keep these structures protected >> for now. If the API starts to get too bloated, then I guess I have to >> reconsider that. But I don't think we are there yet. >> >> I have been thinking about moving the USB PD negotiation details to a >> separate structure that could be more accessible for everybody. That >> should allow me continue to protect my precious structures. But I have >> not yet put much though into that. > >
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; 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); + /* ------------------------------------------ */ /* API for Multiplexer/DeMultiplexer Switches */ diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h index 4946eca742d5..f692d9ee0f13 100644 --- a/include/linux/usb/typec.h +++ b/include/linux/usb/typec.h @@ -217,12 +217,19 @@ struct typec_operations { enum typec_port_type type); }; +enum usb_pd_svdm_ver { + SVDM_VER_1_0 = 0, + SVDM_VER_2_0 = 1, + SVDM_VER_MAX = SVDM_VER_2_0, +}; + /* * struct typec_capability - USB Type-C Port Capabilities * @type: Supported power role of the port * @data: Supported data role of the port * @revision: USB Type-C Specification release. Binary coded decimal * @pd_revision: USB Power Delivery Specification revision if supported + * @svdm_version: USB PD Structured VDM version if supported * @prefer_role: Initial role preference (DRP ports). * @accessory: Supported Accessory Modes * @fwnode: Optional fwnode of the port @@ -236,6 +243,7 @@ struct typec_capability { enum typec_port_data data; u16 revision; /* 0120H = "1.2" */ u16 pd_revision; /* 0300H = "3.0" */ + enum usb_pd_svdm_ver svdm_version; int prefer_role; enum typec_accessory accessory[TYPEC_MAX_ACCESSORY]; unsigned int orientation_aware:1; @@ -286,4 +294,6 @@ int typec_find_orientation(const char *name); int typec_find_port_power_role(const char *name); int typec_find_power_role(const char *name); int typec_find_port_data_role(const char *name); +void typec_set_svdm_version(struct typec_port *port, enum usb_pd_svdm_ver); +enum usb_pd_svdm_ver typec_get_svdm_version(struct typec_port *port); #endif /* __LINUX_USB_TYPEC_H */
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(+)