| Message ID | 20210131151832.215931-1-kyletso@google.com |
|---|---|
| Headers | show |
| Series | common SVDM version and VDO from dt | expand |
On Mon, Feb 1, 2021 at 12:02 AM Guenter Roeck <linux@roeck-us.net> wrote: > > On 1/31/21 7:18 AM, Kyle Tso wrote: > > Commit a079973f462a ("usb: typec: tcpm: Remove tcpc_config > > configuration mechanism") removed the tcpc_config which includes the > > Sink VDO and it is not yet added back with fwnode. Add it now. > > > > Signed-off-by: Kyle Tso <kyletso@google.com> > > --- > > Changes since v1: > > - updated the commit message > > > > drivers/usb/typec/tcpm/tcpm.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > > index 403a483645dd..84c8a52f8af1 100644 > > --- a/drivers/usb/typec/tcpm/tcpm.c > > +++ b/drivers/usb/typec/tcpm/tcpm.c > > @@ -5677,6 +5677,18 @@ static int tcpm_fw_get_caps(struct tcpm_port *port, > > port->new_source_frs_current = frs_current; > > } > > > > + ret = fwnode_property_read_u32_array(fwnode, "sink-vdos", NULL, 0); > > fwnode_property_count_u32(), maybe ? > That's the same and looks like fwnode_property_count_u32 is better to read. I will revise it in the next version. > > + if (ret <= 0 && ret != -EINVAL) { > > + return -EINVAL; > > Why return any error except -EINVAL (including return values of 0) as -EINVAL, > and -EINVAL as no error ? > sink-vdos is not a mandatory property which means -EINVAL is acceptable. If the return < 0 and the value is not -EINVAL, it means that the error is other than "not present" in the device tree. If the return == 0, it means that the sink-vdos is present in the device tree but no value inside it. Both of the above situations are not acceptable. > > + } else if (ret > 0) { > > + port->nr_snk_vdo = min(ret, VDO_MAX_OBJECTS); > > + ret = fwnode_property_read_u32_array(fwnode, "sink-vdos", > > + port->snk_vdo, > > + port->nr_snk_vdo); > > + if (ret < 0) > > + return -EINVAL; > > static analyzer code used to complain about overriding error codes. > Not sure if that is still true. Either case, why not return the > original error ? > Returning the original error codes is good. I just followed the return value of other error handling in this function. will revise it in the next version. Thanks, Kyle > Thanks, > Guenter > > > + } > > + > > return 0; > > } > > > > >
On 1/31/21 9:11 PM, Kyle Tso wrote: > On Mon, Feb 1, 2021 at 12:02 AM Guenter Roeck <linux@roeck-us.net> wrote: >> >> On 1/31/21 7:18 AM, Kyle Tso wrote: >>> Commit a079973f462a ("usb: typec: tcpm: Remove tcpc_config >>> configuration mechanism") removed the tcpc_config which includes the >>> Sink VDO and it is not yet added back with fwnode. Add it now. >>> >>> Signed-off-by: Kyle Tso <kyletso@google.com> >>> --- >>> Changes since v1: >>> - updated the commit message >>> >>> drivers/usb/typec/tcpm/tcpm.c | 12 ++++++++++++ >>> 1 file changed, 12 insertions(+) >>> >>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c >>> index 403a483645dd..84c8a52f8af1 100644 >>> --- a/drivers/usb/typec/tcpm/tcpm.c >>> +++ b/drivers/usb/typec/tcpm/tcpm.c >>> @@ -5677,6 +5677,18 @@ static int tcpm_fw_get_caps(struct tcpm_port *port, >>> port->new_source_frs_current = frs_current; >>> } >>> >>> + ret = fwnode_property_read_u32_array(fwnode, "sink-vdos", NULL, 0); >> >> fwnode_property_count_u32(), maybe ? >> > That's the same and looks like fwnode_property_count_u32 is better to read. > I will revise it in the next version. > >>> + if (ret <= 0 && ret != -EINVAL) { >>> + return -EINVAL; >> >> Why return any error except -EINVAL (including return values of 0) as -EINVAL, >> and -EINVAL as no error ? >> > sink-vdos is not a mandatory property which means -EINVAL is acceptable. > > If the return < 0 and the value is not -EINVAL, it means that the > error is other than "not present" in the device tree. > If the return == 0, it means that the sink-vdos is present in the > device tree but no value inside it. I think that should return -ENODATA. Not sure if/when it would actually return 0. > Both of the above situations are not acceptable. > Personally I would prefer that a bit more explicit in the code, ie handle errors first and drop the else statement below. But maybe that is just me. >>> + } else if (ret > 0) { >>> + port->nr_snk_vdo = min(ret, VDO_MAX_OBJECTS); >>> + ret = fwnode_property_read_u32_array(fwnode, "sink-vdos", >>> + port->snk_vdo, >>> + port->nr_snk_vdo); >>> + if (ret < 0) >>> + return -EINVAL; >> >> static analyzer code used to complain about overriding error codes. >> Not sure if that is still true. Either case, why not return the >> original error ? >> > Returning the original error codes is good. I just followed the return > value of other error handling in this function. > will revise it in the next version. > Never mind, with the rest of the code being similar I guess that either static analyzers gave up complaining or they already have a field day anyway. Thanks, Guenter > Thanks, > Kyle > > > >> Thanks, >> Guenter >> >>> + } >>> + >>> return 0; >>> } >>> >>> >>
On Mon, Feb 1, 2021 at 12:21 AM Guenter Roeck <linux@roeck-us.net> wrote: > > On 1/31/21 7:18 AM, Kyle Tso wrote: > > Changes since v1: > > - removed the "local" variables (svdm_version) in tcpm.c and > > (altmodes/ucsi)/displayport.c > > - added a member "svdm_version" in struct typec_capabilities indicating > > the default SVDM version of the port > > - added a member "common_svdm_ver" in struct typec_port indicating the > > common SVDM version between the port and the partner > > I personally find the "common" in the variable and function names unnecessary. > I would prefer using something like svdm_version instead of common_svdm_ver. > The reason for the common_ prefix is just for the readability. That's totally fine with me to remove the prefix. Will fix this in the next version. > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > > index 0afd8ef692e8..403a483645dd 100644 > > --- a/drivers/usb/typec/tcpm/tcpm.c > > +++ b/drivers/usb/typec/tcpm/tcpm.c > > @@ -1470,11 +1470,13 @@ static void tcpm_register_partner_altmodes(struct tcpm_port *port) > > } > > > > #define supports_modal(port) PD_IDH_MODAL_SUPP((port)->partner_ident.id_header) > > +#define common_svdm_ver(typec) (typec_get_common_svdm_version(typec)) > > I think that is unnecessary and confusing. We now have typec_get_common_svdm_version() > as well as common_svdm_ver() and COMMON_SVDM_VER() macros. I would suggest to just use > the function name (and maybe drop the 'common_' prefix from it). > will fix this in the next version. > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c > > index ca3f4194ad90..b8d693cc7b77 100644 > > --- a/drivers/usb/typec/ucsi/ucsi.c > > +++ b/drivers/usb/typec/ucsi/ucsi.c > > @@ -764,6 +764,7 @@ static void ucsi_handle_connector_change(struct work_struct *work) > > > > if (con->status.change & UCSI_CONSTAT_CONNECT_CHANGE) { > > typec_set_pwr_role(con->port, role); > > + typec_set_common_svdm_version(con->port, con->typec_cap.svdm_version); > > > > I am a bit concerned that svdm_version is added to typec_capabilities but not > consistently used by all drivers registering with typec. I am not sure I > understand if and how the value in typec_capabilities is used by the typec core. > I am not sure about it as well :p From my POV, that is just the same nature as the "pd_revision" is in typec_capabilities which means the capabilities the port has regardless of the port partners. The port needs to reset the operating mode to it's designed SVDM version upon detach. I think typec_capabilities is a good place to store this information. What do you think? BTW, the reset value of the variable "negotiated_rev" in tcpm/tcpm.c looks weird to me. It is reset to "PD_MAX_REV" in SNK_STARTUP and SRC_STARTUP. However, the tcpm.c might not always support the max revision of PD. IMO, the pd_revision in typec_capabilities is a better choice compared to PD_MAX_REV. > > diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h > > index 54475323f83b..df0cb1e595a1 100644 > > --- a/include/linux/usb/typec.h > > +++ b/include/linux/usb/typec.h > > @@ -206,12 +206,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 > > @@ -225,6 +232,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; > > @@ -275,4 +283,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_common_svdm_version(struct typec_port *port, enum usb_pd_svdm_ver); > > +enum usb_pd_svdm_ver typec_get_common_svdm_version(struct typec_port *port); > > #endif /* __LINUX_USB_TYPEC_H */ > > > Thanks, Kyle
patch v1 is here: https://lore.kernel.org/linux-devicetree/20210126084544.682641-1-kyletso@google.com/ Changes from v1: ================= usb: typec: Determine common SVDM Versions - removed the "local" variables (svdm_version) in tcpm.c and (altmodes/ucsi)/displayport.c - added a member "svdm_version" in struct typec_capabilities indicating the default SVDM version of the port - added a member "common_svdm_ver" in struct typec_port indicating the common SVDM version between the port and the partner - implemented the get/set API of the common_svdm_ver in typec/class.c so that clients can read/update the common SVDM version - added more definitions of Product Type VDOs dt-bindings: connector: Add SVDM VDO properties - updated the dt-bindings documentations - added more definitions of Product Type VDOs usb: typec: tcpm: Get Sink VDO from fwnode - updated the commit message Kyle Tso (3): usb: typec: Determine common SVDM Versions dt-bindings: connector: Add SVDM VDO properties usb: typec: tcpm: Get Sink VDO from fwnode .../bindings/connector/usb-connector.yaml | 11 + drivers/usb/typec/altmodes/displayport.c | 5 +- drivers/usb/typec/class.c | 21 +- drivers/usb/typec/tcpm/tcpm.c | 61 +++- drivers/usb/typec/ucsi/displayport.c | 10 +- drivers/usb/typec/ucsi/ucsi.c | 2 + include/dt-bindings/usb/pd.h | 311 ++++++++++++++++- include/linux/usb/pd_vdo.h | 315 ++++++++++++++---- include/linux/usb/typec.h | 10 + 9 files changed, 656 insertions(+), 90 deletions(-)