diff mbox series

[4/7] usb: typec: tcpm: Start using struct typec_operations

Message ID 20191001094858.68643-5-heikki.krogerus@linux.intel.com (mailing list archive)
State Superseded
Headers show
Series usb: typec: Small API improvement | expand

Commit Message

Heikki Krogerus Oct. 1, 2019, 9:48 a.m. UTC 
Supplying the operation callbacks as part of a struct
typec_operations instead of as part of struct
typec_capability during port registration.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/tcpm/tcpm.c | 47 ++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 26 deletions(-)

Comments

Guenter Roeck Oct. 1, 2019, 1:30 p.m. UTC | #1 
On 10/1/19 2:48 AM, Heikki Krogerus wrote:
> Supplying the operation callbacks as part of a struct
> typec_operations instead of as part of struct
> typec_capability during port registration.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>   drivers/usb/typec/tcpm/tcpm.c | 47 ++++++++++++++++-------------------
>   1 file changed, 21 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 96562744101c..0fde7e042d5f 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -390,12 +390,6 @@ static enum tcpm_state tcpm_default_state(struct tcpm_port *port)
>   	return SRC_UNATTACHED;
>   }
>   
> -static inline
> -struct tcpm_port *typec_cap_to_tcpm(const struct typec_capability *cap)
> -{
> -	return container_of(cap, struct tcpm_port, typec_caps);
> -}
> -
>   static bool tcpm_port_is_disconnected(struct tcpm_port *port)
>   {
>   	return (!port->attached && port->cc1 == TYPEC_CC_OPEN &&
> @@ -3970,10 +3964,9 @@ void tcpm_pd_hard_reset(struct tcpm_port *port)
>   }
>   EXPORT_SYMBOL_GPL(tcpm_pd_hard_reset);
>   
> -static int tcpm_dr_set(const struct typec_capability *cap,
> -		       enum typec_data_role data)
> +static int tcpm_dr_set(struct typec_port *p, enum typec_data_role data)
>   {
> -	struct tcpm_port *port = typec_cap_to_tcpm(cap);
> +	struct tcpm_port *port = typec_get_drvdata(p);
>   	int ret;
>   
>   	mutex_lock(&port->swap_lock);
> @@ -4038,10 +4031,9 @@ static int tcpm_dr_set(const struct typec_capability *cap,
>   	return ret;
>   }
>   
> -static int tcpm_pr_set(const struct typec_capability *cap,
> -		       enum typec_role role)
> +static int tcpm_pr_set(struct typec_port *p, enum typec_role role)
>   {
> -	struct tcpm_port *port = typec_cap_to_tcpm(cap);
> +	struct tcpm_port *port = typec_get_drvdata(p);
>   	int ret;
>   
>   	mutex_lock(&port->swap_lock);
> @@ -4082,10 +4074,9 @@ static int tcpm_pr_set(const struct typec_capability *cap,
>   	return ret;
>   }
>   
> -static int tcpm_vconn_set(const struct typec_capability *cap,
> -			  enum typec_role role)
> +static int tcpm_vconn_set(struct typec_port *p, bool source)
>   {
> -	struct tcpm_port *port = typec_cap_to_tcpm(cap);
> +	struct tcpm_port *port = typec_get_drvdata(p);
>   	int ret;
>   
>   	mutex_lock(&port->swap_lock);
> @@ -4096,7 +4087,7 @@ static int tcpm_vconn_set(const struct typec_capability *cap,
>   		goto port_unlock;
>   	}
>   
> -	if (role == port->vconn_role) {
> +	if (source == port->vconn_role) {

source is boolean, vconn_role is enum typec_role.
The original typec code took advantage of typec_role == TYPEC_SINK matching false,
and typec_role == TYPEC_SOURCE matching true, but I don't think it is a good
idea to carry that down to low level drivers. This will confuse everyone who wants
to contribute a driver in the future.

>   		ret = 0;
>   		goto port_unlock;
>   	}
> @@ -4122,9 +4113,9 @@ static int tcpm_vconn_set(const struct typec_capability *cap,
>   	return ret;
>   }
>   
> -static int tcpm_try_role(const struct typec_capability *cap, int role)
> +static int tcpm_try_role(struct typec_port *p, int role)
>   {
> -	struct tcpm_port *port = typec_cap_to_tcpm(cap);
> +	struct tcpm_port *port = typec_get_drvdata(p);
>   	struct tcpc_dev	*tcpc = port->tcpc;
>   	int ret = 0;
>   
> @@ -4331,10 +4322,9 @@ static void tcpm_init(struct tcpm_port *port)
>   	tcpm_set_state(port, PORT_RESET, 0);
>   }
>   
> -static int tcpm_port_type_set(const struct typec_capability *cap,
> -			      enum typec_port_type type)
> +static int tcpm_port_type_set(struct typec_port *p, enum typec_port_type type)
>   {
> -	struct tcpm_port *port = typec_cap_to_tcpm(cap);
> +	struct tcpm_port *port = typec_get_drvdata(p);
>   
>   	mutex_lock(&port->lock);
>   	if (type == port->port_type)
> @@ -4359,6 +4349,14 @@ static int tcpm_port_type_set(const struct typec_capability *cap,
>   	return 0;
>   }
>   
> +static const struct typec_operations tcpm_ops = {
> +	.try_role = tcpm_try_role,
> +	.dr_set = tcpm_dr_set,
> +	.pr_set = tcpm_pr_set,
> +	.vconn_set = tcpm_vconn_set,
> +	.port_type_set = tcpm_port_type_set
> +};
> +
>   void tcpm_tcpc_reset(struct tcpm_port *port)
>   {
>   	mutex_lock(&port->lock);
> @@ -4770,11 +4768,8 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
>   	port->typec_caps.fwnode = tcpc->fwnode;
>   	port->typec_caps.revision = 0x0120;	/* Type-C spec release 1.2 */
>   	port->typec_caps.pd_revision = 0x0300;	/* USB-PD spec release 3.0 */
> -	port->typec_caps.dr_set = tcpm_dr_set;
> -	port->typec_caps.pr_set = tcpm_pr_set;
> -	port->typec_caps.vconn_set = tcpm_vconn_set;
> -	port->typec_caps.try_role = tcpm_try_role;
> -	port->typec_caps.port_type_set = tcpm_port_type_set;
> +	port->typec_caps.driver_data = port;
> +	port->typec_caps.ops = &tcpm_ops;
>   
>   	port->partner_desc.identity = &port->partner_ident;
>   	port->port_type = port->typec_caps.type;
>
Heikki Krogerus Oct. 4, 2019, 8:46 a.m. UTC | #2 
On Tue, Oct 01, 2019 at 06:30:42AM -0700, Guenter Roeck wrote:
> > @@ -4082,10 +4074,9 @@ static int tcpm_pr_set(const struct typec_capability *cap,
> >   	return ret;
> >   }
> > -static int tcpm_vconn_set(const struct typec_capability *cap,
> > -			  enum typec_role role)
> > +static int tcpm_vconn_set(struct typec_port *p, bool source)
> >   {
> > -	struct tcpm_port *port = typec_cap_to_tcpm(cap);
> > +	struct tcpm_port *port = typec_get_drvdata(p);
> >   	int ret;
> >   	mutex_lock(&port->swap_lock);
> > @@ -4096,7 +4087,7 @@ static int tcpm_vconn_set(const struct typec_capability *cap,
> >   		goto port_unlock;
> >   	}
> > -	if (role == port->vconn_role) {
> > +	if (source == port->vconn_role) {
> 
> source is boolean, vconn_role is enum typec_role.
> The original typec code took advantage of typec_role == TYPEC_SINK matching false,
> and typec_role == TYPEC_SOURCE matching true, but I don't think it is a good
> idea to carry that down to low level drivers. This will confuse everyone who wants
> to contribute a driver in the future.

OK, I'll keep the parameter as emum typec_role.


thanks,
diff mbox series

Patch

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 96562744101c..0fde7e042d5f 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -390,12 +390,6 @@  static enum tcpm_state tcpm_default_state(struct tcpm_port *port)
 	return SRC_UNATTACHED;
 }
 
-static inline
-struct tcpm_port *typec_cap_to_tcpm(const struct typec_capability *cap)
-{
-	return container_of(cap, struct tcpm_port, typec_caps);
-}
-
 static bool tcpm_port_is_disconnected(struct tcpm_port *port)
 {
 	return (!port->attached && port->cc1 == TYPEC_CC_OPEN &&
@@ -3970,10 +3964,9 @@  void tcpm_pd_hard_reset(struct tcpm_port *port)
 }
 EXPORT_SYMBOL_GPL(tcpm_pd_hard_reset);
 
-static int tcpm_dr_set(const struct typec_capability *cap,
-		       enum typec_data_role data)
+static int tcpm_dr_set(struct typec_port *p, enum typec_data_role data)
 {
-	struct tcpm_port *port = typec_cap_to_tcpm(cap);
+	struct tcpm_port *port = typec_get_drvdata(p);
 	int ret;
 
 	mutex_lock(&port->swap_lock);
@@ -4038,10 +4031,9 @@  static int tcpm_dr_set(const struct typec_capability *cap,
 	return ret;
 }
 
-static int tcpm_pr_set(const struct typec_capability *cap,
-		       enum typec_role role)
+static int tcpm_pr_set(struct typec_port *p, enum typec_role role)
 {
-	struct tcpm_port *port = typec_cap_to_tcpm(cap);
+	struct tcpm_port *port = typec_get_drvdata(p);
 	int ret;
 
 	mutex_lock(&port->swap_lock);
@@ -4082,10 +4074,9 @@  static int tcpm_pr_set(const struct typec_capability *cap,
 	return ret;
 }
 
-static int tcpm_vconn_set(const struct typec_capability *cap,
-			  enum typec_role role)
+static int tcpm_vconn_set(struct typec_port *p, bool source)
 {
-	struct tcpm_port *port = typec_cap_to_tcpm(cap);
+	struct tcpm_port *port = typec_get_drvdata(p);
 	int ret;
 
 	mutex_lock(&port->swap_lock);
@@ -4096,7 +4087,7 @@  static int tcpm_vconn_set(const struct typec_capability *cap,
 		goto port_unlock;
 	}
 
-	if (role == port->vconn_role) {
+	if (source == port->vconn_role) {
 		ret = 0;
 		goto port_unlock;
 	}
@@ -4122,9 +4113,9 @@  static int tcpm_vconn_set(const struct typec_capability *cap,
 	return ret;
 }
 
-static int tcpm_try_role(const struct typec_capability *cap, int role)
+static int tcpm_try_role(struct typec_port *p, int role)
 {
-	struct tcpm_port *port = typec_cap_to_tcpm(cap);
+	struct tcpm_port *port = typec_get_drvdata(p);
 	struct tcpc_dev	*tcpc = port->tcpc;
 	int ret = 0;
 
@@ -4331,10 +4322,9 @@  static void tcpm_init(struct tcpm_port *port)
 	tcpm_set_state(port, PORT_RESET, 0);
 }
 
-static int tcpm_port_type_set(const struct typec_capability *cap,
-			      enum typec_port_type type)
+static int tcpm_port_type_set(struct typec_port *p, enum typec_port_type type)
 {
-	struct tcpm_port *port = typec_cap_to_tcpm(cap);
+	struct tcpm_port *port = typec_get_drvdata(p);
 
 	mutex_lock(&port->lock);
 	if (type == port->port_type)
@@ -4359,6 +4349,14 @@  static int tcpm_port_type_set(const struct typec_capability *cap,
 	return 0;
 }
 
+static const struct typec_operations tcpm_ops = {
+	.try_role = tcpm_try_role,
+	.dr_set = tcpm_dr_set,
+	.pr_set = tcpm_pr_set,
+	.vconn_set = tcpm_vconn_set,
+	.port_type_set = tcpm_port_type_set
+};
+
 void tcpm_tcpc_reset(struct tcpm_port *port)
 {
 	mutex_lock(&port->lock);
@@ -4770,11 +4768,8 @@  struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
 	port->typec_caps.fwnode = tcpc->fwnode;
 	port->typec_caps.revision = 0x0120;	/* Type-C spec release 1.2 */
 	port->typec_caps.pd_revision = 0x0300;	/* USB-PD spec release 3.0 */
-	port->typec_caps.dr_set = tcpm_dr_set;
-	port->typec_caps.pr_set = tcpm_pr_set;
-	port->typec_caps.vconn_set = tcpm_vconn_set;
-	port->typec_caps.try_role = tcpm_try_role;
-	port->typec_caps.port_type_set = tcpm_port_type_set;
+	port->typec_caps.driver_data = port;
+	port->typec_caps.ops = &tcpm_ops;
 
 	port->partner_desc.identity = &port->partner_ident;
 	port->port_type = port->typec_caps.type;