| Message ID | 20210202161733.932215-1-kyletso@google.com |
|---|---|
| Headers | show |
| Series | common SVDM version and VDO from dt | expand |
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, -- heikki
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
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. > >