On Tue, Jan 12, 2021 at 9:29 PM Heikki Krogerus
<heikki.krogerus@xxxxxxxxxxxxxxx> wrote:
>
> On Wed, Jan 06, 2021 at 12:39:25AM +0800, Kyle Tso wrote:
> > This patch provides the implementation of Collision Avoidance introduced
> > in PD3.0. The start of each Atomic Message Sequence (AMS) initiated by
> > the port will be denied if the current AMS is not interruptible. The
> > Source port will set the CC to SinkTxNG if it is going to initiate an
> > AMS, and SinkTxOk otherwise. Meanwhile, any AMS initiated by a Sink port
> > will be denied in TCPM if the port partner (Source) sets SinkTxNG except
> > for HARD_RESET and SOFT_RESET.
> >
> > Signed-off-by: Kyle Tso <kyletso@xxxxxxxxxx>
> > Signed-off-by: Will McVicker <willmcvicker@xxxxxxxxxx>
>
> So did you and Will develop this patch together?
>
Not really.
Will cherry-picked the patch from our old branch to a later one which
is more close to Upstream.
And I cherry-picked his version so the signed-off is here.
I will remove the signed-off if that is the right move.
> Few nitpicks below.
>
> > +static void tcpm_set_cc(struct tcpm_port *port, enum typec_cc_status cc)
> > +{
> > + tcpm_log(port, "cc:=%d", cc);
> > + port->cc_req = cc;
> > + port->tcpc->set_cc(port->tcpc, cc);
> > +}
> > +
> > +static enum typec_cc_status tcpm_rp_cc(struct tcpm_port *port);
>
> I think you should move the function here instead of adding the
> prototype for it.
>
will fix this in the next patch version.
> > + case CMD_DISCOVER_MODES:
> > + res = tcpm_ams_start(port, DISCOVER_MODES);
> > + break;
> > + case CMD_ENTER_MODE:
> > + res = tcpm_ams_start(port,
> > + DFP_TO_UFP_ENTER_MODE);
>
> One line is enough:
>
> res = tcpm_ams_start(port, DFP_TO_UFP_ENTER_MODE);
>
will fix this in the next patch version.
> > + break;
> > + case CMD_EXIT_MODE:
> > + res = tcpm_ams_start(port,
> > + DFP_TO_UFP_EXIT_MODE);
>
> Ditto.
>
will fix this in the next patch version.
> > + break;
> > + case CMD_ATTENTION:
> > + res = tcpm_ams_start(port, ATTENTION);
> > + break;
> > + case VDO_CMD_VENDOR(0) ... VDO_CMD_VENDOR(15):
> > + res = tcpm_ams_start(port, STRUCTURED_VDMS);
> > + break;
> > + default:
> > + res = -EOPNOTSUPP;
> > + break;
> > + }
> >
> > - port->vdm_retries = 0;
> > - port->vdm_state = VDM_STATE_BUSY;
> > - timeout = vdm_ready_timeout(port->vdo_data[0]);
> > - mod_vdm_delayed_work(port, timeout);
> > + if (res < 0)
> > + return;
> > }
> > +
> > + port->vdm_state = VDM_STATE_SEND_MESSAGE;
> > + mod_vdm_delayed_work(port, (port->negotiated_rev >= PD_REV30) &&
> > + (port->pwr_role == TYPEC_SOURCE) &&
> > + (PD_VDO_SVDM(vdo_hdr)) &&
> > + (PD_VDO_CMDT(vdo_hdr) == CMDT_INIT) ?
> > + PD_T_SINK_TX : 0);
>
> I don't think you need all those brackets. This would look better, and
> I bet it would make also scripts/checkpatch.pl happy:
>
> mod_vdm_delayed_work(port, (port->negotiated_rev >= PD_REV30 &&
> port->pwr_role == TYPEC_SOURCE &&
> PD_VDO_SVDM(vdo_hdr) &&
> PD_VDO_CMDT(vdo_hdr) == CMDT_INIT) ?
> PD_T_SINK_TX : 0);
>
will fix this in the next patch version.
> > + /*
> > + * If previous AMS is interrupted, switch to the upcoming
> > + * state.
> > + */
> > + upcoming_state = port->upcoming_state;
> > + if (port->upcoming_state != INVALID_STATE) {
> > + port->upcoming_state = INVALID_STATE;
> > + tcpm_set_state(port, upcoming_state, 0);
> > + break;
> > + }
>
> I don't see the local upcoming_state variable is being used anywhere
> outside of these conditions, so please set it inside the condition
> block:
>
> if (port->upcoming_state != INVALID_STATE) {
> upcoming_state = port->upcoming_state;
> port->upcoming_state = INVALID_STATE;
> tcpm_set_state(port, upcoming_state, 0);
> break;
> }
>
will do
> > + upcoming_state = port->upcoming_state;
> > + if (port->upcoming_state != INVALID_STATE) {
> > + port->upcoming_state = INVALID_STATE;
> > + tcpm_set_state(port, upcoming_state, 0);
> > + break;
> > + }
>
> Same here.
>
will do
thanks,
Kyle
