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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
