Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

[Heikki Krogerus]

On Mon, May 20, 2019 at 09:45:46AM +0000, Biju Das wrote:
> 
> 
> Hi Heikki,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> > node
> > 
> > On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> > > Hi Heikki,
> > >
> > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > usb_role_switch by node
> > > >
> > > > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun wrote:
> > > > > Hi,
> > > > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus wrote:
> > > > > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun wrote:
> > > > > > > > Add fwnode_usb_role_switch_get() to make easier to get
> > > > > > > > usb_role_switch by fwnode which register it.
> > > > > > > > It's useful when there is not device_connection registered
> > > > > > > > between two drivers and only knows the fwnode which register
> > > > > > > > usb_role_switch.
> > > > > > > >
> > > > > > > > Signed-off-by: Chunfeng Yun <chunfeng.yun@xxxxxxxxxxxx>
> > > > > > > > Tested-by: Biju Das <biju.das@xxxxxxxxxxxxxx>
> > > > > > >
> > > > > > > Acked-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> > > > > >
> > > > > > Hold on. I just noticed Rob's comment on patch 2/6, where he
> > > > > > points out that you don't need to use device graph since the
> > > > > > controller is the parent of the connector. Doesn't that mean you
> > > > > > don't really need this API?
> > > > > No, I still need it.
> > > > > The change is about the way how to get fwnode; when use device
> > > > > graph, get fwnode by of_graph_get_remote_node(); but now will get
> > > > > fwnode by of_get_parent();
> > > >
> > > > OK, I get that, but I'm still not convinced about if something like
> > > > this function is needed at all. I also have concerns regarding how
> > > > you are using the function. I'll explain in comment to the patch 5/6 in this
> > series...
> > >
> > > FYI, Currently  I am also using this api in my patch series.
> > > https://patchwork.kernel.org/patch/10944637/
> > 
> > Yes, and I have the same question for you I jusb asked in comment I added
> > to the patch 5/6 of this series. Why isn't usb_role_switch_get() enough?
> 
> Currently no issue. It will work with this api as well, since the port node is part of controller node.
> For eg:-
> https://patchwork.kernel.org/patch/10944627/
> 
> However if any one adds port node inside the connector node, then this api may won't work as expected.
> Currently I get below error
> 
> [    2.299703] OF: graph: no port node found in /soc/i2c@e6500000/hd3ss3220@47

We need to understand why is that happening?

It looks like we have an issue somewhere in the code, and instead of
fixing that, you are working around it. Let's not do that.

> For eg:-
> 
> 	hd3ss3220@47 {
> 		compatible = "ti,hd3ss3220";
> 		...
> 		....
> 		usb_con: connector {
>                                      ....
>                                      ....
> 			port {
> 				hd3ss3220_ep: endpoint@0 {
> 					reg = <0>;
> 					remote-endpoint = <&usb3peri_role_switch>;
> 				};
> 			};
> 		};
> 	};
> 
> Regards,
> Biju

thanks,

-- 
heikki