Re: [PATCH v2 3/4] usb: typec: Factor out non-PD fwnode properties

[Heikki Krogerus]

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@xxxxxxxxxxxx>
> ---
> 
> 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,

-- 
heikki