Hi Kyle,
On Tue, Dec 05, 2023 at 11:01:14AM +0800, Kyle Tso wrote:
> Refactor tcpm_fw_get_caps to support the multiple pd capabilities got
> from fwnode. For backward compatibility, the original single capability
> is still applicable. The fetched data is stored in the newly defined
> structure "pd_data" and there is an array "pd_list" to store the
> pointers to them. A dedicated array "pds" is used to store the handles
> of the registered usb_power_delivery instances.
>
> Also implement the .pd_get and .pd_set ops which are introduced in
> commit a7cff92f0635 ("usb: typec: USB Power Delivery helpers for ports
> and partners"). Once the .pd_set is called, the current capability will
> be updated and state machine will re-negotiate the power contract if
> possible.
>
> Signed-off-by: Kyle Tso <kyletso@xxxxxxxxxx>
> ---
> v4 -> v5:
> - no change
>
> drivers/usb/typec/tcpm/tcpm.c | 389 ++++++++++++++++++++++++++--------
> 1 file changed, 303 insertions(+), 86 deletions(-)
<snip>
> @@ -6124,12 +6243,11 @@ static int tcpm_port_register_pd(struct tcpm_port *port)
> return ret;
> }
>
> -static int tcpm_fw_get_caps(struct tcpm_port *port,
> - struct fwnode_handle *fwnode)
> +static int tcpm_fw_get_properties(struct tcpm_port *port, struct fwnode_handle *fwnode)
The function names got me confused first :). Even though you now call
this "properties", I think the term "capablities" can in practice
still mean two things in this driver (you are for example still
calling typec_get_fw_cap() from this function).
So how about you don't change the name of this function, but
instead..
> {
> const char *opmode_str;
> + u32 frs_current;
> int ret;
> - u32 mw, frs_current;
>
> if (!fwnode)
> return -EINVAL;
> @@ -6149,28 +6267,10 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
>
> port->port_type = port->typec_caps.type;
> port->pd_supported = !fwnode_property_read_bool(fwnode, "pd-disable");
> -
> port->slow_charger_loop = fwnode_property_read_bool(fwnode, "slow-charger-loop");
> - if (port->port_type == TYPEC_PORT_SNK)
> - goto sink;
> -
> - /* Get Source PDOs for the PD port or Source Rp value for the non-PD port */
> - if (port->pd_supported) {
> - ret = fwnode_property_count_u32(fwnode, "source-pdos");
> - if (ret == 0)
> - return -EINVAL;
> - else if (ret < 0)
> - return ret;
> + port->self_powered = fwnode_property_read_bool(fwnode, "self-powered");
>
> - port->nr_src_pdo = min(ret, PDO_MAX_OBJECTS);
> - ret = fwnode_property_read_u32_array(fwnode, "source-pdos",
> - port->src_pdo, port->nr_src_pdo);
> - if (ret)
> - return ret;
> - ret = tcpm_validate_caps(port, port->src_pdo, port->nr_src_pdo);
> - if (ret)
> - return ret;
> - } else {
> + if (!port->pd_supported) {
> ret = fwnode_property_read_string(fwnode, "typec-power-opmode", &opmode_str);
> if (ret)
> return ret;
> @@ -6180,43 +6280,156 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
> port->src_rp = tcpm_pwr_opmode_to_rp(ret);
> }
>
> - if (port->port_type == TYPEC_PORT_SRC)
> - return 0;
> + /* FRS can only be supported by DRP ports */
> + if (port->port_type == TYPEC_PORT_DRP) {
> + ret = fwnode_property_read_u32(fwnode, "new-source-frs-typec-current",
> + &frs_current);
> + if (!ret && frs_current <= FRS_5V_3A)
> + port->new_source_frs_current = frs_current;
> + }
>
> -sink:
> - port->self_powered = fwnode_property_read_bool(fwnode, "self-powered");
> + return 0;
> +}
> +
> +static int tcpm_fw_get_caps(struct tcpm_port *port, struct fwnode_handle *fwnode)
..make that tcpm_fw_get_pd_capabilities() (or something like that) to
be more clear?
Also, since you are only calling this ones, why not just call this
directly from tcpm_fw_get_cap() instead of right after it?
thanks,
--
heikki
