Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 29 Jun 2023 17:38:20 +0000
From:   Prashant Malani <pmalani@chromium.org>
To:     Utkarsh Patel <utkarsh.h.patel@intel.com>
Cc:     linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org,
        heikki.krogerus@linux.intel.com, bleung@chromium.org
Subject: Re: [PATCH v2 1/2] platform/chrome: cros_ec_typec: Configure Retimer
 cable type
Message-ID: <ZJ3BjFExLBWXZxqm@chromium.org>
References: <20230628173727.701140-1-utkarsh.h.patel@intel.com>
 <20230628173727.701140-2-utkarsh.h.patel@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230628173727.701140-2-utkarsh.h.patel@intel.com>
Precedence: bulk

Hi Utkarsh,

Thanks for sending the patch.

On Jun 28 10:37, Utkarsh Patel wrote:
> Connector class driver only configure cable type active or passive.
> With this change it will also configure if the cable type is retimer or

nit: Please use imperative form ("Configure if the cable type is...")

> redriver if required by AP. 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. 
> ---
> 
> ---
>  drivers/platform/chrome/cros_ec_typec.c | 33 ++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index 25f9767c28e8..557f396d1c00 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -406,6 +406,25 @@ static int cros_typec_usb_safe_state(struct cros_typec_port *port)
>  	return ret;
>  }
>  
> +static int cros_typec_get_cable_vdo(struct cros_typec_data *typec, int port_num,
> +				    uint16_t svid)

u16 type is used in the kernel.

Also, if this function returns a VDO, the return type should be u32, but... (see later)

> +{
> +	struct cros_typec_port *port = typec->ports[port_num];

Pass the struct cros_typec_port directly (and then drop the port_num argument).

> +	struct list_head *head = &port->plug_mode_list;
> +	struct cros_typec_altmode_node *node;
> +
> +	list_for_each_entry(node, head, list) {
> +		if (node->amode->svid == svid)
> +			break;

Return the vdo here directly; that way, if you reach past the list iteration,
we know for sure the SVID wasn't found and you can unconditionally return the error
case.

> +	}
> +
> +	if (node->amode->svid != svid)
> +		return 0;

I think it is more correct here to have an int return type (so the "not found" case can
return -1 or the right error code), and then have the cable VDO as a pointer argument.

> +
> +	return node->amode->vdo;
> +}
> +
> +
>  /*
>   * Spoof the VDOs that were likely communicated by the partner for TBT alt
>   * mode.
> @@ -416,6 +435,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;
> +	uint32_t cable_vdo;
u32.

>  	int ret;
>  
>  	if (typec->pd_ctrl_ver < 2) {
> @@ -442,6 +462,11 @@ static int cros_typec_enable_tbt(struct cros_typec_data *typec,
>  
>  	data.cable_mode |= TBT_SET_CABLE_ROUNDED(pd_ctrl->cable_gen);

Probably a separate patch, but can we get rid of this too, since the cable_vdo should
have this information?

>  
> +	cable_vdo = cros_typec_get_cable_vdo(typec, port_num, USB_TYPEC_TBT_SID);
> +
> +	if (cable_vdo & TBT_CABLE_RETIMER)
> +		data.cable_mode |= TBT_CABLE_RETIMER;

Why just not or the cable_vdo into existing cable_mode"? :

data.cable_mode |= cable_vdo;

> +
>  	/* Enter Mode VDO */
>  	data.enter_vdo = TBT_SET_CABLE_SPEED(pd_ctrl->cable_speed);
>  
> @@ -513,17 +538,23 @@ 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;
> +	uint32_t cable_vdo;

u32

>  
>  	data.eudo = EUDO_USB_MODE_USB4 << EUDO_USB_MODE_SHIFT;
>  
> +	cable_vdo = cros_typec_get_cable_vdo(typec, port_num, 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 (!(cable_vdo & TBT_CABLE_RETIMER) &&

The !(cable_vdo & TBT_CABLE_RETIMER) check shouldn't be necessary; the
earlier "else if" already ensures that by the time we reach here, this
clause is satisfied.

> +		 (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
>