RE: [PATCH 2/2] usb: typec: mux: Remove requirement for the "orientation-switch" device property

[Jun Li]

Hi
> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> Sent: Monday, May 31, 2021 4:26 PM
> To: Jun Li <jun.li@xxxxxxx>; Greg Kroah-Hartman
> <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: Hans de Goede <hdegoede@xxxxxxxxxx>; linux-usb@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 2/2] usb: typec: mux: Remove requirement for the
> "orientation-switch" device property
> 
> On Mon, May 31, 2021 at 10:57:04AM +0300, Heikki Krogerus wrote:
> > On Mon, May 31, 2021 at 10:24:35AM +0300, Heikki Krogerus wrote:
> > > On Fri, May 28, 2021 at 07:26:43AM +0000, Jun Li wrote:
> > > > Hi,
> > > > > -----Original Message-----
> > > > > From: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> > > > > Sent: Wednesday, May 26, 2021 11:36 PM
> > > > > To: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Hans de
> > > > > Goede <hdegoede@xxxxxxxxxx>; Jun Li <jun.li@xxxxxxx>
> > > > > Cc: linux-usb@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> > > > > Subject: [PATCH 2/2] usb: typec: mux: Remove requirement for the
> > > > > "orientation-switch" device property
> > > > >
> > > > > The additional boolean device property "orientation-switch"
> > > > > is not needed when the connection is described with device
> > > > > graph, so removing the check and the requirement for it.
> > > > >
> > > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> > > > > ---
> > > > >  drivers/usb/typec/mux.c | 3 ---
> > > > >  1 file changed, 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
> > > > > index e40a555724fb6..603f3e698cc0b 100644
> > > > > --- a/drivers/usb/typec/mux.c
> > > > > +++ b/drivers/usb/typec/mux.c
> > > > > @@ -30,9 +30,6 @@ static void *typec_switch_match(struct
> > > > > fwnode_handle *fwnode, const char *id,  {
> > > > >  	struct device *dev;
> > > > >
> > > > > -	if (id && !fwnode_property_present(fwnode, id))
> > > > > -		return NULL;
> > > > > -
> > > >
> > > > May this change the result of fwnode_connection_find_match() if
> > > > there are multiple remote-endpoint node?
> > > >
> > > > After the 2 patches change, typec_switch_match() will never return
> > > > NULL, so
> > > >
> > > >   17 static void *
> > > >   18 fwnode_graph_devcon_match(struct fwnode_handle *fwnode, const
> char *con_id,
> > > >   19                           void *data, devcon_match_fn_t match)
> > > >   20 {
> > > >   21         struct fwnode_handle *node;
> > > >   22         struct fwnode_handle *ep;
> > > >   23         void *ret;
> > > >   24
> > > >   25         fwnode_graph_for_each_endpoint(fwnode, ep) {
> > > >   26                 node = fwnode_graph_get_remote_port_parent(ep);
> > > >   27                 if (!fwnode_device_is_available(node))
> > > >   28                         continue;
> > > >   29
> > > >   30                 ret = match(node, con_id, data);// ret can't be
> NULL;
> > > >   31                 fwnode_handle_put(node);
> > > >   32                 if (ret) {
> > > > 							 /*
> > > > 							  * So loop will go to here and stop
> > > > 							  * checking next ep, even this ep
> > > > 							  * actually is not for typec_switch
> > > > 							  */
> > > >   33                         fwnode_handle_put(ep);
> > > >   34                         return ret;
> > > >   35                 }
> > > >   36         }
> > > >   37         return NULL;
> > > >   38 }
> > > >
> > > > fwnode_graph_devcon_match() Will return ERR_PTR(-EPROBE_DEFER)
> > > > even this ep's remote parent already probed but it's not for
> > > > typec_switch.
> > >
> > > You are correct. With device graph I guess we really always need the
> > > extra device property after all.
> > >
> > > So let's forget about this one.
> >
> > Oh no. This patch just landed into Greg's usb-next. I'll prepare the
> > revert. I'm sorry about this.
> 
> Actually, if we always need that extra (boolean) device property to identify
> the device class when OF graph is used, 

Looks like yes, as we need a way to know if the current fwnode
is for the target device we are looking for, to return probe
defer correctly.

> shouldn't we just do that always
> in fwnode_graph_devcon_match()?

This depends on if we want to limit this to be a boolean property
(to mark this is the target fwnode), or make it to be more generic
so user can define it in its ->match().

Now there are only 2 users of it, role switch and typec mux, both work
as a boolean property for con_id.

Li Jun
> 
> diff --git a/drivers/base/property.c b/drivers/base/property.c index
> 1421e9548857b..238da64375bb1 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -1263,6 +1263,13 @@ fwnode_graph_devcon_match(struct fwnode_handle
> *fwnode, const char *con_id,
>                 if (!fwnode_device_is_available(node))
>                         continue;
> 
> +               /*
> +                * With device graph @con_id is expected to be the name of
> the
> +                * "device class" of the fwnode.
> +                */
> +               if (con_id && !fwnode_property_present(node, con_id))
> +                       continue;
> +
>                 ret = match(node, con_id, data);
>                 fwnode_handle_put(node);
>                 if (ret) {
> 
> thanks,
> 
> --
> heikki