| Message ID | 1649759407-24049-3-git-send-email-quic_linyyuan@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: typec: ucsi: allow retry to find role switch | expand |
On Tue, Apr 12, 2022 at 06:30:06PM +0800, Linyu Yuan wrote: > In error path of ucsi_init(), it will clean all valid ucsi connector, > and samiliar operation also happen in ucsi_unregister(), > add a common function for two places. > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> > --- > drivers/usb/typec/ucsi/ucsi.c | 46 +++++++++++++++++++++---------------------- > 1 file changed, 23 insertions(+), 23 deletions(-) > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c > index 77ac0b7..684fc4f 100644 > --- a/drivers/usb/typec/ucsi/ucsi.c > +++ b/drivers/usb/typec/ucsi/ucsi.c > @@ -1187,6 +1187,27 @@ static int ucsi_register_port(struct ucsi *ucsi, int index) > return ret; > } > > +static void ucsi_connector_clean(struct ucsi *ucsi) > +{ > + struct ucsi_connector *con; > + > + if (!ucsi->connector) > + return; > + > + for (con = ucsi->connector; con->port; con++) { > + cancel_work_sync(&con->work); > + ucsi_unregister_partner(con); > + ucsi_unregister_altmodes(con, UCSI_RECIPIENT_CON); > + ucsi_unregister_port_psy(con); > + if (con->wq) > + destroy_workqueue(con->wq); > + typec_unregister_port(con->port); > + } > + > + kfree(ucsi->connector); > + ucsi->connector = NULL; > +} This looks OK, but the name of the function is confusing to me. Can you make that ucsi_remove_connectors(), or perhaps ucsi_unregister_connectors() to keep it alligned with the other function names in this driver? > /** > * ucsi_init - Initialize UCSI interface > * @ucsi: UCSI to be initialized > @@ -1195,7 +1216,6 @@ static int ucsi_register_port(struct ucsi *ucsi, int index) > */ > static int ucsi_init(struct ucsi *ucsi) > { > - struct ucsi_connector *con; > u64 command; > int ret; > int i; > @@ -1250,15 +1270,7 @@ static int ucsi_init(struct ucsi *ucsi) > return 0; > > err_unregister: > - for (con = ucsi->connector; con->port; con++) { > - ucsi_unregister_partner(con); > - ucsi_unregister_altmodes(con, UCSI_RECIPIENT_CON); > - ucsi_unregister_port_psy(con); > - if (con->wq) > - destroy_workqueue(con->wq); > - typec_unregister_port(con->port); > - con->port = NULL; > - } > + ucsi_connector_clean(ucsi); > > err_reset: > memset(&ucsi->cap, 0, sizeof(ucsi->cap)); > @@ -1364,7 +1376,6 @@ EXPORT_SYMBOL_GPL(ucsi_register); > void ucsi_unregister(struct ucsi *ucsi) > { > u64 cmd = UCSI_SET_NOTIFICATION_ENABLE; > - int i; > > /* Make sure that we are not in the middle of driver initialization */ > cancel_work_sync(&ucsi->work); > @@ -1372,18 +1383,7 @@ void ucsi_unregister(struct ucsi *ucsi) > /* Disable notifications */ > ucsi->ops->async_write(ucsi, UCSI_CONTROL, &cmd, sizeof(cmd)); > > - for (i = 0; i < ucsi->cap.num_connectors; i++) { > - cancel_work_sync(&ucsi->connector[i].work); > - ucsi_unregister_partner(&ucsi->connector[i]); > - ucsi_unregister_altmodes(&ucsi->connector[i], > - UCSI_RECIPIENT_CON); > - ucsi_unregister_port_psy(&ucsi->connector[i]); > - if (ucsi->connector[i].wq) > - destroy_workqueue(ucsi->connector[i].wq); > - typec_unregister_port(ucsi->connector[i].port); > - } > - > - kfree(ucsi->connector); > + ucsi_connector_clean(ucsi); > } > EXPORT_SYMBOL_GPL(ucsi_unregister); > > -- > 2.7.4 thanks,
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Sent: Wednesday, April 13, 2022 3:38 PM > To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; linux- > usb@vger.kernel.org; Jack Pham (QUIC) <quic_jackp@quicinc.com> > Subject: Re: [PATCH 2/3] usb: typec: ucsi: add a common function > ucsi_connector_clean() > > On Tue, Apr 12, 2022 at 06:30:06PM +0800, Linyu Yuan wrote: > > In error path of ucsi_init(), it will clean all valid ucsi connector, > > and samiliar operation also happen in ucsi_unregister(), > > add a common function for two places. > > > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> > > --- > > drivers/usb/typec/ucsi/ucsi.c | 46 +++++++++++++++++++++---------------- > ------ > > 1 file changed, 23 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c > > index 77ac0b7..684fc4f 100644 > > --- a/drivers/usb/typec/ucsi/ucsi.c > > +++ b/drivers/usb/typec/ucsi/ucsi.c > > @@ -1187,6 +1187,27 @@ static int ucsi_register_port(struct ucsi *ucsi, int > index) > > return ret; > > } > > > > +static void ucsi_connector_clean(struct ucsi *ucsi) > > +{ > > + struct ucsi_connector *con; > > + > > + if (!ucsi->connector) > > + return; > > + > > + for (con = ucsi->connector; con->port; con++) { > > + cancel_work_sync(&con->work); > > + ucsi_unregister_partner(con); > > + ucsi_unregister_altmodes(con, UCSI_RECIPIENT_CON); > > + ucsi_unregister_port_psy(con); > > + if (con->wq) > > + destroy_workqueue(con->wq); > > + typec_unregister_port(con->port); > > + } > > + > > + kfree(ucsi->connector); > > + ucsi->connector = NULL; > > +} > > This looks OK, but the name of the function is confusing to me. Can > you make that ucsi_remove_connectors(), or perhaps > ucsi_unregister_connectors() to keep it alligned with the other > function names in this driver? Thanks, will choose ucsi_unregister_connectors(). > > > /** > > * ucsi_init - Initialize UCSI interface > > * @ucsi: UCSI to be initialized > > @@ -1195,7 +1216,6 @@ static int ucsi_register_port(struct ucsi *ucsi, int > index) > > */ > > static int ucsi_init(struct ucsi *ucsi) > > { > > - struct ucsi_connector *con; > > u64 command; > > int ret; > > int i; > > @@ -1250,15 +1270,7 @@ static int ucsi_init(struct ucsi *ucsi) > > return 0; > > > > err_unregister: > > - for (con = ucsi->connector; con->port; con++) { > > - ucsi_unregister_partner(con); > > - ucsi_unregister_altmodes(con, UCSI_RECIPIENT_CON); > > - ucsi_unregister_port_psy(con); > > - if (con->wq) > > - destroy_workqueue(con->wq); > > - typec_unregister_port(con->port); > > - con->port = NULL; > > - } > > + ucsi_connector_clean(ucsi); > > > > err_reset: > > memset(&ucsi->cap, 0, sizeof(ucsi->cap)); > > @@ -1364,7 +1376,6 @@ EXPORT_SYMBOL_GPL(ucsi_register); > > void ucsi_unregister(struct ucsi *ucsi) > > { > > u64 cmd = UCSI_SET_NOTIFICATION_ENABLE; > > - int i; > > > > /* Make sure that we are not in the middle of driver initialization */ > > cancel_work_sync(&ucsi->work); > > @@ -1372,18 +1383,7 @@ void ucsi_unregister(struct ucsi *ucsi) > > /* Disable notifications */ > > ucsi->ops->async_write(ucsi, UCSI_CONTROL, &cmd, sizeof(cmd)); > > > > - for (i = 0; i < ucsi->cap.num_connectors; i++) { > > - cancel_work_sync(&ucsi->connector[i].work); > > - ucsi_unregister_partner(&ucsi->connector[i]); > > - ucsi_unregister_altmodes(&ucsi->connector[i], > > - UCSI_RECIPIENT_CON); > > - ucsi_unregister_port_psy(&ucsi->connector[i]); > > - if (ucsi->connector[i].wq) > > - destroy_workqueue(ucsi->connector[i].wq); > > - typec_unregister_port(ucsi->connector[i].port); > > - } > > - > > - kfree(ucsi->connector); > > + ucsi_connector_clean(ucsi); > > } > > EXPORT_SYMBOL_GPL(ucsi_unregister); > > > > -- > > 2.7.4 > > thanks, > > -- > heikki
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c index 77ac0b7..684fc4f 100644 --- a/drivers/usb/typec/ucsi/ucsi.c +++ b/drivers/usb/typec/ucsi/ucsi.c @@ -1187,6 +1187,27 @@ static int ucsi_register_port(struct ucsi *ucsi, int index) return ret; } +static void ucsi_connector_clean(struct ucsi *ucsi) +{ + struct ucsi_connector *con; + + if (!ucsi->connector) + return; + + for (con = ucsi->connector; con->port; con++) { + cancel_work_sync(&con->work); + ucsi_unregister_partner(con); + ucsi_unregister_altmodes(con, UCSI_RECIPIENT_CON); + ucsi_unregister_port_psy(con); + if (con->wq) + destroy_workqueue(con->wq); + typec_unregister_port(con->port); + } + + kfree(ucsi->connector); + ucsi->connector = NULL; +} + /** * ucsi_init - Initialize UCSI interface * @ucsi: UCSI to be initialized @@ -1195,7 +1216,6 @@ static int ucsi_register_port(struct ucsi *ucsi, int index) */ static int ucsi_init(struct ucsi *ucsi) { - struct ucsi_connector *con; u64 command; int ret; int i; @@ -1250,15 +1270,7 @@ static int ucsi_init(struct ucsi *ucsi) return 0; err_unregister: - for (con = ucsi->connector; con->port; con++) { - ucsi_unregister_partner(con); - ucsi_unregister_altmodes(con, UCSI_RECIPIENT_CON); - ucsi_unregister_port_psy(con); - if (con->wq) - destroy_workqueue(con->wq); - typec_unregister_port(con->port); - con->port = NULL; - } + ucsi_connector_clean(ucsi); err_reset: memset(&ucsi->cap, 0, sizeof(ucsi->cap)); @@ -1364,7 +1376,6 @@ EXPORT_SYMBOL_GPL(ucsi_register); void ucsi_unregister(struct ucsi *ucsi) { u64 cmd = UCSI_SET_NOTIFICATION_ENABLE; - int i; /* Make sure that we are not in the middle of driver initialization */ cancel_work_sync(&ucsi->work); @@ -1372,18 +1383,7 @@ void ucsi_unregister(struct ucsi *ucsi) /* Disable notifications */ ucsi->ops->async_write(ucsi, UCSI_CONTROL, &cmd, sizeof(cmd)); - for (i = 0; i < ucsi->cap.num_connectors; i++) { - cancel_work_sync(&ucsi->connector[i].work); - ucsi_unregister_partner(&ucsi->connector[i]); - ucsi_unregister_altmodes(&ucsi->connector[i], - UCSI_RECIPIENT_CON); - ucsi_unregister_port_psy(&ucsi->connector[i]); - if (ucsi->connector[i].wq) - destroy_workqueue(ucsi->connector[i].wq); - typec_unregister_port(ucsi->connector[i].port); - } - - kfree(ucsi->connector); + ucsi_connector_clean(ucsi); } EXPORT_SYMBOL_GPL(ucsi_unregister);
In error path of ucsi_init(), it will clean all valid ucsi connector, and samiliar operation also happen in ucsi_unregister(), add a common function for two places. Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> --- drivers/usb/typec/ucsi/ucsi.c | 46 +++++++++++++++++++++---------------------- 1 file changed, 23 insertions(+), 23 deletions(-)