Re: [PATCH v2 1/2] usb: typec: Add USB Power Delivery sink port support

From: Felipe Balbi
Date: Wed Jul 27 2016 - 04:15:30 EST



Hi,

Bin Gao <bin.gao@xxxxxxxxxxxxxxx> writes:
> This patch implements a simple USB Power Delivery sink port state machine.
> It assumes the hardware only handles PD packet transmitting and receiving
> over the CC line of the USB Type-C connector. The state transition is
> completely controlled by software. This patch only implement the sink port
> function and it doesn't support source port and port swap yet.
>
> This patch depends on these two patches:
> https://lkml.org/lkml/2016/6/29/349
> https://lkml.org/lkml/2016/6/29/350
>
> Signed-off-by: Bin Gao <bin.gao@xxxxxxxxx>
> Changes in v2:
> - Removed work queue so messages are directly handled in phy driver's interrupt context
> - used pr_debug instead of pr_info for message dump
> - Converted PD driver to tristate and typec driver is independent of it

this should be after the tearline (---) below. We don't want this in
changelog ;-)

> +static void handle_source_cap(struct pd_sink_port *port, u8 msg_revision,
> + u8 nr_objs, u8 *buf)
> +{
> + int i;
> + u32 *obj;
> + u8 type;
> + struct pd_source_cap *cap = port->source_caps;
> +
> + /*
> + * The PD spec revision included in SOURCE_CAPABILITY message is the
> + * highest revision that the Source supports.
> + */
> + port->pd_rev = msg_revision;
> +
> + /*
> + * First we need to save all PDOs - they may be used in the future.
> + * USB PD spec says we must use PDOs in the most recent
> + * SOURCE_CAPABILITY message. Here we replace old PDOs with new ones.
> + */
> + port->nr_source_caps = 0;
> + for (i = 0; i < nr_objs; i++) {
> + obj = (u32 *)(buf + i * PD_OBJ_SIZE);
> + type = (*obj >> SOURCE_CAP_TYPE_BIT) & SOURCE_CAP_TYPE_MASK;
> + switch (type) {
> + case PS_TYPE_FIXED:
> + cap->ps_type = PS_TYPE_FIXED;
> + cap->fixed = *(struct pd_pdo_src_fixed *)obj;
> + break;
> + case PS_TYPE_VARIABLE:
> + cap->ps_type = PS_TYPE_VARIABLE;
> + cap->variable = *(struct pd_pdo_variable *)obj;
> + break;
> + case PS_TYPE_BATTERY:
> + cap->ps_type = PS_TYPE_BATTERY;
> + cap->battery = *(struct pd_pdo_battery *)obj;
> + break;
> + default: /* shouldn't come here */
> + pr_err("Invalid Source Capability type: %u.\n", type);
> + continue;
> + }
> + port->nr_source_caps++;
> + cap++;
> + }
> +
> + if (port->nr_source_caps == 0) {
> + pr_err("There is no valid PDOs in SOURCE_CAPABILITY message\n");
> + return;
> + }
> +
> + /* If a contract is not established, we need send a REQUEST message */
> + if (port->state == PD_SINK_STATE_WAIT_FOR_SRC_CAP) {

this is wrong. Read the fluxchart in figure 8-42. Source can decide to
send another Source Capability before receiving our Good CRC and we need
to work with that. This state check is, at a minimum, wrong. I'd
actually just go ahead and remove it.

> + if (!send_request(port))
> + port->state = PD_SINK_STATE_REQUEST_SENT;
> + }
> +}

--
balbi

Attachment: signature.asc
Description: PGP signature