| Message ID | 20210131151832.215931-1-kyletso@google.com |
|---|---|
| Headers | show |
| Series | common SVDM version and VDO from dt | expand |
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 ? > + 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 ? > + } 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 ? 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
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(-)