| Message ID | 20220202221948.5690-4-samuel@sholland.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: typec: WUSB3801 devicetree bindings and driver | expand |
Hi, On Wed, Feb 02, 2022 at 04:19:46PM -0600, Samuel Holland wrote: > Basic programmable non-PD Type-C port controllers do not need the full > TCPM library, but they share the same devicetree binding and the same > typec_capability structure. Factor out a helper for parsing those > properties which map to fields in struct typec_capability, so the code > can be shared between TCPM and basic non-TCPM drivers. > > Signed-off-by: Samuel Holland <samuel@sholland.org> > --- > > Changes in v2: > - Always put the return values from typec_find_* in a signed variable > for error checking. > > drivers/usb/typec/class.c | 52 +++++++++++++++++++++++++++++++++++ > drivers/usb/typec/tcpm/tcpm.c | 32 +-------------------- > include/linux/usb/typec.h | 3 ++ > 3 files changed, 56 insertions(+), 31 deletions(-) > > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c > index 45a6f0c807cb..b67ba9478c82 100644 > --- a/drivers/usb/typec/class.c > +++ b/drivers/usb/typec/class.c > @@ -1894,6 +1894,58 @@ void *typec_get_drvdata(struct typec_port *port) > } > EXPORT_SYMBOL_GPL(typec_get_drvdata); > > +int typec_get_fw_cap(struct typec_capability *cap, > + struct fwnode_handle *fwnode) > +{ > + const char *cap_str; > + int ret; > + > + /* > + * This fwnode has a "compatible" property, but is never populated as a > + * struct device. Instead we simply parse it to read the properties. > + * This it breaks fw_devlink=on. To maintain backward compatibility > + * with existing DT files, we work around this by deleting any > + * fwnode_links to/from this fwnode. > + */ > + fw_devlink_purge_absent_suppliers(fwnode); Let's not put that call here. That function is broken. I think it in practice assumes that there can only be one device linked to fwnode, but that's not true. The fwnodes can be, and are, shared. So by using it we may end up doing things to some completely wrong devices. So let's keep that call in the drivers that really have to have it for now. I think that function - fw_devlink_purge_absent_suppliers() - needs some serious rethinking. There is some deeper problem. I have a feeling that all the functions that rely on the fwnode->dev member are broken. We need a proper reverce search mechanism that can be used to find the devices linked to fwnodes. But I have no idea how to that could be done. > + cap->fwnode = fwnode; > + > + ret = fwnode_property_read_string(fwnode, "power-role", &cap_str); > + if (ret < 0) > + return ret; > + > + ret = typec_find_port_power_role(cap_str); > + if (ret < 0) > + return ret; > + cap->type = ret; > + > + /* USB data support is optional */ > + ret = fwnode_property_read_string(fwnode, "data-role", &cap_str); > + if (ret == 0) { > + ret = typec_find_port_data_role(cap_str); > + if (ret < 0) > + return ret; > + cap->data = ret; > + } > + > + /* Get the preferred power role for a DRP */ > + if (cap->type == TYPEC_PORT_DRP) { > + cap->prefer_role = TYPEC_NO_PREFERRED_ROLE; > + > + ret = fwnode_property_read_string(fwnode, "try-power-role", &cap_str); > + if (ret == 0) { > + ret = typec_find_power_role(cap_str); > + if (ret < 0) > + return ret; > + cap->prefer_role = ret; > + } > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(typec_get_fw_cap); thanks,
On Wed, Feb 02, 2022 at 04:19:46PM -0600, Samuel Holland wrote: > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > index 5fce795b69c7..8b58aa6e3509 100644 > --- a/drivers/usb/typec/tcpm/tcpm.c > +++ b/drivers/usb/typec/tcpm/tcpm.c > @@ -5935,32 +5935,10 @@ static int tcpm_fw_get_caps(struct tcpm_port *port, > if (!fwnode) > return -EINVAL; > > - /* > - * This fwnode has a "compatible" property, but is never populated as a > - * struct device. Instead we simply parse it to read the properties. > - * This it breaks fw_devlink=on. To maintain backward compatibility > - * with existing DT files, we work around this by deleting any > - * fwnode_links to/from this fwnode. > - */ > - fw_devlink_purge_absent_suppliers(fwnode); > - > - /* USB data support is optional */ > - ret = fwnode_property_read_string(fwnode, "data-role", &cap_str); > - if (ret == 0) { > - ret = typec_find_port_data_role(cap_str); > - if (ret < 0) > - return ret; > - port->typec_caps.data = ret; > - } > - > - ret = fwnode_property_read_string(fwnode, "power-role", &cap_str); > + ret = typec_get_fw_cap(&port->typec_caps, fwnode); > if (ret < 0) > return ret; > > - ret = typec_find_port_power_role(cap_str); > - if (ret < 0) > - return ret; > - port->typec_caps.type = ret; > port->port_type = port->typec_caps.type; > port->pd_supported = !fwnode_property_read_bool(fwnode, "pd-disable"); > > @@ -5997,14 +5975,6 @@ static int tcpm_fw_get_caps(struct tcpm_port *port, > if (port->port_type == TYPEC_PORT_SRC) > return 0; > > - /* Get the preferred power role for DRP */ > - ret = fwnode_property_read_string(fwnode, "try-power-role", &cap_str); > - if (ret < 0) > - return ret; > - > - port->typec_caps.prefer_role = typec_find_power_role(cap_str); > - if (port->typec_caps.prefer_role < 0) > - return -EINVAL; > sink: > port->self_powered = fwnode_property_read_bool(fwnode, "self-powered"); It looks like after this there are no more users for that cap_str variable. You need to remove that too. thanks,
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c index 45a6f0c807cb..b67ba9478c82 100644 --- a/drivers/usb/typec/class.c +++ b/drivers/usb/typec/class.c @@ -1894,6 +1894,58 @@ void *typec_get_drvdata(struct typec_port *port) } EXPORT_SYMBOL_GPL(typec_get_drvdata); +int typec_get_fw_cap(struct typec_capability *cap, + struct fwnode_handle *fwnode) +{ + const char *cap_str; + int ret; + + /* + * This fwnode has a "compatible" property, but is never populated as a + * struct device. Instead we simply parse it to read the properties. + * This it breaks fw_devlink=on. To maintain backward compatibility + * with existing DT files, we work around this by deleting any + * fwnode_links to/from this fwnode. + */ + fw_devlink_purge_absent_suppliers(fwnode); + + cap->fwnode = fwnode; + + ret = fwnode_property_read_string(fwnode, "power-role", &cap_str); + if (ret < 0) + return ret; + + ret = typec_find_port_power_role(cap_str); + if (ret < 0) + return ret; + cap->type = ret; + + /* USB data support is optional */ + ret = fwnode_property_read_string(fwnode, "data-role", &cap_str); + if (ret == 0) { + ret = typec_find_port_data_role(cap_str); + if (ret < 0) + return ret; + cap->data = ret; + } + + /* Get the preferred power role for a DRP */ + if (cap->type == TYPEC_PORT_DRP) { + cap->prefer_role = TYPEC_NO_PREFERRED_ROLE; + + ret = fwnode_property_read_string(fwnode, "try-power-role", &cap_str); + if (ret == 0) { + ret = typec_find_power_role(cap_str); + if (ret < 0) + return ret; + cap->prefer_role = ret; + } + } + + return 0; +} +EXPORT_SYMBOL_GPL(typec_get_fw_cap); + /** * typec_port_register_altmode - Register USB Type-C Port Alternate Mode * @port: USB Type-C Port that supports the alternate mode diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c index 5fce795b69c7..8b58aa6e3509 100644 --- a/drivers/usb/typec/tcpm/tcpm.c +++ b/drivers/usb/typec/tcpm/tcpm.c @@ -5935,32 +5935,10 @@ static int tcpm_fw_get_caps(struct tcpm_port *port, if (!fwnode) return -EINVAL; - /* - * This fwnode has a "compatible" property, but is never populated as a - * struct device. Instead we simply parse it to read the properties. - * This it breaks fw_devlink=on. To maintain backward compatibility - * with existing DT files, we work around this by deleting any - * fwnode_links to/from this fwnode. - */ - fw_devlink_purge_absent_suppliers(fwnode); - - /* USB data support is optional */ - ret = fwnode_property_read_string(fwnode, "data-role", &cap_str); - if (ret == 0) { - ret = typec_find_port_data_role(cap_str); - if (ret < 0) - return ret; - port->typec_caps.data = ret; - } - - ret = fwnode_property_read_string(fwnode, "power-role", &cap_str); + ret = typec_get_fw_cap(&port->typec_caps, fwnode); if (ret < 0) return ret; - ret = typec_find_port_power_role(cap_str); - if (ret < 0) - return ret; - port->typec_caps.type = ret; port->port_type = port->typec_caps.type; port->pd_supported = !fwnode_property_read_bool(fwnode, "pd-disable"); @@ -5997,14 +5975,6 @@ static int tcpm_fw_get_caps(struct tcpm_port *port, if (port->port_type == TYPEC_PORT_SRC) return 0; - /* Get the preferred power role for DRP */ - ret = fwnode_property_read_string(fwnode, "try-power-role", &cap_str); - if (ret < 0) - return ret; - - port->typec_caps.prefer_role = typec_find_power_role(cap_str); - if (port->typec_caps.prefer_role < 0) - return -EINVAL; sink: port->self_powered = fwnode_property_read_bool(fwnode, "self-powered"); diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h index 7ba45a97eeae..fdf737d48b3b 100644 --- a/include/linux/usb/typec.h +++ b/include/linux/usb/typec.h @@ -295,6 +295,9 @@ int typec_set_mode(struct typec_port *port, int mode); void *typec_get_drvdata(struct typec_port *port); +int typec_get_fw_cap(struct typec_capability *cap, + struct fwnode_handle *fwnode); + int typec_find_pwr_opmode(const char *name); int typec_find_orientation(const char *name); int typec_find_port_power_role(const char *name);
Basic programmable non-PD Type-C port controllers do not need the full TCPM library, but they share the same devicetree binding and the same typec_capability structure. Factor out a helper for parsing those properties which map to fields in struct typec_capability, so the code can be shared between TCPM and basic non-TCPM drivers. Signed-off-by: Samuel Holland <samuel@sholland.org> --- Changes in v2: - Always put the return values from typec_find_* in a signed variable for error checking. drivers/usb/typec/class.c | 52 +++++++++++++++++++++++++++++++++++ drivers/usb/typec/tcpm/tcpm.c | 32 +-------------------- include/linux/usb/typec.h | 3 ++ 3 files changed, 56 insertions(+), 31 deletions(-)