mbox series

[0/3] usb: typec: ucsi: allow retry to find role switch

Message ID 1649759407-24049-1-git-send-email-quic_linyyuan@quicinc.com
Headers show
Series usb: typec: ucsi: allow retry to find role switch | expand

Message

Linyu Yuan April 12, 2022, 10:30 a.m. UTC 
when one role switch module built as module, it may load later then
ucsi, this change series allow retry from ucsi.

Linyu Yuan (3):
  usb: typec: ucsi: set con->port to NULL when register port fail
  usb: typec: ucsi: add a common function ucsi_connector_clean()
  usb: typec: ucsi: retry find role swithch when module load late

 drivers/usb/typec/ucsi/ucsi.c | 75 +++++++++++++++++++++++--------------------
 drivers/usb/typec/ucsi/ucsi.h |  2 +-
 2 files changed, 41 insertions(+), 36 deletions(-)

Comments

Heikki Krogerus April 13, 2022, 7:38 a.m. UTC | #1 
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,
Linyu Yuan April 13, 2022, 7:45 a.m. UTC | #2 
> 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