Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1451110 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 1/2] usb: typec: Add USB Power Delivery sink port support Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-07-27 10:20 +0200
    Re: [PATCH v2 1/2] usb: typec: Add USB Power Delivery sink port  support Bin Gao <bin.gao@linux.intel.com> - 2016-07-27 19:30 +0200

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

[Multipart message — attachments visible in raw view] — view raw

Hi,

Bin Gao <bin.gao@linux.intel.com> 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@intel.com>
> 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

[toc] | [next] | [standalone]

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

On Wed, Jul 27, 2016 at 11:13:43AM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Bin Gao <bin.gao@linux.intel.com> 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@intel.com>
> > 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

I'll fix these in next revision. Thanks for your review.

-Bin

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web