| Message ID | 20210526153548.61276-3-heikki.krogerus@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: typec: mux: a few improvements | expand |
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 = 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. Li Jun > dev = class_find_device(&typec_mux_class, NULL, fwnode, > switch_fwnode_match); > > -- > 2.30.2
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 = 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. thanks, -- heikki
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 = 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. thanks, -- heikki
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 = 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, shouldn't we just do that always in fwnode_graph_devcon_match()? 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
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 > > 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 = 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
> > > 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. OK. Let's leave to the users for now. thanks, -- heikki
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; - dev = class_find_device(&typec_mux_class, NULL, fwnode, switch_fwnode_match);
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(-)