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

[Oliver Neukum]

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

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

> +
> +		/* Check if the connector is connected */
> +		if (WARN_ON(ucsi_get_constat(con, &constat) != 0))
> +			continue;
> +
> +		if (constat.connected)
> +			ucsi_connect(con, &constat);
> +	}
> +
> +	/* Enable all notifications */
> +	ctrl->cmd = UCSI_SET_NOTIFICATION_ENABLE;
> +	ctrl->data = UCSI_ENABLE_NTFY_ALL;
> +	ret = ucsi_run_cmd(ucsi, NULL, 0);
> +	if (ret)
> +		goto err;
> +
> +	return 0;
> +err:
> +	if (i > 0)
> +		for (; i >= 0; i--, con--)
> +			typec_unregister_port(con->port);
> +
> +	kfree(ucsi->connector);
> +	return ret;
> +}
> +EXPORT_SYMBOL(ucsi_init);
> +