Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Jun Li <jun.li@nxp.com>
To:     Heikki Krogerus <heikki.krogerus@linux.intel.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC:     Hans de Goede <hdegoede@redhat.com>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH 2/2] usb: typec: mux: Remove requirement for the
 "orientation-switch" device property
Thread-Topic: [PATCH 2/2] usb: typec: mux: Remove requirement for the
 "orientation-switch" device property
Thread-Index: AQHXUkTF1Hli5+/RLUyh4hA8exUVWqr4dZKQgATB44CAAAkYAIAACCYAgAArbUA=
Date:   Mon, 31 May 2021 11:08:14 +0000
Message-ID: <VI1PR04MB5935F74E297A467D846AADEC893F9@VI1PR04MB5935.eurprd04.prod.outlook.com>
References: <20210526153548.61276-1-heikki.krogerus@linux.intel.com>
 <20210526153548.61276-3-heikki.krogerus@linux.intel.com>
 <VI1PR04MB59350F5BC9129F9E0B21773889229@VI1PR04MB5935.eurprd04.prod.outlook.com>
 <YLSPLxfyavO+AkLY@kuha.fi.intel.com> <YLSW0IoHdnzRSzUW@kuha.fi.intel.com>
 <YLSdptmScGo5TJ1D@kuha.fi.intel.com>
In-Reply-To: <YLSdptmScGo5TJ1D@kuha.fi.intel.com>
Accept-Language: zh-CN, en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: c595cc4a-2216-443d-5996-08d9242462d6
X-MS-Exchange-CrossTenant-originalarrivaltime: 31 May 2021 11:08:14.4323
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: G949ueXocSYBjcIdEPEjiZA6QRAhu/WgRZz38oO5+DB60COGS2JqCkAnGRRh3pW1
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR0401MB2559
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi
> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Sent: Monday, May 31, 2021 4:26 PM
> To: Jun Li <jun.li@nxp.com>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>
> Cc: Hans de Goede <hdegoede@redhat.com>; linux-usb@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/2] usb: typec: mux: Remove requirement for the
> "orientation-switch" device property
>=20
> 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@linux.intel.com>
> > > > > Sent: Wednesday, May 26, 2021 11:36 PM
> > > > > To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Hans de
> > > > > Goede <hdegoede@redhat.com>; Jun Li <jun.li@nxp.com>
> > > > > Cc: linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org
> > > > > 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@linux.intel.com>
> > > > > ---
> > > > >  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 =3D fwnode_graph_get_remote_port_parent(e=
p);
> > > >   27                 if (!fwnode_device_is_available(node))
> > > >   28                         continue;
> > > >   29
> > > >   30                 ret =3D 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.
>=20
> Actually, if we always need that extra (boolean) device property to ident=
ify
> the device class when OF graph is used,=20

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
>=20
> 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;
>=20
> +               /*
> +                * With device graph @con_id is expected to be the name o=
f
> the
> +                * "device class" of the fwnode.
> +                */
> +               if (con_id && !fwnode_property_present(node, con_id))
> +                       continue;
> +
>                 ret =3D match(node, con_id, data);
>                 fwnode_handle_put(node);
>                 if (ret) {
>=20
> thanks,
>=20
> --
> heikki