Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 28 Mar 2023 17:08:56 +0300
From:   Heikki Krogerus <heikki.krogerus@linux.intel.com>
To:     Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Cc:     linux@roeck-us.net, andersson@kernel.org, robh@kernel.org,
        gregkh@linuxfoundation.org, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH 1/1] usb: typec: tcpm: Support role-switch remote
 endpoint in connector
Message-ID: <ZCL0+COeAkinuaBL@kuha.fi.intel.com>
References: <20230325011552.2241155-1-bryan.odonoghue@linaro.org>
 <20230325011552.2241155-2-bryan.odonoghue@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230325011552.2241155-2-bryan.odonoghue@linaro.org>
Precedence: bulk

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@linaro.org>
> ---
>  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