Re: [RFC PATCH 1/1] usb: typec: tcpm: Support role-switch remote endpoint in connector

[Heikki Krogerus]

On Sat, Mar 25, 2023 at 01:15:52AM +0000, Bryan O'Donoghue wrote:
> Right now in TCPM when we want to send a role-switch message the
> remote-endpoint must appear inside of the TCPM bound node, not in the
> connector associated with TCPM.
> 
> &typec {
> 	status = "okay";
> 
> 	port {
> 		typec_role_switch: endpoint {
> 			remote-endpoint = <&dwc3_role_switch>;
> 		};
> 	};
> 
> 	connector {
> 		compatible = "usb-c-connector";
> 
> 		power-role = "source";
> 		data-role = "dual";
> 		self-powered;
> 
> 		ports {
> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 
> 			port@0 {
> 				reg = <0>;
> 				typec_mux: endpoint {
> 					remote-endpoint = <&phy_typec_mux>;
> 				};
> 			};
> 		};
> 	};
> };
> 
> This change makes it possible to declare the remote-endpoint inside of the
> connector of the TCPM e.g.
> 
> &typec {
> 	status = "okay";
> 
> 	connector {
> 		compatible = "usb-c-connector";
> 
> 		power-role = "source";
> 		data-role = "dual";
> 		self-powered;
> 
> 		ports {
> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 
> 			port@0 {
> 				reg = <0>;
> 				typec_role_switch: endpoint {
> 					remote-endpoint = <&dwc3_role_switch>;
> 				};
> 			};
> 			port@1 {
> 				reg = <1>;
> 				typec_mux: endpoint {
> 					remote-endpoint = <&phy_typec_mux>;
> 				};
> 			};
> 		};
> 	};
> };
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 1ee774c263f08..a62fecf3bb44c 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -6515,6 +6515,7 @@ static enum hrtimer_restart send_discover_timer_handler(struct hrtimer *timer)
>  struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
>  {
>  	struct tcpm_port *port;
> +	struct fwnode_handle *fwnode;
>  	int err;
>  
>  	if (!dev || !tcpc ||
> @@ -6582,6 +6583,14 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
>  		goto out_destroy_wq;
>  	}
>  
> +	if (!port->role_sw) {
> +		fwnode = device_get_named_child_node(port->dev, "connector");

You already have that assigned to tcpc->fwnode, no?

At least drivers/usb/typec/tcpm/tcpci.c assignes it there, and I think
so do the other port controller drivers.

> +		if (fwnode) {
> +			port->role_sw = fwnode_usb_role_switch_get(fwnode);
> +			fwnode_handle_put(fwnode);
> +		}
> +	}

If that is the case, then I think this is enough:

        if (!port->role_sw)
		port->role_sw = fwnode_usb_role_switch_get(tcpc->fwnode);

Or maybe I missed something. If I did then sorry for the noise :-)

thanks,

-- 
heikki