diff mbox series

[v3,1/2] platform/chrome: cros_ec_typec: Configure Retimer cable type

Message ID 20230707065318.18681-2-utkarsh.h.patel@intel.com (mailing list archive)
State Superseded
Headers show
Series Add support to configure active retimer cable | expand

Commit Message

Patel, Utkarsh H July 7, 2023, 6:53 a.m. UTC 
Connector class driver only configure cable type active or passive.
Configure if the cable type is retimer or redriver with this change.
This detail will be provided as a part of cable discover mode VDO.

Signed-off-by: Utkarsh Patel <utkarsh.h.patel@intel.com>
---
Changes in v2:
- Implemented use of cable discover mode vdo.
- Removed adittional changes to host command.

Changes in v3:
- Changed the return method in cros_typec_get_cable_vdo.
- Changed passed parameters in cros_typec_get_cable_vdo.
- Corrected definition for unsigned integers as kerenl standard.
- Assigning cable_vdo values directly in to cable_mode.
- Removed unncessary checks for Retimer cable type.
---
---
 drivers/platform/chrome/cros_ec_typec.c | 26 ++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

Comments

Prashant Malani July 7, 2023, 7:22 p.m. UTC | #1 
Hi Utkarsh,

On Thu, Jul 6, 2023 at 11:51 PM Utkarsh Patel <utkarsh.h.patel@intel.com> wrote:
>
> Connector class driver only configure cable type active or passive.
> Configure if the cable type is retimer or redriver with this change.
> This detail will be provided as a part of cable discover mode VDO.
>
> Signed-off-by: Utkarsh Patel <utkarsh.h.patel@intel.com>
> ---
> Changes in v2:
> - Implemented use of cable discover mode vdo.
> - Removed adittional changes to host command.
>
> Changes in v3:
> - Changed the return method in cros_typec_get_cable_vdo.
> - Changed passed parameters in cros_typec_get_cable_vdo.
> - Corrected definition for unsigned integers as kerenl standard.
> - Assigning cable_vdo values directly in to cable_mode.
> - Removed unncessary checks for Retimer cable type.
> ---
> ---
>  drivers/platform/chrome/cros_ec_typec.c | 26 ++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index 25f9767c28e8..0ea085fec55a 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -406,6 +406,20 @@ static int cros_typec_usb_safe_state(struct cros_typec_port *port)
>         return ret;
>  }
>
> +static int cros_typec_get_cable_vdo(struct cros_typec_port *port, u16 svid)
Return type should be u32.
Also, since you're not using common return patterns (-ve return value), please
add a kernel doc comment specifying what the return value holds ("0 if VDO
is not found" etc.)

> +{
> +       struct list_head *head = &port->plug_mode_list;
> +       struct cros_typec_altmode_node *node;
> +       int ret = 0;
> +
> +       list_for_each_entry(node, head, list) {
> +               if (node->amode->svid == svid)
> +                       return node->amode->vdo;
> +       }
> +
> +       return ret;
> +}
> +
>  /*
>   * Spoof the VDOs that were likely communicated by the partner for TBT alt
>   * mode.
> @@ -416,6 +430,7 @@ static int cros_typec_enable_tbt(struct cros_typec_data *typec,
>  {
>         struct cros_typec_port *port = typec->ports[port_num];
>         struct typec_thunderbolt_data data;
> +       u32 cable_vdo;
If we're using this in only 1 place, we can just inline it completely:
          data.cable_mode |= cros_typec_get_cable_vdo(port, USB_TYPEC_TBT_SID);

>         int ret;
>
>         if (typec->pd_ctrl_ver < 2) {
> @@ -432,6 +447,10 @@ static int cros_typec_enable_tbt(struct cros_typec_data *typec,
>
>         /* Cable Discover Mode VDO */
>         data.cable_mode = TBT_MODE;
> +
> +       cable_vdo = cros_typec_get_cable_vdo(port, USB_TYPEC_TBT_SID);
> +       data.cable_mode |= cable_vdo;
> +
>         data.cable_mode |= TBT_SET_CABLE_SPEED(pd_ctrl->cable_speed);
>
>         if (pd_ctrl->control_flags & USB_PD_CTRL_OPTICAL_CABLE)
> @@ -513,17 +532,22 @@ static int cros_typec_enable_usb4(struct cros_typec_data *typec,
>  {
>         struct cros_typec_port *port = typec->ports[port_num];
>         struct enter_usb_data data;
> +       u32 cable_vdo;
>
>         data.eudo = EUDO_USB_MODE_USB4 << EUDO_USB_MODE_SHIFT;
>
> +       cable_vdo = cros_typec_get_cable_vdo(port, USB_TYPEC_TBT_SID);

Same deal here, if we're only using this variable in one place, just
inline the call
inside the else if() statement.

> +
>         /* Cable Speed */
>         data.eudo |= pd_ctrl->cable_speed << EUDO_CABLE_SPEED_SHIFT;
>
>         /* Cable Type */
>         if (pd_ctrl->control_flags & USB_PD_CTRL_OPTICAL_CABLE)
>                 data.eudo |= EUDO_CABLE_TYPE_OPTICAL << EUDO_CABLE_TYPE_SHIFT;
> -       else if (pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_CABLE)
> +       else if (cable_vdo & TBT_CABLE_RETIMER)
>                 data.eudo |= EUDO_CABLE_TYPE_RE_TIMER << EUDO_CABLE_TYPE_SHIFT;
> +       else if (pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_CABLE)
> +               data.eudo |= EUDO_CABLE_TYPE_RE_DRIVER << EUDO_CABLE_TYPE_SHIFT;
>
>         data.active_link_training = !!(pd_ctrl->control_flags &
>                                        USB_PD_CTRL_ACTIVE_LINK_UNIDIR);
> --
> 2.25.1
>
Patel, Utkarsh H July 11, 2023, 4:06 p.m. UTC | #2 
Hi Prashant,

Thank you for the review. 

> > diff --git a/drivers/platform/chrome/cros_ec_typec.c
> > b/drivers/platform/chrome/cros_ec_typec.c
> > index 25f9767c28e8..0ea085fec55a 100644
> > --- a/drivers/platform/chrome/cros_ec_typec.c
> > +++ b/drivers/platform/chrome/cros_ec_typec.c
> > @@ -406,6 +406,20 @@ static int cros_typec_usb_safe_state(struct
> cros_typec_port *port)
> >         return ret;
> >  }
> >
> > +static int cros_typec_get_cable_vdo(struct cros_typec_port *port, u16
> > +svid)
> Return type should be u32.
> Also, since you're not using common return patterns (-ve return value),
> please add a kernel doc comment specifying what the return value holds ("0
> if VDO is not found" etc.)

Ack.

> 
> > +{
> > +       struct list_head *head = &port->plug_mode_list;
> > +       struct cros_typec_altmode_node *node;
> > +       int ret = 0;
> > +
> > +       list_for_each_entry(node, head, list) {
> > +               if (node->amode->svid == svid)
> > +                       return node->amode->vdo;
> > +       }
> > +
> > +       return ret;
> > +}
> > +
> >  /*
> >   * Spoof the VDOs that were likely communicated by the partner for TBT
> alt
> >   * mode.
> > @@ -416,6 +430,7 @@ static int cros_typec_enable_tbt(struct
> > cros_typec_data *typec,  {
> >         struct cros_typec_port *port = typec->ports[port_num];
> >         struct typec_thunderbolt_data data;
> > +       u32 cable_vdo;
> If we're using this in only 1 place, we can just inline it completely:
>           data.cable_mode |= cros_typec_get_cable_vdo(port,
> USB_TYPEC_TBT_SID);
> 

Ack.

> >         int ret;
> >
> >         if (typec->pd_ctrl_ver < 2) {
> > @@ -432,6 +447,10 @@ static int cros_typec_enable_tbt(struct
> > cros_typec_data *typec,
> >
> >         /* Cable Discover Mode VDO */
> >         data.cable_mode = TBT_MODE;
> > +
> > +       cable_vdo = cros_typec_get_cable_vdo(port, USB_TYPEC_TBT_SID);
> > +       data.cable_mode |= cable_vdo;
> > +
> >         data.cable_mode |= TBT_SET_CABLE_SPEED(pd_ctrl->cable_speed);
> >
> >         if (pd_ctrl->control_flags & USB_PD_CTRL_OPTICAL_CABLE) @@
> > -513,17 +532,22 @@ static int cros_typec_enable_usb4(struct
> > cros_typec_data *typec,  {
> >         struct cros_typec_port *port = typec->ports[port_num];
> >         struct enter_usb_data data;
> > +       u32 cable_vdo;
> >
> >         data.eudo = EUDO_USB_MODE_USB4 << EUDO_USB_MODE_SHIFT;
> >
> > +       cable_vdo = cros_typec_get_cable_vdo(port, USB_TYPEC_TBT_SID);
> 
> Same deal here, if we're only using this variable in one place, just inline the
> call inside the else if() statement.
> 

Ack. 


Sincerely,
Utkarsh Patel.
diff mbox series

Patch

diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index 25f9767c28e8..0ea085fec55a 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -406,6 +406,20 @@  static int cros_typec_usb_safe_state(struct cros_typec_port *port)
 	return ret;
 }
 
+static int cros_typec_get_cable_vdo(struct cros_typec_port *port, u16 svid)
+{
+	struct list_head *head = &port->plug_mode_list;
+	struct cros_typec_altmode_node *node;
+	int ret = 0;
+
+	list_for_each_entry(node, head, list) {
+		if (node->amode->svid == svid)
+			return node->amode->vdo;
+	}
+
+	return ret;
+}
+
 /*
  * Spoof the VDOs that were likely communicated by the partner for TBT alt
  * mode.
@@ -416,6 +430,7 @@  static int cros_typec_enable_tbt(struct cros_typec_data *typec,
 {
 	struct cros_typec_port *port = typec->ports[port_num];
 	struct typec_thunderbolt_data data;
+	u32 cable_vdo;
 	int ret;
 
 	if (typec->pd_ctrl_ver < 2) {
@@ -432,6 +447,10 @@  static int cros_typec_enable_tbt(struct cros_typec_data *typec,
 
 	/* Cable Discover Mode VDO */
 	data.cable_mode = TBT_MODE;
+
+	cable_vdo = cros_typec_get_cable_vdo(port, USB_TYPEC_TBT_SID);
+	data.cable_mode |= cable_vdo;
+
 	data.cable_mode |= TBT_SET_CABLE_SPEED(pd_ctrl->cable_speed);
 
 	if (pd_ctrl->control_flags & USB_PD_CTRL_OPTICAL_CABLE)
@@ -513,17 +532,22 @@  static int cros_typec_enable_usb4(struct cros_typec_data *typec,
 {
 	struct cros_typec_port *port = typec->ports[port_num];
 	struct enter_usb_data data;
+	u32 cable_vdo;
 
 	data.eudo = EUDO_USB_MODE_USB4 << EUDO_USB_MODE_SHIFT;
 
+	cable_vdo = cros_typec_get_cable_vdo(port, USB_TYPEC_TBT_SID);
+
 	/* Cable Speed */
 	data.eudo |= pd_ctrl->cable_speed << EUDO_CABLE_SPEED_SHIFT;
 
 	/* Cable Type */
 	if (pd_ctrl->control_flags & USB_PD_CTRL_OPTICAL_CABLE)
 		data.eudo |= EUDO_CABLE_TYPE_OPTICAL << EUDO_CABLE_TYPE_SHIFT;
-	else if (pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_CABLE)
+	else if (cable_vdo & TBT_CABLE_RETIMER)
 		data.eudo |= EUDO_CABLE_TYPE_RE_TIMER << EUDO_CABLE_TYPE_SHIFT;
+	else if (pd_ctrl->control_flags & USB_PD_CTRL_ACTIVE_CABLE)
+		data.eudo |= EUDO_CABLE_TYPE_RE_DRIVER << EUDO_CABLE_TYPE_SHIFT;
 
 	data.active_link_training = !!(pd_ctrl->control_flags &
 				       USB_PD_CTRL_ACTIVE_LINK_UNIDIR);