Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Heikki Krogerus]

On Wed, Feb 10, 2016 at 12:19:53PM +0100, Oliver Neukum wrote:
> 
> > +static int ucsi_run_cmd(struct ucsi *ucsi, void *data, size_t size)
> > +{
> > +	int status;
> > +	int ret;
> > +
> > +	dev_vdbg(ucsi->dev, "%s control 0x%llx\n", __func__,
> > +		 ucsi->ppm->data->control);
> > +
> > +	ret = ucsi->ppm->cmd(ucsi->ppm);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* REVISIT: We may need to set UCSI_CCI_CMD_COMPLETE flag here */
> > +	wait_for_completion(&ucsi->complete);
> > +
> > +	status = ucsi->status;
> > +	if (status != UCSI_ERROR && size)
> > +		memcpy(data, ucsi->ppm->data->message_in, size);
> > +
> > +	ret = ucsi_ack(ucsi, UCSI_ACK_CMD);
> > +	if (ret)
> > +		goto out;
> > +
> > +	if (status == UCSI_ERROR) {
> > +		u16 error;
> > +
> > +		ucsi->ppm->data->control = UCSI_GET_ERROR_STATUS;
> > +		ret = ucsi->ppm->cmd(ucsi->ppm);
> > +		if (ret)
> > +			goto out;
> > +
> > +		wait_for_completion(&ucsi->complete);
> > +
> > +		/* Something has really gone wrong */
> > +		if (ucsi->status == UCSI_ERROR) {
> > +			ret = -ENODEV;
> > +			goto out;
> > +		}
> > +
> > +		memcpy(&error, ucsi->ppm->data->message_in, sizeof(error));
> > +
> > +		ret = ucsi_ack(ucsi, UCSI_ACK_CMD);
> > +		if (ret)
> > +			goto out;
> > +
> > +		switch (error) {
> > +		case UCSI_ERROR_INVALID_CON_NUM:
> > +			ret = -ENXIO;
> > +			break;
> > +		case UCSI_ERROR_INCOMPATIBLE_PARTNER:
> > +		case UCSI_ERROR_CC_COMMUNICATION_ERR:
> > +		case UCSI_ERROR_CONTRACT_NEGOTIATION_FAIL:
> > +			ret = -EIO;
> > +			break;
> 
> This looks like you mapped all the interesting error condition
> on a single error code and gave out other error codes to
> conditions of lesser importance.

OK, I'll fix those.

> > +		case UCSI_ERROR_DEAD_BATTERY:
> > +			dev_warn(ucsi->dev, "Dead Battery Condition!\n");
> > +			ret = -EPERM;
> > +			break;
> > +		case UCSI_ERROR_UNREGONIZED_CMD:
> > +		case UCSI_ERROR_INVALID_CMD_ARGUMENT:
> > +		default:
> > +			ret = -EINVAL;
> > +			break;
> > +		}
> > +	}
> > +out:
> > +	ucsi->ppm->data->control = 0;
> > +	return ret;
> > +}
> 
> [..]
> 
> > +/**
> > + * ucsi_init - Initialize an UCSI Interface
> > + * @ucsi: The UCSI Interface
> > + *
> > + * Registers all the USB Type-C ports governed by the PPM of @ucsi and enables
> > + * all the notifications from the PPM.
> > + */
> > +int ucsi_init(struct ucsi *ucsi)
> > +{
> > +	struct ucsi_control *ctrl = (void *)&ucsi->ppm->data->control;
> > +	struct ucsi_connector *con;
> > +	int ret;
> > +	int i;
> > +
> > +	/* Enable basic notifications */
> > +	ctrl->cmd = UCSI_SET_NOTIFICATION_ENABLE;
> > +	ctrl->data = UCSI_ENABLE_NTFY_CMD_COMPLETE | UCSI_ENABLE_NTFY_ERROR;
> > +	ret = ucsi_run_cmd(ucsi, NULL, 0);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Get PPM capabilities */
> > +	ctrl->cmd = UCSI_GET_CAPABILITY;
> > +	ret = ucsi_run_cmd(ucsi, &ucsi->cap, sizeof(ucsi->cap));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ucsi->connector = kcalloc(ucsi->cap.num_connectors,
> > +				  sizeof(struct ucsi_connector), GFP_KERNEL);
> > +	if (!ucsi->connector)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0, con = ucsi->connector; i < ucsi->cap.num_connectors;
> > +	     i++, con++) {
> > +		struct typec_capability *cap = &con->typec_cap;
> > +		struct ucsi_connector_status constat;
> > +
> > +		/* Get connector capability */
> > +		ctrl->cmd = UCSI_GET_CONNECTOR_CAPABILITY;
> > +		ctrl->data = i + 1;
> > +		ret = ucsi_run_cmd(ucsi, &con->cap, sizeof(con->cap));
> > +		if (ret)
> > +			goto err;
> > +
> > +		/* Register the connector */
> > +
> > +		if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DRP)
> > +			cap->type = TYPEC_PORT_DRP;
> > +		else if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DFP)
> > +			cap->type = TYPEC_PORT_DFP;
> > +		else if (con->cap.op_mode & UCSI_CONCAP_OPMODE_UFP)
> > +			cap->type = TYPEC_PORT_UFP;
> > +
> > +		cap->usb_pd = !!(ucsi->cap.attributes &
> > +				       UCSI_CAP_ATTR_USB_PD);
> > +		cap->audio_accessory = !!(con->cap.op_mode &
> > +					  UCSI_CONCAP_OPMODE_AUDIO_ACCESSORY);
> > +		cap->debug_accessory = !!(con->cap.op_mode &
> > +					  UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY);
> > +
> > +		/* TODO: Alt modes */
> > +
> > +		cap->dr_swap = ucsi_dr_swap;
> > +		cap->pr_swap = ucsi_pr_swap;
> > +
> > +		con->port = typec_register_port(ucsi->dev, cap);
> > +		if (IS_ERR(con->port)) {
> > +			ret = PTR_ERR(con->port);
> > +			goto err;
> > +		}
> > +
> > +		con->num = i + 1;
> > +		con->ucsi = ucsi;
> > +		INIT_WORK(&con->work, ucsi_connector_change);
> 
> You init the work later. Does this depend on the firmware
> generating no events before they are enabled? Depending on firmware
> is a bad idea.

If there is a real risk of getting connector change events before we
have enabled them, I think the only thing we can do is add checks to
ucsi_interrupt() to make sure that we don't try to handle events from
connectors that are not yet initialized.


Thanks,

-- 
heikki