| Message ID | 20210202161733.932215-1-kyletso@google.com |
|---|---|
| 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, -- heikki
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, -- heikki diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c index 8f77669f9cf4f..04238b0a5d47f 100644 --- a/drivers/usb/typec/class.c +++ b/drivers/usb/typec/class.c @@ -36,6 +36,8 @@ struct typec_partner { enum typec_accessory accessory; struct ida mode_ids; int num_altmodes; + + enum usb_pd_svdm_ver svdm_version; }; struct typec_port { @@ -792,6 +794,18 @@ typec_partner_register_altmode(struct typec_partner *partner, } EXPORT_SYMBOL_GPL(typec_partner_register_altmode); +/** + * typec_partner_set_svdm_version - Set negotiated SVDM version + * @partner: The partner. + * ... + */ +void typec_partner_set_svdm_version(struct typec_partner *partner, + enum usb_pd_svdm_ver svdm_version) +{ + partner->svdm_version = svdm_version; +} +EXPORT_SYMBOL_GPL(typec_partner_set_svdm_version); + /** * typec_register_partner - Register a USB Type-C Partner * @port: The USB Type-C Port the partner is connected to @@ -1847,6 +1861,27 @@ EXPORT_SYMBOL_GPL(typec_set_mode); /* --------------------------------------- */ +/** + * typec_get_negotiated_svdm_version - Get negotiated SVDM version + * @port: The port. + * ... + */ +int typec_get_negotiated_svdm_version(struct typec_port *port) +{ + enum usb_pd_svdm_ver svdm_version; + struct device *partner_dev; + + partner_dev = device_find_child(&port->dev, NULL, partner_match); + if (!partner_dev) + return -ENODEV; + + svdm_version = to_typec_partner(partner_dev)->svdm_version; + put_device(partner_dev); + + return svdm_version +} +EXPORT_SYMBOL_GPL(typec_partner_set_svdm_version); + /** * typec_get_drvdata - Return private driver data pointer * @port: USB Type-C port diff --git a/include/linux/usb/typec_altmode.h b/include/linux/usb/typec_altmode.h index 5e0a7b7647c3b..91e119e37ba70 100644 --- a/include/linux/usb/typec_altmode.h +++ b/include/linux/usb/typec_altmode.h @@ -132,6 +132,16 @@ typec_altmode_get_orientation(struct typec_altmode *altmode) return typec_get_orientation(typec_altmode2port(altmode)); } +/** + * typec_get_negotiated_svdm_version - Get negotiated SVDM version + * ... + */ +static inline int +typec_altmode_get_svdm_version(struct typec_altmode *altmode) +{ + return typec_get_negotiated_svdm_version(typec_altmode2port(altmode)); +} + /** * struct typec_altmode_driver - USB Type-C alternate mode device driver * @id_table: Null terminated array of SVIDs
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.
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. > >