RE: [PATCH v2 1/2] platform/chrome: cros_ec_typec: Configure Retimer cable type

[Patel, Utkarsh H]

Hi Prashant,

Thank you for the review and feedback.

> 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...")

Ack.

> 
> > 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@xxxxxxxxx>
> >

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

Ack. 

> 
> 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).

Ack.

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

Ack.

> 
> > +	}
> > +
> > +	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.

Ack.

> > +	uint32_t cable_vdo;
> u32.

Ack.

> 
> >  	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?

Yes, it will need separate patch as there are other capabilities in cable mode and all can be removed once this change goes through.

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

Agree, with this all other configs for cable mode can be removed and mux driver will just use cable mode VDO as is.

> 
> > +
> >  	/* 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

Ack.

> 
> >
> >  	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.

Ack.

Sincerely,
Utkarsh Patel.